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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 11:52, Jiqian Chen wrote:
> @@ -67,6 +68,41 @@ 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;
> +        pci_sbdf_t sbdf;
> +
> +        if ( !is_pci_passthrough_enabled() )
> +            return -EOPNOTSUPP;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&dev, arg, 1) != 0 )
> +            break;
> +        sbdf = PCI_SBDF(dev.seg, dev.bus, dev.devfn);
> +
> +        ret = xsm_resource_setup_pci(XSM_PRIV, sbdf.sbdf);
> +        if ( ret )
> +            break;

Daniel, is re-use of this hook appropriate here?

> +        pcidevs_lock();
> +        pdev = pci_get_pdev(NULL, sbdf);
> +        if ( !pdev )
> +        {
> +            pcidevs_unlock();
> +            ret = -ENODEV;
> +            break;
> +        }
> +
> +        write_lock(&pdev->domain->pci_lock);
> +        ret = vpci_reset_device_state(pdev);
> +        write_unlock(&pdev->domain->pci_lock);
> +        pcidevs_unlock();

Can't this be done right after the write_lock()?

> +        if ( ret )
> +            printk(XENLOG_ERR "%pp: failed to reset PCI device state\n", 
> &sbdf);

s/PCI/vPCI/ (at least as long as nothing else is done here)

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -115,6 +115,16 @@ int vpci_assign_device(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev)
> +{
> +    ASSERT(pcidevs_locked());

Is this necessary for ...

> +    ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
> +
> +    vpci_deassign_device(pdev);
> +    return vpci_assign_device(pdev);

... either of these calls? Both functions do themselves only have the
2nd of the asserts you add.

> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -296,6 +296,13 @@ DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_add_t);
>   */
>  #define PHYSDEVOP_prepare_msix          30
>  #define PHYSDEVOP_release_msix          31
> +/*
> + * 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.
> + */
> +#define PHYSDEVOP_pci_device_state_reset     32

Nit: Just a single blank as a separator will do here, for going past the
columnar arrangement of other #define-s anyway.

>  struct physdev_pci_device {
>      /* IN */
>      uint16_t seg;

Is re-using this struct for this new sub-op sufficient? IOW are all
possible resets equal, and hence it doesn't need specifying what kind of
reset was done? For example, other than FLR most reset variants reset all
functions in one go aiui. Imo that would better require only a single
hypercall, just to avoid possible confusion. It also reads as if FLR would
not reset as many registers as other reset variants would.

This may seem to not matter right now, but this is a stable interface you
add, and hence modifying it later will be cumbersome, if possible at all.

Jan