[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Date: Wed, 19 Jun 2024 03:39:55 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=UHwP6RvVoC1Qe2a9MsmnLxvwXeRhc2OA9H5xZ9TTw9c=; b=LiNMiaXfbERub4aXsDsCtwaygadXK3x6rxw8ha39LetnBmvKNKsEHezE4Pre5Ys+BjWN4/bSDQfrATzhAf2g6N/C9ZWYjK7/6EL7IHwoHcOSXqov/JHWY7m2xuWCem5kY/z+aWETo5GDSIh4VneRtmo5pSz6NAPTIvHRuU4elzbIpC3obeTPjwNt2bKQnQfBvbT2UY+3fU8KVQT7bhcmtBK1H2FN2UEM+IRFXa1Zks9dtozAbuBqlA+NuJ7vbYwRgY9zlO2XzWVeyLIYErdL1LcerDnAc4cdUuN5k7LS7/qTnxZBHXxnK2e2ETjCx07LIHQMWCC1CFSxgX3YoyZrww==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PH/4BNwtSh53qtQTA+noW2eYfSU8tMTFPzVy3u2fyHqOJHhC9uZ0T8II3ZTrDGginvILFWHUopaEvqy27z7MxfP7dKyXSqxpfXF8TnhE7sZNtiI4gVyA00Uc7ZIIybIE4tHyBqUCyJZtjwdcX1NpU+2JGG13lppw3HjvlPDyzIVEjMrZnrdU7rogW7JV8uEwpnv2EIB0QIHRxJ9sTdoqMKv2GIOUsMPowAYKWuTaLqPF2BAA3HlnWb3oL1aDmq9MQmt7X4Wk/ZeMehuiykrWJWH56P1+nn20sL3TFaEZs8TqjEH2QWp7eJl2xNZGrjQ6tRRHxJdmEbNc53jghgzu7Q==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Anthony PERARD <anthony@xxxxxxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, "Hildebrand, Stewart" <Stewart.Hildebrand@xxxxxxx>, "Huang, Ray" <Ray.Huang@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Delivery-date: Wed, 19 Jun 2024 03:40:08 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHawJTga4AgQ2zhd0+hWXdwZesfOrHMAOYAgAGQCAD//6IuAIABxSoA
  • Thread-topic: [XEN PATCH v10 1/5] xen/vpci: Clear all vpci status of device

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.

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.