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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.18 at 14:04, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/09/18 08:24, Jan Beulich wrote:
>>>>> 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?
> 
> No - that defeats the purpose of making the destroy path idempotent. 
> For us to remove the max_vcpus hypercall, _domain_destroy() must be
> capable of correctly cleaning up a domain from any state of
> initialisation, including if the relevant init calls haven't been made yet.

I agree up to here.

> These rangeset_new() calls cannot move earlier than the first action
> which might fail (which is the XSM init call to get the security label
> correct).

But I must be overlooking something crucial here: If _domain_destroy()
was idempotent, how does it matter at what point the rangesets get
initialized?

>>> 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().
> 
> It is pirq 2 which explodes, which is the first of the redundant pirq
> structures allocated for legacy routing.
> 
> I'm not sure I understand this code well enough to comment on why
> domain_pirq_to_irq() returns a positive value at this point, but I'm
> going to go out on a limb and suggest it might be related to our
> unnecessary(?) preallocation.

I've meanwhile considered this as the reason, too. And iirc the
pre-allocation is because guests (including Dom0) bypass some of
the setup they would do for non-legacy IRQs. This may have been
just a XenoLinux (mis)behavior, but even then I'm not convinced
we could easily alter things.

Jan


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