Re: [Xen-devel] Rats nest with domain pirq initialisation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.18 at 20:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>>  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.

Moving this up looks reasonable to me. "Simple" initialization can
certainly be done early (i.e. before arch_domain_create()), don't
you think?

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

But what IRQ do we find to unmap here? There can't be any that have
been mapped, when ->irq_caps is still NULL. IOW I don't currently see
how domain_pirq_to_irq() would legitimately return a positive value at
this point in time, yet that's what guards the calls to unmap_domain_pirq().

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

+1

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

Wasn't it that it was symmetric originally, and the grant/map side has been
split perhaps a couple of years ago? If so, the unmap side splitting was
perhaps simply missed?

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

As per above - I think either free_domain_pirqs() should gain a single
such NULL check, or domain_pirq_to_irq() should be made sure doesn't
return positive values prior to ->irq_caps having been set up.

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

I vaguely recall there was some hope to get rid of this, but I don't
recall the prereqs necessary.

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

Space saving, yes. Just like it is forbidden to access d->arch.hvm
for a PV d, accessing pirq->arch.hvm is forbidden to access for a
PV domain's pirq. What point is there to allocate the space then?

Jan


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