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

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

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

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

[-- 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 --]