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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.19 at 11:17, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> There is one code path where I haven't managed to properly extract
> possible stubdomain in use:
> pci_remove_device()
>  -> pci_cleanup_msi()
>    -> msi_free_irqs()
>      -> msi_free_irq()
>        -> destroy_irq()
> 
> For now I've hardcoded hardware_domain there (in msi_free_irqs). Can it happen
> when device is still assigned to some domU?

In case this question is still open: No, it can't with current code,
and provided Dom0 behaves correctly.

> @@ -368,13 +369,13 @@ static int __init hpet_assign_irq(struct 
> hpet_event_channel *ch)
>  {
>      int irq;
>  
> -    if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
> +    if ( (irq = create_irq(NUMA_NO_NODE, hardware_domain)) < 0 )
>          return irq;
>  
>      ch->msi.irq = irq;
>      if ( hpet_setup_msi_irq(ch) )
>      {
> -        destroy_irq(irq);
> +        destroy_irq(irq, hardware_domain);
>          return -EINVAL;
>      }

Why don't you take the opportunity here (and elsewhere) and properly
remove hwdom access to such internal-to-Xen IRQs? Simply pass NULL
here, and skip permission granting in this case (create_irq() already
checks for NULL anyway).

> @@ -190,19 +190,19 @@ int create_irq(nodeid_t node)
>          desc->arch.used = IRQ_UNUSED;
>          irq = ret;
>      }
> -    else if ( hardware_domain )
> +    else if ( dm_domain )
>      {
> -        ret = irq_permit_access(hardware_domain, irq);
> +        ret = irq_permit_access(dm_domain, irq);

Doesn't this imply that Dom0 has no way of cleaning up after the
guest/stubdom pair? IOW I wonder whether both dm and hwdom
should be granted access.

> @@ -2095,7 +2099,9 @@ int map_domain_pirq(
>                  irq = info->arch.irq;
>              }
>              msi_desc->irq = -1;
> -            msi_free_irq(msi_desc);
> +            msi_free_irq(msi_desc,
> +                         current->domain->target == d ? current->domain
> +                                                      : hardware_domain);

Note how ->irq gets set to -1 prior to the call (and also in at least
one other instance), which will lead to skipping of the destroy_irq()
call, and hence skipping of the permission removal. Or wait, that's
going to be taken care of in the caller as it seems. If this is also
your understanding, then please add a sentence to the description
pointing this out. The split logic isn't really helpful here (I know it
was me who wrote it, in an attempt to avoid re-writing everything
basically from scratch).

Jan



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