Rats nest with domain pirq initialisation

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/08/18 11:01, Andrew Cooper wrote:
> This is in preparation to set up d->max_cpus and d->vcpu[] in domain_create(),
> and allow later parts of domain construction to have access to the values.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/common/domain.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index be51426..0c44f27 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -322,6 +322,23 @@ struct domain *domain_create(domid_t domid,
>          else
>              d->guest_type = guest_type_pv;
>  
> +        if ( !is_hardware_domain(d) )
> +            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> +        else
> +            d->nr_pirqs = extra_hwdom_irqs ? nr_static_irqs + extra_hwdom_irqs
> +                                           : arch_hwdom_irqs(domid);
> +        if ( d->nr_pirqs > nr_irqs )
> +            d->nr_pirqs = nr_irqs;
> +
> +        radix_tree_init(&d->pirq_tree);
> +    }
> +
> +    if ( (err = arch_domain_create(d, config)) != 0 )
> +        goto fail;
> +    init_status |= INIT_arch;
> +
> +    if ( !is_idle_domain(d) )
> +    {
>          watchdog_domain_init(d);
>          init_status |= INIT_watchdog;
>  
> @@ -352,16 +369,6 @@ struct domain *domain_create(domid_t domid,

Between these two hunks is:

        d->iomem_caps = rangeset_new(d, "I/O Memory",
RANGESETF_prettyprint_hex);
        d->irq_caps   = rangeset_new(d, "Interrupts", 0);

which is important, because it turns out that x86's
arch_domain_destroy() depends on d->irq_caps already being initialised.

The path which blows up is:

arch_domain_destroy()
  free_domain_pirqs()
    unmap_domain_pirq()
      irq_deny_access()
        rangeset_remove_singleton((d)->irq_caps, i)

Unlike the boolean-nature rangeset_contains_*() helpers, I don't think
it is reasonable to make rangeset_remove_*() tolerate a NULL rangeset.

The behaviour of automatically revoking irq access is dubious at best. 
It is asymmetric with the XEN_DOMCTL_irq_permission, and a caller would
reasonably expect not to have to re-grant identical permissions as the
irq is mapped/unmapped.  Does anyone know why we have this suspect
behaviour in the first place?

One way or another, this path needs to become idempotent, but simply
throwing some NULL pointer checks into unmap_domain_pirq() doesn't feel
like the right thing to do.


A separate mess is that we appear to allocate full pirq structures for
all legacy irqs for every single domain, in init_domain_irq_mapping(). 
At the very least, this is wasteful as very few domains get access to
real hardware in the first place.

The other thing I notice is that alloc_pirq_struct() is downright
dangerous, as it deliberately tries to allocate half a struct pirq for
the !hvm case.  I can only assume this is a space saving measure, but
there is absolutely no help in the commit message which introduced it
(c/s c24536b636f).

~Andrew

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