* [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error @ 2023-08-20 10:56 Jiajie Chen 2023-08-21 1:24 ` bibo mao 0 siblings, 1 reply; 5+ messages in thread From: Jiajie Chen @ 2023-08-20 10:56 UTC (permalink / raw) To: qemu-devel Cc: richard.henderson, gaosong, yangxiaojuan, zhaotianrui, Jiajie Chen In hw/acpi/aml-build.c:build_pptt() function, the code assumes that the ACPI processor id equals to the cpu index, for example if we have 8 cpus, then the ACPI processor id should be in range 0-7. However, in hw/loongarch/acpi-build.c:build_madt() function we broke the assumption. If we have 8 cpus again, the ACPI processor id in MADT table would be in range 1-8. It violates the following description taken from ACPI spec 6.4 table 5.138: If the processor structure represents an actual processor, this field must match the value of ACPI processor ID field in the processor’s entry in the MADT. It will break the latest Linux 6.5-rc6 with the following error message: ACPI PPTT: PPTT table found, but unable to locate core 7 (8) Invalid BIOS PPTT Here 7 is the last cpu index, 8 is the ACPI processor id learned from MADT. With this patch, Linux can properly detect SMT threads when "-smp 8,sockets=1,cores=4,threads=2" is passed: Thread(s) per core: 2 Core(s) per socket: 2 Socket(s): 2 The detection of number of sockets is still wrong, but that is out of scope of the commit. Signed-off-by: Jiajie Chen <c@jia.je> --- hw/loongarch/acpi-build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c index 0b62c3a2f7..ae292fc543 100644 --- a/hw/loongarch/acpi-build.c +++ b/hw/loongarch/acpi-build.c @@ -127,7 +127,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, LoongArchMachineState *lams) build_append_int_noprefix(table_data, 17, 1); /* Type */ build_append_int_noprefix(table_data, 15, 1); /* Length */ build_append_int_noprefix(table_data, 1, 1); /* Version */ - build_append_int_noprefix(table_data, i + 1, 4); /* ACPI Processor ID */ + build_append_int_noprefix(table_data, i, 4); /* ACPI Processor ID */ build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */ build_append_int_noprefix(table_data, 1, 4); /* Flags */ } -- 2.41.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error 2023-08-20 10:56 [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error Jiajie Chen @ 2023-08-21 1:24 ` bibo mao 2023-08-21 1:29 ` Jiajie Chen 0 siblings, 1 reply; 5+ messages in thread From: bibo mao @ 2023-08-21 1:24 UTC (permalink / raw) To: Jiajie Chen Cc: richard.henderson, gaosong, zhaotianrui, qemu-devel, 李香来 + Add xianglai Good catch. In theory, it is logical id, and it can be not equal to physical id. However it must be equal to _UID in cpu dsdt table which is missing now. Can pptt table parse error be fixed if cpu dsdt table is added? Regards Bibo Mao 在 2023/8/20 18:56, Jiajie Chen 写道: > In hw/acpi/aml-build.c:build_pptt() function, the code assumes that the > ACPI processor id equals to the cpu index, for example if we have 8 > cpus, then the ACPI processor id should be in range 0-7. > > However, in hw/loongarch/acpi-build.c:build_madt() function we broke the > assumption. If we have 8 cpus again, the ACPI processor id in MADT table > would be in range 1-8. It violates the following description taken from > ACPI spec 6.4 table 5.138: > > If the processor structure represents an actual processor, this field > must match the value of ACPI processor ID field in the processor’s entry > in the MADT. > > It will break the latest Linux 6.5-rc6 with the > following error message: > > ACPI PPTT: PPTT table found, but unable to locate core 7 (8) > Invalid BIOS PPTT > > Here 7 is the last cpu index, 8 is the ACPI processor id learned from > MADT. > > With this patch, Linux can properly detect SMT threads when "-smp > 8,sockets=1,cores=4,threads=2" is passed: > > Thread(s) per core: 2 > Core(s) per socket: 2 > Socket(s): 2 > > The detection of number of sockets is still wrong, but that is out of > scope of the commit. > > Signed-off-by: Jiajie Chen <c@jia.je> > --- > hw/loongarch/acpi-build.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c > index 0b62c3a2f7..ae292fc543 100644 > --- a/hw/loongarch/acpi-build.c > +++ b/hw/loongarch/acpi-build.c > @@ -127,7 +127,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, LoongArchMachineState *lams) > build_append_int_noprefix(table_data, 17, 1); /* Type */ > build_append_int_noprefix(table_data, 15, 1); /* Length */ > build_append_int_noprefix(table_data, 1, 1); /* Version */ > - build_append_int_noprefix(table_data, i + 1, 4); /* ACPI Processor ID */ > + build_append_int_noprefix(table_data, i, 4); /* ACPI Processor ID */ > build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */ > build_append_int_noprefix(table_data, 1, 4); /* Flags */ > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error 2023-08-21 1:24 ` bibo mao @ 2023-08-21 1:29 ` Jiajie Chen 2023-08-21 2:00 ` bibo mao 2023-08-22 0:55 ` bibo mao 0 siblings, 2 replies; 5+ messages in thread From: Jiajie Chen @ 2023-08-21 1:29 UTC (permalink / raw) To: bibo mao Cc: richard.henderson, gaosong, zhaotianrui, qemu-devel, 李香来 [-- Attachment #1: Type: text/plain, Size: 3255 bytes --] On 2023/8/21 09:24, bibo mao wrote: > + Add xianglai > > Good catch. > > In theory, it is logical id, and it can be not equal to physical id. > However it must be equal to _UID in cpu dsdt table which is missing > now. Yes, the logical id can be different from index. The spec says: If the processor structure represents an actual processor, this field must match the value of ACPI processor ID field in the processor’s entry in the MADT. If the processor structure represents a group of associated processors, the structure might match a processor container in the name space. In that case this entry will match the value of the _UID method of the associated processor container. Where there is a match it must be represented. The flags field, described in/Processor Structure Flags/, includes a bit to describe whether the ACPI processor ID is valid. I believe PPTT, MADT and DSDT should all adhere to the same logical id mapping. > > Can pptt table parse error be fixed if cpu dsdt table is added? > > Regards > Bibo Mao > > > 在 2023/8/20 18:56, Jiajie Chen 写道: >> In hw/acpi/aml-build.c:build_pptt() function, the code assumes that the >> ACPI processor id equals to the cpu index, for example if we have 8 >> cpus, then the ACPI processor id should be in range 0-7. >> >> However, in hw/loongarch/acpi-build.c:build_madt() function we broke the >> assumption. If we have 8 cpus again, the ACPI processor id in MADT table >> would be in range 1-8. It violates the following description taken from >> ACPI spec 6.4 table 5.138: >> >> If the processor structure represents an actual processor, this field >> must match the value of ACPI processor ID field in the processor’s entry >> in the MADT. >> >> It will break the latest Linux 6.5-rc6 with the >> following error message: >> >> ACPI PPTT: PPTT table found, but unable to locate core 7 (8) >> Invalid BIOS PPTT >> >> Here 7 is the last cpu index, 8 is the ACPI processor id learned from >> MADT. >> >> With this patch, Linux can properly detect SMT threads when "-smp >> 8,sockets=1,cores=4,threads=2" is passed: >> >> Thread(s) per core: 2 >> Core(s) per socket: 2 >> Socket(s): 2 >> >> The detection of number of sockets is still wrong, but that is out of >> scope of the commit. >> >> Signed-off-by: Jiajie Chen<c@jia.je> >> --- >> hw/loongarch/acpi-build.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c >> index 0b62c3a2f7..ae292fc543 100644 >> --- a/hw/loongarch/acpi-build.c >> +++ b/hw/loongarch/acpi-build.c >> @@ -127,7 +127,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, LoongArchMachineState *lams) >> build_append_int_noprefix(table_data, 17, 1); /* Type */ >> build_append_int_noprefix(table_data, 15, 1); /* Length */ >> build_append_int_noprefix(table_data, 1, 1); /* Version */ >> - build_append_int_noprefix(table_data, i + 1, 4); /* ACPI Processor ID */ >> + build_append_int_noprefix(table_data, i, 4); /* ACPI Processor ID */ >> build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */ >> build_append_int_noprefix(table_data, 1, 4); /* Flags */ >> } [-- Attachment #2: Type: text/html, Size: 5916 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error 2023-08-21 1:29 ` Jiajie Chen @ 2023-08-21 2:00 ` bibo mao 2023-08-22 0:55 ` bibo mao 1 sibling, 0 replies; 5+ messages in thread From: bibo mao @ 2023-08-21 2:00 UTC (permalink / raw) To: Jiajie Chen Cc: richard.henderson, gaosong, zhaotianrui, qemu-devel, 李香来 在 2023/8/21 09:29, Jiajie Chen 写道: > > On 2023/8/21 09:24, bibo mao wrote: >> + Add xianglai >> >> Good catch. >> >> In theory, it is logical id, and it can be not equal to physical id. >> However it must be equal to _UID in cpu dsdt table which is missing >> now. > > Yes, the logical id can be different from index. The spec says: > > If the processor structure represents an actual processor, this field must match the value of ACPI processor ID field in the processor’s entry in the MADT. If the processor structure represents a group of associated processors, the structure might match a processor container in the name space. In that case this entry will match the value of the _UID method of the associated processor container. Where there is a match it must be represented. The flags field, described in /Processor Structure Flags/, includes a bit to describe whether the ACPI processor ID is valid. > > I believe PPTT, MADT and DSDT should all adhere to the same logical id mapping. yes, if logical id is set the same with physical id, it can solve the problem and also it may hide the potential problem about qemu/kernel etc. So I prefer to different methods only if it comply the spec, so that we can find the issue and solve it in early stage. Regards Bibo Mao > >> Can pptt table parse error be fixed if cpu dsdt table is added? >> >> Regards >> Bibo Mao >> >> >> 在 2023/8/20 18:56, Jiajie Chen 写道: >>> In hw/acpi/aml-build.c:build_pptt() function, the code assumes that the >>> ACPI processor id equals to the cpu index, for example if we have 8 >>> cpus, then the ACPI processor id should be in range 0-7. >>> >>> However, in hw/loongarch/acpi-build.c:build_madt() function we broke the >>> assumption. If we have 8 cpus again, the ACPI processor id in MADT table >>> would be in range 1-8. It violates the following description taken from >>> ACPI spec 6.4 table 5.138: >>> >>> If the processor structure represents an actual processor, this field >>> must match the value of ACPI processor ID field in the processor’s entry >>> in the MADT. >>> >>> It will break the latest Linux 6.5-rc6 with the >>> following error message: >>> >>> ACPI PPTT: PPTT table found, but unable to locate core 7 (8) >>> Invalid BIOS PPTT >>> >>> Here 7 is the last cpu index, 8 is the ACPI processor id learned from >>> MADT. >>> >>> With this patch, Linux can properly detect SMT threads when "-smp >>> 8,sockets=1,cores=4,threads=2" is passed: >>> >>> Thread(s) per core: 2 >>> Core(s) per socket: 2 >>> Socket(s): 2 >>> >>> The detection of number of sockets is still wrong, but that is out of >>> scope of the commit. >>> >>> Signed-off-by: Jiajie Chen <c@jia.je> >>> --- >>> hw/loongarch/acpi-build.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c >>> index 0b62c3a2f7..ae292fc543 100644 >>> --- a/hw/loongarch/acpi-build.c >>> +++ b/hw/loongarch/acpi-build.c >>> @@ -127,7 +127,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, LoongArchMachineState *lams) >>> build_append_int_noprefix(table_data, 17, 1); /* Type */ >>> build_append_int_noprefix(table_data, 15, 1); /* Length */ >>> build_append_int_noprefix(table_data, 1, 1); /* Version */ >>> - build_append_int_noprefix(table_data, i + 1, 4); /* ACPI Processor ID */ >>> + build_append_int_noprefix(table_data, i, 4); /* ACPI Processor ID */ >>> build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */ >>> build_append_int_noprefix(table_data, 1, 4); /* Flags */ >>> } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error 2023-08-21 1:29 ` Jiajie Chen 2023-08-21 2:00 ` bibo mao @ 2023-08-22 0:55 ` bibo mao 1 sibling, 0 replies; 5+ messages in thread From: bibo mao @ 2023-08-22 0:55 UTC (permalink / raw) To: Jiajie Chen Cc: richard.henderson, gaosong, zhaotianrui, qemu-devel, 李香来 在 2023/8/21 09:29, Jiajie Chen 写道: > > On 2023/8/21 09:24, bibo mao wrote: >> + Add xianglai >> >> Good catch. >> >> In theory, it is logical id, and it can be not equal to physical id. >> However it must be equal to _UID in cpu dsdt table which is missing >> now. > > Yes, the logical id can be different from index. The spec says: > > If the processor structure represents an actual processor, this field must match the value of ACPI processor ID field in the processor’s entry in the MADT. If the processor structure represents a group of associated processors, the structure might match a processor container in the name space. In that case this entry will match the value of the _UID method of the associated processor container. Where there is a match it must be represented. The flags field, described in /Processor Structure Flags/, includes a bit to describe whether the ACPI processor ID is valid. > > I believe PPTT, MADT and DSDT should all adhere to the same logical id mapping. yes, you are right and I had a mistake. Logical id in MADT/DSDT/PPTT should be the same, there is physical id associated with arch_ids->cpus[i].arch_id already. And get_arch_id for cpuclass to get physical id and DSDT table are missing about LoongArch platform, however it is another issue. Reviewed-by: Bibo Mao <maobibo@loongson.cn> Regards Bibo Mao > >> Can pptt table parse error be fixed if cpu dsdt table is added? >> >> Regards >> Bibo Mao >> >> >> 在 2023/8/20 18:56, Jiajie Chen 写道: >>> In hw/acpi/aml-build.c:build_pptt() function, the code assumes that the >>> ACPI processor id equals to the cpu index, for example if we have 8 >>> cpus, then the ACPI processor id should be in range 0-7. >>> >>> However, in hw/loongarch/acpi-build.c:build_madt() function we broke the >>> assumption. If we have 8 cpus again, the ACPI processor id in MADT table >>> would be in range 1-8. It violates the following description taken from >>> ACPI spec 6.4 table 5.138: >>> >>> If the processor structure represents an actual processor, this field >>> must match the value of ACPI processor ID field in the processor’s entry >>> in the MADT. >>> >>> It will break the latest Linux 6.5-rc6 with the >>> following error message: >>> >>> ACPI PPTT: PPTT table found, but unable to locate core 7 (8) >>> Invalid BIOS PPTT >>> >>> Here 7 is the last cpu index, 8 is the ACPI processor id learned from >>> MADT. >>> >>> With this patch, Linux can properly detect SMT threads when "-smp >>> 8,sockets=1,cores=4,threads=2" is passed: >>> >>> Thread(s) per core: 2 >>> Core(s) per socket: 2 >>> Socket(s): 2 >>> >>> The detection of number of sockets is still wrong, but that is out of >>> scope of the commit. >>> >>> Signed-off-by: Jiajie Chen <c@jia.je> >>> --- >>> hw/loongarch/acpi-build.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c >>> index 0b62c3a2f7..ae292fc543 100644 >>> --- a/hw/loongarch/acpi-build.c >>> +++ b/hw/loongarch/acpi-build.c >>> @@ -127,7 +127,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, LoongArchMachineState *lams) >>> build_append_int_noprefix(table_data, 17, 1); /* Type */ >>> build_append_int_noprefix(table_data, 15, 1); /* Length */ >>> build_append_int_noprefix(table_data, 1, 1); /* Version */ >>> - build_append_int_noprefix(table_data, i + 1, 4); /* ACPI Processor ID */ >>> + build_append_int_noprefix(table_data, i, 4); /* ACPI Processor ID */ >>> build_append_int_noprefix(table_data, arch_id, 4); /* Core ID */ >>> build_append_int_noprefix(table_data, 1, 4); /* Flags */ >>> } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-22 0:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-20 10:56 [PATCH] hw/loongarch: Fix ACPI processor id off-by-one error Jiajie Chen 2023-08-21 1:24 ` bibo mao 2023-08-21 1:29 ` Jiajie Chen 2023-08-21 2:00 ` bibo mao 2023-08-22 0:55 ` bibo mao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).