| Message ID | 20230816080658.3562730-1-qian.wen@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix overflow of the max number of IDs for logic processor and core | expand |
On Wed, Aug 16, 2023 at 04:06:56PM +0800, Qian Wen <qian.wen@intel.com> wrote: > CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical > processors in this physical package. > CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor cores > in the physical package. > > The current qemu code doesn't limit the value written to these two fields. > If the guest has a huge number of cores, APs (application processor) will > fail to bring up and the wrong info will be reported. > According to HW behavior, setting max value written to CPUID.1.EBX[23:16] > to 255, and CPUID.4:EAX[31:26] to 63. > > --- > Changes v2 -> v3: > - Add patch 2. > - Revise the commit message and comment to be clearer. > - Using MIN() for limitation. > Changes v1 -> v2: > - Revise the commit message and comment to more clearer. > - Rebased to v8.1.0-rc2. > > Qian Wen (2): > target/i386: Avoid cpu number overflow in legacy topology > target/i386: Avoid overflow of the cache parameter enumerated by leaf 4 > > target/i386/cpu.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > base-commit: 0d52116fd82cdd1f4a88837336af5b6290c364a4 > -- > 2.25.1 > The patch itself looks good. Can we add test cases? We have some in qemu/tests/unit/test-x86-cpuid.c.
On 8/18/2023 3:33 AM, Isaku Yamahata wrote: > On Wed, Aug 16, 2023 at 04:06:56PM +0800, > Qian Wen <qian.wen@intel.com> wrote: > >> CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical >> processors in this physical package. >> CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor cores >> in the physical package. >> >> The current qemu code doesn't limit the value written to these two fields. >> If the guest has a huge number of cores, APs (application processor) will >> fail to bring up and the wrong info will be reported. >> According to HW behavior, setting max value written to CPUID.1.EBX[23:16] >> to 255, and CPUID.4:EAX[31:26] to 63. >> >> --- >> Changes v2 -> v3: >> - Add patch 2. >> - Revise the commit message and comment to be clearer. >> - Using MIN() for limitation. >> Changes v1 -> v2: >> - Revise the commit message and comment to more clearer. >> - Rebased to v8.1.0-rc2. >> >> Qian Wen (2): >> target/i386: Avoid cpu number overflow in legacy topology >> target/i386: Avoid overflow of the cache parameter enumerated by leaf 4 >> >> target/i386/cpu.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> base-commit: 0d52116fd82cdd1f4a88837336af5b6290c364a4 >> -- >> 2.25.1 >> > The patch itself looks good. Can we add test cases? > We have some in qemu/tests/unit/test-x86-cpuid.c. Hi Isaku, thanks for your comments! I take a look, the test-x86-cpuid.c has some tests for topology functions, e.g., apicid_smt/core/die_width, apicid_core/die/pkg_offset, x86_apicid_from_cpu_idx... Do you mean adding another test for function cpu_x86_cpuid() like below? If so, it seems that some effort is required to instantiate CPUX86State for input of cpu_x86_cpuid, but the result of this test case is obvious. So, is it necessary to add this test? + uint32_t unused, eax, ebx; + /* CPUID.1.EBX[23:16]: Maximum number of addressable IDs for logical + processors in this physical package. + */ + cpu_x86_cpuid(env, 1, 0, &unused, &ebx, &unused, &unused); + g_assert_cmpuint(ebx & 0xFF0000, ==, 0xFF0000); + + /* CPUID.4:EAX[31:26]: Maximum number of addressable IDs for processor + cores in the physical package. + */ + cpu_x86_cpuid(env, 4, 0, &eax, &unused, &unused, &unused); + g_assert_cmpuint(ebx & 0xFC000000, ==, 0xFC000000); Thanks, Qian