Re: [PATCH] xen: Remove config dependency in XEN_PRIVCMD definition

[Jürgen Groß]

On 10.10.24 00:46, Stefano Stabellini wrote:
> On Wed, 9 Oct 2024, Jan Beulich wrote:
> > On 09.10.2024 08:20, 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, its
> > > purpose is to pass the combination of compilation that CONFIG_XEN_PRIVCMD=y
> > > and CONFIG_XEN_PCIDEV_BACKEND=m, because in that combination, xen-pciback
> > > is compiled as a module but xen-privcmd is built-in, so xen-privcmd can't
> > > find the implementation of pcistub_get_gsi_from_sbdf.
> > >
> > > But 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 and cover original commit's requirement, just remove
> > > that dependency, because the code "IS_REACHABLE(CONFIG_XEN_PCIDEV_BACKEND)"
> > > of original commit is enough to meet the requirement.
> > >
> > > Fixes: 2fae6bb7be32 ("xen/privcmd: Add new syscall to get gsi from dev")
> > > Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >
> > This lacks a Reported-by:.
> >
> > > --- 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
> >
> > The report wasn't about a build problem, but a runtime one. Removing the
> > dependency here doesn't change anything in the dependency of xen-privcmd
> > on xen-pciback, as the use of pcistub_get_gsi_from_sbdf() continues to
> > exist.
> >
> > Consider the case of XEN_PCIDEV_BACKEND=m and XEN_PRIVCMD=m, which
> > I guess is what Marek is using in his config. Both drivers are available
> > in such a configuration, yet loading of xen-privcmd then requires to
> > load xen-pciback first. And that latter load attempt will fail in a DomU.
> > The two drivers simply may not have any dependency in either direction.
>
> The idea is that there should be no hard dependency on
> pcistub_get_gsi_from_sbdf(). If it is available, the service will be
> used, otherwise an error will be reported.
>
> The problem is that IS_REACHABLE is a compile-time check. What we need
> is a runtime check instead. Maybe symbol_get or try_module_get ?

This is a rather clumsy solution IMO.

I'm seeing the following solutions:

1. Don't fail to to load the pciback driver in a DomU, but only disable
   any functionality.

2. Move the drivers/xen/xen-pciback/pci_stub.c functionality in a module
   of its own, allowing the privcmd driver to be loaded without needing
   the rest of pciback.

3. Add a hook to e.g. drivers/xen/pci.c instead for calling of
   pcistub_get_gsi_from_sbdf() directly. pciback could register the real
   call address. If pciback isn't loaded, the hook can return an error.
   This would remove the explicit dependency of privcmd on pciback.

I'd prefer the 3rd variant.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature