Re: [PATCH v4 04/11] vpci: add hooks for PCI device assign/de-assign

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 15.11.21 19:06, Jan Beulich wrote:
> On 05.11.2021 07:56, 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.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>> Since v3:
>>   - remove toolstack roll-back description from the commit message
>>     as error are to be handled with proper cleanup in Xen itself
>>   - remove __must_check
>>   - remove redundant rc check while assigning devices
>>   - fix redundant CONFIG_HAS_VPCI check for CONFIG_HAS_VPCI_GUEST_SUPPORT
>>   - use REGISTER_VPCI_INIT machinery to run required steps on device
>>     init/assign: add run_vpci_init helper
>> 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 |  6 ++++
>>   xen/drivers/vpci/vpci.c       | 57 ++++++++++++++++++++++++++++++-----
>>   xen/include/xen/vpci.h        | 16 ++++++++++
>>   4 files changed, 75 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
>> index db94393f47a6..780490cf8e39 100644
>> --- a/xen/drivers/Kconfig
>> +++ b/xen/drivers/Kconfig
>> @@ -15,4 +15,8 @@ source "drivers/video/Kconfig"
>>   config HAS_VPCI
>>      bool
>>   
>> +config HAS_VPCI_GUEST_SUPPORT
>> +    bool
>> +    depends on HAS_VPCI
>> +
>>   endmenu
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index a9d31293ac09..529a4f50aa80 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -873,6 +873,10 @@ static int deassign_device(struct domain *d, uint16_t 
>> seg, uint8_t bus,
>>       if ( ret )
>>           goto out;
>>   
>> +    ret = vpci_deassign_device(d, pdev);
>> +    if ( ret )
>> +        goto out;
>> +
>>       if ( pdev->domain == hardware_domain  )
>>           pdev->quarantine = false;
>>   
>> @@ -1445,6 +1449,8 @@ 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);
>>       }
>>   
>> +    rc = vpci_assign_device(d, pdev);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> Don't you need to call vpci_deassign_device() higher up in this
> function for the prior owner of the device?
Yes, this does make more sense, e.g. vpci_deassign_device(pdev->domain, pdev)
before assigning pdev to an IOMMU which updates pdev->domain and then...
>
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    /* It only makes sense to assign for hwdom or guest domain. */
>> +    if ( is_system_domain(d) || !has_vpci(d) )
>> +        return 0;
>> +
>> +    vpci_remove_device_handlers(pdev);
> This removes handlers in d, not in the prior owner domain. Is this
> really intended? And if it really is meant to remove the new domain's
> handlers (of which there ought to be none) - why is this necessary?
the above won't be needed
>
> Jan
>
Thank you,
Oleksandr