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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 13.10.21 14:29, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:14AM +0300, 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.
> It's kind of hard to see what would need to be rolled back, as the
> functions are just dummies right now that don't perform any actions.
>
> I don't think the toolstack should be the one to deal with the
> fallout, as it could leave Xen in a broken state. The current commit
> message doesn't provide any information about why it has been designed
> this way.
Yes, we discussed in other patches that we need not rely on the
toolstack and perform cleanup ourselves, so this the code from the
future to illustrate the roll-back:

int vpci_assign_device(struct domain *d, const 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;

     rc = vpci_bar_add_handlers(d, pdev);
     if ( rc )
         goto fail;

     rc = vpci_add_virtual_device(d, pdev);
     if ( rc )
     {
         gdprintk(XENLOG_ERR,
                  "%pp: failed to add virtual device for %pd: %d\n",
                  &pdev->sbdf, d, rc);
         goto fail;
     }

     return 0;

fail:
     /*
      * We are trying to clean up as much as we can, so ignore the return
      * value of vpci_deassign_device below, so we can return the
      * error which caused the failure.
      */
     vpci_deassign_device(d, pdev);
     return rc;
}

So, I will drop the part about the toolstack and cleanup from the commit message
>
>> 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 +++++++++
>>   xen/drivers/vpci/vpci.c       | 23 +++++++++++++++++++++++
>>   xen/include/xen/vpci.h        | 20 ++++++++++++++++++++
>>   4 files changed, 56 insertions(+)
>>
>> 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
> I would assume this is to go away once the work is finished? I don't
> think it makes sense to split vPCI code between domU/dom0 on a build
> time basis.
>
>> +
>>   endmenu
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 9f804a50e780..805ab86ed555 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -870,6 +870,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;
>>   
>> @@ -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;
>> +
>> +    rc = vpci_assign_device(d, pdev);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 1666402d55b8..0fe86cb30d23 100644
>> --- 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. */
>> +    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 */
>> +
>>   #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 2e910d0b1f90..ecc08f2c0f65 100644
>> --- 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)
> You don't need to check for CONFIG_HAS_VPCI, as
> CONFIG_HAS_VPCI_GUEST_SUPPORT already depends on CONFIG_HAS_VPCI being
> set.
>
Will fix
>> +/* 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;
>> +};
> You need the __must_check attributes here also to match the prototypes
> above.
Yes, it was already discussed and I will remove __must_check.
> Thanks, Roger.
Thank you,
Oleksandr