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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2024 11:00, Jiqian Chen wrote:
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -2,11 +2,17 @@
>  #include <xen/guest_access.h>
>  #include <xen/hypercall.h>
>  #include <xen/init.h>
> +#include <xen/vpci.h>
>  
>  #ifndef COMPAT
>  typedef long ret_t;
>  #endif
>  
> +static const struct pci_device_state_reset_method
> +                    pci_device_state_reset_methods[] = {
> +    [ DEVICE_RESET_FLR ].reset_fn = vpci_reset_device_state,
> +};

What about the other three DEVICE_RESET_*? In particular ...

> @@ -67,6 +73,43 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_pci_device_state_reset: {
> +        struct pci_device_state_reset dev_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_reset, arg, 1) != 0 )
> +            break;
> +        dev = &dev_reset.dev;
> +        sbdf = PCI_SBDF(dev->seg, dev->bus, 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();
> +        ret = 
> pci_device_state_reset_methods[dev_reset.reset_type].reset_fn(pdev);

... you're setting this up for calling NULL. In fact there's also no bounds
check for the array index.

Also, nit (further up): Opening figure braces for a new scope go onto their
own line. Then again I notice that apparenly _all_ other instances in this
file are doing it the wrong way, too.

Finally, is the "dev" local variable really needed? It effectively hides that
PCI_SBDF() is invoked on the hypercall arguments.

> +        write_unlock(&pdev->domain->pci_lock);
> +        if ( ret )
> +            printk(XENLOG_ERR "%pp: failed to reset vPCI device state\n", 
> &sbdf);

Maybe downgrade to dprintk()? The caller ought to handle the error anyway.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -172,6 +172,15 @@ int vpci_assign_device(struct pci_dev *pdev)
>  
>      return rc;
>  }
> +
> +int vpci_reset_device_state(struct pci_dev *pdev)

As a target of an indirect call this needs to be annotated cf_check (both
here and in the declaration, unlike __must_check, which is sufficient to
have on just the declaration).

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -156,6 +156,22 @@ struct pci_dev {
>      struct vpci *vpci;
>  };
>  
> +struct pci_device_state_reset_method {
> +    int (*reset_fn)(struct pci_dev *pdev);
> +};
> +
> +enum pci_device_state_reset_type {
> +    DEVICE_RESET_FLR,
> +    DEVICE_RESET_COLD,
> +    DEVICE_RESET_WARM,
> +    DEVICE_RESET_HOT,
> +};
> +
> +struct pci_device_state_reset {
> +    struct physdev_pci_device dev;
> +    enum pci_device_state_reset_type reset_type;
> +};

This is the struct to use as hypercall argument. How can it live outside of
any public header? Also, when moving it there, beware that you should not
use enum-s there. Only handles and fixed-width types are permitted.

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -38,6 +38,7 @@ int __must_check vpci_assign_device(struct pci_dev *pdev);
>  
>  /* Remove all handlers and free vpci related structures. */
>  void vpci_deassign_device(struct pci_dev *pdev);
> +int __must_check vpci_reset_device_state(struct pci_dev *pdev);

What's the purpose of this __must_check, when the sole caller is calling
this through a function pointer, which isn't similarly annotated?

Jan