Re: Rats nest with domain pirq initialisation

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

On 05/09/18 13:25, Jan Beulich wrote:
>>>> On 05.09.18 at 14:04, <andrew.cooper3@citrix.com> wrote:
>> On 05/09/18 08:24, Jan Beulich wrote:
>>>>>> On 04.09.18 at 20:44, <andrew.cooper3@citrix.com> 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@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.
>>> 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?

_domain_destroy() is idempotent (for the very small quantity of state it
currently looks after).  The problem is that arch_domain_destroy() is
not idempotent, and needs needs to become so, and moving the
rangeset_new() calls as you originally suggested is not a fix for
arch_domain_destroy()'s idempotency bug.

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

Bypass which setup?  One way or another they have to bind the irq before
it can be used, so I still don't see why any structure preallocation is
needed.  (Reservation of legacy irq numbers, perahps.)

~Andrew

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