Page MenuHomeFreeBSD

bhyve: Add legacy debug/test interfaces for kvm unit tests.
ClosedPublic

Authored by adam_fenn.io on Nov 6 2020, 10:32 PM.

Details

Summary

Implement the legacy debug/test interfaces expected by KVM-unit-tests'
realmode, emulator, and ioapic tests.

Test Plan

Verified via KVM-unit-tests' realmode, emulator, and ioapic tests.

(It should be noted that emulator and ioapic still fail, but, now, for
reasons other than needing these test interfaces).

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fix style mistake in Makefile.

grehan retitled this revision from bhyve: Add legacy debug/test interfaces to bhyve: Add legacy debug/test interfaces for kvm unit tests..Nov 12 2020, 8:59 AM

A snapshot of the KVM unit tests, slightly modified to work with bhyve and Adam's change, is at https://people.freebsd.org/~grehan/kvm_unit_tests/kut.tgz

markj added inline comments.
usr.sbin/bhyve/pci_lpc.c
119 ↗(On Diff #79289)

I think this could be simplified a bit:

if (pctestdev_present) {
    ...
} else if (pctestdev_parse(str) == 0) {
    ...
}
usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

Do we need to handle overflow here?

267 ↗(On Diff #79289)

Perhaps group this with pctestdev_getname() above.

Incorporate @markj's review feedback and style(9)-ize pctestdev_init()'s local variable declarations.

Thanks a lot for the review and feedback, @markj! Diff updated and question/response added accordingly.

usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

So my read was that instructions like outw %eax, 0xe3 would still end up here and, in light of this being a test interface, I thought it made sense to make any misuses immediately apparent.

Have I misunderstood something?

usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

I was really asking whether the existing check is sufficient, but I see now that bhyve ensures that IOPORT_BASE <= port < IOPORT_BASE + IOPORT_LEN, so no other checks are required. I was a bit confused because pctestdev_iomem_io() asserts that addr >= IOMEM_BASE but this function doesn't have a corresponding assertion.

usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

I was really asking whether the existing check is sufficient, but I see now that bhyve ensures that IOPORT_BASE <= port < IOPORT_BASE + IOPORT_LEN, so no other checks are required.

Oh ok. Right, as per inout's handler registration implementation, this handler can only be called if port is in [IOPORT_BASE, IOPORT_BASE + IOPORT_LEN).

I was a bit confused because pctestdev_iomem_io() asserts that addr >= IOMEM_BASE but this function doesn't have a corresponding assertion.

Ah, thanks! I think I meant to remove this assertion per analogous reasoning (mem handler registration appears to have the same behavior).

Does that sound good?

markj added inline comments.
usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

That seems reasonable to me. Overall the diff looks right to me, thanks.

This revision is now accepted and ready to land.Nov 16 2020, 4:43 PM

Remove unnecessary assertion.

This revision now requires review to proceed.Nov 16 2020, 5:40 PM
adam_fenn.io added inline comments.
usr.sbin/bhyve/pctestdev.c
222 ↗(On Diff #79289)

Diff updated with assertion removed. Thanks again for the review!

This revision is now accepted and ready to land.Nov 17 2020, 12:22 PM