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

Re: [RFC XEN PATCH v2 1/3] xen/vpci: Clear all vpci status of device


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Date: Thu, 30 Nov 2023 06:22:28 +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=3Fs4XBn4beRAUJ2xxC22hpTah8PuIfagUoiNU5WIeHg=; b=mZinRS+Q1/ZPfW7a69IYmFCvndGSxN1uoPoRsNuCWE53h4qaQdsunOVQIjT2jJpYqq/Wd7MlBk8wLxGc16//nNInrHB6DA6aG81LninN0gUUlrf2iLORgQ8+cVffC+OukRC9t8ujZoOAdpuCRGMYjbECWSBe18lbeH+QGv3jAgiSe/IuKVX322v42aVSq9D4xGqzXPxf61SX7iDv+8stOuttXJr45Gw47D+j6ybFojYYYhvJFrFZmHmCfYBVdKF3jllZD0luuq86s4eEmVuxGKBbW/hO/kVGv+bqvUWgeXERKlZvZehZonWBCTzbi9nKAWsGDILcoMkX6M8xBNLy1A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=H+kBsk4A3d2/RRyT49OWS+jaAJIJqSwIdFSSj9DxBVI99euf3u9jgoOwHA1ksqBcxVh4mfmZx0ZnNcfQ2L1uCILQ2G1fRwhkavpkR7zOAia1+qDW2DBEvQbNNAFBvpMeJt9dF5b8ZDoIPWBBjKoO+1+WJkC9iPTw/9NJ4KdpoVd29eMFX1DQ4LPSFL4+Dr3lGEzZ0H1Ow5okj4Ve11rvkyczCAukDbsA+dFll0FOUAvCSSYLITb2fIhLBfhQcaWnFZ5mwntDob438CvGcQXtPSDIXUXfNkz5jWeF1w9W6YabZsznsrKdg/YOh5RY0KhuI01bfD/0oNQCHBYnhEi4GQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Jan Beulich <jbeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Hildebrand, Stewart" <Stewart.Hildebrand@xxxxxxx>, "Deucher, Alexander" <Alexander.Deucher@xxxxxxx>, "Ragiadakou, Xenia" <Xenia.Ragiadakou@xxxxxxx>, "Stabellini, Stefano" <stefano.stabellini@xxxxxxx>, "Huang, Ray" <Ray.Huang@xxxxxxx>, "Huang, Honglei1" <Honglei1.Huang@xxxxxxx>, "Zhang, Julia" <Julia.Zhang@xxxxxxx>, "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Delivery-date: Thu, 30 Nov 2023 06:22:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHaHsLfq+dfGUvRWUqPIEH3O9fXrbCPyyeAgAMgFYA=
  • Thread-topic: [RFC XEN PATCH v2 1/3] xen/vpci: Clear all vpci status of device

Hi Roger and Daniel P. Smith,

On 2023/11/28 22:08, Roger Pau Monné wrote:
> On Fri, Nov 24, 2023 at 06:41:34PM +0800, Jiqian Chen wrote:
>> When a device has been reset on dom0 side, the vpci on Xen
>> side won't get notification, so the cached state in vpci is
>> all out of date compare with the real device state.
>> To solve that problem, this patch add new hypercall to clear
>> all vpci device state. And when reset device happens on dom0
>> side, dom0 can call this hypercall to notify vpci.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>> ---
>>  xen/arch/x86/hvm/hypercall.c  |  1 +
>>  xen/drivers/passthrough/pci.c | 21 +++++++++++++++++++++
>>  xen/drivers/pci/physdev.c     | 14 ++++++++++++++
>>  xen/drivers/vpci/vpci.c       |  9 +++++++++
>>  xen/include/public/physdev.h  |  2 ++
>>  xen/include/xen/pci.h         |  1 +
>>  xen/include/xen/vpci.h        |  6 ++++++
>>  7 files changed, 54 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
>> index eeb73e1aa5..6ad5b4d5f1 100644
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -84,6 +84,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
>> arg)
>>      case PHYSDEVOP_pci_mmcfg_reserved:
>>      case PHYSDEVOP_pci_device_add:
>>      case PHYSDEVOP_pci_device_remove:
>> +    case PHYSDEVOP_pci_device_state_reset:
>>      case PHYSDEVOP_dbgp_op:
>>          if ( !is_hardware_domain(currd) )
>>              return -ENOSYS;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 04d00c7c37..f871715585 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -824,6 +824,27 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>      return ret;
>>  }
>>  
>> +int pci_reset_device_state(u16 seg, u8 bus, u8 devfn)
> 
> You could use pci_sbdf_t here instead of 3 parameters.
Will change in next version, thank you.

> 
> I'm however unsure whether we really need this helper just to fetch
> the pdev and call vpci_reset_device_state(), I think you could place
> this logic directly in pci_physdev_op().  Unless there are plans to
> use such logic outside of pci_physdev_op().
If I place the logic of pci_reset_device_state directly in pci_physdev_op. I 
think I need to declare vpci_reset_device_state in pci.h? If it is suitable, I 
will change in next version.

> 
>> +{
>> +    struct pci_dev *pdev;
>> +    int ret = -ENODEV;
> 
> Some XSM check should be introduced fro this operation, as none of the
> existing ones is suitable.  See xsm_resource_unplug_pci() for example.
> 
> xsm_resource_reset_state_pci() or some such I would assume.
I don't know what I should do in XSM function(assume it is 
xsm_resource_reset_state_pci).
Hi Daniel P. Smith, could you please give me some suggestions?
Just to check the XSM_PRIV action?

> 
> (adding the XSM maintainer for feedback).
> 
>> +
>> +    pcidevs_lock();
>> +
>> +    pdev = pci_get_pdev(NULL, PCI_SBDF(seg, bus, devfn));
>> +    if ( !pdev )
>> +        goto error;
>> +
>> +    ret = vpci_reset_device_state(pdev);
>> +    if (ret)
>> +        printk(XENLOG_ERR "PCI reset device %pp state failed\n", 
>> &pdev->sbdf);
>> +
>> +error:
>> +    pcidevs_unlock();
>> +
>> +    return ret;
>> +}
>> +
>>  /* Caller should hold the pcidevs_lock */
>>  static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>                             uint8_t devfn)
>> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
>> index 42db3e6d13..cfdb545654 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -67,6 +67,20 @@ ret_t pci_physdev_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pci_device_state_reset: {
>> +        struct physdev_pci_device dev;
>> +
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -EOPNOTSUPP;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
>> +            break;
>> +
>> +        ret = pci_reset_device_state(dev.seg, dev.bus, dev.devfn);
>> +        break;
>> +    }
>> +
>>      default:
>>          ret = -ENOSYS;
>>          break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 3bec9a4153..e9c3eb1cd6 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -103,6 +103,15 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>  
>>      return rc;
>>  }
>> +
>> +int vpci_reset_device_state(struct pci_dev *pdev)
>> +{
>> +    ASSERT(pcidevs_locked());
>> +
>> +    vpci_remove_device(pdev);
>> +    return vpci_add_handlers(pdev);
>> +}
>> +
>>  #endif /* __XEN__ */
>>  
>>  static int vpci_register_cmp(const struct vpci_register *r1,
>> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
>> index f0c0d4727c..4156948903 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -305,6 +305,8 @@ struct physdev_pci_device {
>>  typedef struct physdev_pci_device physdev_pci_device_t;
>>  DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
>>  
>> +#define PHYSDEVOP_pci_device_state_reset      32
> 
> This needs some comment in order to explain the expected behaviour,
> and might be better placed a bit up after PHYSDEVOP_release_msix.
I will add some comment and move it closer to PHYSDEVOP_release_msix in next 
version. Thank you.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

 


Rackspace

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