[v2] xen: Remove dependency between pciback and privcmd

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

Posted by Jiqian Chen 2 months, 1 week ago

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);
 	}
 #endif
 
diff --git a/include/xen/acpi.h b/include/xen/acpi.h
index daa96a22d257..ef4f70e4a557 100644
--- a/include/xen/acpi.h
+++ b/include/xen/acpi.h
@@ -91,13 +91,9 @@ static inline int xen_acpi_get_gsi_info(struct pci_dev *dev,
 }
 #endif
 
-#ifdef CONFIG_XEN_PCI_STUB
-int pcistub_get_gsi_from_sbdf(unsigned int sbdf);
-#else
-static inline int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
-{
-	return -1;
-}
-#endif
+typedef int (*get_gsi_from_sbdf_t)(u32 sbdf);
+
+void xen_acpi_register_get_gsi_func(get_gsi_from_sbdf_t func);
+int xen_acpi_get_gsi_from_sbdf(unsigned int sbdf);
 
 #endif	/* _XEN_ACPI_H */
-- 
2.34.1

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

Posted by Stefano Stabellini 2 months, 1 week ago

On Thu, 10 Oct 2024, 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;

This can be static


> +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);
>  	}
>  #endif
>  
> diff --git a/include/xen/acpi.h b/include/xen/acpi.h
> index daa96a22d257..ef4f70e4a557 100644
> --- a/include/xen/acpi.h
> +++ b/include/xen/acpi.h
> @@ -91,13 +91,9 @@ static inline int xen_acpi_get_gsi_info(struct pci_dev *dev,
>  }
>  #endif
>  
> -#ifdef CONFIG_XEN_PCI_STUB
> -int pcistub_get_gsi_from_sbdf(unsigned int sbdf);
> -#else
> -static inline int pcistub_get_gsi_from_sbdf(unsigned int sbdf)
> -{
> -	return -1;
> -}
> -#endif
> +typedef int (*get_gsi_from_sbdf_t)(u32 sbdf);
> +
> +void xen_acpi_register_get_gsi_func(get_gsi_from_sbdf_t func);
> +int xen_acpi_get_gsi_from_sbdf(unsigned int sbdf);
>  
>  #endif	/* _XEN_ACPI_H */
> -- 
> 2.34.1
>

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

Posted by Jürgen Groß 2 months, 1 week ago

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

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

Posted by Chen, Jiqian 2 months, 1 week ago

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@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().
Should I need to add a new unregister function to reset, or just pass NULL to xen_acpi_register_get_gsi_func ?

> 
> 
> Juergen

-- 
Best regards,
Jiqian Chen.

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

Posted by Jürgen Groß 2 months, 1 week ago

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@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().
> 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

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

Posted by Jan Beulich 2 months, 1 week ago

On 10.10.2024 09:58, Jiqian Chen wrote:
> --- 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;

Perhaps better -EOPNOTSUPP?

> +}
> +EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_from_sbdf);
> \ No newline at end of file

Can you please take care of this as well while touching the file? Or
maybe you are doing so, but the diff doesn't show it properly?

> @@ -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);
>  	}
>  #endif

Why here rather than directly in xen_pcibk_init()? And why no change to
xen_pcibk_cleanup() to remove the hook again on unload? Which will then
raise the question of possible race conditions.

Jan

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

Posted by Chen, Jiqian 2 months, 1 week ago

On 2024/10/10 16:17, Jan Beulich wrote:
> On 10.10.2024 09:58, Jiqian Chen wrote:
>> --- 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;
> 
> Perhaps better -EOPNOTSUPP?
OK, will change.
> 
>> +}
>> +EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_from_sbdf);
>> \ No newline at end of file
> 
> Can you please take care of this as well while touching the file? Or
> maybe you are doing so, but the diff doesn't show it properly?
Yes, will change.
> 
>> @@ -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);
>>  	}
>>  #endif
> 
> Why here rather than directly in xen_pcibk_init()? And why no change to
> xen_pcibk_cleanup() to remove the hook again on unload? Which will then
> raise the question of possible race conditions.
You are right, will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.