[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Rats nest with domain pirq initialisation
On 05/09/18 13:25, Jan Beulich wrote: >>>> 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? _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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |