* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-04-06 12:29 [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on Brice Goglin
@ 2022-04-06 13:44 ` Jonathan Cameron via
2022-06-17 15:59 ` Brice Goglin
2022-06-20 13:27 ` Igor Mammedov
2 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron via @ 2022-04-06 13:44 UTC (permalink / raw)
To: Brice Goglin
Cc: qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
On Wed, 6 Apr 2022 14:29:56 +0200
Brice Goglin <Brice.Goglin@inria.fr> wrote:
> From: Brice Goglin <Brice.Goglin@inria.fr>
>
> The "Memory Proximity Domain Attributes" structure of the ACPI HMAT
> has a "Processor Proximity Domain Valid" flag that is currently
> always set because Qemu -numa requires initiator=X when hmat=on.
>
> Unsetting this flag allows to create more complex memory topologies
> by having multiple best initiators for a single memory target.
>
> This patch allows -numa with initiator=X when hmat=on by keeping
> the default value MAX_NODES in numa_state->nodes[i].initiator.
> All places reading numa_state->nodes[i].initiator already check
> whether it's different from MAX_NODES before using it. And
> hmat_build_table_structs() already unset the Valid flag when needed.
>
> Tested with
> qemu-system-x86_64 -accel kvm \
> -machine pc,hmat=on \
> -drive if=pflash,format=raw,file=./OVMF.fd \
> -drive media=disk,format=qcow2,file=efi.qcow2 \
> -smp 4 \
> -m 3G \
> -object memory-backend-ram,size=1G,id=ram0 \
> -object memory-backend-ram,size=1G,id=ram1 \
> -object memory-backend-ram,size=1G,id=ram2 \
> -numa node,nodeid=0,memdev=ram0,cpus=0-1 \
> -numa node,nodeid=1,memdev=ram1,cpus=2-3 \
> -numa node,nodeid=2,memdev=ram2 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
>
> This exposes NUMA node2 at same distance from both node0 and node1 as seen in lstopo:
>
> Machine (2966MB total) + Package P#0
> NUMANode P#2 (979MB)
> Group0
> NUMANode P#0 (980MB)
> Core P#0 + PU P#0
> Core P#1 + PU P#1
> Group0
> NUMANode P#1 (1007MB)
> Core P#2 + PU P#2
> Core P#3 + PU P#3
>
> Signed-off-by: Brice Goglin <Brice.Goglin@inria.fr>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Useful fix / extension of what can be described.
Thanks,
Jonathan
> ---
> hw/core/machine.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..9884ef7ac6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1012,9 +1012,7 @@ static void numa_validate_initiator(NumaState *numa_state)
>
> for (i = 0; i < numa_state->num_nodes; i++) {
> if (numa_info[i].initiator == MAX_NODES) {
> - error_report("The initiator of NUMA node %d is missing, use "
> - "'-numa node,initiator' option to declare it", i);
> - exit(1);
> + continue;
> }
>
> if (!numa_info[numa_info[i].initiator].present) {
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-04-06 12:29 [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on Brice Goglin
2022-04-06 13:44 ` Jonathan Cameron via
@ 2022-06-17 15:59 ` Brice Goglin
2022-06-20 13:27 ` Igor Mammedov
2 siblings, 0 replies; 8+ messages in thread
From: Brice Goglin @ 2022-06-17 15:59 UTC (permalink / raw)
To: qemu-devel
Cc: Liu Jingqi, Tao Xu, Eduardo Habkost, Marcel Apfelbaum,
Philippe Mathieu-Daudé,
Yanan Wang
[-- Attachment #1.1: Type: text/plain, Size: 3727 bytes --]
Hello
Could somebody please apply (or reject) this commit?
Thanks
Brice
Le 06/04/2022 à 14:29, Brice Goglin a écrit :
> From: Brice Goglin<Brice.Goglin@inria.fr>
>
> The "Memory Proximity Domain Attributes" structure of the ACPI HMAT
> has a "Processor Proximity Domain Valid" flag that is currently
> always set because Qemu -numa requires initiator=X when hmat=on.
>
> Unsetting this flag allows to create more complex memory topologies
> by having multiple best initiators for a single memory target.
>
> This patch allows -numa with initiator=X when hmat=on by keeping
> the default value MAX_NODES in numa_state->nodes[i].initiator.
> All places reading numa_state->nodes[i].initiator already check
> whether it's different from MAX_NODES before using it. And
> hmat_build_table_structs() already unset the Valid flag when needed.
>
> Tested with
> qemu-system-x86_64 -accel kvm \
> -machine pc,hmat=on \
> -drive if=pflash,format=raw,file=./OVMF.fd \
> -drive media=disk,format=qcow2,file=efi.qcow2 \
> -smp 4 \
> -m 3G \
> -object memory-backend-ram,size=1G,id=ram0 \
> -object memory-backend-ram,size=1G,id=ram1 \
> -object memory-backend-ram,size=1G,id=ram2 \
> -numa node,nodeid=0,memdev=ram0,cpus=0-1 \
> -numa node,nodeid=1,memdev=ram1,cpus=2-3 \
> -numa node,nodeid=2,memdev=ram2 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
>
> This exposes NUMA node2 at same distance from both node0 and node1 as seen in lstopo:
>
> Machine (2966MB total) + Package P#0
> NUMANode P#2 (979MB)
> Group0
> NUMANode P#0 (980MB)
> Core P#0 + PU P#0
> Core P#1 + PU P#1
> Group0
> NUMANode P#1 (1007MB)
> Core P#2 + PU P#2
> Core P#3 + PU P#3
>
> Signed-off-by: Brice Goglin<Brice.Goglin@inria.fr>
> ---
> hw/core/machine.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..9884ef7ac6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1012,9 +1012,7 @@ static void numa_validate_initiator(NumaState *numa_state)
>
> for (i = 0; i < numa_state->num_nodes; i++) {
> if (numa_info[i].initiator == MAX_NODES) {
> - error_report("The initiator of NUMA node %d is missing, use "
> - "'-numa node,initiator' option to declare it", i);
> - exit(1);
> + continue;
> }
>
> if (!numa_info[numa_info[i].initiator].present) {
> --
> 2.30.2
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-04-06 12:29 [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on Brice Goglin
2022-04-06 13:44 ` Jonathan Cameron via
2022-06-17 15:59 ` Brice Goglin
@ 2022-06-20 13:27 ` Igor Mammedov
2022-06-20 15:24 ` Brice Goglin
2 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2022-06-20 13:27 UTC (permalink / raw)
To: Brice Goglin
Cc: qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
On Wed, 6 Apr 2022 14:29:56 +0200
Brice Goglin <Brice.Goglin@inria.fr> wrote:
> From: Brice Goglin <Brice.Goglin@inria.fr>
>
> The "Memory Proximity Domain Attributes" structure of the ACPI HMAT
> has a "Processor Proximity Domain Valid" flag that is currently
> always set because Qemu -numa requires initiator=X when hmat=on.
>
> Unsetting this flag allows to create more complex memory topologies
> by having multiple best initiators for a single memory target.
>
> This patch allows -numa with initiator=X when hmat=on by keeping
> the default value MAX_NODES in numa_state->nodes[i].initiator.
> All places reading numa_state->nodes[i].initiator already check
> whether it's different from MAX_NODES before using it. And
> hmat_build_table_structs() already unset the Valid flag when needed.
>
> Tested with
> qemu-system-x86_64 -accel kvm \
> -machine pc,hmat=on \
> -drive if=pflash,format=raw,file=./OVMF.fd \
> -drive media=disk,format=qcow2,file=efi.qcow2 \
> -smp 4 \
> -m 3G \
> -object memory-backend-ram,size=1G,id=ram0 \
> -object memory-backend-ram,size=1G,id=ram1 \
> -object memory-backend-ram,size=1G,id=ram2 \
> -numa node,nodeid=0,memdev=ram0,cpus=0-1 \
> -numa node,nodeid=1,memdev=ram1,cpus=2-3 \
> -numa node,nodeid=2,memdev=ram2 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=0,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=0,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=0,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-latency,latency=20 \
> -numa hmat-lb,initiator=1,target=0,hierarchy=memory,data-type=access-bandwidth,bandwidth=5242880 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-latency,latency=10 \
> -numa hmat-lb,initiator=1,target=1,hierarchy=memory,data-type=access-bandwidth,bandwidth=10485760 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-latency,latency=30 \
> -numa hmat-lb,initiator=1,target=2,hierarchy=memory,data-type=access-bandwidth,bandwidth=1048576 \
>
> This exposes NUMA node2 at same distance from both node0 and node1 as seen in lstopo:
>
> Machine (2966MB total) + Package P#0
> NUMANode P#2 (979MB)
> Group0
> NUMANode P#0 (980MB)
> Core P#0 + PU P#0
> Core P#1 + PU P#1
> Group0
> NUMANode P#1 (1007MB)
> Core P#2 + PU P#2
> Core P#3 + PU P#3
here should be a dis-assembled dump of generated HMAT table
+ a test case, see tests/qtest/bios-tables-test.c
for the process (at tho top) and test examples
>
> Signed-off-by: Brice Goglin <Brice.Goglin@inria.fr>
> ---
> hw/core/machine.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..9884ef7ac6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1012,9 +1012,7 @@ static void numa_validate_initiator(NumaState *numa_state)
>
> for (i = 0; i < numa_state->num_nodes; i++) {
> if (numa_info[i].initiator == MAX_NODES) {
> - error_report("The initiator of NUMA node %d is missing, use "
> - "'-numa node,initiator' option to declare it", i);
> - exit(1);
> + continue;
> }
>
> if (!numa_info[numa_info[i].initiator].present) {
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-06-20 13:27 ` Igor Mammedov
@ 2022-06-20 15:24 ` Brice Goglin
2022-06-20 16:05 ` Igor Mammedov
0 siblings, 1 reply; 8+ messages in thread
From: Brice Goglin @ 2022-06-20 15:24 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
[-- Attachment #1.1: Type: text/plain, Size: 1082 bytes --]
Le 20/06/2022 à 15:27, Igor Mammedov a écrit Machine (2966MB total) +
Package P#0
>> NUMANode P#2 (979MB)
>> Group0
>> NUMANode P#0 (980MB)
>> Core P#0 + PU P#0
>> Core P#1 + PU P#1
>> Group0
>> NUMANode P#1 (1007MB)
>> Core P#2 + PU P#2
>> Core P#3 + PU P#3
> here should be a dis-assembled dump of generated HMAT table
Hello
Like what I added at the end of
https://github.com/bgoglin/qemu/commit/d9b3f5cb1514adafa644afcc2a363f2dc9795a32
?
> + a test case, see tests/qtest/bios-tables-test.c
> for the process (at tho top) and test examples
https://github.com/bgoglin/qemu/commit/643dfa2de8b3e1f5b5675825e5d1be5c93a9549c
This passes make check V=1 but I am really not sure about what I did.
The doc is far from easy for new contributors. Only HMAT matters here,
but it looks like it wanted some other tables too. Also I don't know
about pc vs piix4 vs q35, what "tcg" is, etc.
Advices appreciated.
How are we supposed to send patches that contain binary changes?
Brice
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-06-20 15:24 ` Brice Goglin
@ 2022-06-20 16:05 ` Igor Mammedov
2022-06-21 10:49 ` Brice Goglin
0 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2022-06-20 16:05 UTC (permalink / raw)
To: Brice Goglin
Cc: qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
On Mon, 20 Jun 2022 17:24:18 +0200
Brice Goglin <Brice.Goglin@inria.fr> wrote:
> Le 20/06/2022 à 15:27, Igor Mammedov a écrit Machine (2966MB total) +
> Package P#0
> >> NUMANode P#2 (979MB)
> >> Group0
> >> NUMANode P#0 (980MB)
> >> Core P#0 + PU P#0
> >> Core P#1 + PU P#1
> >> Group0
> >> NUMANode P#1 (1007MB)
> >> Core P#2 + PU P#2
> >> Core P#3 + PU P#3
> > here should be a dis-assembled dump of generated HMAT table
>
>
> Hello
>
> Like what I added at the end of
> https://github.com/bgoglin/qemu/commit/d9b3f5cb1514adafa644afcc2a363f2dc9795a32
> ?
yep, only full version including headers.
(install acpica-tools on host and then when
you run your test with 'make V=1 check-qtest', it will dump
diff on console for you)
> > + a test case, see tests/qtest/bios-tables-test.c
> > for the process (at tho top) and test examples
>
>
> https://github.com/bgoglin/qemu/commit/643dfa2de8b3e1f5b5675825e5d1be5c93a9549c
>
> This passes make check V=1 but I am really not sure about what I did.
> The doc is far from easy for new contributors. Only HMAT matters here,
improvements to doc comment are welcome
> but it looks like it wanted some other tables too.
if test can't find a table with data.variant, it defaults to checking
against suffix-less variant if such exists.
you will need to provide all tables for your config that differ from
default one (i.e. not only HMAT one).
also CI runs on memory constrained systems, so limit RAM usage
to possible minimum (see other numa tests)
> Also I don't know
> about pc vs piix4 vs q35, what "tcg" is, etc.
your case is not tcg or kvm specific, so put it in generic x86 section
(just like you did) and I'd pick q35 as used machine (though it should
work with pc too)
>
> Advices appreciated.
>
>
> How are we supposed to send patches that contain binary changes?
here is a recent example of adding a new test:
https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg01320.html
https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg01320.html
https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg01320.html
idea of splitting it on several patches is to keeps
series bisect-able
1. whitelist changes/add empty files if necessary,
2. add test which will spit warnings and one can see what has been changed/added
3. update expected blobs and remove whitelisting, so any unexpected change
will trigger test failure
(compile after each step to make sure patches are split correctly)
>
> Brice
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-06-20 16:05 ` Igor Mammedov
@ 2022-06-21 10:49 ` Brice Goglin
2022-06-23 11:24 ` Jonathan Cameron via
0 siblings, 1 reply; 8+ messages in thread
From: Brice Goglin @ 2022-06-21 10:49 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
[-- Attachment #1.1: Type: text/plain, Size: 1079 bytes --]
Le 20/06/2022 à 18:05, Igor Mammedov a écrit :
> On Mon, 20 Jun 2022 17:24:18 +0200
> Brice Goglin <Brice.Goglin@inria.fr> wrote:
>
>> Le 20/06/2022 à 15:27, Igor Mammedov a écrit Machine (2966MB total) +
>> Package P#0
>>>> NUMANode P#2 (979MB)
>>>> Group0
>>>> NUMANode P#0 (980MB)
>>>> Core P#0 + PU P#0
>>>> Core P#1 + PU P#1
>>>> Group0
>>>> NUMANode P#1 (1007MB)
>>>> Core P#2 + PU P#2
>>>> Core P#3 + PU P#3
>>> here should be a dis-assembled dump of generated HMAT table
>>
>> Hello
>>
>> Like what I added at the end of
>> https://github.com/bgoglin/qemu/commit/d9b3f5cb1514adafa644afcc2a363f2dc9795a32
>> ?
> yep, only full version including headers.
Thanks,
I split the patch adding the new test in 3 patches, reduced memory to
128M total, etc.
I am not sure I understood what I am supposed to put in commit messages.
Can you check the 4 patches on top of
https://github.com/bgoglin/qemu/commits/hmat-noinitiator before I resend
them to the list?
Brice
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hmat acpi: Don't require initiator value in -numa when hmat=on
2022-06-21 10:49 ` Brice Goglin
@ 2022-06-23 11:24 ` Jonathan Cameron via
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron via @ 2022-06-23 11:24 UTC (permalink / raw)
To: Brice Goglin
Cc: Igor Mammedov, qemu-devel, Eduardo Habkost, Liu Jingqi, Tao Xu,
Philippe Mathieu-Daudé,
Yanan Wang
On Tue, 21 Jun 2022 12:49:09 +0200
Brice Goglin <Brice.Goglin@inria.fr> wrote:
> Le 20/06/2022 à 18:05, Igor Mammedov a écrit :
> > On Mon, 20 Jun 2022 17:24:18 +0200
> > Brice Goglin <Brice.Goglin@inria.fr> wrote:
> >
> >> Le 20/06/2022 à 15:27, Igor Mammedov a écrit Machine (2966MB total) +
> >> Package P#0
> >>>> NUMANode P#2 (979MB)
> >>>> Group0
> >>>> NUMANode P#0 (980MB)
> >>>> Core P#0 + PU P#0
> >>>> Core P#1 + PU P#1
> >>>> Group0
> >>>> NUMANode P#1 (1007MB)
> >>>> Core P#2 + PU P#2
> >>>> Core P#3 + PU P#3
> >>> here should be a dis-assembled dump of generated HMAT table
> >>
> >> Hello
> >>
> >> Like what I added at the end of
> >> https://github.com/bgoglin/qemu/commit/d9b3f5cb1514adafa644afcc2a363f2dc9795a32
> >> ?
> > yep, only full version including headers.
>
>
> Thanks,
>
> I split the patch adding the new test in 3 patches, reduced memory to
> 128M total, etc.
>
> I am not sure I understood what I am supposed to put in commit messages.
> Can you check the 4 patches on top of
> https://github.com/bgoglin/qemu/commits/hmat-noinitiator before I resend
> them to the list?
>
> Brice
>
I left a couple of trivial comments on github, but basically looks good
to me.
Jonathan
^ permalink raw reply [flat|nested] 8+ messages in thread