[V7,02/11] vpci: add hooks for PCI device assign/de-assign

Commit Message

Oleksandr Tyshchenko July 19, 2022, 5:42 p.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

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 <[e-mail]>
---
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
- re-work according to the new locking scheme (ASSERTs)
- OT: rebased
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 | 11 +++++++++++
 xen/drivers/vpci/vpci.c       | 31 +++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h        | 15 +++++++++++++++
 4 files changed, 61 insertions(+)

Comments

Jan Beulich July 27, 2022, 10:03 a.m. UTC | #1

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> @@ -1558,6 +1560,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
>      struct pci_dev *pdev;
> +    uint8_t old_devfn;

Why "old"? There's nothing "new" here. Perhaps "orig", considering ...

> @@ -1594,6 +1599,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>                            pci_to_dev(pdev), flag)) )
>          goto done;
>  
> +    old_devfn = devfn;
> +
>      for ( ; pdev->phantom_stride; rc = 0 )
>      {
>          devfn += pdev->phantom_stride;

... this updating of devfn is what you mean to deal with? Then again
I see no need for a separate variable in the first place. The input
(seg,bus,devfn) tuple is translated to a pdev, so you can simply ...

> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>                          pci_to_dev(pdev), flag);
>      }
>  
> +    rc = vpci_assign_device(pdev);
> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )

... use pdev->devfn here.

> +        domain_crash(d);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

This log message will want to appear _before_ the domain_crash()
related output, or you will want to add another log message there.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -92,6 +92,37 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    ASSERT(pcidevs_write_locked());
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return 0;
> +
> +    rc = vpci_add_handlers(pdev);
> +    if ( rc )
> +        vpci_deassign_device(pdev);
> +
> +    return rc;
> +}
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +void vpci_deassign_device(struct pci_dev *pdev)
> +{
> +    ASSERT(pcidevs_write_locked());
> +
> +    if ( !has_vpci(pdev->domain) )
> +        return;

There's no need for this check since ...

> +    vpci_remove_device(pdev);

... this function already has it. At which point the question is why
a separate function needs to exist in the first place. To match with
vpci_assign_device(), a simple #define to alias is would be enough.
(This is one of the cases where personally I think a #define is
superior to an inline wrapper.)

Unless, of course, later patches extend this function. If so, the
commit message giving this as justification for the introduction of
(apparent) redundancy would be helpful.

Jan

Oleksandr Tyshchenko July 27, 2022, 2:01 p.m. UTC | #2

On 27.07.22 13:03, Jan Beulich wrote:


Hello Jan


> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> @@ -1558,6 +1560,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>   {
>>       const struct domain_iommu *hd = dom_iommu(d);
>>       struct pci_dev *pdev;
>> +    uint8_t old_devfn;
> Why "old"? There's nothing "new" here. Perhaps "orig", considering ...


ok


>
>> @@ -1594,6 +1599,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>                             pci_to_dev(pdev), flag)) )
>>           goto done;
>>   
>> +    old_devfn = devfn;
>> +
>>       for ( ; pdev->phantom_stride; rc = 0 )
>>       {
>>           devfn += pdev->phantom_stride;
> ... this updating of devfn is what you mean to deal with?

As I understand that was the intention of the author. At least I don't 
see other reasons.



> Then again
> I see no need for a separate variable in the first place. The input
> (seg,bus,devfn) tuple is translated to a pdev, so you can simply ...
>
>> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>                           pci_to_dev(pdev), flag);
>>       }
>>   
>> +    rc = vpci_assign_device(pdev);
>> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )
> ... use pdev->devfn here.


Thanks, good point, will drop old_devfn and use pdev->devfn. I am 
wondering whether the printk after "done:" label (and other possible 
printk-s down the code) should really use pdev->devfn instead of devfn 
in PCI_SBDF construct?



>
>> +        domain_crash(d);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> This log message will want to appear _before_ the domain_crash()
> related output, or you will want to add another log message there.

I will probably add another log message before domain_crash() leaving 
this one as is.

printk(XENLOG_ERR "%pd: %pp was left partially assigned\n", d, 
&PCI_SBDF(seg, bus, devfn));


>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -92,6 +92,37 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>   
>>       return rc;
>>   }
>> +
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    ASSERT(pcidevs_write_locked());
>> +
>> +    if ( !has_vpci(pdev->domain) )
>> +        return 0;
>> +
>> +    rc = vpci_add_handlers(pdev);
>> +    if ( rc )
>> +        vpci_deassign_device(pdev);
>> +
>> +    return rc;
>> +}
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +void vpci_deassign_device(struct pci_dev *pdev)
>> +{
>> +    ASSERT(pcidevs_write_locked());
>> +
>> +    if ( !has_vpci(pdev->domain) )
>> +        return;
> There's no need for this check since ...
>
>> +    vpci_remove_device(pdev);
> ... this function already has it. At which point the question is why
> a separate function needs to exist in the first place. To match with
> vpci_assign_device(), a simple #define to alias is would be enough.
> (This is one of the cases where personally I think a #define is
> superior to an inline wrapper.)


Agree, but ...


>
> Unless, of course, later patches extend this function. If so, the
> commit message giving this as justification for the introduction of
> (apparent) redundancy would be helpful.

... exactly, the later patches extend this function. I will update 
commit description.



>
> Jan

Jan Beulich July 27, 2022, 2:35 p.m. UTC | #3

On 27.07.2022 16:01, Oleksandr wrote:
> On 27.07.22 13:03, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>>                           pci_to_dev(pdev), flag);
>>>       }
>>>   
>>> +    rc = vpci_assign_device(pdev);
>>> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )
>> ... use pdev->devfn here.
> 
> 
> Thanks, good point, will drop old_devfn and use pdev->devfn. I am 
> wondering whether the printk after "done:" label (and other possible 
> printk-s down the code) should really use pdev->devfn instead of devfn 
> in PCI_SBDF construct?

Yes, that's intended: If assigning a phantom function fails, this
should be distinguishable from failure to assign the real device.

Jan

Oleksandr Tyshchenko July 27, 2022, 4:49 p.m. UTC | #4

On 27.07.22 17:35, Jan Beulich wrote:


Hello Jan

> On 27.07.2022 16:01, Oleksandr wrote:
>> On 27.07.22 13:03, Jan Beulich wrote:
>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>>>                            pci_to_dev(pdev), flag);
>>>>        }
>>>>    
>>>> +    rc = vpci_assign_device(pdev);
>>>> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )
>>> ... use pdev->devfn here.
>>
>> Thanks, good point, will drop old_devfn and use pdev->devfn. I am
>> wondering whether the printk after "done:" label (and other possible
>> printk-s down the code) should really use pdev->devfn instead of devfn
>> in PCI_SBDF construct?
> Yes, that's intended: If assigning a phantom function fails, this
> should be distinguishable from failure to assign the real device.


Thank you for the clarification.

Hmm, so before this patch the assigning of phantom functions was the 
last action before the "done:" label. So I will probably need to check 
for "rc" before calling vpci_assign_device().

Something like that:


[snip]

@@ -1602,6 +1606,17 @@ 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;
+
+    devfn = pdev->devfn;
+    rc = vpci_assign_device(pdev);
+    if ( rc && deassign_device(d, seg, bus, devfn) )
+    {
+        printk(XENLOG_ERR "%pd: %pp was left partially assigned\n",
+               d, &PCI_SBDF(seg, bus, devfn));
+        domain_crash(d);
+    }

   done:
      if ( rc )

[snip]


>
> Jan

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 f93922acc8..56af1dbd97 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1019,6 +1019,8 @@  static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
     if ( ret )
         goto out;
 
+    vpci_deassign_device(pdev);
+
     if ( pdev->domain == hardware_domain  )
         pdev->quarantine = false;
 
@@ -1558,6 +1560,7 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
 {
     const struct domain_iommu *hd = dom_iommu(d);
     struct pci_dev *pdev;
+    uint8_t old_devfn;
     int rc = 0;
 
     if ( !is_iommu_enabled(d) )
@@ -1577,6 +1580,8 @@  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;
 
+    vpci_deassign_device(pdev);
+
     rc = pdev_msix_assign(d, pdev);
     if ( rc )
         goto done;
@@ -1594,6 +1599,8 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
                           pci_to_dev(pdev), flag)) )
         goto done;
 
+    old_devfn = devfn;
+
     for ( ; pdev->phantom_stride; rc = 0 )
     {
         devfn += pdev->phantom_stride;
@@ -1603,6 +1610,10 @@  static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
                         pci_to_dev(pdev), flag);
     }
 
+    rc = vpci_assign_device(pdev);
+    if ( rc && deassign_device(d, seg, bus, old_devfn) )
+        domain_crash(d);
+
  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 674c9b347d..d187901422 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -92,6 +92,37 @@  int vpci_add_handlers(struct pci_dev *pdev)
 
     return rc;
 }
+
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/* Notify vPCI that device is assigned to guest. */
+int vpci_assign_device(struct pci_dev *pdev)
+{
+    int rc;
+
+    ASSERT(pcidevs_write_locked());
+
+    if ( !has_vpci(pdev->domain) )
+        return 0;
+
+    rc = vpci_add_handlers(pdev);
+    if ( rc )
+        vpci_deassign_device(pdev);
+
+    return rc;
+}
+
+/* Notify vPCI that device is de-assigned from guest. */
+void vpci_deassign_device(struct pci_dev *pdev)
+{
+    ASSERT(pcidevs_write_locked());
+
+    if ( !has_vpci(pdev->domain) )
+        return;
+
+    vpci_remove_device(pdev);
+}
+#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 7ab39839ff..e5501b9207 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -254,6 +254,21 @@  static inline bool __must_check vpci_process_pending(struct vcpu *v)
 }
 #endif
 
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/* Notify vPCI that device is assigned/de-assigned to/from guest. */
+int vpci_assign_device(struct pci_dev *pdev);
+void vpci_deassign_device(struct pci_dev *pdev);
+#else
+static inline int vpci_assign_device(struct pci_dev *pdev)
+{
+    return 0;
+};
+
+static inline void vpci_deassign_device(struct pci_dev *pdev)
+{
+};
+#endif
+
 #endif
 
 /*