| Message ID | 20180904192153.17210-2-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: vgic-v3: Bug fixes | expand |
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@arm.com> > > --- > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > 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. Cheers,
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@arm.com> >> >> --- >> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> >> 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. I have to admit that I'm surprised/concerned that Xen has lasted this long with such a fundamental gap in domain creation. ~Andrew
Hi Andrew, On 09/04/2018 08:53 PM, 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@arm.com> >>> >>> --- >>> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> 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. I think it is doable without too much trouble on Arm. I can have a look at this for Xen 4.12 once this series is merged. > > As for your patch here, its a gross hack, but its probably the best > which can be done. I have to admit that I'm surprised/concerned that > Xen has lasted this long with such a fundamental gap in domain creation. I am surprised too. Hopefully your rework will make everything easier to use. Cheers,
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@arm.com> > >> > >> --- > >> > >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> > >> 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. 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
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@arm.com> >>>> >>>> --- >>>> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>> >>>> 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. Cheers,
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@arm.com> > > > > > > > > > > --- > > > > > > > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > > > > > > > > > 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...
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@arm.com> >>>>>> >>>>>> --- >>>>>> >>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> >>>>>> 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. Cheers,
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@arm.com> >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>> >>>>>>> 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. ~Andrew
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@arm.com> > >>>>>>> > >>>>>>> --- > >>>>>>> > >>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >>>>>>> > >>>>>>> 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@kernel.org> :-)
On 29/09/18 00:45, Stefano Stabellini wrote: > 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@arm.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>>> >>>>>>>>> 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@kernel.org> Do bear in mind that this patch is only for 4.11 and earlier. I've already fixed staging (i.e. 4.12) when it comes to knowing d->max_vcpus :) ~Andrew
Hi, On 09/29/2018 12:48 AM, Andrew Cooper wrote: > On 29/09/18 00:45, Stefano Stabellini wrote: >> 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@arm.com> >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>>>> >>>>>>>>>> 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@kernel.org> > > Do bear in mind that this patch is only for 4.11 and earlier. I've > already fixed staging (i.e. 4.12) when it comes to knowing d->max_vcpus :) I thought we agreed that patch is necessary for 4.12 as d->max_vcpus is initialized after arch_domain_init? I am not planning to do the rework in short term. Did you do more work on around domain_create recently? Cheers,
On 01/10/18 10:43, Julien Grall wrote: > Hi, > > On 09/29/2018 12:48 AM, Andrew Cooper wrote: >> On 29/09/18 00:45, Stefano Stabellini wrote: >>> 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@arm.com> >>>>>>>>>>> >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>>>>> >>>>>>>>>>> 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@kernel.org> >> >> Do bear in mind that this patch is only for 4.11 and earlier. I've >> already fixed staging (i.e. 4.12) when it comes to knowing >> d->max_vcpus :) > I thought we agreed that patch is necessary for 4.12 as d->max_vcpus > is initialized after arch_domain_init? Oh right. > I am not planning to do the rework in short term. Did you do more work > on around domain_create recently? There are multiple related patch series out on xen-devel atm, but I expect I need to spin a new version of each of them. I'll see if I have some time to put towards it. Are you happy in principle with the arch_check_domainconfig() plan? ~Andrew
Hi Andrew, On 10/01/2018 10:53 AM, Andrew Cooper wrote: > On 01/10/18 10:43, Julien Grall wrote: >> Hi, >> >> On 09/29/2018 12:48 AM, Andrew Cooper wrote: >>> On 29/09/18 00:45, Stefano Stabellini wrote: >>>> 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@arm.com> >>>>>>>>>>>> >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>>>>>> >>>>>>>>>>>> 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@kernel.org> >>> >>> Do bear in mind that this patch is only for 4.11 and earlier. I've >>> already fixed staging (i.e. 4.12) when it comes to knowing >>> d->max_vcpus :) >> I thought we agreed that patch is necessary for 4.12 as d->max_vcpus >> is initialized after arch_domain_init? > > Oh right. > >> I am not planning to do the rework in short term. Did you do more work >> on around domain_create recently? > > There are multiple related patch series out on xen-devel atm, but I > expect I need to spin a new version of each of them. I'll see if I have > some time to put towards it. Are you happy in principle with the > arch_check_domainconfig() plan? I am happy in principle. If you don't have time to work on it, I will try to have a look before Xen 4.12. Cheers,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 4b42739a52..df1bab3a35 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1573,9 +1573,11 @@ static const struct mmio_handler_ops vgic_distr_mmio_handler = { .write = vgic_v3_distr_mmio_write, }; +static int vgic_v3_real_domain_init(struct domain *d); + static int vgic_v3_vcpu_init(struct vcpu *v) { - int i; + int i, rc; paddr_t rdist_base; struct vgic_rdist_region *region; unsigned int last_cpu; @@ -1584,6 +1586,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v) struct domain *d = v->domain; /* + * This is the earliest place where the number of vCPUs is + * known. This is required to initialize correctly the vGIC v3 + * domain structure. We only to do that when vCPU 0 is + * initilialized. + */ + if ( v->vcpu_id == 0 ) + { + rc = vgic_v3_real_domain_init(d); + if ( rc ) + return rc; + } + + /* * Find the region where the re-distributor lives. For this purpose, * we look one region ahead as we have only the first CPU in hand. */ @@ -1641,7 +1656,7 @@ static inline unsigned int vgic_v3_rdist_count(struct domain *d) GUEST_GICV3_RDIST_REGIONS; } -static int vgic_v3_domain_init(struct domain *d) +static int vgic_v3_real_domain_init(struct domain *d) { struct vgic_rdist_region *rdist_regions; int rdist_count, i, ret; @@ -1733,6 +1748,16 @@ static int vgic_v3_domain_init(struct domain *d) return 0; } +static int vgic_v3_domain_init(struct domain *d) +{ + /* + * The domain initialization for vGIC v3 is delayed until the first vCPU + * is created. This because the initialization may require to know the + * number of vCPUs that is not known when creating the domain. + */ + return 0; +} + static void vgic_v3_domain_free(struct domain *d) { vgic_v3_its_free_domain(d);
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@arm.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> 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. --- xen/arch/arm/vgic-v3.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)