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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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().

> +{
> +    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.

(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.

Thanks, Roger.