On Wed, 18 Dec 2024 at 02:19, Nicholas Piggin <npiggin@gmail.com> wrote:
On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:
Hey Nicholas,
I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
wrong, but I've asked some questions and made some comments inline:
Hey Phil,
Thanks for the review, looks like you are the expert now :)
The "hot potato" method for determining maintainership. :-)
On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:
Surely we should only propagate the error and fail realize() iff s->msix
is
ON_OFF_AUTO_ON?
For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.
Yep you're right... you had been testing with msix disabled. I wonder if
there is a good way to force fail this in qtests?
I'm really the wrong person to ask about qtest, I'm only just beginning to
get to grips with it. It seems the only real reason msix_init fails other
than misconfiguration of the device/BAR is when msi_nonbroken = false.
At least on x86(-64), msi_nonbroken=true is unconditionally set in
apic_realize(). (I think real hardware would not support MSI(-X) on the
i440FX chipset - I was fairly certain it was the PCI root/southbridge
catching the writes to the reserved memory region, and I didn't think the
PIIX did this; but at least in QEMU it doesn't seem to be implemented in a
chipset-dependent way.) I'm not sure it's possible to run QEMU without an
APIC?
On aarch64, the GICv3 needs to explicitly enable support (via the ITS), so
perhaps it's possible to set up an aarch64 qtest with ITS disabled? It
looks like the 'virt' machine type only supports the ITS from version 6.2,
so older versions will disable it.
Sorry, clutching at straws here.
+
+ pci_register_bar(dev, s->msix_bar_nr,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ msix_bar);
Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
Even if it is safe, is it sensible? If we're calling it twice for the
same
BAR, and the arguments of either of the calls changes in future, the
other
needs to change too. Doesn't seem ideal.
Good catch. It looks like it "works" so long as the bar wasn't mapped,
but I'm sure bad practice... Interesting there is no assertion in
there though. I'll fix it though.
I notice there's a msix_init_exclusive_bar()… I wonder if it'd be simpler
to use that and modify it so it allows you to choose a size and layout for
the BAR, rather than adding all that extra code to deal with the extra BAR
in the XHCI?
(It already calls pci_register_bar() and msix_init() internally, but seems
to set the BAR's size to 4096 and places the PBA at halfway through the
BAR. Perhaps rename it to something like
msix_init_exclusive_bar_with_layout and pass the bar_size and
bar_pba_offset in as parameters; then make msix_init_exclusive_bar() a
wrapper for that function with the existing defaults for those variables?)
Just kicking around some ideas here, I have no idea if that actually ends
up making things simpler…
Thanks,
Nick