diff mbox series

[v9,04/16] vpci: add hooks for PCI device assign/de-assign

Message ID 20230829231912.4091958-5-volodymyr_babchuk@epam.com (mailing list archive)
State Superseded
Headers show
Series PCI devices passthrough on Arm, part 3 | expand

Commit Message

Volodymyr Babchuk Aug. 29, 2023, 11:19 p.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

When a PCI device gets assigned/de-assigned we need to
initialize/de-initialize vPCI state for the device.

Also, rename vpci_add_handlers() to vpci_assign_device() and
vpci_remove_device() to vpci_deassign_device() to better reflect role
of the functions.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
Since v9:
- removed previous  vpci_[de]assign_device function and renamed
  existing handlers
- dropped attempts to handle errors in assign_device() function
- do not call vpci_assign_device for dom_io
- use d instead of pdev->domain
- use IS_ENABLED macro
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 | 31 +++++++++++++++++++++++++++----
 xen/drivers/vpci/header.c     |  2 +-
 xen/drivers/vpci/vpci.c       |  6 +++---
 xen/include/xen/vpci.h        | 10 +++++-----
 5 files changed, 40 insertions(+), 13 deletions(-)

Comments

Jan Beulich Sept. 12, 2023, 9:37 a.m. UTC | #1 
On 30.08.2023 01:19, Volodymyr Babchuk wrote:
> @@ -1481,6 +1488,13 @@ 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;
>  
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> +    {
> +        write_lock(&pdev->domain->pci_lock);
> +        vpci_deassign_device(pdev);
> +        write_unlock(&pdev->domain->pci_lock);
> +    }

Why is the DomIO special case ...

> @@ -1506,6 +1520,15 @@ 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;
> +
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
> +    {
> +        write_lock(&d->pci_lock);
> +        rc = vpci_assign_device(pdev);
> +        write_unlock(&d->pci_lock);
> +    }

... relevant only here?

Jan
Volodymyr Babchuk Sept. 12, 2023, 11:41 p.m. UTC | #2 
Hi Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 30.08.2023 01:19, Volodymyr Babchuk wrote:
>> @@ -1481,6 +1488,13 @@ 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;
>>  
>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
>> +    {
>> +        write_lock(&pdev->domain->pci_lock);
>> +        vpci_deassign_device(pdev);
>> +        write_unlock(&pdev->domain->pci_lock);
>> +    }
>
> Why is the DomIO special case ...

vpci_deassign_device() does nothing if vPCI was initialized for a
domain. So it not wrong to call this function even if pdev belongs to dom_io.

>> @@ -1506,6 +1520,15 @@ 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;
>> +
>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
>> +    {
>> +        write_lock(&d->pci_lock);
>> +        rc = vpci_assign_device(pdev);
>> +        write_unlock(&d->pci_lock);
>> +    }
>
> ... relevant only here?
>

There is no sense to initialize vPCI for dom_io.
Jan Beulich Sept. 13, 2023, 5:58 a.m. UTC | #3 
On 13.09.2023 01:41, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 30.08.2023 01:19, Volodymyr Babchuk wrote:
>>> @@ -1481,6 +1488,13 @@ 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;
>>>  
>>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
>>> +    {
>>> +        write_lock(&pdev->domain->pci_lock);
>>> +        vpci_deassign_device(pdev);
>>> +        write_unlock(&pdev->domain->pci_lock);
>>> +    }
>>
>> Why is the DomIO special case ...
> 
> vpci_deassign_device() does nothing if vPCI was initialized for a
> domain. So it not wrong to call this function even if pdev belongs to dom_io.

Well, okay, but then you acquire a lock just to do nothing (apart
from the apparent asymmetry).

>>> @@ -1506,6 +1520,15 @@ 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;
>>> +
>>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
>>> +    {
>>> +        write_lock(&d->pci_lock);
>>> +        rc = vpci_assign_device(pdev);
>>> +        write_unlock(&d->pci_lock);
>>> +    }
>>
>> ... relevant only here?
>>
> 
> There is no sense to initialize vPCI for dom_io.

Of course.

Jan
Volodymyr Babchuk Sept. 13, 2023, 11:53 p.m. UTC | #4 
Hi,

Jan Beulich <jbeulich@suse.com> writes:

> On 13.09.2023 01:41, Volodymyr Babchuk wrote:
>> Jan Beulich <jbeulich@suse.com> writes:
>>> On 30.08.2023 01:19, Volodymyr Babchuk wrote:
>>>> @@ -1481,6 +1488,13 @@ 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;
>>>>  
>>>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
>>>> +    {
>>>> +        write_lock(&pdev->domain->pci_lock);
>>>> +        vpci_deassign_device(pdev);
>>>> +        write_unlock(&pdev->domain->pci_lock);
>>>> +    }
>>>
>>> Why is the DomIO special case ...
>> 
>> vpci_deassign_device() does nothing if vPCI was initialized for a
>> domain. So it not wrong to call this function even if pdev belongs to dom_io.
>
> Well, okay, but then you acquire a lock just to do nothing (apart
> from the apparent asymmetry).

Yes, I agree. I'll add the same check as below. Thanks for the review.
Roger Pau Monné Sept. 20, 2023, 8:39 a.m. UTC | #5 
On Tue, Aug 29, 2023 at 11:19:43PM +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> When a PCI device gets assigned/de-assigned we need to
> initialize/de-initialize vPCI state for the device.
> 
> Also, rename vpci_add_handlers() to vpci_assign_device() and
> vpci_remove_device() to vpci_deassign_device() to better reflect role
> of the functions.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> Since v9:
> - removed previous  vpci_[de]assign_device function and renamed
>   existing handlers
> - dropped attempts to handle errors in assign_device() function
> - do not call vpci_assign_device for dom_io
> - use d instead of pdev->domain
> - use IS_ENABLED macro
> 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 | 31 +++++++++++++++++++++++++++----
>  xen/drivers/vpci/header.c     |  2 +-
>  xen/drivers/vpci/vpci.c       |  6 +++---
>  xen/include/xen/vpci.h        | 10 +++++-----
>  5 files changed, 40 insertions(+), 13 deletions(-)
> 
> 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 4f18293900..64281f2d5e 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -757,7 +757,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>           * For devices not discovered by Xen during boot, add vPCI handlers
>           * when Dom0 first informs Xen about such devices.
>           */
> -        ret = vpci_add_handlers(pdev);
> +        ret = vpci_assign_device(pdev);
>          if ( ret )
>          {
>              printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
> @@ -771,7 +771,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>          if ( ret )
>          {
>              write_lock(&hardware_domain->pci_lock);
> -            vpci_remove_device(pdev);
> +            vpci_deassign_device(pdev);
>              list_del(&pdev->domain_list);
>              write_unlock(&hardware_domain->pci_lock);
>              pdev->domain = NULL;
> @@ -819,7 +819,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>          if ( pdev->bus == bus && pdev->devfn == devfn )
>          {
> -            vpci_remove_device(pdev);
> +            vpci_deassign_device(pdev);
>              pci_cleanup_msi(pdev);
>              ret = iommu_remove_device(pdev);
>              if ( pdev->domain )
> @@ -877,6 +877,13 @@ static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>              goto out;
>      }
>  
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> +    {
> +        write_lock(&d->pci_lock);
> +        vpci_deassign_device(pdev);
> +        write_unlock(&d->pci_lock);
> +    }

I'm confused by this one, shouldn't the code rely on has_vpci()
instead?  (which is already checked for in vpci_deassign_device()).

If you have a system without CONFIG_HAS_VPCI_GUEST_SUPPORT but vPCI is
used by dom0 you likely still need the hooks in {,de}assign_device()
so that vPCI status is properly handled for dom0 as the devices get
deassigned to dom0 and assigned to a guest? (and maybe moved back to
dom0 at a later point).

> +
>      devfn = pdev->devfn;
>      ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
>                       pci_to_dev(pdev));
> @@ -1147,7 +1154,7 @@ static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
>                PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn) );
>  
>      write_lock(&ctxt->d->pci_lock);
> -    err = vpci_add_handlers(pdev);
> +    err = vpci_assign_device(pdev);
>      write_unlock(&ctxt->d->pci_lock);
>      if ( err )
>          printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
> @@ -1481,6 +1488,13 @@ 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;
>  
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> +    {
> +        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;
> @@ -1506,6 +1520,15 @@ 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;

rc can't be != 0 here, as the increment statement in the for loop
above will zero rc at each iteration.

> +
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
> +    {
> +        write_lock(&d->pci_lock);
> +        rc = vpci_assign_device(pdev);
> +        write_unlock(&d->pci_lock);
> +    }

Why do you need the extra d != dom_io check here?  has_vpci() will
fail for dom_io, no need to special case it here.

Thanks, Roger.
Roger Pau Monné Sept. 20, 2023, 8:41 a.m. UTC | #6 
On Wed, Sep 13, 2023 at 11:53:56PM +0000, Volodymyr Babchuk wrote:
> 
> Hi,
> 
> Jan Beulich <jbeulich@suse.com> writes:
> 
> > On 13.09.2023 01:41, Volodymyr Babchuk wrote:
> >> Jan Beulich <jbeulich@suse.com> writes:
> >>> On 30.08.2023 01:19, Volodymyr Babchuk wrote:
> >>>> @@ -1481,6 +1488,13 @@ 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;
> >>>>  
> >>>> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
> >>>> +    {
> >>>> +        write_lock(&pdev->domain->pci_lock);
> >>>> +        vpci_deassign_device(pdev);
> >>>> +        write_unlock(&pdev->domain->pci_lock);
> >>>> +    }
> >>>
> >>> Why is the DomIO special case ...
> >> 
> >> vpci_deassign_device() does nothing if vPCI was initialized for a
> >> domain. So it not wrong to call this function even if pdev belongs to dom_io.
> >
> > Well, okay, but then you acquire a lock just to do nothing (apart
> > from the apparent asymmetry).
> 
> Yes, I agree. I'll add the same check as below. Thanks for the review.

Hm, no, I would rather rely on the has_vpci check inside of
vpci_{,de}assign_device() than open coding dom_io checks elsewhere.

This is not a hot path, the extra lock taking is likely negligible
compared to the cost of assign operations.

Thanks, Roger.
diff mbox series

Patch

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 4f18293900..64281f2d5e 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -757,7 +757,7 @@  int pci_add_device(u16 seg, u8 bus, u8 devfn,
          * For devices not discovered by Xen during boot, add vPCI handlers
          * when Dom0 first informs Xen about such devices.
          */
-        ret = vpci_add_handlers(pdev);
+        ret = vpci_assign_device(pdev);
         if ( ret )
         {
             printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
@@ -771,7 +771,7 @@  int pci_add_device(u16 seg, u8 bus, u8 devfn,
         if ( ret )
         {
             write_lock(&hardware_domain->pci_lock);
-            vpci_remove_device(pdev);
+            vpci_deassign_device(pdev);
             list_del(&pdev->domain_list);
             write_unlock(&hardware_domain->pci_lock);
             pdev->domain = NULL;
@@ -819,7 +819,7 @@  int pci_remove_device(u16 seg, u8 bus, u8 devfn)
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
         {
-            vpci_remove_device(pdev);
+            vpci_deassign_device(pdev);
             pci_cleanup_msi(pdev);
             ret = iommu_remove_device(pdev);
             if ( pdev->domain )
@@ -877,6 +877,13 @@  static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
             goto out;
     }
 
+    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
+    {
+        write_lock(&d->pci_lock);
+        vpci_deassign_device(pdev);
+        write_unlock(&d->pci_lock);
+    }
+
     devfn = pdev->devfn;
     ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
                      pci_to_dev(pdev));
@@ -1147,7 +1154,7 @@  static void __hwdom_init setup_one_hwdom_device(const struct setup_hwdom *ctxt,
               PCI_SLOT(devfn) == PCI_SLOT(pdev->devfn) );
 
     write_lock(&ctxt->d->pci_lock);
-    err = vpci_add_handlers(pdev);
+    err = vpci_assign_device(pdev);
     write_unlock(&ctxt->d->pci_lock);
     if ( err )
         printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
@@ -1481,6 +1488,13 @@  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;
 
+    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
+    {
+        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;
@@ -1506,6 +1520,15 @@  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;
+
+    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) && d != dom_io)
+    {
+        write_lock(&d->pci_lock);
+        rc = vpci_assign_device(pdev);
+        write_unlock(&d->pci_lock);
+    }
 
  done:
     if ( rc )
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 177a6b57a5..3b797df82f 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -190,7 +190,7 @@  bool vpci_process_pending(struct vcpu *v)
              * killed in order to avoid leaking stale p2m mappings on
              * failure.
              */
-            vpci_remove_device(v->vpci.pdev);
+            vpci_deassign_device(v->vpci.pdev);
         write_unlock(&v->domain->pci_lock);
     }
 
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cb45904114..135d390218 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -36,7 +36,7 @@  extern vpci_register_init_t *const __start_vpci_array[];
 extern vpci_register_init_t *const __end_vpci_array[];
 #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
 
-void vpci_remove_device(struct pci_dev *pdev)
+void vpci_deassign_device(struct pci_dev *pdev)
 {
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
@@ -69,7 +69,7 @@  void vpci_remove_device(struct pci_dev *pdev)
     pdev->vpci = NULL;
 }
 
-int vpci_add_handlers(struct pci_dev *pdev)
+int vpci_assign_device(struct pci_dev *pdev)
 {
     unsigned int i;
     const unsigned long *ro_map;
@@ -103,7 +103,7 @@  int vpci_add_handlers(struct pci_dev *pdev)
     }
 
     if ( rc )
-        vpci_remove_device(pdev);
+        vpci_deassign_device(pdev);
 
     return rc;
 }
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 0b8a2a3c74..2a0ae34500 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -25,11 +25,11 @@  typedef int vpci_register_init_t(struct pci_dev *dev);
   static vpci_register_init_t *const x##_entry  \
                __used_section(".data.vpci." p) = x
 
-/* Add vPCI handlers to device. */
-int __must_check vpci_add_handlers(struct pci_dev *dev);
+/* Assign vPCI to device by adding handlers to device. */
+int __must_check vpci_assign_device(struct pci_dev *dev);
 
 /* Remove all handlers and free vpci related structures. */
-void vpci_remove_device(struct pci_dev *pdev);
+void vpci_deassign_device(struct pci_dev *pdev);
 
 /* Add/remove a register handler. */
 int __must_check vpci_add_register(struct vpci *vpci,
@@ -235,12 +235,12 @@  bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 #else /* !CONFIG_HAS_VPCI */
 struct vpci_vcpu {};
 
-static inline int vpci_add_handlers(struct pci_dev *pdev)
+static inline int vpci_assign_device(struct pci_dev *pdev)
 {
     return 0;
 }
 
-static inline void vpci_remove_device(struct pci_dev *pdev) { }
+static inline void vpci_deassign_device(struct pci_dev *pdev) { }
 
 static inline void vpci_dump_msi(void) { }