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.
> 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.
> --- 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?
> + if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
> + {
> + ret = -EOPNOTSUPP;
> + break;
> + }
Such a restriction imo wants explaining in a comment.
> + 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?
> + 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.
> +
> +
> /* 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.
Jan
