[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/3] [not-for-unstable] xen/arm: vgic-v3: Delay the initialization of the domain information
On Sat, 29 Sep 2018, Andrew Cooper wrote: > On 28/09/18 21:35, Julien Grall wrote: > > > > > > On 09/28/2018 12:11 AM, Stefano Stabellini wrote: > >> On Wed, 26 Sep 2018, Julien Grall wrote: > >>> Hi Stefano, > >>> > >>> On 09/25/2018 09:45 PM, Stefano Stabellini wrote: > >>>> On Tue, 4 Sep 2018, Andrew Cooper wrote: > >>>>> On 04/09/18 20:35, Julien Grall wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 09/04/2018 08:21 PM, Julien Grall wrote: > >>>>>>> A follow-up patch will require to know the number of vCPUs when > >>>>>>> initializating the vGICv3 domain structure. However this > >>>>>>> information > >>>>>>> is > >>>>>>> not available at domain creation. This is only known once > >>>>>>> XEN_DOMCTL_max_vpus is called for that domain. > >>>>>>> > >>>>>>> In order to get the max vCPUs around, delay the domain part of the > >>>>>>> vGIC > >>>>>>> v3 initialization until the first vCPU of the domain is > >>>>>>> initialized. > >>>>>>> > >>>>>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx> > >>>>>>> > >>>>>>> --- > >>>>>>> > >>>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > >>>>>>> > >>>>>>> This is nasty but I can't find a better way for Xen 4.11 and older. > >>>>>>> This > >>>>>>> is not necessary for unstable as the number of vCPUs is known at > >>>>>>> domain > >>>>>>> creation. > >>>>>>> > >>>>>>> Andrew, I have CCed you to know whether you have a better idea > >>>>>>> where > >>>>>>> to > >>>>>>> place this call on Xen 4.11 and older. > >>>>>> > >>>>>> I just noticed that d->max_vcpus is initialized after > >>>>>> arch_domain_create. So without this patch on Xen 4.12, it will > >>>>>> not work. > >>>>>> > >>>>>> This is getting nastier because arch_domain_init is the one > >>>>>> initialize > >>>>>> the value returned by dom0_max_vcpus. So I am not entirely sure what > >>>>>> to do here. > >>>>> > >>>>> The positioning after arch_domain_create() is unfortunate, but I > >>>>> couldn’t manage better with ARM's current behaviour and Jan's > >>>>> insistence > >>>>> that the allocation of d->vcpu was common. I'd prefer if the > >>>>> dependency > >>>>> could be broken and the allocation moved earlier. > >>>>> > >>>>> One option might be to have an arch_check_domainconfig() (or > >>>>> similar?) > >>>>> which is called very early on and can sanity check the values, > >>>>> including > >>>>> cross-checking the vgic and max_vcpus settings? It could even be > >>>>> responsible for mutating XEN_DOMCTL_CONFIG_GIC_NATIVE into the > >>>>> correct > >>>>> real value. > >>>>> > >>>>> As for your patch here, its a gross hack, but its probably the best > >>>>> which can be done. > >>>> > >>>> *Sighs* > >>>> If that is what we have to do, it is as ugly as hell, but that is what > >>>> we'll do. > >>> > >>> This is the best we can do with the current code base. I think it > >>> would be > >>> worth reworking the code to make it nicer. I will add it in my TODO > >>> list. > >>> > >>>> > >>>> My only suggestion to marginally improve it would be instead of: > >>>> > >>>>> + if ( v->vcpu_id == 0 ) > >>>>> + { > >>>>> + rc = vgic_v3_real_domain_init(d); > >>>>> + if ( rc ) > >>>>> + return rc; > >>>>> + } > >>>> > >>>> to check on d->arch.vgic.rdist_regions instead: > >>>> > >>>> if ( d->arch.vgic.rdist_regions == NULL ) > >>>> { > >>>> // initialize domain > >>> > >>> I would prefer to keep v->vcpu_id == 0 just in case we end up to > >>> re-order the > >>> allocation in the future. > >> > >> I was suggesting to check on (rdist_regions == NULL) exactly for > >> potential re-ordering, in case in the future we end up calling > >> vcpu_vgic_init differently and somehow vcpu_init(vcpu1) is done before > >> before vcpu_init(vcpu0). Ideally we would like a way to check that > >> vgic_v3_real_domain_init has been called before and I thought > >> rdist_regions == NULL could do just that... > > > > What I meant by re-ordering is we manage to allocate the > > re-distributors before the vCPUs are created but still need > > vgic_v3_real_domain_init for other purpose. > > > > But vCPU initialization is potentially other issue. > > > > Anyway. both way have drawbacks. Yet I still prefer checking on the > > vCPU. It less likely vCPU0 will not be the first one initialized. > > With the exception of the idle domain, all vcpus are strictly allocated > in packed ascending order. Loads of other stuff will break if that > changed, so I wouldn't worry about it. > > Furthermore, there is no obvious reason for this behaviour to ever change. OK, let's go with Julien's patch. We need a new tag for this, something like: Acked-but-disliked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> :-) _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |