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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4524 bytes --]

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@invisiblethingslab.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> 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().


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]