Re: [Xen-devel] [PATCH v6 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2019 17:37, Marek Marczykowski-Górecki  wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -254,7 +254,13 @@ void __init clear_irq_vector(int irq)
>  /*
>   * Dynamic irq allocate and deallocation for MSI
>   */
> -int create_irq(nodeid_t node)
> +
> +/*
> + * create_irq - allocate irq for MSI
> + * @d domain that will get permission over the allocated irq; this permission
> + * will automatically be revoked on destroy_irq
> + */
> +int create_irq(nodeid_t node, struct domain *d)

I think there's nothing wrong with the pointer getting constified,
but see also below.

> @@ -282,23 +288,30 @@ int create_irq(nodeid_t node)
>          }
>          ret = assign_irq_vector(irq, mask);
>      }
> +    ASSERT(desc->arch.creator_domid == DOMID_INVALID);
>      if (ret < 0)

I think this insertion wants to gain blank lines on both sides.

>      {
>          desc->arch.used = IRQ_UNUSED;
>          irq = ret;
>      }
> -    else if ( hardware_domain )
> +    else if ( d )
>      {
> -        ret = irq_permit_access(hardware_domain, irq);
> +        ASSERT(d == current->domain);

Why pass in the domain then in the first place? Could by just a
boolean, couldn't it? Suitably named it might even eliminate
the need for the explanatory comment (see also below).

> +        ret = irq_permit_access(d, irq);
>          if ( ret )
>              printk(XENLOG_G_ERR
> -                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
> -                   irq, ret);
> +                   "Could not grant Dom%u access to IRQ%d (error %d)\n",
> +                   d->domain_id, irq, ret);

Please use %pd here (and elsewhere).

> +        else
> +            desc->arch.creator_domid = d->domain_id;
>      }
>  
>      return irq;
>  }
>  
> +/*
> + * destroy_irq - deallocate irq for MSI
> + */
>  void destroy_irq(unsigned int irq)

I don't think this is a very helpful comment to add; in fact I think
the respective part on the other function would better be dropped as
well, seeing the further comment ahead of both functions. (Otherwise
I'd have to point out that this is a single line comment.)

> @@ -307,14 +320,25 @@ void destroy_irq(unsigned int irq)
>  
>      BUG_ON(!MSI_IRQ(irq));
>  
> -    if ( hardware_domain )
> +    if ( desc->arch.creator_domid != DOMID_INVALID )
>      {
> -        int err = irq_deny_access(hardware_domain, irq);
> +        struct domain *d = get_domain_by_id(desc->arch.creator_domid);
>  
> -        if ( err )
> -            printk(XENLOG_G_ERR
> -                   "Could not revoke Dom0 access to IRQ%u (error %d)\n",
> -                   irq, err);
> +        if ( d && irq_access_permitted(d, irq) )
> +        {
> +            int err;
> +
> +            err = irq_deny_access(d, irq);

Please keep prior code structure, i.e. the function call being the
initializer of the variable.

> +            if ( err )
> +                printk(XENLOG_G_ERR
> +                       "Could not revoke Dom%u access to IRQ%u (error %d)\n",
> +                       d->domain_id, irq, err);
> +        }

Why the irq_access_permitted() check around this? You go to some
lengths to explain this in the description, but if the domain has
no permission over the IRQ (because of domain ID re-use),
irq_deny_access() will simply do nothing, won't it? I.e. the way
this gets done and explained (saying that MSI IRQs can't be
shared between domains) wants to change a little.

> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -45,6 +45,11 @@ struct arch_irq_desc {
>          unsigned move_cleanup_count;
>          u8 move_in_progress : 1;
>          s8 used;
> +        /*
> +         * weak reference to domain having permission over this IRQ (which 
> can
> +         * be different from the domain actually havint the IRQ assigned)
> +         */
> +        domid_t creator_domid;

Comment style (should start with a capital letter).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel