[2/9] vpci: Add hooks for PCI device assign/de-assign

Commit Message

Oleksandr Andrushchenko Sept. 3, 2021, 10:08 a.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]>
---
 xen/drivers/passthrough/pci.c |  9 +++++++++
 xen/drivers/vpci/vpci.c       | 21 +++++++++++++++++++++
 xen/include/xen/vpci.h        | 16 ++++++++++++++++
 3 files changed, 46 insertions(+)

Comments

Jan Beulich Sept. 6, 2021, 1:23 p.m. UTC | #1

On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -864,6 +864,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;
>  
> @@ -1425,6 +1429,11 @@ 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);
>      }
>  
> +    if ( rc )
> +        goto done;
> +
> +    rc = vpci_assign_device(d, pdev);
> +
>   done:
>      if ( rc )
>          printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",

I have to admit that I'm worried by the further lack of unwinding in
case of error: We're not really good at this, I agree, but it would
be quite nice if the problem didn't get worse. At the very least if
the device was de-assigned from Dom0 and assignment to a DomU failed,
imo you will want to restore Dom0's settings.

Also in the latter case don't you need to additionally call
vpci_deassign_device() for the prior owner of the device?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +/* Notify vPCI that device is assigned to guest. */
> +int vpci_assign_device(struct domain *d, struct pci_dev *dev)
> +{
> +    /* It only makes sense to assign for hwdom or guest domain. */
> +    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )

Please don't open-code is_system_domain(). I also think you want to
flip the two sides of the ||, to avoid evaluating whatever has_vcpi()
expands to for system domains. (Both again below.)

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>  /* Add vPCI handlers to device. */
>  int __must_check vpci_add_handlers(struct pci_dev *dev);
>  
> +/* Notify vPCI that device is assigned to guest. */
> +int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
> +
> +/* Notify vPCI that device is de-assigned from guest. */
> +int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);

Is the expectation that "dev" may get altered? If not, it may want to
become pointer-to-const. (For "d" there might be the need to acquire
locks, so I guess it might not be a god idea to constify that one.)

I also think that one comment ought to be enough for the two functions.

Jan

Oleksandr Andrushchenko Sept. 7, 2021, 8:33 a.m. UTC | #2

Hello, Jan!

On 06.09.21 16:23, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -864,6 +864,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;
>>   
>> @@ -1425,6 +1429,11 @@ 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);
>>       }
>>   
>> +    if ( rc )
>> +        goto done;
>> +
>> +    rc = vpci_assign_device(d, pdev);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> I have to admit that I'm worried by the further lack of unwinding in
> case of error: We're not really good at this, I agree, but it would
> be quite nice if the problem didn't get worse. At the very least if
> the device was de-assigned from Dom0 and assignment to a DomU failed,
> imo you will want to restore Dom0's settings.

In the current design the error path is handled by the toolstack

via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,

so this is why it is "ok" to have the code structured in the

assign_device as it is, e.g. roll back will be handled on deassign_device.

So, it is up to the toolstack to re-assign the device to Dom0 or DomIO(?)

in case of error and we do rely on the toolstack in Xen.

>
> Also in the latter case don't you need to additionally call
> vpci_deassign_device() for the prior owner of the device?

Even if we wanted to help the toolstack with the roll-back in case of an error

this would IMO make things even worth, e.g. we will de-assign for vPCI, but will

leave IOMMU path untouched which would result in some mess.

So, my only guess here is that we should rely on the toolstack completely as

it was before PCI passthrough on Arm.

>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -86,6 +86,27 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>>   
>>       return rc;
>>   }
>> +
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct domain *d, struct pci_dev *dev)
>> +{
>> +    /* It only makes sense to assign for hwdom or guest domain. */
>> +    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
> Please don't open-code is_system_domain(). I also think you want to
> flip the two sides of the ||, to avoid evaluating whatever has_vcpi()
> expands to for system domains. (Both again below.)
Good catch, I missed is_system_domain completely, thank you!
>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -26,6 +26,12 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>>   /* Add vPCI handlers to device. */
>>   int __must_check vpci_add_handlers(struct pci_dev *dev);
>>   
>> +/* Notify vPCI that device is assigned to guest. */
>> +int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);
> Is the expectation that "dev" may get altered? If not, it may want to
> become pointer-to-const. (For "d" there might be the need to acquire
> locks, so I guess it might not be a god idea to constify that one.)
Just checked that and it is indeed possible to constify. Will do
>
> I also think that one comment ought to be enough for the two functions.
Sure
>
> Jan
>
Thank you,

Oleksandr

Jan Beulich Sept. 7, 2021, 8:44 a.m. UTC | #3

On 07.09.2021 10:33, Oleksandr Andrushchenko wrote:
> On 06.09.21 16:23, Jan Beulich wrote:
>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -864,6 +864,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;
>>>   
>>> @@ -1425,6 +1429,11 @@ 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);
>>>       }
>>>   
>>> +    if ( rc )
>>> +        goto done;
>>> +
>>> +    rc = vpci_assign_device(d, pdev);
>>> +
>>>    done:
>>>       if ( rc )
>>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
>> I have to admit that I'm worried by the further lack of unwinding in
>> case of error: We're not really good at this, I agree, but it would
>> be quite nice if the problem didn't get worse. At the very least if
>> the device was de-assigned from Dom0 and assignment to a DomU failed,
>> imo you will want to restore Dom0's settings.
> 
> In the current design the error path is handled by the toolstack
> via XEN_DOMCTL_assign_device/XEN_DOMCTL_deassign_device,
> so this is why it is "ok" to have the code structured in the
> assign_device as it is, e.g. roll back will be handled on deassign_device.
> So, it is up to the toolstack to re-assign the device to Dom0 or DomIO(?)
> in case of error and we do rely on the toolstack in Xen.
> 
>>
>> Also in the latter case don't you need to additionally call
>> vpci_deassign_device() for the prior owner of the device?
> 
> Even if we wanted to help the toolstack with the roll-back in case of an error
> this would IMO make things even worth, e.g. we will de-assign for vPCI, but will
> leave IOMMU path untouched which would result in some mess.
> So, my only guess here is that we should rely on the toolstack completely as
> it was before PCI passthrough on Arm.

Well, okay, but please make this explicit in the description then.

Jan

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 25304dbe9956..deef986acbb4 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -864,6 +864,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;
 
@@ -1425,6 +1429,11 @@  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);
     }
 
+    if ( rc )
+        goto done;
+
+    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 b05530f2a6b0..ee0ad63a3c12 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -86,6 +86,27 @@  int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
 
     return rc;
 }
+
+/* Notify vPCI that device is assigned to guest. */
+int vpci_assign_device(struct domain *d, struct pci_dev *dev)
+{
+    /* It only makes sense to assign for hwdom or guest domain. */
+    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
+        return 0;
+
+    return 0;
+}
+
+/* Notify vPCI that device is de-assigned from guest. */
+int vpci_deassign_device(struct domain *d, struct pci_dev *dev)
+{
+    /* It only makes sense to de-assign from hwdom or guest domain. */
+    if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
+        return 0;
+
+    return 0;
+}
+
 #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 b861f438cc78..e7a1a09ab4c9 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -26,6 +26,12 @@  typedef int vpci_register_init_t(struct pci_dev *dev);
 /* Add vPCI handlers to device. */
 int __must_check vpci_add_handlers(struct pci_dev *dev);
 
+/* Notify vPCI that device is assigned to guest. */
+int __must_check vpci_assign_device(struct domain *d, struct pci_dev *dev);
+
+/* Notify vPCI that device is de-assigned from guest. */
+int __must_check vpci_deassign_device(struct domain *d, struct pci_dev *dev);
+
 /* Remove all handlers and free vpci related structures. */
 void vpci_remove_device(struct pci_dev *pdev);
 /* Remove all handlers for the device given. */
@@ -220,6 +226,16 @@  static inline int vpci_add_handlers(struct pci_dev *pdev)
     return 0;
 }
 
+static inline int vpci_assign_device(struct domain *d, struct pci_dev *dev)
+{
+    return 0;
+};
+
+static inline int vpci_deassign_device(struct domain *d, struct pci_dev *dev)
+{
+    return 0;
+};
+
 static inline void vpci_dump_msi(void) { }
 
 static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,