| Message ID | 20240226030146.179465-1-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Set correct per-cpu cpu_core_mask | expand |
Hi Henry, On 26/02/2024 04:01, Henry Wang wrote: > In the common sysctl command XEN_SYSCTL_physinfo, the value of > cores_per_socket is calculated based on the cpu_core_mask of CPU0. > Currently on Arm this is a fixed value 1 (can be checked via xl info), > which is not correct. This is because during the Arm CPU online > process at boot time, setup_cpu_sibling_map() only sets the per-cpu > cpu_core_mask for itself. > > cores_per_socket refers to the number of cores that belong to the same > socket (NUMA node). Currently Xen on Arm does not support physical > CPU hotplug and NUMA. Therefore if the MT bit (bit 24) in MPIDR_EL1 > is 0, cores_per_socket means all possible CPUs detected from the device > tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map() > accordingly. Drop the in-code comment which seems to be outdated. > > Signed-off-by: Henry Wang <xin.wang2@amd.com> NIT: You first sent this patch as part of NUMA series: https://lore.kernel.org/xen-devel/20231120025431.14845-16-Henry.Wang@arm.com/ Shouldn't you retain the Arm's authorship? > --- > xen/arch/arm/smpboot.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index a84e706d77..d616778655 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -66,7 +66,6 @@ static bool cpu_is_dead; > > /* ID of the PCPU we're running on */ > DEFINE_PER_CPU(unsigned int, cpu_id); > -/* XXX these seem awfully x86ish... */ > /* representing HT siblings of each logical CPU */ > DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask); > /* representing HT and core siblings of each logical CPU */ > @@ -89,6 +88,11 @@ static int setup_cpu_sibling_map(int cpu) > cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu)); > cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)); > > + /* PE not implemented using a multithreading type approach. */ > + if ( system_cpuinfo.mpidr.mt == 0 ) Do we need this check? It mt was true, cpu_sibling_mask would be incorrect anyway (it would still be 1). ~Michal
[AMD Official Use Only - General] Hi Michal, > -----Original Message----- > Subject: Re: [PATCH] xen/arm: Set correct per-cpu cpu_core_mask > > Hi Henry, > > On 26/02/2024 04:01, Henry Wang wrote: > > Signed-off-by: Henry Wang <xin.wang2@amd.com> > NIT: You first sent this patch as part of NUMA series: > https://lore.kernel.org/xen-devel/20231120025431.14845-16- > Henry.Wang@arm.com/ > Shouldn't you retain the Arm's authorship? Ah good point, in fact I don't really know, since I basically rewrote the patch I thought it is not really needed. I will add it back in v2 since you mentioned this. > > --- > > xen/arch/arm/smpboot.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > + /* PE not implemented using a multithreading type approach. */ > > + if ( system_cpuinfo.mpidr.mt == 0 ) > Do we need this check? It mt was true, cpu_sibling_mask would be incorrect > anyway (it would still be 1). I added this check for playing safe, because I only want to do the correct thing in this patch and avoid make things worse for MT case. With this patch, non-MT case can be improved and the MT case is remain unchanged. But I agree with you, and I would be more than happy if I can run a MT setup and finish the "else" part with this patch or follow-ups. Do you know maybe qemu can allow me to emulate a MT setup so that I can fix it properly in v2? Thanks! Kind regards, Henry > > ~Michal
On 26/02/2024 10:54, Wang, Henry wrote: > [AMD Official Use Only - General] > > Hi Michal, > >> -----Original Message----- >> Subject: Re: [PATCH] xen/arm: Set correct per-cpu cpu_core_mask >> >> Hi Henry, >> >> On 26/02/2024 04:01, Henry Wang wrote: >>> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> NIT: You first sent this patch as part of NUMA series: >> https://lore.kernel.org/xen-devel/20231120025431.14845-16- >> Henry.Wang@arm.com/ >> Shouldn't you retain the Arm's authorship? > > Ah good point, in fact I don't really know, since I basically rewrote > the patch I thought it is not really needed. I will add it back in v2 since > you mentioned this. > >>> --- >>> xen/arch/arm/smpboot.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> + /* PE not implemented using a multithreading type approach. */ >>> + if ( system_cpuinfo.mpidr.mt == 0 ) >> Do we need this check? It mt was true, cpu_sibling_mask would be incorrect >> anyway (it would still be 1). > > I added this check for playing safe, because I only want to do the correct thing > in this patch and avoid make things worse for MT case. With this patch, non-MT > case can be improved and the MT case is remain unchanged. > > But I agree with you, and I would be more than happy if I can run a MT setup and > finish the "else" part with this patch or follow-ups. Do you know maybe qemu can > allow me to emulate a MT setup so that I can fix it properly in v2? Thanks! A65 is the only Arm CPU with SMT and I'm not aware of Qemu being able to emulate it. AFAICT, in Xen on Arm we assume no SMT, hence my question about your check. With or without it, some parts would still be incorrect (like cpu_sibling_mask), so what's the point in having a partial check. I would keep your solution without the check. Others may have a different opinion though. ~Michal
Hi Michal, On 2/26/2024 6:43 PM, Michal Orzel wrote: > On 26/02/2024 10:54, Wang, Henry wrote: >>>> xen/arch/arm/smpboot.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> + /* PE not implemented using a multithreading type approach. */ >>>> + if ( system_cpuinfo.mpidr.mt == 0 ) >>> Do we need this check? It mt was true, cpu_sibling_mask would be incorrect >>> anyway (it would still be 1). >> I added this check for playing safe, because I only want to do the correct thing >> in this patch and avoid make things worse for MT case. With this patch, non-MT >> case can be improved and the MT case is remain unchanged. >> >> But I agree with you, and I would be more than happy if I can run a MT setup and >> finish the "else" part with this patch or follow-ups. Do you know maybe qemu can >> allow me to emulate a MT setup so that I can fix it properly in v2? Thanks! > A65 is the only Arm CPU with SMT and I'm not aware of Qemu being able to emulate it. > AFAICT, in Xen on Arm we assume no SMT, hence my question about your check. With or without it, > some parts would still be incorrect (like cpu_sibling_mask), so what's the point in having a partial check. > I would keep your solution without the check. Others may have a different opinion though. You made a point. So I think we can wait a bit more for others to share their thoughts and I will change accordingly after. Just taking a note from the chat that we just had - maybe remove the if check and add an ASSERT() of the non-MT would also be a good idea (not sure if this change will break some existing HW setups though). Kind regards, Henry > ~Michal
Hi everyone, On 2/26/2024 6:43 PM, Michal Orzel wrote: >>>> xen/arch/arm/smpboot.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> + /* PE not implemented using a multithreading type approach. */ >>>> + if ( system_cpuinfo.mpidr.mt == 0 ) >>> Do we need this check? It mt was true, cpu_sibling_mask would be incorrect >>> anyway (it would still be 1). >> I added this check for playing safe, because I only want to do the correct thing >> in this patch and avoid make things worse for MT case. With this patch, non-MT >> case can be improved and the MT case is remain unchanged. >> >> But I agree with you, and I would be more than happy if I can run a MT setup and >> finish the "else" part with this patch or follow-ups. Do you know maybe qemu can >> allow me to emulate a MT setup so that I can fix it properly in v2? Thanks! > A65 is the only Arm CPU with SMT and I'm not aware of Qemu being able to emulate it. > AFAICT, in Xen on Arm we assume no SMT, hence my question about your check. With or without it, > some parts would still be incorrect (like cpu_sibling_mask), so what's the point in having a partial check. > I would keep your solution without the check. Others may have a different opinion though. Since there isn't much discussion followed-up in this thread, I am wondering do we have more inputs/opinions on this topic? If everyone agrees, I've followed Michal's suggestion in v2 [1]. [1] https://lore.kernel.org/xen-devel/20240228015822.56108-1-xin.wang2@amd.com/ Kind regards, Henry > ~Michal
Hi Henry, On 08/03/2024 03:05, Henry Wang wrote: > Hi everyone, > > On 2/26/2024 6:43 PM, Michal Orzel wrote: >>>>> xen/arch/arm/smpboot.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> + /* PE not implemented using a multithreading type approach. */ >>>>> + if ( system_cpuinfo.mpidr.mt == 0 ) >>>> Do we need this check? It mt was true, cpu_sibling_mask would be incorrect >>>> anyway (it would still be 1). >>> I added this check for playing safe, because I only want to do the correct thing >>> in this patch and avoid make things worse for MT case. With this patch, non-MT >>> case can be improved and the MT case is remain unchanged. >>> >>> But I agree with you, and I would be more than happy if I can run a MT setup and >>> finish the "else" part with this patch or follow-ups. Do you know maybe qemu can >>> allow me to emulate a MT setup so that I can fix it properly in v2? Thanks! >> A65 is the only Arm CPU with SMT and I'm not aware of Qemu being able to emulate it. >> AFAICT, in Xen on Arm we assume no SMT, hence my question about your check. With or without it, >> some parts would still be incorrect (like cpu_sibling_mask), so what's the point in having a partial check. >> I would keep your solution without the check. Others may have a different opinion though. > > Since there isn't much discussion followed-up in this thread, I am > wondering do we have more inputs/opinions on this topic? If everyone > agrees, I've followed Michal's suggestion in v2 [1]. I clearly forgot to say in v2 that I'm ok with this change, so: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index a84e706d77..d616778655 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -66,7 +66,6 @@ static bool cpu_is_dead; /* ID of the PCPU we're running on */ DEFINE_PER_CPU(unsigned int, cpu_id); -/* XXX these seem awfully x86ish... */ /* representing HT siblings of each logical CPU */ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask); /* representing HT and core siblings of each logical CPU */ @@ -89,6 +88,11 @@ static int setup_cpu_sibling_map(int cpu) cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu)); cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)); + /* PE not implemented using a multithreading type approach. */ + if ( system_cpuinfo.mpidr.mt == 0 ) + cpumask_or(per_cpu(cpu_core_mask, cpu), + per_cpu(cpu_core_mask, cpu), &cpu_possible_map); + return 0; }
In the common sysctl command XEN_SYSCTL_physinfo, the value of cores_per_socket is calculated based on the cpu_core_mask of CPU0. Currently on Arm this is a fixed value 1 (can be checked via xl info), which is not correct. This is because during the Arm CPU online process at boot time, setup_cpu_sibling_map() only sets the per-cpu cpu_core_mask for itself. cores_per_socket refers to the number of cores that belong to the same socket (NUMA node). Currently Xen on Arm does not support physical CPU hotplug and NUMA. Therefore if the MT bit (bit 24) in MPIDR_EL1 is 0, cores_per_socket means all possible CPUs detected from the device tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map() accordingly. Drop the in-code comment which seems to be outdated. Signed-off-by: Henry Wang <xin.wang2@amd.com> --- xen/arch/arm/smpboot.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)