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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Roger,

Roger Pau Monné <roger.pau@citrix.com> writes:

> On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> 
>> 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@epam.com>
>> ---
>> Since v8:
>> - removed vpci_deassign_device
>> Since v6:
>> - do not pass struct domain to vpci_{assign|deassign}_device as
>>   pdev->domain can be used
>> - do not leave the device assigned (pdev->domain == new domain) in case
>>   vpci_assign_device fails: try to de-assign and if this also fails, then
>>   crash the domain
>> Since v5:
>> - do not split code into run_vpci_init
>> - do not check for is_system_domain in vpci_{de}assign_device
>> - do not use vpci_remove_device_handlers_locked and re-allocate
>>   pdev->vpci completely
>> - make vpci_deassign_device void
>> Since v4:
>>  - de-assign vPCI from the previous domain on device assignment
>>  - do not remove handlers in vpci_assign_device as those must not
>>    exist at that point
>> 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 | 21 +++++++++++++++++++++
>>  xen/drivers/vpci/vpci.c       | 18 ++++++++++++++++++
>>  xen/include/xen/vpci.h        | 15 +++++++++++++++
>>  4 files changed, 58 insertions(+)
>> 
>> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
>> index db94393f47..780490cf8e 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 6f8692cd9c..265d359704 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -885,6 +885,10 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>      if ( ret )
>>          goto out;
>>  
>> +    write_lock(&pdev->domain->pci_lock);
>> +    vpci_deassign_device(pdev);
>> +    write_unlock(&pdev->domain->pci_lock);
>> +
>>      if ( pdev->domain == hardware_domain  )
>>          pdev->quarantine = false;
>>  
>> @@ -1484,6 +1488,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>      if ( pdev->broken && d != hardware_domain && d != dom_io )
>>          goto done;
>>  
>> +    write_lock(&pdev->domain->pci_lock);
>> +    vpci_deassign_device(pdev);
>> +    write_unlock(&pdev->domain->pci_lock);
>> +
>>      rc = pdev_msix_assign(d, pdev);
>>      if ( rc )
>>          goto done;
>> @@ -1509,6 +1517,19 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>          rc = iommu_call(hd->platform_ops, assign_device, d, devfn,
>>                          pci_to_dev(pdev), flag);
>>      }
>> +    if ( rc )
>> +        goto done;
>> +
>> +    devfn = pdev->devfn;
>> +    write_lock(&pdev->domain->pci_lock);
>> +    rc = vpci_assign_device(pdev);
>> +    write_unlock(&pdev->domain->pci_lock);
>> +    if ( rc && deassign_device(d, seg, bus, devfn) )
>> +    {
>> +        printk(XENLOG_ERR "%pd: %pp was left partially assigned\n",
>> +               d, &PCI_SBDF(seg, bus, devfn));
>
> &pdev->sbdf?  Then you can get of the devfn usage above.

Yes, thanks.

>> +        domain_crash(d);
>
> This seems like a bit different from the other error paths in the
> function, isn't it fine to return an error and let the caller handle
> the deassign?

I believe, intention was to not leave device in an unknown state: we
failed both assign_device() and deassign_device() call, so what to do
now? But yes, I think you are right and it is better to let caller to
decide what to do next.

>
> Also, if we really need to call deassign_device() we must do so for
> all possible phantom devices, see the above loop around
> iommu_call(..., assing_device, ...);

But deassign_device() has the loop for all phantom devices that already
does all the work. Unless I miss something, of course.

>> +    }
>>  
>>   done:
>>      if ( rc )
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index a6d2cf8660..a97710a806 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -107,6 +107,24 @@ int 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 pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    if ( !has_vpci(pdev->domain) )
>> +        return 0;
>> +
>> +    rc = vpci_add_handlers(pdev);
>> +    if ( rc )
>> +        vpci_deassign_device(pdev);
>
> Why do you need this handler, vpci_add_handlers() when failing will
> already call vpci_remove_device(), which is what
> vpci_deassign_device() does.
>
>> +
>> +    return rc;
>> +}
>> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>> +
>>  #endif /* __XEN__ */
>>  
>>  static int vpci_register_cmp(const struct vpci_register *r1,
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 0b8a2a3c74..44296623e1 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -264,6 +264,21 @@ static inline bool __must_check vpci_process_pending(struct vcpu *v)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Notify vPCI that device is assigned/de-assigned to/from guest. */
>> +int vpci_assign_device(struct pci_dev *pdev);
>> +#define vpci_deassign_device vpci_remove_device
>> +#else
>> +static inline int vpci_assign_device(struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +};
>> +
>> +static inline void vpci_deassign_device(struct pci_dev *pdev)
>> +{
>> +};
>> +#endif
>
> I don't think there's much point in introducing new functions, see
> above.  I'm fine if the current ones want to be renamed to
> vpci_{,de}assign_device(), but adding defines like the above just
> makes the code harder to follow.

Good idea, thanks, I'll just rename the original functions.

-- 
WBR, Volodymyr