Re: [XEN PATCH v10 1/5] xen/vpci: Clear all vpci status of device

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/6/18 16:33, Jan Beulich wrote:
> On 18.06.2024 08:25, Chen, Jiqian wrote:
>> On 2024/6/17 22:17, Jan Beulich wrote:
>>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>>> --- a/xen/drivers/pci/physdev.c
>>>> +++ b/xen/drivers/pci/physdev.c
>>>> @@ -2,11 +2,17 @@
>>>>  #include <xen/guest_access.h>
>>>>  #include <xen/hypercall.h>
>>>>  #include <xen/init.h>
>>>> +#include <xen/vpci.h>
>>>>  
>>>>  #ifndef COMPAT
>>>>  typedef long ret_t;
>>>>  #endif
>>>>  
>>>> +static const struct pci_device_state_reset_method
>>>> +                    pci_device_state_reset_methods[] = {
>>>> +    [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
>>>> +};
>>>
>>> What about the other three DEVICE_RESET_*? In particular ...
>> I don't know how to implement the other three types of reset.
>> This is a design form so that corresponding processing functions can be 
>> added later if necessary. Do I need to set them to NULL pointers in this 
>> array?
> 
> No.
> 
>> Does this form conform to your previous suggestion of using only one 
>> hypercall to handle all types of resets?
> 
> Yes, at least in principle. Question here is: To be on the safe side,
> wouldn't we better reset state for all forms of reset, leaving possible
> relaxation of that for later? At which point the function wouldn't need
> calling indirectly, and instead would be passed the reset type as an
> argument.
If I understood correctly, next version should be?
Use macros to represent different reset types.
Add switch cases in PHYSDEVOP_pci_device_state_reset to handle different reset 
functions.
Add reset_type as a function parameter to vpci_reset_device_state for possible 
future use.

+    case PHYSDEVOP_pci_device_state_reset:
+    {
+        struct pci_device_state_reset dev_reset;
+        struct pci_dev *pdev;
+        pci_sbdf_t sbdf;
+
+        if ( !is_pci_passthrough_enabled() )
+            return -EOPNOTSUPP;
+
+        ret = -EFAULT;
+        if ( copy_from_guest(&dev_reset, arg, 1) != 0 )
+            break;
+
+        sbdf = PCI_SBDF(dev_reset.dev.seg,
+                        dev_reset.dev.bus,
+                        dev_reset.dev.evfn);
+
+        ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
+        if ( ret )
+            break;
+
+        pcidevs_lock();
+        pdev = pci_get_pdev(NULL, sbdf);
+        if ( !pdev )
+        {
+            pcidevs_unlock();
+            ret = -ENODEV;
+            break;
+        }
+
+        write_lock(&pdev->domain->pci_lock);
+        pcidevs_unlock();
+        /* Implement FLR, other reset types may be implemented in future */
+        switch ( dev_reset.reset_type )
+        {
+        case PCI_DEVICE_STATE_RESET_COLD:
+        case PCI_DEVICE_STATE_RESET_WARM:
+        case PCI_DEVICE_STATE_RESET_HOT:
+        case PCI_DEVICE_STATE_RESET_FLR:
+            ret = vpci_reset_device_state(pdev, dev_reset.reset_type);
+            break;
+        }
+        write_unlock(&pdev->domain->pci_lock);
+
+        if ( ret )
+            dprintk(XENLOG_ERR,
+                    "%pp: failed to reset vPCI device state\n", &sbdf);
+        break;
+    }

> 
>>> Also, nit (further up): Opening figure braces for a new scope go onto their
>> OK, will change in next version.
>>> own line. Then again I notice that apparenly _all_ other instances in this
>>> file are doing it the wrong way, too.
>> Do I need to change them in this patch?
> 
> No.
> 
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>>>>  
>>>>      return rc;
>>>>  }
>>>> +
>>>> +int vpci_reset_device_state(struct pci_dev *pdev)
>>>
>>> As a target of an indirect call this needs to be annotated cf_check (both
>>> here and in the declaration, unlike __must_check, which is sufficient to
>>> have on just the declaration).
>> OK, will add cf_check in next version.
> 
> Which may not be necessary if you go the route suggested above.
> 
>>>> --- a/xen/include/xen/pci.h
>>>> +++ b/xen/include/xen/pci.h
>>>> @@ -156,6 +156,22 @@ struct pci_dev {
>>>>      struct vpci *vpci;
>>>>  };
>>>>  
>>>> +struct pci_device_state_reset_method {
>>>> +    int (*reset_fn)(struct pci_dev *pdev);
>>>> +};
>>>> +
>>>> +enum pci_device_state_reset_type {
>>>> +    DEVICE_RESET_FLR,
>>>> +    DEVICE_RESET_COLD,
>>>> +    DEVICE_RESET_WARM,
>>>> +    DEVICE_RESET_HOT,
>>>> +};
>>>> +
>>>> +struct pci_device_state_reset {
>>>> +    struct physdev_pci_device dev;
>>>> +    enum pci_device_state_reset_type reset_type;
>>>> +};
>>>
>>> This is the struct to use as hypercall argument. How can it live outside of
>>> any public header? Also, when moving it there, beware that you should not
>>> use enum-s there. Only handles and fixed-width types are permitted.t
>> Yes, I put them there before, but enum is not permitted.
>> Then, do you have other suggested type to use to distinguish different types 
>> of resets, because enum can't work in the public header?
> 
> Do like we do everywhere else: Use a set of #define-s.
> 
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev 
>>>> *pdev);
>>>>  
>>>>  /* Remove all handlers and free vpci related structures. */
>>>>  void vpci_deassign_device(struct pci_dev *pdev);
>>>> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);
>>>
>>> What's the purpose of this __must_check, when the sole caller is calling
>>> this through a function pointer, which isn't similarly annotated?
>> This is what I added before introducing function pointers, but after 
>> modifying the implementation, it was not taken into account.
>> I will remove __must_check
> 
> Why remove? Is it relevant for the return value to be checked? Or if it
> isn't, why would there be a return value?
> 
> Jan
> 
>> and change to cf_check, according to your above comment.
> 

-- 
Best regards,
Jiqian Chen.