Re: [PATCH] tools/libxl: Skip invalid IRQs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2025 21:26, Jason Andryuk wrote:
> A PCI device's irq field is an 8-bit number.  A value of 0xff indicates
> that the device is not connected.

Nit: "... that the device IRQ is not ..."

>  Additionally, the Linux ACPI code can
> convert these 0xff values to IRQ_NOTCONNECTED(0x80000000) because
> "0x80000000 is guaranteed to be outside the available range of
> interrupts and easy to distinguish from other possible incorrect
> values."  When the hypercall to assign that IRQ fails, device
> passthrough as a whole fails.
> 
> Add checking for a valid IRQ and skip the IRQ handling for PCI devices
> outside that range.  This allows for passthrough of devices without
> legacy IRQs.

Which makes the code here even more Linux-centric, I guess.

A couple of related notes, yet most not directly affecting this patch:

> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -26,6 +26,9 @@
>  #define PCI_BDF_XSPATH         "%04x-%02x-%02x-%01x"
>  #define PCI_PT_QDEV_ID         "pci-pt-%02x_%02x.%01x"
>  
> +/* PCI Interrupt Line is an 8-bit value, 0xff means disconnected. */
> +#define PCI_IRQ_LINE_LIMIT     0xff
> +
>  static unsigned int pci_encode_bdf(libxl_device_pci *pci)
>  {
>      unsigned int value;
> @@ -1495,7 +1498,8 @@ static void pci_add_dm_done(libxl__egc *egc,
>              LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>              goto out_no_irq;
>          }
> -        if ((fscanf(f, "%u", &irq) == 1) && irq) {
> +        if ((fscanf(f, "%u", &irq) == 1) &&

For this, "irq" ought to be unsigned int. Same below.

> +            irq > 0 && irq < PCI_IRQ_LINE_LIMIT) {

Not sure about this in libxl's style, but it feels inconsistent to have
parentheses around one relational expression but then not around the
others. Personally I'd drop them all, but the alternative clearly is to
add missing ones.

>              r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>              if (r < 0) {
>                  LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d 
> (error=%d)",
> @@ -2257,7 +2261,8 @@ skip_bar:
>              goto skip_legacy_irq;
>          }
>  
> -        if ((fscanf(f, "%u", &irq) == 1) && irq) {
> +        if ((fscanf(f, "%u", &irq) == 1) &&
> +            irq > 0 && irq < PCI_IRQ_LINE_LIMIT) {
>              rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
>              if (rc < 0) {
>                  /*

This is doing things in sensible order: unmap, then remove permissions.
The map side though adds permissions only after mapping. That's kind of
necessary because the value to pass into xc_domain_irq_permission() is
an output of xc_physdev_map_pirq(). Yet then the latter should have
failed for lack of permissions, unless permissions were granted another
way? In which case what's the point of granting permissions here?

Jan