Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

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

On 2023/12/15 16:29, Roger Pau Monné wrote:
> On Fri, Dec 15, 2023 at 07:20:24AM +0000, Chen, Jiqian wrote:
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index d3507d13a029..f665d17afbf5 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1486,6 +1486,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +        int gsi = 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)",
>> @@ -1494,7 +1495,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid,
>>                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index f5a71ee5f78d..782c4a7a70a4 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -658,7 +658,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>> u_domctl)
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        irq = pirq_access_permitted(current->domain, pirq);
>> +
>> +        if ( is_hvm_domain(current->domain) )
>> +            irq = pirq;
>> +        else
>> +            irq = pirq_access_permitted(current->domain, pirq);
> 
> You are dropping an irq_access_permitted() check here for the HVM
> case, as pirq_access_permitted() translates from pirq to irq and also
> checks for permissions.
> 
> This would need to be something along the lines of:
> 
> irq = 0;
> if ( is_hvm_domain(current->domain) &&
>      irq_access_permitted(current->domain, pirq) )
Oh, yes, it should add this check.

>     irq = pirq;
> else
>     irq = pirq_access_permitted(current->domain, pirq);
> 
> And then I wonder whether it wouldn't be best to uniformly use a GSI
> for both PV and HVM.
If we only look at the value(seems the number of gsi == pirq == irq in PV), it 
seems that gsi can also be used uniformly for PV.
And then here should be. 
if ( irq_access_permitted(current->domain, pirq) )
        irq = pirq;
else
{
        ret = -EPERM;
        break;
}

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.