Re: [PATCH v3 02/11] vpci: Add hooks for PCI device assign/de-assign

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 30.09.21 11:21, Jan Beulich wrote:
> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> When a PCI device gets assigned/de-assigned some work on vPCI side needs
>> to be done for that device. Introduce a pair of hooks so vPCI can handle
>> that.
>>
>> Please note, that in the current design the error path is handled by
>> the toolstack via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,
>> so this is why it is acceptable not to de-assign devices if vPCI's
>> assign fails, e.g. the roll back will be handled on deassign_device when
>> it is called by the toolstack.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>> Since v2:
>> - define CONFIG_HAS_VPCI_GUEST_SUPPORT so dead code is not compiled
>>    for x86
>> Since v1:
>>   - constify struct pci_dev where possible
>>   - do not open code is_system_domain()
>>   - extended the commit message
>> ---
>>   xen/drivers/Kconfig           |  4 ++++
>>   xen/drivers/passthrough/pci.c |  9 +++++++++
> The Cc list does not match these files getting modified. Please make
> sure you Cc maintainers, so they have a chance of noticing that
> changes are being made which they may have an opinion on.
Sure, I will go over the patches and put required Cc: next time
>
>> @@ -1429,6 +1433,11 @@ static int assign_device(struct domain *d, u16 seg, 
>> u8 bus, u8 devfn, u32 flag)
>>           rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
>> flag);
>>       }
>>   
>> +    if ( rc )
>> +        goto done;
>  From all I can tell this is dead code.
Before the change rc was set in the loop. And then we fall through
to the "done" label. I do agree that the way this code is done the
value of that rc will only reflect the last assignment done in the loop,
but with my change I didn't want to change the existing behavior,
thus "if ( rc"
>
>> +    rc = vpci_assign_device(d, pdev);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -86,6 +86,29 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>>   
>>       return rc;
>>   }
>> +
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
>> +{
>> +    /* It only makes sense to assign for hwdom or guest domain. */
> Could you clarify for me in how far this code path is indeed intended
> to be taken by hwdom? Because if it is, I'd like to further understand
> the interaction with setup_hwdom_pci_devices().
setup_hwdom_pci_devices is not used on Arm as we do rely on
Dom0 to perform PCI host initialization and PCI device enumeration.

This is because of the fact that on Arm it is not a trivial task to
initialize a PCI host bridge in Xen, e.g. you need to properly initialize
power domains, clocks, quirks etc. for different SoCs.
All these make the task too complex and it was decided that at the
moment we do not want to bring PCI device drivers in Xen for that.
It was also decided that we expect Dom0 to take care of initialization
and enumeration.
Some day, when firmware can do PCI initialization for us and then we
can easily access ECAM, this will change. Then setup_hwdom_pci_devices
will be used on Arm as well.

Thus, we need to take care that Xen knows about the discovered
PCI devices via assign_device etc.
>
>> +    if ( is_system_domain(d) || !has_vpci(d) )
>> +        return 0;
>> +
>> +    return 0;
>> +}
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
>> +{
>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
>> +    if ( is_system_domain(d) || !has_vpci(d) )
>> +        return 0;
>> +
>> +    return 0;
>> +}
>> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> At this point of the series #ifdef is the less preferable variant of
> arranging for dead code to get compiled out.
What is that other preferable way then?
>   I expect later patches
> will change that?
No, it is going to be this way all the time
>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -242,6 +242,26 @@ static inline bool vpci_process_pending(struct vcpu *v)
>>   }
>>   #endif
>>   
>> +#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_HAS_VPCI_GUEST_SUPPORT)
>> +/* Notify vPCI that device is assigned/de-assigned to/from guest. */
>> +int __must_check vpci_assign_device(struct domain *d,
>> +                                    const struct pci_dev *dev);
>> +int __must_check vpci_deassign_device(struct domain *d,
>> +                                      const struct pci_dev *dev);
>> +#else
>> +static inline int vpci_assign_device(struct domain *d,
>> +                                     const struct pci_dev *dev)
>> +{
>> +    return 0;
>> +};
>> +
>> +static inline int vpci_deassign_device(struct domain *d,
>> +                                       const struct pci_dev *dev)
>> +{
>> +    return 0;
>> +};
>> +#endif
> Please put the __must_check also on the stubs, or else people only
> build-testing x86 may not notice that they might be introducing
> build failures on Arm. Then again I'm not sure whether in this case
> the attributes are necessary in the first place.
I will remove __must_check
>
> Jan
>