diff mbox series

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

Message ID 20211125110251.2877218-6-andr2000@gmail.com (mailing list archive)
State New, archived
Headers show
Series PCI devices passthrough on Arm, part 3 | expand

Commit Message

Oleksandr Andrushchenko Nov. 25, 2021, 11:02 a.m. UTC
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 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(-)

Comments

Roger Pau Monné Jan. 12, 2022, 12:12 p.m. UTC | #1
On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko 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 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.

>  {
> -    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.

> +        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.

Thanks, Roger.
Roger Pau Monné Jan. 13, 2022, 11:40 a.m. UTC | #2
On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> +#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;
> +
> +    spin_lock(&pdev->vpci_lock);
> +    rc = run_vpci_init(pdev);

Following my comment below, this will likely need to call
vpci_add_handlers in order to allocate the pdev->vpci field.

It's not OK to carry the contents of pdev->vpci across domain
assignations, as the device should be reset, and thus the content of
pdev->vpci would be stale.

> +    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)
> +{
> +    /* It only makes sense to de-assign from hwdom or guest domain. */
> +    if ( is_system_domain(d) || !has_vpci(d) )
> +        return 0;
> +
> +    spin_lock(&pdev->vpci_lock);
> +    vpci_remove_device_handlers_locked(pdev);

You need to free the pdev->vpci structure on deassign. I would expect
the device to be reset on deassign, so keeping the pdev->vpci contents
would be wrong.

Thanks, Roger.
Oleksandr Andrushchenko Jan. 31, 2022, 8:43 a.m. UTC | #3
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@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 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
Oleksandr Andrushchenko Jan. 31, 2022, 8:45 a.m. UTC | #4
Hi, Roger!

On 13.01.22 13:40, Roger Pau Monné wrote:
> On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> +#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;
>> +
>> +    spin_lock(&pdev->vpci_lock);
>> +    rc = run_vpci_init(pdev);
> Following my comment below, this will likely need to call
> vpci_add_handlers in order to allocate the pdev->vpci field.
>
> It's not OK to carry the contents of pdev->vpci across domain
> assignations, as the device should be reset, and thus the content of
> pdev->vpci would be stale.
>
>> +    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)
>> +{
>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
>> +    if ( is_system_domain(d) || !has_vpci(d) )
>> +        return 0;
>> +
>> +    spin_lock(&pdev->vpci_lock);
>> +    vpci_remove_device_handlers_locked(pdev);
> You need to free the pdev->vpci structure on deassign. I would expect
> the device to be reset on deassign, so keeping the pdev->vpci contents
> would be wrong.
Sure, I will re-allocate pdev->vpci then
>
> Thanks, Roger.
Thank you,
Oleksandr
Oleksandr Andrushchenko Feb. 1, 2022, 8:56 a.m. UTC | #5
Hi, Roger!

On 31.01.22 10:45, Oleksandr Andrushchenko wrote:
> Hi, Roger!
>
> On 13.01.22 13:40, Roger Pau Monné wrote:
>> On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>> +#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;
>>> +
>>> +    spin_lock(&pdev->vpci_lock);
>>> +    rc = run_vpci_init(pdev);
>> Following my comment below, this will likely need to call
>> vpci_add_handlers in order to allocate the pdev->vpci field.
>>
>> It's not OK to carry the contents of pdev->vpci across domain
>> assignations, as the device should be reset, and thus the content of
>> pdev->vpci would be stale.
>>
>>> +    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)
>>> +{
>>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
>>> +    if ( is_system_domain(d) || !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    spin_lock(&pdev->vpci_lock);
>>> +    vpci_remove_device_handlers_locked(pdev);
>> You need to free the pdev->vpci structure on deassign. I would expect
>> the device to be reset on deassign, so keeping the pdev->vpci contents
>> would be wrong.
> Sure, I will re-allocate pdev->vpci then
After thinking a bit more on this I have realized that we cannot free
pdev->vpci on de-assign. The reason for that is the fact that vpci
structure contains vital data which is collected and managed at different
stages: for example, BAR types are collected while we run for the
hardware domain and in init_bars we collect the types of the BARS etc.
This is then used while assigning device to construct guest's representation
of the device. Freeing vpci will lead to that data is lost and the required
data is not populated into vpci.
So, it is no possible to free vpci structure and I am about to leave the
approach as it is.
>> Thanks, Roger.
> Thank you,
> Oleksandr
Thank you,
Oleksandr
Roger Pau Monné Feb. 1, 2022, 10:23 a.m. UTC | #6
On Tue, Feb 01, 2022 at 08:56:49AM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 31.01.22 10:45, Oleksandr Andrushchenko wrote:
> > Hi, Roger!
> >
> > On 13.01.22 13:40, Roger Pau Monné wrote:
> >> On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote:
> >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>> +#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;
> >>> +
> >>> +    spin_lock(&pdev->vpci_lock);
> >>> +    rc = run_vpci_init(pdev);
> >> Following my comment below, this will likely need to call
> >> vpci_add_handlers in order to allocate the pdev->vpci field.
> >>
> >> It's not OK to carry the contents of pdev->vpci across domain
> >> assignations, as the device should be reset, and thus the content of
> >> pdev->vpci would be stale.
> >>
> >>> +    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)
> >>> +{
> >>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
> >>> +    if ( is_system_domain(d) || !has_vpci(d) )
> >>> +        return 0;
> >>> +
> >>> +    spin_lock(&pdev->vpci_lock);
> >>> +    vpci_remove_device_handlers_locked(pdev);
> >> You need to free the pdev->vpci structure on deassign. I would expect
> >> the device to be reset on deassign, so keeping the pdev->vpci contents
> >> would be wrong.
> > Sure, I will re-allocate pdev->vpci then
> After thinking a bit more on this I have realized that we cannot free
> pdev->vpci on de-assign. The reason for that is the fact that vpci
> structure contains vital data which is collected and managed at different
> stages: for example, BAR types are collected while we run for the
> hardware domain and in init_bars we collect the types of the BARS etc.
> This is then used while assigning device to construct guest's representation
> of the device. Freeing vpci will lead to that data is lost and the required
> data is not populated into vpci.
> So, it is no possible to free vpci structure and I am about to leave the
> approach as it is.

We discussed this on IRC, and we have agreed that it's possible to
free pdev->vpci on deassign since in any case we need to call
init_bars (and other capability init functions) when the device is
assigned to setup the register traps and fetch the required
information in order to fill pdev->vpci.

Roger.
diff mbox series

Patch

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;
+
     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)
 {
-    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) )
+        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)
+{
+    /* It only makes sense to de-assign from hwdom or guest domain. */
+    if ( is_system_domain(d) || !has_vpci(d) )
+        return 0;
+
+    spin_lock(&pdev->vpci_lock);
+    vpci_remove_device_handlers_locked(pdev);
+    spin_unlock(&pdev->vpci_lock);
+
+    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 cfff87e5801e..ed127a08a953 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -261,6 +261,22 @@  static inline void vpci_cancel_pending_locked(struct pci_dev *pdev)
 }
 #endif
 
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/* Notify vPCI that device is assigned/de-assigned to/from guest. */
+int vpci_assign_device(struct domain *d, struct pci_dev *pdev);
+int vpci_deassign_device(struct domain *d, struct pci_dev *pdev);
+#else
+static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
+{
+    return 0;
+};
+
+static inline int vpci_deassign_device(struct domain *d, struct pci_dev *pdev)
+{
+    return 0;
+};
+#endif
+
 #endif
 
 /*