Page MenuHomeFreeBSD

bhyve: Avoid using a packed struct for xhci port registers

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



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

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

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

rG FreeBSD src repository
Lint Not Applicable
Tests Not Applicable

Event Timeline

markj requested review of this revision.Wed, Nov 16, 2:21 PM
corvink added inline comments.

Should we add a default case? Something like:

This revision is now accepted and ready to land.Thu, Nov 17, 7:24 AM
jhb added inline comments.

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).


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


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.


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

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