On 15.11.21 19:06, Jan Beulich wrote:
> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@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_andrushche...@epam.com>
>> ---
>> 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 | 6 ++++
>> xen/drivers/vpci/vpci.c | 57 ++++++++++++++++++++++++++++++-----
>> xen/include/xen/vpci.h | 16 ++++++++++
>> 4 files changed, 75 insertions(+), 8 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 a9d31293ac09..529a4f50aa80 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -873,6 +873,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;
>>
>> @@ -1445,6 +1449,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",
> Don't you need to call vpci_deassign_device() higher up in this
> function for the prior owner of the device?
Yes, this does make more sense, e.g. vpci_deassign_device(pdev->domain, pdev)
before assigning pdev to an IOMMU which updates pdev->domain and then...
>
>> +#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;
>> +
>> + vpci_remove_device_handlers(pdev);
> This removes handlers in d, not in the prior owner domain. Is this
> really intended? And if it really is meant to remove the new domain's
> handlers (of which there ought to be none) - why is this necessary?
the above won't be needed
>
> Jan
>
Thank you,
Oleksandr
