Page MenuHomeFreeBSD

bhyve: Avoid using a packed struct for xhci port registers
ClosedPublic

Authored by markj on Wed, Nov 16, 2:21 PM.

Details

Summary

I believe the __packed annotation is there only because
pci_xhci_portregs_read() is treating the register set as an array of
uint32_t. clang warns about taking the address of portregs->portsc
because it is a packed member and thus might not have expected
alignment.

For some reason the compiler warns about this only after commit
fd104a6ebc35 ("bhyve: Use XHCI_PORTREG_PTR in one place that open-coded
it.").

Fix the problem by simply selecting the field to read with a switch
statement. This mimics pci_xhci_portregs_write(). While here, switch
to using some symbolic constants.

No functional change intended.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Wed, Nov 16, 2:21 PM
corvink added inline comments.
usr.sbin/bhyve/pci_xhci.c
2169

Should we add a default case? Something like:

default:
  assert(false);
  break;
This revision is now accepted and ready to land.Thu, Nov 17, 7:24 AM
jhb added inline comments.
usr.sbin/bhyve/pci_xhci.c
2169

We have an __assert_unreachable() macro in <sys/cdefs.h> in the kernel for these that uses the compiler's __builtin_unreachable(). We should use something similar for these sorts of cases rather than assert(false).

usr.sbin/bhyve/pci_xhci.c
2169

Is there anything preventing an unaligned write to a virtualized BAR? If not, then the default case is indeed reachable. We can log a warning and return 0 in that case. pci_xhci_portregs_write() currently silently ignores an unaligned write

usr.sbin/bhyve/pci_xhci.c
2169

Hmm, well, these are reads, not writes, so we can't really ignore them (have to return something). The old code didn't ignore them, instead it "aligned" the read. That is since offset / sizeof(uint32_t) truncated in the old code, reading from offset 13 actually read the register at offset 12.

We should either preserve that (e.g. switch (offset & ~3) or switch (offset / sizeof(uint32_t) and fixing the indices), or we should add a default case for misaligned reads.

For invalid reads the normal behavior would be to return 0xffffffff rather than 0.

usr.sbin/bhyve/pci_xhci.c
2169

I'd add an assertion. This allows us in to easily detect guests which do unaligned accesses. If there are some guests doing unaligned accesses, we could discuss the correct return value. Maybe some strange guests read these register byte by byte, so we shouldn't return 0xff or 0 by default.

__builtin_unreachable() is not applicable for this use case as it could be reachable.

Add a default case for port register I/O: return 0xffffffff
for unaligned reads. Also print debug messages for unaligned
reads and writes of port registers.

This revision now requires review to proceed.Fri, Nov 18, 3:16 PM
usr.sbin/bhyve/pci_xhci.c
2169

I suspect it doesn't make sense to bother aligning reads when writes are not similarly handled. I just added an explicit default case and a debug print so that the problem stands out when debugging the device model.

This revision is now accepted and ready to land.Fri, Nov 18, 5:04 PM