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
