Re: [XEN PATCH v10 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

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

On 2024/6/17 23:32, Jan Beulich wrote:
> On 17.06.2024 11:00, Jiqian Chen wrote:
>> Some type of domain don't have PIRQs, like PVH, it doesn't do
>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>> to guest base on PVH dom0, callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq and
>> irq on Xen side.
>> What's more, current hypercall XEN_DOMCTL_irq_permission requires
>> passing in pirq, it is not suitable for dom0 that doesn't have
>> PIRQs.
>>
>> So, add a 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: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> RFC: it needs review and needs to wait for the corresponding third patch on 
>> linux kernel side to be merged.
>> ---
>>  tools/include/xenctrl.h            |  5 +++
>>  tools/libs/ctrl/xc_domain.c        | 15 +++++++
>>  tools/libs/light/libxl_pci.c       | 67 +++++++++++++++++++++++++++---
>>  xen/arch/x86/domctl.c              | 43 +++++++++++++++++++
>>  xen/arch/x86/include/asm/io_apic.h |  2 +
>>  xen/arch/x86/io_apic.c             | 17 ++++++++
>>  xen/arch/x86/mpparse.c             |  3 +-
>>  xen/include/public/domctl.h        |  8 ++++
>>  xen/xsm/flask/hooks.c              |  1 +
>>  9 files changed, 153 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
>> index a0381f74d24b..f3feb6848e25 100644
>> --- a/tools/include/xenctrl.h
>> +++ b/tools/include/xenctrl.h
>> @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch,
>>                               uint32_t pirq,
>>                               bool allow_access);
>>  
>> +int xc_domain_gsi_permission(xc_interface *xch,
>> +                             uint32_t domid,
>> +                             uint32_t gsi,
>> +                             bool allow_access);
>> +
>>  int xc_domain_iomem_permission(xc_interface *xch,
>>                                 uint32_t domid,
>>                                 unsigned long first_mfn,
>> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
>> index f2d9d14b4d9f..8540e84fda93 100644
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch,
>>      return do_domctl(xch, &domctl);
>>  }
>>  
>> +int xc_domain_gsi_permission(xc_interface *xch,
>> +                             uint32_t domid,
>> +                             uint32_t gsi,
>> +                             bool allow_access)
>> +{
>> +    struct xen_domctl domctl = {
>> +        .cmd = XEN_DOMCTL_gsi_permission,
>> +        .domain = domid,
>> +        .u.gsi_permission.gsi = gsi,
>> +        .u.gsi_permission.allow_access = allow_access,
>> +    };
>> +
>> +    return do_domctl(xch, &domctl);
>> +}
>> +
>>  int xc_domain_iomem_permission(xc_interface *xch,
>>                                 uint32_t domid,
>>                                 unsigned long first_mfn,
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 376f91759ac6..f027f22c0028 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
>> +#ifdef CONFIG_X86
>> +    xc_domaininfo_t info;
>> +#endif
>>  
>>      /* Convenience aliases */
>>      bool starting = pas->starting;
>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc,
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +#ifdef CONFIG_X86
>> +        /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission 
>> */
>> +        r = xc_domain_getinfo_single(ctx->xch, 0, &info);
> 
> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but
> I didn't check if that can be used with the underlying hypercall(s). Otherwise
> you want to pass the actual domid of the local domain here.
But the action of granting permission is from dom0 to domU, what I need to get 
is the infomation of dom0,
The actual domid here is domU's id I think, it is not useful.

> 
>>          if (r < 0) {
>> -            LOGED(ERROR, domainid,
>> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> +            LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", 
>> errno);
>>              fclose(f);
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
>> +        if (info.flags & XEN_DOMINF_hvm_guest &&
> 
> You want to parenthesize the & here.
Will change in next version.

> 
>> +            !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) &&
>> +            gsi > 0) {
> 
> So if gsi < 0 failure of xc_domain_getinfo_single() would needlessly result
> in failure of this function?
In next version, the judgment of gsi>0 will be placed in the next layer of if, 
and then the error will be handled.

> 
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -36,6 +36,7 @@
>>  #include <asm/xstate.h>
>>  #include <asm/psr.h>
>>  #include <asm/cpu-policy.h>
>> +#include <asm/io_apic.h>
>>  
>>  static int update_domain_cpu_policy(struct domain *d,
>>                                      xen_domctl_cpu_policy_t *xdpc)
>> @@ -237,6 +238,48 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        int irq;
>> +        bool allow = domctl->u.gsi_permission.allow_access;
> 
> See my earlier comments on this conversion of 8 bits into just one.
Do you mean that I need to check allow_access is >= 0?
But allow_access is u8, it can't be negative.

> 
>> +        /* Check all pads are zero */
>> +        ret = -EINVAL;
>> +        for ( i = 0;
>> +              i < sizeof(domctl->u.gsi_permission.pad) /
>> +                  sizeof(domctl->u.gsi_permission.pad[0]);
> 
> Please don't open-code ARRAY_SIZE().
Will change in next version.

> 
>> +              ++i )
>> +            if ( domctl->u.gsi_permission.pad[i] )
>> +                goto out;
>> +
>> +        /*
>> +         * If current domain is PV or it has PIRQ flag, it has a mapping
>> +         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
>> +         * to grant irq permission.
>> +         */
>> +        ret = -EOPNOTSUPP;
>> +        if ( is_pv_domain(currd) || has_pirq(currd) )
>> +            goto out;
> 
> I'm curious what other x86 maintainers think: I for one would not impose such
> an artificial restriction.
Emmm, so I need to remove this check.

> 
>> +        ret = -EINVAL;
>> +        if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 )
>> +            goto out;
>> +
>> +        ret = -EPERM;
>> +        if ( !irq_access_permitted(currd, irq) ||
>> +             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
>> +            goto out;
>> +
>> +        if ( allow )
>> +            ret = irq_permit_access(d, irq);
>> +        else
>> +            ret = irq_deny_access(d, irq);
>> +
>> +    out:
> 
> Please use a less generic name for such a label local to just one case
> block. However, with ..
Ok, will change in next version.

> 
>> +        break;
> 
> .. this being all that's done here: Why have a label in the first place?
> 
> Jan

-- 
Best regards,
Jiqian Chen.