Re: [PATCH v2] xen: Remove dependency between pciback and privcmd

[Jürgen Groß]

On 10.10.24 10:43, Chen, Jiqian wrote:
> On 2024/10/10 16:39, Jürgen Groß wrote:
> > On 10.10.24 09:58, Jiqian Chen wrote:
> > > Commit 2fae6bb7be32 ("xen/privcmd: Add new syscall to get gsi from dev")
> > > adds a weak reverse dependency to the config XEN_PRIVCMD definition, that
> > > dependency causes xen-privcmd can't be loaded on domU, because
> > > dependent xen-pciback is always not be loaded successfully on domU.
> > >
> > > To solve above problem, remove that dependency, and do not call
> > > pcistub_get_gsi_from_sbdf() directly, instead add a hook in
> > > drivers/xen/apci.c, xen-pciback register the real call function, then in
> > > privcmd_ioctl_pcidev_get_gsi call that hook.
> > >
> > > Fixes: 2fae6bb7be32 ("xen/privcmd: Add new syscall to get gsi from dev")
> > > Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> > > ---
> > > v1->v2 changes:
> > > Added hook xen_acpi_get_gsi_from_sbdf.
> > > Changed pcistub_get_gsi_from_sbdf to static and pciback register it as the real hook function.
> > > ---
> > >    drivers/xen/Kconfig                |  1 -
> > >    drivers/xen/acpi.c                 | 17 +++++++++++++++++
> > >    drivers/xen/privcmd.c              |  6 ++----
> > >    drivers/xen/xen-pciback/pci_stub.c |  4 ++--
> > >    include/xen/acpi.h                 | 12 ++++--------
> > >    5 files changed, 25 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > > index 72ddee4c1544..f7d6f47971fd 100644
> > > --- a/drivers/xen/Kconfig
> > > +++ b/drivers/xen/Kconfig
> > > @@ -261,7 +261,6 @@ config XEN_SCSI_BACKEND
> > >    config XEN_PRIVCMD
> > >        tristate "Xen hypercall passthrough driver"
> > >        depends on XEN
> > > -    imply XEN_PCIDEV_BACKEND
> > >        default m
> > >        help
> > >          The hypercall passthrough driver allows privileged user programs to
> > > diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
> > > index 9e2096524fbc..0bff2f3a87d2 100644
> > > --- a/drivers/xen/acpi.c
> > > +++ b/drivers/xen/acpi.c
> > > @@ -125,3 +125,20 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
> > >        return 0;
> > >    }
> > >    EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_info);
> > > +
> > > +get_gsi_from_sbdf_t get_gsi_from_sbdf = NULL;
> > > +
> > > +void xen_acpi_register_get_gsi_func(get_gsi_from_sbdf_t func)
> > > +{
> > > +    get_gsi_from_sbdf = func;
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_acpi_register_get_gsi_func);
> > > +
> > > +int xen_acpi_get_gsi_from_sbdf(u32 sbdf)
> > > +{
> > > +    if (get_gsi_from_sbdf)
> > > +        return get_gsi_from_sbdf(sbdf);
> > > +
> > > +    return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_from_sbdf);
> > > \ No newline at end of file
> > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > > index 3273cb8c2a66..4f75bc876454 100644
> > > --- a/drivers/xen/privcmd.c
> > > +++ b/drivers/xen/privcmd.c
> > > @@ -850,15 +850,13 @@ static long privcmd_ioctl_mmap_resource(struct file *file,
> > >    static long privcmd_ioctl_pcidev_get_gsi(struct file *file, void __user *udata)
> > >    {
> > >    #if defined(CONFIG_XEN_ACPI)
> > > -    int rc = -EINVAL;
> > > +    int rc;
> > >        struct privcmd_pcidev_get_gsi kdata;
> > >          if (copy_from_user(&kdata, udata, sizeof(kdata)))
> > >            return -EFAULT;
> > >    -    if (IS_REACHABLE(CONFIG_XEN_PCIDEV_BACKEND))
> > > -        rc = pcistub_get_gsi_from_sbdf(kdata.sbdf);
> > > -
> > > +    rc = xen_acpi_get_gsi_from_sbdf(kdata.sbdf);
> > >        if (rc < 0)
> > >            return rc;
> > >    diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
> > > index 2f3da5ac62cd..900e6199eec7 100644
> > > --- a/drivers/xen/xen-pciback/pci_stub.c
> > > +++ b/drivers/xen/xen-pciback/pci_stub.c
> > > @@ -227,7 +227,7 @@ static struct pci_dev *pcistub_device_get_pci_dev(struct xen_pcibk_device *pdev,
> > >    }
> > >      #ifdef CONFIG_XEN_ACPI
> > > -int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
> > > +static int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
> > >    {
> > >        struct pcistub_device *psdev;
> > >        int domain = (sbdf >> 16) & 0xffff;
> > > @@ -242,7 +242,6 @@ int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
> > >          return psdev->gsi;
> > >    }
> > > -EXPORT_SYMBOL_GPL(pcistub_get_gsi_from_sbdf);
> > >    #endif
> > >      struct pci_dev *pcistub_get_pci_dev_by_slot(struct xen_pcibk_device *pdev,
> > > @@ -484,6 +483,7 @@ static int pcistub_init_device(struct pcistub_device *psdev)
> > >            if (err)
> > >                goto config_release;
> > >            psdev->gsi = gsi;
> > > +        xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
> >
> > I don't think this is the right place for registering the function.
> >
> > It should be done at the end of xen_pcibk_init() (guarded with CONFIG_XEN_ACPI).
> >
> > Additionally you should reset the function pointer NULL in xen_pcibk_cleanup().
> Should I need to add a new unregister function to reset, or just pass NULL to xen_acpi_register_get_gsi_func ?

I think passing NULL is fine.

But you'll need some kind of locking to avoid a race between a call of
the hook and parallel unloading the pciback module.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature