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

[Anthony PERARD <anthony.perard@vates.tech>]

On Mon, Jul 08, 2024 at 07:41:24PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index a4029e3ac810..d869bbec769e 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1774,6 +1774,16 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>  {
>  }
>  
> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    return -1;

It's best to return an ERROR_* for libxl error code instead of -1.
ERROR_NI seems to be the one, it probably means not-implemented. Or
maybe ERROR_INVAL would do to.

> +}
> +
> +int libxl__arch_hvm_unmap_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    return -1;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da0794e..3d25997921cc 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,6 +1479,16 @@ static void pci_add_dm_done(libxl__egc *egc,
>      fclose(f);
>      if (!pci_supp_legacy_irq())
>          goto out_no_irq;
> +
> +    /*
> +     * When dom0 is PVH and mapping a x86 gsi to pirq for domU,
> +     * should use gsi to grant irq permission.
> +     */
> +    if (!libxl__arch_hvm_map_gsi(gc, pci_encode_bdf(pci), domid))

Could you store the result of libxl__arch_hvm_map_gsi() in `rc', then
test that in the condition?

> +        goto pci_permissive;

Why do you skip part of the function on success?
But also, please avoid the "goto" coding style, in libxl, it's tolerated
for error handling when used to skip to the end of function to have a
single path (or error path) out of a function.

> +    else
> +        LOGED(WARN, domid, "libxl__arch_hvm_map_gsi failed (err=%d)", errno);

No one reads logs unless there's a failure or something doesn't work. So
here we just ignore failure returned by libxl__arch_hvm_map_gsi(), is it
the right things to do? Usually, just ignoring error is wrong.

FYI: LOGE* already logs errno.

> +
>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                  pci->bus, pci->dev, pci->func);
>      f = fopen(sysfs_path, "r");
> @@ -1505,6 +1516,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>      }
>      fclose(f);
>  
> +pci_permissive:
>      /* Don't restrict writes to the PCI config space from this VM */
>      if (pci->permissive) {
>          if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
> @@ -2229,6 +2241,11 @@ skip_bar:
>      if (!pci_supp_legacy_irq())
>          goto skip_legacy_irq;
>  
> +    if (!libxl__arch_hvm_unmap_gsi(gc, pci_encode_bdf(pci), domid))
> +        goto skip_legacy_irq;
> +    else
> +        LOGED(WARN, domid, "libxl__arch_hvm_unmap_gsi failed (err=%d)", errno);
> +
>      sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                             pci->bus, pci->dev, pci->func);
>  
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 60643d6f5376..e7756d323cb6 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -879,6 +879,117 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>                                   libxl_defbool_val(src->b_info.u.hvm.pirq));
>  }
>  
> +struct pcidev_map_pirq {
> +    uint32_t sbdf;
> +    uint32_t pirq;
> +    XEN_LIST_ENTRY(struct pcidev_map_pirq) entry;
> +};
> +
> +static pthread_mutex_t pcidev_pirq_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static XEN_LIST_HEAD(, struct pcidev_map_pirq) pcidev_pirq_list =
> +    XEN_LIST_HEAD_INITIALIZER(pcidev_pirq_list);
> +
> +int libxl__arch_hvm_map_gsi(libxl__gc *gc, uint32_t sbdf, uint32_t domid)
> +{
> +    int pirq = -1, gsi, r;
> +    xc_domaininfo_t info;
> +    struct pcidev_map_pirq *pcidev_pirq;
> +    libxl_ctx *ctx = libxl__gc_owner(gc);

Instead of declaring "ctx", you can use the macro "CTX" when you need
"ctx".

> +
> +    r = xc_domain_getinfo_single(ctx->xch, LIBXL_TOOLSTACK_DOMID, &info);
> +    if (r < 0) {
> +        LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
> +        return r;

libxl_*() functions should return only libxl error code, that is return
code from other libxl_* functions, useally store in 'rc', or one of ERROR_*.

> +    }
> +    if ((info.flags & XEN_DOMINF_hvm_guest) &&
> +        !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ)) {
> +        gsi = xc_physdev_gsi_from_pcidev(ctx->xch, sbdf);
> +        if (gsi < 0) {
> +            return ERROR_FAIL;
> +        }
> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
> +        if (r < 0) {
> +            LOGED(ERROR, domid, "xc_physdev_map_pirq gsi=%d (error=%d)",
> +                  gsi, errno);
> +            return r;
> +        }
> +        r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +        if (r < 0) {
> +            LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d (error=%d)",
> +                  gsi, errno);
> +            return r;
> +        }
> +    } else {
> +        return ERROR_FAIL;

Is it really an error?

I few values can be returned here,
  * ERROR_INVAL meaing that the function was called on a dom0 that don't
    do "GSI",
  * 0, that is success, because the function check if it need to do
    anything, and since there's nothing to do, we can return success.

> +    }
> +
> +    /* Save the pirq for the usage of unmapping */
> +    pcidev_pirq = malloc(sizeof(struct pcidev_map_pirq));
> +    if (!pcidev_pirq) {
> +        LOGED(ERROR, domid, "no memory for saving pirq of pcidev info");
> +        return ERROR_NOMEM;
> +    }
> +    pcidev_pirq->sbdf = sbdf;
> +    pcidev_pirq->pirq = pirq;
> +
> +    assert(!pthread_mutex_lock(&pcidev_pirq_mutex));
> +    XEN_LIST_INSERT_HEAD(&pcidev_pirq_list, pcidev_pirq, entry);

I don't think that's going to work as you expect. libxl isn't a daemon
(or sometime it is but used for several domains), so anything store in
memory will be lost, or would be shared with other guest.

Do you need this mappins sbdf<-> pirq ? Is there a way to query this
information later from the environement? If not, you will need to store
the data somewhere else, probably in "libxl_domain_config *d_config" as
libxl can retrive the data with libxl__get_domain_configuration().
There's also the posibility to store that info in xenstore, but we
should probably avoid that.

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech