1. Patchew
  2. Xen
  3. [PATCH 00/11] PCI devices passthrough on Arm, part 2
  4. View patch

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

Oleksandr Andrushchenko posted 11 patches 4 years, 5 months ago
There is a newer version of this series
[PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported
Posted by Oleksandr Andrushchenko 4 years, 5 months ago 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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@epam.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Juergen Gross <jgross@suse.com>
---
 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
     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
         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
         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
Re: [PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported
Posted by Stefano Stabellini 4 years, 4 months ago 
On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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@epam.com>
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>  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
>
Re: [PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported
Posted by Oleksandr Andrushchenko 4 years, 4 months ago 
Hi, Stefano!

On 10.09.21 22:06, Stefano Stabellini wrote:
> On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> 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@epam.com>
>> Cc: Ian Jackson <iwj@xenproject.org>
>> Cc: Juergen Gross <jgross@suse.com>
>> ---
>>   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
>>
Re: [PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported
Posted by Juergen Gross 4 years, 5 months ago 
On 03.09.21 10:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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@epam.com>
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>   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

Why #ifndef? Shouldn't this be #ifdef (same below multiple times)?


Juergen
Re: [PATCH 08/11] libxl: Only map legacy PCI IRQs if they are supported
Posted by Oleksandr Andrushchenko 4 years, 5 months ago 
Hello, Juergen!

On 03.09.21 13:26, Juergen Gross wrote:
> On 03.09.21 10:33, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> 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@epam.com>
>> Cc: Ian Jackson <iwj@xenproject.org>
>> Cc: Juergen Gross <jgross@suse.com>
>> ---
>>   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
>
> Why #ifndef? Shouldn't this be #ifdef (same below multiple times)?

Yes, you are right. I have to revert the logic, e.g. s/ifndef/ifdef

Other than that, are you ok with CONFIG_PCI_SUPP_LEGACY_IRQ name?

Thank you and sorry for the noise,

Oleksandr

>
>
> Juergen

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.