Re: [RFC XEN PATCH v14 5/5] tools: Add new function to do PIRQ (un)map on PVH dom0

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/9/4 01:16, Anthony PERARD wrote:
> On Tue, Sep 03, 2024 at 03:04:24PM +0800, Jiqian Chen wrote:
>> When dom0 is PVH, and passthrough a device to dumU, xl will
>> use the gsi number of device to do a pirq mapping, see
>> pci_add_dm_done->xc_physdev_map_pirq, but the gsi number is
>> got from file /sys/bus/pci/devices/<sbdf>/irq, that confuses
>> irq and gsi, they are in different space and are not equal,
>> so it will fail when mapping.
>> To solve this issue, use xc_physdev_gsi_from_dev to get the
> 
> The function name has changed, it's not xc_physdev_gsi_from_dev anymore
> ;-). It is always possible to write commit description without naming
> the functions that are going to be use, they are in the patch anyway.
> But, the description should be updated to reflect changes in previous
> patches.
Will change in next version, and include all your below comments, thank you 
very much!

> 
>> real gsi and add a new function xc_physdev_map_pirq_gsi to get
>> a free pirq for gsi(why not use current function
>> xc_physdev_map_pirq, because it doesn't support to allocate a
>> free pirq, what's more, to prevent changing it and affecting
>> its callers, so add xc_physdev_map_pirq_gsi).
>>
>> Besides, PVH dom0 doesn't have PIRQ flag, it doesn't do
>> PHYSDEVOP_map_pirq for each gsi. So grant function callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
>> domain_pirq_to_irq. And old hypercall XEN_DOMCTL_irq_permission
>> requires passing in pirq, it is not suitable for dom0 that
>> doesn't have PIRQs to grant irq permission.
>> To solve this issue, use the new hypercall
>> XEN_DOMCTL_gsi_permission to grant the permission of irq(
>> translate from gsi) to dumU when dom0 has no PIRQs.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Chen Jiqian <Jiqian.Chen@xxxxxxx>
>> ---
>> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
>> index 460a8e779ce8..c752cd1f4410 100644
>> --- a/tools/libs/ctrl/xc_physdev.c
>> +++ b/tools/libs/ctrl/xc_physdev.c
>> @@ -95,6 +95,33 @@ int xc_physdev_map_pirq_msi(xc_interface *xch,
>>      return rc;
>>  }
>>  
>> +int xc_physdev_map_pirq_gsi(xc_interface *xch,
>> +                            uint32_t domid,
>> +                            int gsi,
>> +                            int *pirq)
>> +{
>> +    int rc;
>> +    struct physdev_map_pirq map;
>> +
>> +    if ( !pirq )
>> +    {
>> +        errno = EINVAL;
>> +        return -1;
>> +    }
>> +    memset(&map, 0, sizeof(struct physdev_map_pirq));
>> +    map.domid = domid;
>> +    map.type = MAP_PIRQ_TYPE_GSI;
>> +    map.index = gsi;
> 
> You can write instead, when declaring `map`:
> 
>     struct physdev_map_pirq map = {
>         .domid = domid,
>         .type = MAP_PIRQ_TYPE_GSI,
>         .index = gsi,
>     };
> 
> Then there's no need to call memset(), the compiler will know what to
> do.
> 
>> +    map.pirq = *pirq;
>> +
>> +    rc = do_physdev_op(xch, PHYSDEVOP_map_pirq, &map, sizeof(map));
>> +
>> +    if ( !rc )
>> +        *pirq = map.pirq;
>> +
>> +    return rc;
>> +}
>> +
>>  int xc_physdev_unmap_pirq(xc_interface *xch,
>>                            uint32_t domid,
>>                            int pirq)
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da0794e..2014a67e6e56 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -17,6 +17,7 @@
>>  #include "libxl_osdeps.h" /* must come before any other headers */
>>  
>>  #include "libxl_internal.h"
>> +#include "libxl_arch.h"
>>  
>>  #define PCI_BDF                "%04x:%02x:%02x.%01x"
>>  #define PCI_BDF_SHORT          "%02x:%02x.%01x"
>> @@ -1478,32 +1479,43 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      fclose(f);
>>      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");
>> -    if (f == NULL) {
>> -        LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> -        goto out_no_irq;
>> -    }
>> -    if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> -        r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>> -        if (r < 0) {
>> -            LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> -                  irq, r);
>> -            fclose(f);
>> -            rc = ERROR_FAIL;
>> +
>> +    /* When dom0 is PVH, should use gsi to map pirq and grant permission */
>> +    rc = libxl__arch_local_domain_has_pirq_notion(gc);
>> +    if (!rc) {
> 
> Here, you can libxl__arch_local_domain_has_pirq_notion() within the if()
> condition because the function returns a bool. (Also, `rc` is for libxl
> error code, so we can make the mistake here in thinking that there an
> error code been been store.) Alternatively, you could declare "bool ok"
> and use that.
> 
> After that, it will be easier to read the condition has "if has pirq" or
> "if ok" instead of "if no error".
> 
> 
> 
>> +        rc = libxl__arch_hvm_map_gsi(gc, pci_encode_bdf(pci), domid);
>> +        if (rc) {
>> +            LOGED(ERROR, domainid, "libxl__arch_hvm_map_gsi failed");
> 
> I think LOGD() instead of LOGED() would be enough here.
> libxl__arch_hvm_map_gsi() already logs `strerror(errno)` so there's no
> need to print it again.
> 
>>              goto out;
>>          }
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> -        if (r < 0) {
>> -            LOGED(ERROR, domainid,
>> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> -            fclose(f);
>> -            rc = ERROR_FAIL;
>> -            goto out;
>> +    } else {
>> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> +                                    pci->bus, pci->dev, pci->func);
>> +        f = fopen(sysfs_path, "r");
>> +        if (f == NULL) {
>> +            LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> +            goto out_no_irq;
>> +        }
>> +        if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +            r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>> +            if (r < 0) {
>> +                LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d 
>> (error=%d)",
>> +                    irq, r);
>> +                fclose(f);
>> +                rc = ERROR_FAIL;
>> +                goto out;
>> +            }
>> +            r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +            if (r < 0) {
>> +                LOGED(ERROR, domainid,
>> +                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> +                fclose(f);
>> +                rc = ERROR_FAIL;
>> +                goto out;
>> +            }
>>          }
>> +        fclose(f);
>>      }
>> -    fclose(f);
>>  
>>      /* Don't restrict writes to the PCI config space from this VM */
>>      if (pci->permissive) {
>> @@ -2229,33 +2241,43 @@ skip_bar:
>>      if (!pci_supp_legacy_irq())
>>          goto skip_legacy_irq;
>>  
>> -    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> -                           pci->bus, pci->dev, pci->func);
>> -
>> -    f = fopen(sysfs_path, "r");
>> -    if (f == NULL) {
>> -        LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);
>> -        goto skip_legacy_irq;
>> -    }
>> +    /* When dom0 is PVH, should use gsi to unmap pirq and deny permission */
>> +    rc = libxl__arch_local_domain_has_pirq_notion(gc);
>> +    if (!rc) {
>> +        rc = libxl__arch_hvm_unmap_gsi(gc, pci_encode_bdf(pci), domid);
>> +        if (rc) {
>> +            LOGED(ERROR, domid, "libxl__arch_hvm_unmap_gsi failed");
>> +            goto out;
>> +        }
>> +    } else {
>> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> +                            pci->bus, pci->dev, pci->func);
>>  
>> -    if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> -        rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
>> -        if (rc < 0) {
>> -            /*
>> -             * QEMU may have already unmapped the IRQ. So the error
>> -             * may be spurious. For now, still print an error message as
>> -             * it is not easy to distinguished between valid and
>> -             * spurious error.
>> -             */
>> -            LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>> +        f = fopen(sysfs_path, "r");
>> +        if (f == NULL) {
>> +            LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);
>> +            goto skip_legacy_irq;
>>          }
>> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>> -        if (rc < 0) {
>> -            LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
>> +
>> +        if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +            rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
>> +            if (rc < 0) {
>> +                /*
>> +                * QEMU may have already unmapped the IRQ. So the error
> 
> The * ...     here ^ should be aligned with the * on the previous line.
> 
>> +                * may be spurious. For now, still print an error message as
>> +                * it is not easy to distinguished between valid and
>> +                * spurious error.
>> +                */
>> +                LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>> +            }
>> +            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>> +            if (rc < 0) {
>> +                LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
>> +            }
>>          }
>> -    }
>>  
>> -    fclose(f);
>> +        fclose(f);
>> +    }
>>  
>>  skip_legacy_irq:
>>  
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 60643d6f5376..20e3956f09b8 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
>> +{
>> +    int pirq = -1, gsi, r;
>> +
>> +    gsi = xc_pcidev_get_gsi(CTX->xch, sbdf);
>> +    if (gsi < 0) {
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    r = xc_physdev_map_pirq_gsi(CTX->xch, domid, gsi, &pirq);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "xc_physdev_map_pirq_gsi gsi=%d ret=%d", gsi, 
>> r);
> 
> `r` should be -1, I don't think loggin it is useful..
> 
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    r = xc_domain_gsi_permission(CTX->xch, domid, gsi, 
>> XEN_DOMCTL_GSI_GRANT);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d ret=%d", gsi, 
>> r);
> 
> Same here. And in the next function.
> 
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
>> +{
>> +    int pirq = -1, gsi, r;
>> +
>> +    gsi = xc_pcidev_get_gsi(CTX->xch, sbdf);
>> +    if (gsi < 0) {
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    /* Before unmapping, use mapping to get the already mapped pirq first */
>> +    r = xc_physdev_map_pirq_gsi(CTX->xch, domid, gsi, &pirq);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "xc_physdev_map_pirq_gsi gsi=%d ret=%d", gsi, 
>> r);
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    r = xc_physdev_unmap_pirq(CTX->xch, domid, pirq);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "xc_physdev_unmap_pirq gsi=%d ret=%d", gsi, r);
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    r = xc_domain_gsi_permission(CTX->xch, domid, gsi, 
>> XEN_DOMCTL_GSI_REVOKE);
>> +    if (r < 0) {
>> +        LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d ret=%d", gsi, 
>> r);
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.