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

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: "Chen, Jiqian" <Jiqian.Chen@xxxxxxx>
  • Date: Tue, 12 Dec 2023 06:44:01 +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=q3UhofQ4Z6ggn38aljjXLAReiEklRAth2DDEAU82orQ=; b=S50dNzMRt0vQughq44l+UfAeHdsV0pK0WEvVnA+V8INT6PX2MvRiUS42Bp9+r0dr0Af9xDa0d3zSM3LOx5eJ3IuGsn5KsqDdXYEm+YNdHGmNbmbWWXFETgABV5t5cNuUUxIi0VlALW4umsufvtIixRpGOiAjIIO7LveKehSNNm76Kqt+EhIEYjzyvw9KC9/q0LtsxTHbti64hC/IXwEyxVyb5xle2lDdaisGXJkzl6dRuNvuuk8Jslv31Xcl6T/aYV4IQ5D8e+4j78D4T0ISju/etsm8WaToGFr6kskCgTm/tI8fq0XYP+aVaat3Qenl66jJQyrnzlT2c3/M9Ghvbg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lktgO23/YzRueY0oKisF6AFR7Ad5zRNn+iDicIvDrY/x6HoVqxNLucfgDBaucx6Yd71obT655G8vhP/aI6Ljqmm9QVtIRS4xqtYe/Zyz0/gBps6DDhnAkimeRuSgyVWoFfVG7IPIbxVy5us/gNrFP6qwQNDJZNSkSLV4vWlSrHgTw/2qu2yzD9lzrq8A01fV7Zn8kKt1unuCXe58SoLK/LsFD6my92pWldVhbnZX4zm+bowpdaUA1+ey3JcQpp9tvZ3+ht0pml6EIkWke8V/LtkVEx9whXaKcOYbVf1h61eYhc+AQyPuVRE4hFefRCRC1uOpZIZ16qWqrXVVxub6jA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Jan Beulich <jbeulich@xxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, 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: Tue, 12 Dec 2023 06:44:14 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHaK4eZ4y4R7EwUj0iEeYs1akErIbCkNISAgAGF8wA=
  • Thread-topic: [RFC XEN PATCH v3 1/3] xen/vpci: Clear all vpci status of device

On 2023/12/11 23:22, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:40:07AM +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, add a new hypercall to clear all vpci
>> device state. When the state of device is reset on dom0 side,
>> dom0 can call this hypercall to notify vpci.
>>
>> Co-developed-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>>  xen/arch/x86/hvm/hypercall.c |  1 +
>>  xen/drivers/pci/physdev.c    | 35 +++++++++++++++++++++++++++++++++++
>>  xen/drivers/vpci/vpci.c      |  9 +++++++++
>>  xen/include/public/physdev.h |  8 ++++++++
>>  xen/include/xen/vpci.h       |  6 ++++++
>>  5 files changed, 59 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/pci/physdev.c b/xen/drivers/pci/physdev.c
>> index 42db3e6d13..6ee2edb86a 100644
>> --- a/xen/drivers/pci/physdev.c
>> +++ b/xen/drivers/pci/physdev.c
>> @@ -2,6 +2,7 @@
>>  #include <xen/guest_access.h>
>>  #include <xen/hypercall.h>
>>  #include <xen/init.h>
>> +#include <xen/vpci.h>
>>  
>>  #ifndef COMPAT
>>  typedef long ret_t;
>> @@ -67,6 +68,40 @@ 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;
>> +        struct pci_dev *pdev;
>> +
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -EOPNOTSUPP;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
>> +            break;
>> +
>> +        ret = xsm_resource_setup_pci(XSM_PRIV,
>> +                                     (dev.seg << 16) | (dev.bus << 8) |
>> +                                     dev.devfn);
>> +        if ( ret )
>> +            break;
>> +
>> +        pcidevs_lock();
>> +        pdev = pci_get_pdev(NULL, PCI_SBDF(dev.seg, dev.bus, dev.devfn));
>> +        if ( !pdev )
>> +        {
>> +            ret = -ENODEV;
>> +            pcidevs_unlock();
> 
> Nit: assigning -ENODEV could be done after dropping the lock.
I will adjust the sequence in next version, thanks.

> 
>> +            break;
>> +        }
>> +
>> +        ret = vpci_reset_device_state(pdev);
>> +        if ( ret )
>> +            printk(XENLOG_ERR "PCI reset device %pp state failed\n",
>> +                   &pdev->sbdf);
> 
> Please do the print outside of the locked region, there's no need to
> hold the pcidevs lock after the vpci_reset_device_state() call if you
> use the data in the local 'dev' variable to print the SBDF.
> 
> It would also be fine if you want to introduce a local sbdf variable
> that you can use both here and in the call to pci_get_pdev() above.
> 
> I think it's also easier to parse if the SBDF is at the begging of the
> message:
> 
> "%pp: failed to reset PCI device state\n"
> 
> That's however a question of taste.
Nice suggestion, I will adjust according to all what you said. Thanks.

> 
>> +        pcidevs_unlock();
>> +        break;
>> +    }
>> +
>>      default:
>>          ret = -ENOSYS;
>>          break;
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 72ef277c4f..3c64cb10cc 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -107,6 +107,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..92c2f28bca 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -296,6 +296,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
>>   */
>>  #define PHYSDEVOP_prepare_msix          30
>>  #define PHYSDEVOP_release_msix          31
>> +/*
>> + * On PVH dom0, when device is reset, the vpci on Xen side
>> + * won't get notification, so that the cached state in vpci is
>> + * all out of date with the real device state. Use this to reset
>> + * the vpci state of device.
>> + */
> 
> I get this feeling this is too specific to vpci, when the hypercall
> itself could be used for other purposes in the future.  I would
> instead write:
> 
> /*
>  * Notify the hypervisor that a PCI device has been reset, so that any
>  * internally cached state is regenerated.  Should be called after any
>  * device reset performed by the hardware domain.
>  */
Thanks, will use this in next version.

> 
>> +#define PHYSDEVOP_pci_device_state_reset     32
>> +
>>  struct physdev_pci_device {
>>      /* IN */
>>      uint16_t seg;
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index d20c301a3d..d6377424f0 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -30,6 +30,7 @@ int __must_check vpci_add_handlers(struct pci_dev *pdev);
>>  
>>  /* Remove all handlers and free vpci related structures. */
>>  void vpci_remove_device(struct pci_dev *pdev);
>> +int vpci_reset_device_state(struct pci_dev *pdev);
> 
> __must_check please.
> 
> 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®.