Hi, Stefano!
On 10.09.21 22:06, Stefano Stabellini wrote:
> On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <[email protected]>
>>
>> Arm's PCI passthrough implementation doesn't support legacy interrupts,
>> but MSI/MSI-X. This can be the case for other platforms too.
>> For that reason introduce a new CONFIG_PCI_SUPP_LEGACY_IRQ and add
>> it to the CFLAGS and compile the relevant code in the toolstack only if
>> applicable.
>>
>> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
>> Cc: Ian Jackson <[email protected]>
>> Cc: Juergen Gross <[email protected]>
>> ---
>> tools/libs/light/Makefile | 4 ++++
>> tools/libs/light/libxl_pci.c | 13 +++++++++++++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
>> index 7d8c51d49242..bd3f6be2a183 100644
>> --- a/tools/libs/light/Makefile
>> +++ b/tools/libs/light/Makefile
>> @@ -46,6 +46,10 @@ CFLAGS += -Wno-format-zero-length -Wmissing-declarations \
>> -Wno-declaration-after-statement -Wformat-nonliteral
>> CFLAGS += -I.
>>
>> +ifeq ($(CONFIG_X86),y)
>> +CFLAGS += -DCONFIG_PCI_SUPP_LEGACY_IRQ
>> +endif
>> +
>> SRCS-$(CONFIG_X86) += libxl_cpuid.c
>> SRCS-$(CONFIG_X86) += libxl_x86.c
>> SRCS-$(CONFIG_X86) += libxl_psr.c
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 59f3686fc85e..cd4fea46c3f7 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1434,6 +1434,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>> }
>> }
>> fclose(f);
>> +#ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
> As Juergen pointed out the logic is inverted.
>
> I also think we need to come up with a better way to handle this #ifdef
> logic in this file.
>
> For instance, could we let this function try to open sysfs_path? I
> imagine it would fail, right? If so, we could just have an #ifdef inside
> the failure check.
>
> Alternatively, could we have a small #ifdef right here doing:
>
> #ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
> goto out_no_irq;
> #endif
>
> ?
>
>
> Even better, would be to introduce a static inline as follows:
>
> static inline bool pci_supp_legacy_irq(void)
> {
> #ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
> return false;
> #else
> return true;
> #endif
> }
>
> Then in libxl_pci.c you can avoid all ifdefs:
>
> if (!pci_supp_legacy_irq()))
> goto out_no_irq;
This one seems to be the best with less ifdef'ery
I'll re-work the code as above
>
>
>> sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> pci->bus, pci->dev, pci->func);
>> f = fopen(sysfs_path, "r");
>> @@ -1460,6 +1461,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>> }
>> }
>> fclose(f);
>> +#endif
>>
>> /* Don't restrict writes to the PCI config space from this VM */
>> if (pci->permissive) {
>> @@ -1471,7 +1473,9 @@ static void pci_add_dm_done(libxl__egc *egc,
>> }
>> }
>>
>> +#ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
>> out_no_irq:
>> +#endif
>> if (!isstubdom) {
>> if (pci->rdm_policy == LIBXL_RDM_RESERVE_POLICY_STRICT) {
>> flag &= ~XEN_DOMCTL_DEV_RDM_RELAXED;
>> @@ -1951,7 +1955,9 @@ static void do_pci_remove(libxl__egc *egc,
>> pci_remove_state *prs)
>> pci->bus, pci->dev, pci->func);
>> FILE *f = fopen(sysfs_path, "r");
>> unsigned int start = 0, end = 0, flags = 0, size = 0;
>> +#ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
>> int irq = 0;
>> +#endif
> I'd let this compile if possible.
>
>
>> int i;
>> if (f == NULL) {
>> @@ -1983,6 +1989,7 @@ static void do_pci_remove(libxl__egc *egc,
>> pci_remove_state *prs)
>> }
>> fclose(f);
>> skip1:
>> +#ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
> Here we could do instead:
>
> if (!pci_supp_legacy_irq()))
> goto skip_irq;
>
>
>> sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> pci->bus, pci->dev, pci->func);
>> f = fopen(sysfs_path, "r");
>> @@ -2001,8 +2008,14 @@ skip1:
>> }
>> }
>> fclose(f);
>> +#else
>> + /* Silence error: label at end of compound statement */
>> + ;
>> +#endif
>> }
>> +#ifndef CONFIG_PCI_SUPP_LEGACY_IRQ
>> skip_irq:
>> +#endif
>> rc = 0;
>> out_fail:
>> pci_remove_detached(egc, prs, rc); /* must be last */
>> --
>> 2.25.1
>>
