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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.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.

> --- 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