drivers/xen/Kconfig | 1 - drivers/xen/acpi.c | 24 ++++++++++++++++++++++++ drivers/xen/privcmd.c | 6 ++---- drivers/xen/xen-pciback/pci_stub.c | 11 +++++++++-- include/xen/acpi.h | 12 ++++-------- 5 files changed, 39 insertions(+), 15 deletions(-)
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 isn't always 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>
---
v2->v3 changes:
Added rwlock get_gsi_from_sbdf_lock to avoid races.
Called xen_acpi_register_get_gsi_func to register function in xen_pcibk_init and set NULL to hook in xen_pcibk_cleanup when unloading.
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 | 24 ++++++++++++++++++++++++
drivers/xen/privcmd.c | 6 ++----
drivers/xen/xen-pciback/pci_stub.c | 11 +++++++++--
include/xen/acpi.h | 12 ++++--------
5 files changed, 39 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..d2ee605c5ca1 100644
--- a/drivers/xen/acpi.c
+++ b/drivers/xen/acpi.c
@@ -125,3 +125,27 @@ int xen_acpi_get_gsi_info(struct pci_dev *dev,
return 0;
}
EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_info);
+
+static get_gsi_from_sbdf_t get_gsi_from_sbdf;
+static DEFINE_RWLOCK(get_gsi_from_sbdf_lock);
+
+void xen_acpi_register_get_gsi_func(get_gsi_from_sbdf_t func)
+{
+ write_lock(&get_gsi_from_sbdf_lock);
+ get_gsi_from_sbdf = func;
+ write_unlock(&get_gsi_from_sbdf_lock);
+}
+EXPORT_SYMBOL_GPL(xen_acpi_register_get_gsi_func);
+
+int xen_acpi_get_gsi_from_sbdf(u32 sbdf)
+{
+ int ret = -EOPNOTSUPP;
+
+ read_lock(&get_gsi_from_sbdf_lock);
+ if (get_gsi_from_sbdf)
+ ret = get_gsi_from_sbdf(sbdf);
+ read_unlock(&get_gsi_from_sbdf_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(xen_acpi_get_gsi_from_sbdf);
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..b616b7768c3b 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,
@@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
bus_register_notifier(&pci_bus_type, &pci_stub_nb);
#endif
+#ifdef CONFIG_XEN_ACPI
+ xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
+#endif
+
return err;
}
static void __exit xen_pcibk_cleanup(void)
{
+#ifdef CONFIG_XEN_ACPI
+ xen_acpi_register_get_gsi_func(NULL);
+#endif
+
#ifdef CONFIG_PCI_IOV
bus_unregister_notifier(&pci_bus_type, &pci_stub_nb);
#endif
diff --git a/include/xen/acpi.h b/include/xen/acpi.h
index daa96a22d257..8cb081c633cc 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(u32 sbdf);
#endif /* _XEN_ACPI_H */
--
2.34.1
On 11.10.24 05:42, 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 isn't always 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>
Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
Reason is that drivers/xen/acpi.c is being built for Dom0 only, so having
CONFIG_XEN_ACPI defined is not enough for ensuring the new functions are
present.
I suggest to add a new CONFIG item CONFIG_XEN_DOM0_ACPI like
config XEN_DOM0_ACPI
def_bool y
depends on XEN_ACPI && XEN_DOM0
and then replace all CONFIG_XEN_ACPI instances in your patch with
CONFIG_XEN_DOM0_ACPI. This includes the use case in
privcmd_ioctl_pcidev_get_gsi().
Juergen
On 2024/10/11 20:06, Juergen Gross wrote:
> On 11.10.24 05:42, 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 isn't always 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>
>
> Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
>
> Reason is that drivers/xen/acpi.c is being built for Dom0 only, so having
> CONFIG_XEN_ACPI defined is not enough for ensuring the new functions are
> present.
>
> I suggest to add a new CONFIG item CONFIG_XEN_DOM0_ACPI like
>
> config XEN_DOM0_ACPI
> def_bool y
> depends on XEN_ACPI && XEN_DOM0
>
> and then replace all CONFIG_XEN_ACPI instances in your patch with
> CONFIG_XEN_DOM0_ACPI. This includes the use case in
> privcmd_ioctl_pcidev_get_gsi().
How about add stubs for xen_acpi_register_get_gsi_func and xen_acpi_get_gsi_from_sbdf when "!CONFIG_XEN_DOM0" in acpi.h, like the other functions in that file do.
>
>
> Juergen
--
Best regards,
Jiqian Chen.
On 2024/10/12 10:22, Chen, Jiqian wrote:
> On 2024/10/11 20:06, Juergen Gross wrote:
>> On 11.10.24 05:42, 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 isn't always 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>
>>
>> Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
Could you please attach the link or steps?
Then I can reproduce it, and validate it locally with new changes.
>>
>> Reason is that drivers/xen/acpi.c is being built for Dom0 only, so having
>> CONFIG_XEN_ACPI defined is not enough for ensuring the new functions are
>> present.
>>
>> I suggest to add a new CONFIG item CONFIG_XEN_DOM0_ACPI like
>>
>> config XEN_DOM0_ACPI
>> def_bool y
>> depends on XEN_ACPI && XEN_DOM0
>>
>> and then replace all CONFIG_XEN_ACPI instances in your patch with
>> CONFIG_XEN_DOM0_ACPI. This includes the use case in
>> privcmd_ioctl_pcidev_get_gsi().
>
> How about add stubs for xen_acpi_register_get_gsi_func and xen_acpi_get_gsi_from_sbdf when "!CONFIG_XEN_DOM0" in acpi.h, like the other functions in that file do.
>
>>
>>
>> Juergen
>
--
Best regards,
Jiqian Chen.
On 12.10.24 04:36, Chen, Jiqian wrote:
> On 2024/10/12 10:22, Chen, Jiqian wrote:
>> On 2024/10/11 20:06, Juergen Gross wrote:
>>> On 11.10.24 05:42, 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 isn't always 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>
>>>
>>> Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
> Could you please attach the link or steps?
> Then I can reproduce it, and validate it locally with new changes.
I'm using the attached kernel config.
Juergen
On 2024/10/12 13:48, Jürgen Groß wrote:
> On 12.10.24 04:36, Chen, Jiqian wrote:
>> On 2024/10/12 10:22, Chen, Jiqian wrote:
>>> On 2024/10/11 20:06, Juergen Gross wrote:
>>>> On 11.10.24 05:42, 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 isn't always 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>
>>>>
>>>> Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
>> Could you please attach the link or steps?
>> Then I can reproduce it, and validate it locally with new changes.
>
> I'm using the attached kernel config.
Thanks, I can reproduce the 32-bit build error locally.
And this "Add stubs for xen_acpi_register_get_gsi_func and xen_acpi_get_gsi_from_sbdf when "!CONFIG_XEN_DOM0" in acpi.h, like the other functions in that file do." can fix it.
Is it okay?
>
>
> Juergen
>
--
Best regards,
Jiqian Chen.
On 12.10.24 08:50, Chen, Jiqian wrote:
> On 2024/10/12 13:48, Jürgen Groß wrote:
>> On 12.10.24 04:36, Chen, Jiqian wrote:
>>> On 2024/10/12 10:22, Chen, Jiqian wrote:
>>>> On 2024/10/11 20:06, Juergen Gross wrote:
>>>>> On 11.10.24 05:42, 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 isn't always 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>
>>>>>
>>>>> Unfortunately I'm seeing a build breakage for the 32-bit x86 build.
>>> Could you please attach the link or steps?
>>> Then I can reproduce it, and validate it locally with new changes.
>>
>> I'm using the attached kernel config.
> Thanks, I can reproduce the 32-bit build error locally.
> And this "Add stubs for xen_acpi_register_get_gsi_func and xen_acpi_get_gsi_from_sbdf when "!CONFIG_XEN_DOM0" in acpi.h, like the other functions in that file do." can fix it.
> Is it okay?
Fine with me.
Juergen
On 11.10.24 05:42, 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 isn't always 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>
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
On 11.10.2024 05:42, Jiqian Chen wrote:
> @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
> bus_register_notifier(&pci_bus_type, &pci_stub_nb);
> #endif
>
> +#ifdef CONFIG_XEN_ACPI
> + xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
> +#endif
> +
> return err;
> }
>
> static void __exit xen_pcibk_cleanup(void)
> {
> +#ifdef CONFIG_XEN_ACPI
> + xen_acpi_register_get_gsi_func(NULL);
> +#endif
Just wondering - instead of these two #ifdef-s, ...
> --- 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(u32 sbdf);
... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall readability?
Jan
On 2024/10/11 16:54, Jan Beulich wrote:
> On 11.10.2024 05:42, Jiqian Chen wrote:
>> @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
>> bus_register_notifier(&pci_bus_type, &pci_stub_nb);
>> #endif
>>
>> +#ifdef CONFIG_XEN_ACPI
>> + xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
>> +#endif
>> +
>> return err;
>> }
>>
>> static void __exit xen_pcibk_cleanup(void)
>> {
>> +#ifdef CONFIG_XEN_ACPI
>> + xen_acpi_register_get_gsi_func(NULL);
>> +#endif
>
> Just wondering - instead of these two #ifdef-s, ...
>
>> --- 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(u32 sbdf);
>
> ... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall readability?
I'm not sure if other files do this. But for me, it feels a little strange to use "#ifdef CONFIG_XEN_ACPI #else" in apci.h, like self-containment.
And "#include apci.h" in pic_stub.c is also wraped with CONFIG_XEN_ACPI.
>
> Jan
--
Best regards,
Jiqian Chen.
On 2024/10/11 17:20, Chen, Jiqian wrote:
> On 2024/10/11 16:54, Jan Beulich wrote:
>> On 11.10.2024 05:42, Jiqian Chen wrote:
>>> @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
>>> bus_register_notifier(&pci_bus_type, &pci_stub_nb);
>>> #endif
>>>
>>> +#ifdef CONFIG_XEN_ACPI
>>> + xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
>>> +#endif
>>> +
>>> return err;
>>> }
>>>
>>> static void __exit xen_pcibk_cleanup(void)
>>> {
>>> +#ifdef CONFIG_XEN_ACPI
>>> + xen_acpi_register_get_gsi_func(NULL);
>>> +#endif
>>
>> Just wondering - instead of these two #ifdef-s, ...
>>
>>> --- 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(u32 sbdf);
>>
>> ... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall readability?
> I'm not sure if other files do this. But for me, it feels a little strange to use "#ifdef CONFIG_XEN_ACPI #else" in apci.h, like self-containment.
> And "#include apci.h" in pic_stub.c is also wraped with CONFIG_XEN_ACPI.
OK, I saw other files also do this.
If you insist, I will make modifications in the next version.
Thank you!
>
>>
>> Jan
>
--
Best regards,
Jiqian Chen.
On 11.10.2024 11:33, Chen, Jiqian wrote:
> On 2024/10/11 17:20, Chen, Jiqian wrote:
>> On 2024/10/11 16:54, Jan Beulich wrote:
>>> On 11.10.2024 05:42, Jiqian Chen wrote:
>>>> @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
>>>> bus_register_notifier(&pci_bus_type, &pci_stub_nb);
>>>> #endif
>>>>
>>>> +#ifdef CONFIG_XEN_ACPI
>>>> + xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
>>>> +#endif
>>>> +
>>>> return err;
>>>> }
>>>>
>>>> static void __exit xen_pcibk_cleanup(void)
>>>> {
>>>> +#ifdef CONFIG_XEN_ACPI
>>>> + xen_acpi_register_get_gsi_func(NULL);
>>>> +#endif
>>>
>>> Just wondering - instead of these two #ifdef-s, ...
>>>
>>>> --- 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(u32 sbdf);
>>>
>>> ... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall readability?
>> I'm not sure if other files do this. But for me, it feels a little strange to use "#ifdef CONFIG_XEN_ACPI #else" in apci.h, like self-containment.
>> And "#include apci.h" in pic_stub.c is also wraped with CONFIG_XEN_ACPI.
> OK, I saw other files also do this.
> If you insist, I will make modifications in the next version.
Well, I'm not a maintainer of this code, so I can't very well insist.
Jan
On 11.10.24 12:10, Jan Beulich wrote:
> On 11.10.2024 11:33, Chen, Jiqian wrote:
>> On 2024/10/11 17:20, Chen, Jiqian wrote:
>>> On 2024/10/11 16:54, Jan Beulich wrote:
>>>> On 11.10.2024 05:42, Jiqian Chen wrote:
>>>>> @@ -1757,11 +1756,19 @@ static int __init xen_pcibk_init(void)
>>>>> bus_register_notifier(&pci_bus_type, &pci_stub_nb);
>>>>> #endif
>>>>>
>>>>> +#ifdef CONFIG_XEN_ACPI
>>>>> + xen_acpi_register_get_gsi_func(pcistub_get_gsi_from_sbdf);
>>>>> +#endif
>>>>> +
>>>>> return err;
>>>>> }
>>>>>
>>>>> static void __exit xen_pcibk_cleanup(void)
>>>>> {
>>>>> +#ifdef CONFIG_XEN_ACPI
>>>>> + xen_acpi_register_get_gsi_func(NULL);
>>>>> +#endif
>>>>
>>>> Just wondering - instead of these two #ifdef-s, ...
>>>>
>>>>> --- 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(u32 sbdf);
>>>>
>>>> ... wouldn't a static inline stub (for the !XEN_ACPI case) aid overall readability?
>>> I'm not sure if other files do this. But for me, it feels a little strange to use "#ifdef CONFIG_XEN_ACPI #else" in apci.h, like self-containment.
>>> And "#include apci.h" in pic_stub.c is also wraped with CONFIG_XEN_ACPI.
>> OK, I saw other files also do this.
>> If you insist, I will make modifications in the next version.
>
> Well, I'm not a maintainer of this code, so I can't very well insist.
In this case I'm in favor of having the #ifdefs at the calling site.
The amount of code isn't larger this way, while it is more clear when
reading the code that the calls are only done in the CONFIG_XEN_ACPI
case.
Juergen
© 2016 - 2024 Red Hat, Inc.