Re: [PATCH v5 05/14] vpci: add hooks for PCI device assign/de-assign

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 12.01.22 14:12, Roger Pau Monné wrote:
> On Thu, Nov 25, 2021 at 01:02:42PM +0200, 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 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 | 10 ++++++
>>   xen/drivers/vpci/vpci.c       | 61 +++++++++++++++++++++++++++++------
>>   xen/include/xen/vpci.h        | 16 +++++++++
>>   4 files changed, 82 insertions(+), 9 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 286808b25e65..d9ef91571adf 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -874,6 +874,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;
> Following my comment below, this won't be allowed to fail.
>
>> +
>>       if ( pdev->domain == hardware_domain  )
>>           pdev->quarantine = false;
>>   
>> @@ -1429,6 +1433,10 @@ static int assign_device(struct domain *d, u16 seg, 
>> u8 bus, u8 devfn, u32 flag)
>>       ASSERT(pdev && (pdev->domain == hardware_domain ||
>>                       pdev->domain == dom_io));
>>   
>> +    rc = vpci_deassign_device(pdev->domain, pdev);
>> +    if ( rc )
>> +        goto done;
>> +
>>       rc = pdev_msix_assign(d, pdev);
>>       if ( rc )
>>           goto done;
>> @@ -1446,6 +1454,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",
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 37103e207635..a9e9e8ec438c 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -74,12 +74,26 @@ void vpci_remove_device(struct pci_dev *pdev)
>>       spin_unlock(&pdev->vpci_lock);
>>   }
>>   
>> -int vpci_add_handlers(struct pci_dev *pdev)
>> +static int run_vpci_init(struct pci_dev *pdev)
> Just using add_handlers as function name would be clearer IMO.
Ok, will change
>
>>   {
>> -    struct vpci *vpci;
>>       unsigned int i;
>>       int rc = 0;
>>   
>> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> +    {
>> +        rc = __start_vpci_array[i](pdev);
>> +        if ( rc )
>> +            break;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +int vpci_add_handlers(struct pci_dev *pdev)
>> +{
>> +    struct vpci *vpci;
>> +    int rc;
>> +
>>       if ( !has_vpci(pdev->domain) )
>>           return 0;
>>   
>> @@ -94,19 +108,48 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>       pdev->vpci = vpci;
>>       INIT_LIST_HEAD(&pdev->vpci->handlers);
>>   
>> -    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> -    {
>> -        rc = __start_vpci_array[i](pdev);
>> -        if ( rc )
>> -            break;
>> -    }
>> -
>> +    rc = run_vpci_init(pdev);
>>       if ( rc )
>>           vpci_remove_device_locked(pdev);
>>       spin_unlock(&pdev->vpci_lock);
>>   
>>       return rc;
>>   }
>> +
>> +#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) )
> Do you really need the is_system_domain check? System domains
> shouldn't have the VPCI flag set anyway, so should fail the has_vpci
> test.
No, it seems we do not need this check: will remove
>
>> +        return 0;
>> +
>> +    spin_lock(&pdev->vpci_lock);
>> +    rc = run_vpci_init(pdev);
>> +    spin_unlock(&pdev->vpci_lock);
>> +    if ( rc )
>> +        vpci_deassign_device(d, pdev);
>> +
>> +    return rc;
>> +}
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev)
> There's no need to return any value from this function AFAICT. It
> should have void return type.
Makes sense, I will s/int/void
> Thanks, Roger.
Thank you,
Oleksandr