Re: [XEN PATCH v13 1/6] xen/pci: Add hypercall to support reset of pcidev

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2024 13:08, Jiqian Chen wrote:
> @@ -67,6 +68,57 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_pci_device_reset:
> +    {
> +        struct pci_device_reset dev_reset;
> +        struct pci_dev *pdev;
> +        pci_sbdf_t sbdf;
> +
> +        ret = -EOPNOTSUPP;
> +        if ( !is_pci_passthrough_enabled() )
> +            break;

It occurs to me (only now, sorry): Does this case really need to be an
error? I.e. do we really need to bother callers by having them find out
whether pass-through is supported in the underlying Xen?

> +        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.devfn);
> +
> +        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();
> +        switch ( dev_reset.flags & PCI_DEVICE_RESET_MASK )
> +        {
> +        case PCI_DEVICE_RESET_COLD:
> +        case PCI_DEVICE_RESET_WARM:
> +        case PCI_DEVICE_RESET_HOT:
> +        case PCI_DEVICE_RESET_FLR:
> +            ret = vpci_reset_device(pdev);
> +            break;
> +
> +        default:
> +            ret = -EOPNOTSUPP;

EINVAL

But: What about the other flag bits? You don't check them (anymore; I
thought there was a check there before).

> --- 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_reset 32

Nit: Please pad the 32 to align with the 30 and 31 in context.

Jan