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);
+ }
+
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) { }
--
2.41.0On 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.
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
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.
--
WBR, Volodymyr
© 2016 - 2026 Red Hat, Inc.