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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

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 ?

> 
> 
> Juergen

-- 
Best regards,
Jiqian Chen.