Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2021 10:39, Bertrand Marquis wrote:
>> On 20 Oct 2021, at 09:34, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 20.10.2021 10:27, Roger Pau Monné wrote:
>>> On Tue, Oct 19, 2021 at 05:08:28PM +0100, Bertrand Marquis wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -756,10 +756,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>     if ( !pdev->domain )
>>>>     {
>>>>         pdev->domain = hardware_domain;
>>>> -#ifdef CONFIG_ARM
>>>>         /*
>>>> -         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
>>>> handler
>>>> -         * when Dom0 inform XEN to add the PCI devices in XEN.
>>>> +         * For devices not discovered by Xen during boot, add vPCI 
>>>> handlers
>>>> +         * when Dom0 first informs Xen about such devices.
>>>>          */
>>>>         ret = vpci_add_handlers(pdev);
>>>>         if ( ret )
>>>
>>> Sorry to be a pain, but I think this placement of the call to
>>> vpci_add_handlers is bogus and should instead be done strictly after
>>> the device has been added to the hardware_domain->pdev_list list.
>>>
>>> Otherwise the loop over domain->pdev_list (for_each_pdev) in
>>> modify_bars won't be able to find the device and hit the assert below
>>> it. That can happen in vpci_add_handlers as it will call init_bars
>>> which in turn will call into modify_bars if memory decoding is enabled
>>> for the device.
>>
>> Oh, good point. And I should have thought of this myself, given that
>> I did hit that ASSERT() recently with a hidden device. FTAOD I think
>> this means that the list addition will need to move up (and then
>> would need undoing on the error path(s)).
> 
> Agree, I just need to make sure that calling iommu_add_device is not
> impacted by this. It is probably not but a confirmation would be good.

Like Roger, I'm unaware of any issue there. It would be odd anyway for
that (or about anything) to have a "is [not] on list" check. And the
set of list iterations is pretty limited iirc.

Jan