| Message ID | 20240321035706.165253-1-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xen/arm: Set correct per-cpu cpu_core_mask | expand |
Hi All, Gentle ping since it has been a couple of months, any comments on this updated patch? Thanks! Kind regards, Henry On 3/21/2024 11:57 AM, 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, also we assume there is no multithread. Therefore > 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. Modify the in-code comment which seems to be outdated. Add > a warning to users if Xen is running on processors with multithread > support. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > v3: > - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary > cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)). > - In-code comment adjustments. > - Add a warning for multithread. > v2: > - Do not do the multithread check. > --- > xen/arch/arm/smpboot.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index a84e706d77..b6268be27a 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -66,7 +66,11 @@ 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... */ > +/* > + * Although multithread is part of the Arm spec, there are not many > + * processors support multithread and current Xen on Arm assumes there > + * is no multithread. > + */ > /* 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 */ > @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu) > !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) ) > return -ENOMEM; > > - /* A CPU is a sibling with itself and is always on its own core. */ > + /* > + * Currently we assume there is no multithread and NUMA, so > + * a CPU is a sibling with itself, and the all possible CPUs > + * are supposed to belong to the same socket (NUMA node). > + */ > cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu)); > - cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)); > + cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map); > > return 0; > } > @@ -277,6 +285,10 @@ void __init smp_init_cpus(void) > warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n" > "It has implications on the security and stability of the system,\n" > "unless the cpu affinity of all domains is specified.\n"); > + > + if ( system_cpuinfo.mpidr.mt == 1 ) > + warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n" > + "It might impact the security of the system.\n"); > } > > unsigned int __init smp_get_max_cpus(void)
Hi Henry. On 20/05/2024 04:57, Henry Wang wrote: > Hi All, > > Gentle ping since it has been a couple of months, any comments on this > updated patch? Thanks! Sorry for the late reply. > > Kind regards, > Henry > > On 3/21/2024 11:57 AM, 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, also we assume there is no multithread. Therefore >> 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. Modify the in-code comment which seems to be outdated. Add >> a warning to users if Xen is running on processors with multithread >> support. >> >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> >> --- >> v3: >> - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary >> cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)). >> - In-code comment adjustments. >> - Add a warning for multithread. >> v2: >> - Do not do the multithread check. >> --- >> xen/arch/arm/smpboot.c | 18 +++++++++++++++--- >> 1 file changed, 15 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index a84e706d77..b6268be27a 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -66,7 +66,11 @@ 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... */ >> +/* >> + * Although multithread is part of the Arm spec, there are not many >> + * processors support multithread and current Xen on Arm assumes there NIT: s/support/supporting >> + * is no multithread. >> + */ >> /* 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 */ >> @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu) >> !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) ) >> return -ENOMEM; >> >> - /* A CPU is a sibling with itself and is always on its own core. */ >> + /* >> + * Currently we assume there is no multithread and NUMA, so >> + * a CPU is a sibling with itself, and the all possible CPUs >> + * are supposed to belong to the same socket (NUMA node). >> + */ >> cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu)); >> - cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)); >> + cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map); >> >> return 0; >> } >> @@ -277,6 +285,10 @@ void __init smp_init_cpus(void) >> warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n" >> "It has implications on the security and stability of the system,\n" >> "unless the cpu affinity of all domains is specified.\n"); >> + >> + if ( system_cpuinfo.mpidr.mt == 1 ) >> + warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n" >> + "It might impact the security of the system.\n"); >> } >> >> unsigned int __init smp_get_max_cpus(void) > ~Michal
Hi Michal, On 5/21/2024 3:47 PM, Michal Orzel wrote: > Hi Henry. > > On 3/21/2024 11:57 AM, 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, also we assume there is no multithread. Therefore >>> 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. Modify the in-code comment which seems to be outdated. Add >>> a warning to users if Xen is running on processors with multithread >>> support. >>> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> Signed-off-by: Henry Wang <xin.wang2@amd.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks. >>> /* ID of the PCPU we're running on */ >>> DEFINE_PER_CPU(unsigned int, cpu_id); >>> -/* XXX these seem awfully x86ish... */ >>> +/* >>> + * Although multithread is part of the Arm spec, there are not many >>> + * processors support multithread and current Xen on Arm assumes there > NIT: s/support/supporting Sorry, it should have been spotted locally before sending. Anyway, I will correct this in v4 with your Reviewed-by tag taken. Thanks for pointing this out. Kind regards, Henry >>> __init smp_get_max_cpus(void) > ~Michal
On 21/05/2024 09:51, Henry Wang wrote: > Hi Michal, > > On 5/21/2024 3:47 PM, Michal Orzel wrote: >> Hi Henry. >> >> On 3/21/2024 11:57 AM, 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, also we assume there is no multithread. Therefore >>>> 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. Modify the in-code comment which seems to be outdated. Add >>>> a warning to users if Xen is running on processors with multithread >>>> support. >>>> >>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>>> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > Thanks. > >>>> /* ID of the PCPU we're running on */ >>>> DEFINE_PER_CPU(unsigned int, cpu_id); >>>> -/* XXX these seem awfully x86ish... */ >>>> +/* >>>> + * Although multithread is part of the Arm spec, there are not many >>>> + * processors support multithread and current Xen on Arm assumes there >> NIT: s/support/supporting > > Sorry, it should have been spotted locally before sending. Anyway, I > will correct this in v4 with your Reviewed-by tag taken. Thanks for > pointing this out. I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit. ~Michal
Hi, On 21/05/2024 08:57, Michal Orzel wrote: > > > On 21/05/2024 09:51, Henry Wang wrote: >> Hi Michal, >> >> On 5/21/2024 3:47 PM, Michal Orzel wrote: >>> Hi Henry. >>> >>> On 3/21/2024 11:57 AM, 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, also we assume there is no multithread. Therefore >>>>> 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. Modify the in-code comment which seems to be outdated. Add >>>>> a warning to users if Xen is running on processors with multithread >>>>> support. >>>>> >>>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>>>> Signed-off-by: Henry Wang <xin.wang2@amd.com> >>> Reviewed-by: Michal Orzel <michal.orzel@amd.com> >> >> Thanks. >> >>>>> /* ID of the PCPU we're running on */ >>>>> DEFINE_PER_CPU(unsigned int, cpu_id); >>>>> -/* XXX these seem awfully x86ish... */ >>>>> +/* >>>>> + * Although multithread is part of the Arm spec, there are not many >>>>> + * processors support multithread and current Xen on Arm assumes there >>> NIT: s/support/supporting >> >> Sorry, it should have been spotted locally before sending. Anyway, I >> will correct this in v4 with your Reviewed-by tag taken. Thanks for >> pointing this out. > I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit. Fixed and committed. Cheers, > > ~Michal >
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index a84e706d77..b6268be27a 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -66,7 +66,11 @@ 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... */ +/* + * Although multithread is part of the Arm spec, there are not many + * processors support multithread and current Xen on Arm assumes there + * is no multithread. + */ /* 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 */ @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu) !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) ) return -ENOMEM; - /* A CPU is a sibling with itself and is always on its own core. */ + /* + * Currently we assume there is no multithread and NUMA, so + * a CPU is a sibling with itself, and the all possible CPUs + * are supposed to belong to the same socket (NUMA node). + */ cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu)); - cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)); + cpumask_copy(per_cpu(cpu_core_mask, cpu), &cpu_possible_map); return 0; } @@ -277,6 +285,10 @@ void __init smp_init_cpus(void) warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n" "It has implications on the security and stability of the system,\n" "unless the cpu affinity of all domains is specified.\n"); + + if ( system_cpuinfo.mpidr.mt == 1 ) + warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE PROCESSOR.\n" + "It might impact the security of the system.\n"); } unsigned int __init smp_get_max_cpus(void)