Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Bernhard Beschow]

Am 19. Dezember 2024 09:23:13 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 18 Dec 2024, Phil Dennis-Jordan wrote:
>> 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. :-)
>
>That's how I got some parts I'm maintainer of now. :-)
>
>[...]
>>>> 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?
>
>There's isapc but you can't attach PCI card to that. It seems according to 
>-machine pc,help that there's a PIC=<OnOffAuto> option but no similar for 
>APIC. Maybe that could be added but not sure it would work. (Adding Bernhard 
>to cc to quickly pass on the potato.)

I agree, the only x86 machine with no APIC is the isapc machine. All others 
(i440fx, q35, microvm) have it always enabled. I guess there is just no point 
in disabling it since the APIC is part of the architecture for ages and SMP 
requires it. Although PIIX3+ didn't have an APIC built-in it had interfaces for 
handling a dedicated one which is basically what QEMU emulates.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 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
>>> 
>>