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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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;


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