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

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

On 2024/7/8 22:57, Anthony PERARD wrote:
> 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.
Seems ERROR_INVAL is more suitable. Will change in next version.

> 
>> +}
>> +
>> +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?
Will change in next version.
> 
>> +        goto pci_permissive;
> 
> Why do you skip part of the function on success?
Because libxl__arch_hvm_map_gsi do the same thing for PVH dom0, and the 
following part is for PV dom0.
If libxl__arch_hvm_map_gsi success, it should skip the following part.

> 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.
Maybe I should split the part " xc_domain_getinfo_single(ctx->xch, 
LIBXL_TOOLSTACK_DOMID, &info); " in libxl__arch_hvm_map_gsi to a single 
function.
Then I can distinguish PVH and PV, and do different things for them.

> 
>> +    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.
Will change in next version.
> 
> 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".
Will change in next version.

> 
>> +
>> +    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_*.
OK, will change in next version.

> 
>> +    }
>> +    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",
I think this is more suitable. And then the following code of PV can be done in 
pci_add_dm_done.

>   * 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 ? 
I need to store the pirq that assigned to the gsi. Because 
libxl__arch_hvm_unmap_gsi need pirq to do xc_physdev_unmap_pirq

> Is there a way to query this information later from the environement? 
What I can think of is before xc_physdev_unmap_pirq, use xc_physdev_map_pirq to 
get the already mapped pirq, but I am not sure if it is suitable.

> 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().
However, pirq is dynamically mapped during starting domU, it may not be 
suitable for saving in d_config.

> There's also the posibility to store that info in xenstore, but we
> should probably avoid that.
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.