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

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

On 2024/5/16 22:01, Jan Beulich wrote:
> On 16.05.2024 11:52, Jiqian Chen wrote:
>> Some type of domain don't have PIRQ, like PVH, when
>> passthrough a device to guest on PVH dom0, callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
>> at domain_pirq_to_irq.
>>
>> So, add a new hypercall to grant/revoke gsi permission
>> when dom0 is not PV or dom0 has not PIRQ flag.
> 
> Honestly I find this hard to follow, and thus not really making clear why
> no other existing mechanism could be used.
Sorry, I will describe more clearly in next version.

> 
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
> 
> Below here in an RFC patch you typically would want to put specific items
> you're seeking feedback on. Without that it's hard to tell why this is
> marked RFC.
OK, I will add " RFC: wait for the third patch on kernel side is accepted." in 
next version.

> 
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -237,6 +237,37 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        int allow = domctl->u.gsi_permission.allow_access;
> 
> bool?
Will change.

> 
>> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
> 
> Such a restriction imo wants explaining in a comment.
Will add in next version.

> 
>> +        if ( gsi >= nr_irqs_gsi )
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        if ( !irq_access_permitted(current->domain, gsi) ||
> 
> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
> source overrides may be surfaced by ACPI?
All irqs smaller than nr_irqs_gsi are gsi, aren't they?

> 
>> +             xsm_irq_permission(XSM_HOOK, d, gsi, allow) )
> 
> Here I'm pretty sure you can't very well re-use an existing hook, as the
> value of interest is in a different numbering space, and a possible hook
> function has no way of knowing which one it is. Daniel?
> 
>> +        {
>> +            ret = -EPERM;
>> +            break;
>> +        }
>> +
>> +        if ( allow )
>> +            ret = irq_permit_access(d, gsi);
>> +        else
>> +            ret = irq_deny_access(d, gsi);
> 
> As above I'm afraid you can't assume IRQ == GSI.
> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -447,6 +447,13 @@ struct xen_domctl_irq_permission {
>>  };
>>  
>>  
>> +/* XEN_DOMCTL_gsi_permission */
>> +struct xen_domctl_gsi_permission {
>> +    uint32_t gsi;
>> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi 
>> access */
>> +};
> 
> Explicit padding please, including a check that it's zero on input.
Thanks, I will add in next version.

> 
>> +
>> +
>>  /* XEN_DOMCTL_iomem_permission */
> 
> No double blank lines please. In fact you will want to break the double blank
> lines in leading context, inserting in the middle.
Will remove one.
> 
> Jan

-- 
Best regards,
Jiqian Chen.