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.
