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.hu...@amd.com>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> ---
>
> 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.
