On 05/09/18 13:25, Jan Beulich wrote:
>>>> On 05.09.18 at 14:04, <andrew.coop...@citrix.com> wrote:
>> On 05/09/18 08:24, Jan Beulich wrote:
>>>>>> On 04.09.18 at 20:44, <andrew.coop...@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.coop...@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <jbeul...@suse.com>
>>>>> CC: Stefano Stabellini <sstabell...@kernel.org>
>>>>> CC: Julien Grall <julien.gr...@arm.com>
>>>>> CC: Wei Liu <wei.l...@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
