Re: [PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Stefano!

On 10.09.21 22:06, Stefano Stabellini wrote:
> On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> 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 <oleksandr_andrushchenko@xxxxxxxx>
>> Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
>> Cc: Juergen Gross <jgross@xxxxxxxx>
>> ---
>>   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
>>