bhyve: Avoid using a packed struct for xhci port registers

[markj]

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.

[corvink]

usr.sbin/bhyve/pci_xhci.c

2169

Should we add a default case? Something like:

default:
  assert(false);
  break;

[jhb]

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

[markj]

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

[jhb]

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.

[corvink]

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.

[markj]

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.

[markj]

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.