Page MenuHomeFreeBSD

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

Authored by markj on Nov 16 2022, 2:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 4, 1:26 AM
Unknown Object (File)
Sun, Mar 3, 2:43 AM
Unknown Object (File)
Jan 31 2024, 1:06 PM
Unknown Object (File)
Dec 12 2023, 4:57 PM
Unknown Object (File)
Nov 19 2023, 2:35 PM
Unknown Object (File)
Nov 8 2023, 7:35 PM
Unknown Object (File)
Oct 31 2023, 9:12 PM
Unknown Object (File)
Sep 26 2023, 5:47 PM
Subscribers

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 Skipped
Unit
Tests Skipped
Build Status
Buildable 48416
Build 45302: arc lint + arc unit

Event Timeline

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

Should we add a default case? Something like:

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

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
2166

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
2166

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
2166

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.Nov 18 2022, 3:16 PM
usr.sbin/bhyve/pci_xhci.c
2166

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.Nov 18 2022, 5:04 PM