* [PATCH v4 1/3] spapr: move h_home_node_associativity to spapr_numa.c
2020-09-04 1:04 [PATCH v4 0/3] pseries NUMA distance rework Daniel Henrique Barboza
@ 2020-09-04 1:04 ` Daniel Henrique Barboza
2020-09-04 4:09 ` David Gibson
2020-09-04 1:04 ` [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper Daniel Henrique Barboza
2020-09-04 1:04 ` [PATCH v4 3/3] spapr_numa: use spapr_numa_get_vcpu_assoc() in home_node hcall Daniel Henrique Barboza
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2020-09-04 1:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, david
The implementation of this hypercall will be modified to use
spapr->numa_assoc_arrays input. Moving it to spapr_numa.c makes
make more sense.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr_hcall.c | 40 ---------------------------------------
hw/ppc/spapr_numa.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 40 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index c1d01228c6..c2776b6a7d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1873,42 +1873,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
return ret;
}
-static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
- SpaprMachineState *spapr,
- target_ulong opcode,
- target_ulong *args)
-{
- target_ulong flags = args[0];
- target_ulong procno = args[1];
- PowerPCCPU *tcpu;
- int idx;
-
- /* only support procno from H_REGISTER_VPA */
- if (flags != 0x1) {
- return H_FUNCTION;
- }
-
- tcpu = spapr_find_cpu(procno);
- if (tcpu == NULL) {
- return H_P2;
- }
-
- /* sequence is the same as in the "ibm,associativity" property */
-
- idx = 0;
-#define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
- ((uint64_t)(b) & 0xffffffff))
- args[idx++] = ASSOCIATIVITY(0, 0);
- args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
- args[idx++] = ASSOCIATIVITY(procno, -1);
- for ( ; idx < 6; idx++) {
- args[idx] = -1;
- }
-#undef ASSOCIATIVITY
-
- return H_SUCCESS;
-}
-
static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
SpaprMachineState *spapr,
target_ulong opcode,
@@ -2139,10 +2103,6 @@ static void hypercall_register_types(void)
spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
-
- /* Virtual Processor Home Node */
- spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
- h_home_node_associativity);
}
type_init(hypercall_register_types)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 93a000b729..368c1a494d 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -165,3 +165,48 @@ void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
_FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
maxdomains, sizeof(maxdomains)));
}
+
+static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
+ SpaprMachineState *spapr,
+ target_ulong opcode,
+ target_ulong *args)
+{
+ target_ulong flags = args[0];
+ target_ulong procno = args[1];
+ PowerPCCPU *tcpu;
+ int idx;
+
+ /* only support procno from H_REGISTER_VPA */
+ if (flags != 0x1) {
+ return H_FUNCTION;
+ }
+
+ tcpu = spapr_find_cpu(procno);
+ if (tcpu == NULL) {
+ return H_P2;
+ }
+
+ /* sequence is the same as in the "ibm,associativity" property */
+
+ idx = 0;
+#define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
+ ((uint64_t)(b) & 0xffffffff))
+ args[idx++] = ASSOCIATIVITY(0, 0);
+ args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
+ args[idx++] = ASSOCIATIVITY(procno, -1);
+ for ( ; idx < 6; idx++) {
+ args[idx] = -1;
+ }
+#undef ASSOCIATIVITY
+
+ return H_SUCCESS;
+}
+
+static void spapr_numa_register_types(void)
+{
+ /* Virtual Processor Home Node */
+ spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
+ h_home_node_associativity);
+}
+
+type_init(spapr_numa_register_types)
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 1/3] spapr: move h_home_node_associativity to spapr_numa.c
2020-09-04 1:04 ` [PATCH v4 1/3] spapr: move h_home_node_associativity to spapr_numa.c Daniel Henrique Barboza
@ 2020-09-04 4:09 ` David Gibson
0 siblings, 0 replies; 10+ messages in thread
From: David Gibson @ 2020-09-04 4:09 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]
On Thu, Sep 03, 2020 at 10:04:37PM -0300, Daniel Henrique Barboza wrote:
> The implementation of this hypercall will be modified to use
> spapr->numa_assoc_arrays input. Moving it to spapr_numa.c makes
> make more sense.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Applied to ppc-for-5.2 (though it missed today's pull request, sorry).
> ---
> hw/ppc/spapr_hcall.c | 40 ---------------------------------------
> hw/ppc/spapr_numa.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+), 40 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index c1d01228c6..c2776b6a7d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1873,42 +1873,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> return ret;
> }
>
> -static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
> - SpaprMachineState *spapr,
> - target_ulong opcode,
> - target_ulong *args)
> -{
> - target_ulong flags = args[0];
> - target_ulong procno = args[1];
> - PowerPCCPU *tcpu;
> - int idx;
> -
> - /* only support procno from H_REGISTER_VPA */
> - if (flags != 0x1) {
> - return H_FUNCTION;
> - }
> -
> - tcpu = spapr_find_cpu(procno);
> - if (tcpu == NULL) {
> - return H_P2;
> - }
> -
> - /* sequence is the same as in the "ibm,associativity" property */
> -
> - idx = 0;
> -#define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
> - ((uint64_t)(b) & 0xffffffff))
> - args[idx++] = ASSOCIATIVITY(0, 0);
> - args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> - args[idx++] = ASSOCIATIVITY(procno, -1);
> - for ( ; idx < 6; idx++) {
> - args[idx] = -1;
> - }
> -#undef ASSOCIATIVITY
> -
> - return H_SUCCESS;
> -}
> -
> static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> target_ulong opcode,
> @@ -2139,10 +2103,6 @@ static void hypercall_register_types(void)
> spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
>
> spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> -
> - /* Virtual Processor Home Node */
> - spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> - h_home_node_associativity);
> }
>
> type_init(hypercall_register_types)
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 93a000b729..368c1a494d 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -165,3 +165,48 @@ void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
> _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> maxdomains, sizeof(maxdomains)));
> }
> +
> +static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + target_ulong opcode,
> + target_ulong *args)
> +{
> + target_ulong flags = args[0];
> + target_ulong procno = args[1];
> + PowerPCCPU *tcpu;
> + int idx;
> +
> + /* only support procno from H_REGISTER_VPA */
> + if (flags != 0x1) {
> + return H_FUNCTION;
> + }
> +
> + tcpu = spapr_find_cpu(procno);
> + if (tcpu == NULL) {
> + return H_P2;
> + }
> +
> + /* sequence is the same as in the "ibm,associativity" property */
> +
> + idx = 0;
> +#define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
> + ((uint64_t)(b) & 0xffffffff))
> + args[idx++] = ASSOCIATIVITY(0, 0);
> + args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> + args[idx++] = ASSOCIATIVITY(procno, -1);
> + for ( ; idx < 6; idx++) {
> + args[idx] = -1;
> + }
> +#undef ASSOCIATIVITY
> +
> + return H_SUCCESS;
> +}
> +
> +static void spapr_numa_register_types(void)
> +{
> + /* Virtual Processor Home Node */
> + spapr_register_hypercall(H_HOME_NODE_ASSOCIATIVITY,
> + h_home_node_associativity);
> +}
> +
> +type_init(spapr_numa_register_types)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper
2020-09-04 1:04 [PATCH v4 0/3] pseries NUMA distance rework Daniel Henrique Barboza
2020-09-04 1:04 ` [PATCH v4 1/3] spapr: move h_home_node_associativity to spapr_numa.c Daniel Henrique Barboza
@ 2020-09-04 1:04 ` Daniel Henrique Barboza
2020-09-04 4:10 ` David Gibson
2020-09-04 10:02 ` Greg Kurz
2020-09-04 1:04 ` [PATCH v4 3/3] spapr_numa: use spapr_numa_get_vcpu_assoc() in home_node hcall Daniel Henrique Barboza
2 siblings, 2 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2020-09-04 1:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, david
The work to be done in h_home_node_associativity() intersects
with what is already done in spapr_numa_fixup_cpu_dt(). This
patch creates a new helper, spapr_numa_get_vcpu_assoc(), to
be used for both spapr_numa_fixup_cpu_dt() and
h_home_node_associativity().
While we're at it, use memcpy() instead of loop assignment
to created the returned array.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr_numa.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 368c1a494d..980a6488bf 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -71,13 +71,15 @@ void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
sizeof(spapr->numa_assoc_array[nodeid]))));
}
-int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
- int offset, PowerPCCPU *cpu)
+static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
+ PowerPCCPU *cpu,
+ uint *vcpu_assoc_size)
{
- uint vcpu_assoc_size = NUMA_ASSOC_SIZE + 1;
- uint32_t vcpu_assoc[vcpu_assoc_size];
+ uint32_t *vcpu_assoc = NULL;
int index = spapr_get_vcpu_id(cpu);
- int i;
+
+ *vcpu_assoc_size = (NUMA_ASSOC_SIZE + 1) * sizeof(uint32_t);
+ vcpu_assoc = g_malloc(*vcpu_assoc_size);
/*
* VCPUs have an extra 'cpu_id' value in ibm,associativity
@@ -86,16 +88,24 @@ int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
* cpu_id last.
*/
vcpu_assoc[0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS + 1);
+ memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
+ MAX_DISTANCE_REF_POINTS);
+ vcpu_assoc[MAX_DISTANCE_REF_POINTS + 1] = cpu_to_be32(index);
- for (i = 1; i <= MAX_DISTANCE_REF_POINTS; i++) {
- vcpu_assoc[i] = spapr->numa_assoc_array[cpu->node_id][i];
- }
+ return vcpu_assoc;
+}
+
+int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
+ int offset, PowerPCCPU *cpu)
+{
+ g_autofree uint32_t *vcpu_assoc = NULL;
+ uint vcpu_assoc_size;
- vcpu_assoc[vcpu_assoc_size - 1] = cpu_to_be32(index);
+ vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, cpu, &vcpu_assoc_size);
/* Advertise NUMA via ibm,associativity */
return fdt_setprop(fdt, offset, "ibm,associativity",
- vcpu_assoc, sizeof(vcpu_assoc));
+ vcpu_assoc, vcpu_assoc_size);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper
2020-09-04 1:04 ` [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper Daniel Henrique Barboza
@ 2020-09-04 4:10 ` David Gibson
2020-09-04 9:14 ` Daniel Henrique Barboza
2020-09-04 10:02 ` Greg Kurz
1 sibling, 1 reply; 10+ messages in thread
From: David Gibson @ 2020-09-04 4:10 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]
On Thu, Sep 03, 2020 at 10:04:38PM -0300, Daniel Henrique Barboza wrote:
> The work to be done in h_home_node_associativity() intersects
> with what is already done in spapr_numa_fixup_cpu_dt(). This
> patch creates a new helper, spapr_numa_get_vcpu_assoc(), to
> be used for both spapr_numa_fixup_cpu_dt() and
> h_home_node_associativity().
>
> While we're at it, use memcpy() instead of loop assignment
> to created the returned array.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr_numa.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 368c1a494d..980a6488bf 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -71,13 +71,15 @@ void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
> sizeof(spapr->numa_assoc_array[nodeid]))));
> }
>
> -int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> - int offset, PowerPCCPU *cpu)
> +static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
> + PowerPCCPU *cpu,
> + uint *vcpu_assoc_size)
> {
> - uint vcpu_assoc_size = NUMA_ASSOC_SIZE + 1;
> - uint32_t vcpu_assoc[vcpu_assoc_size];
> + uint32_t *vcpu_assoc = NULL;
> int index = spapr_get_vcpu_id(cpu);
> - int i;
> +
> + *vcpu_assoc_size = (NUMA_ASSOC_SIZE + 1) * sizeof(uint32_t);
> + vcpu_assoc = g_malloc(*vcpu_assoc_size);
>
> /*
> * VCPUs have an extra 'cpu_id' value in ibm,associativity
> @@ -86,16 +88,24 @@ int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> * cpu_id last.
> */
> vcpu_assoc[0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS + 1);
> + memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
> + MAX_DISTANCE_REF_POINTS);
That needs to be MAX_DISTANCE_REF_POINTS * sizeof(uint32_t), doesn't it?
> + vcpu_assoc[MAX_DISTANCE_REF_POINTS + 1] = cpu_to_be32(index);
>
> - for (i = 1; i <= MAX_DISTANCE_REF_POINTS; i++) {
> - vcpu_assoc[i] = spapr->numa_assoc_array[cpu->node_id][i];
> - }
> + return vcpu_assoc;
> +}
> +
> +int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> + int offset, PowerPCCPU *cpu)
> +{
> + g_autofree uint32_t *vcpu_assoc = NULL;
> + uint vcpu_assoc_size;
>
> - vcpu_assoc[vcpu_assoc_size - 1] = cpu_to_be32(index);
> + vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, cpu, &vcpu_assoc_size);
>
> /* Advertise NUMA via ibm,associativity */
> return fdt_setprop(fdt, offset, "ibm,associativity",
> - vcpu_assoc, sizeof(vcpu_assoc));
> + vcpu_assoc, vcpu_assoc_size);> }
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper
2020-09-04 4:10 ` David Gibson
@ 2020-09-04 9:14 ` Daniel Henrique Barboza
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2020-09-04 9:14 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel
On 9/4/20 1:10 AM, David Gibson wrote:
> On Thu, Sep 03, 2020 at 10:04:38PM -0300, Daniel Henrique Barboza wrote:
>> The work to be done in h_home_node_associativity() intersects
>> with what is already done in spapr_numa_fixup_cpu_dt(). This
>> patch creates a new helper, spapr_numa_get_vcpu_assoc(), to
>> be used for both spapr_numa_fixup_cpu_dt() and
>> h_home_node_associativity().
>>
>> While we're at it, use memcpy() instead of loop assignment
>> to created the returned array.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> hw/ppc/spapr_numa.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 368c1a494d..980a6488bf 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -71,13 +71,15 @@ void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
>> sizeof(spapr->numa_assoc_array[nodeid]))));
>> }
>>
>> -int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> - int offset, PowerPCCPU *cpu)
>> +static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu,
>> + uint *vcpu_assoc_size)
>> {
>> - uint vcpu_assoc_size = NUMA_ASSOC_SIZE + 1;
>> - uint32_t vcpu_assoc[vcpu_assoc_size];
>> + uint32_t *vcpu_assoc = NULL;
>> int index = spapr_get_vcpu_id(cpu);
>> - int i;
>> +
>> + *vcpu_assoc_size = (NUMA_ASSOC_SIZE + 1) * sizeof(uint32_t);
>> + vcpu_assoc = g_malloc(*vcpu_assoc_size);
>>
>> /*
>> * VCPUs have an extra 'cpu_id' value in ibm,associativity
>> @@ -86,16 +88,24 @@ int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> * cpu_id last.
>> */
>> vcpu_assoc[0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS + 1);
>> + memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
>> + MAX_DISTANCE_REF_POINTS);
>
> That needs to be MAX_DISTANCE_REF_POINTS * sizeof(uint32_t), doesn't it?
Hmmmm yeah it does. Even if this didn't break spectacularly in my guest (not
sure why), we're doing a similar operation in spapr_numa_write_assoc_lookup_arrays()
using 'sizeof(uint32_t)'. Might as well do the same here.
Thanks,
DHB
>
>> + vcpu_assoc[MAX_DISTANCE_REF_POINTS + 1] = cpu_to_be32(index);
>>
>> - for (i = 1; i <= MAX_DISTANCE_REF_POINTS; i++) {
>> - vcpu_assoc[i] = spapr->numa_assoc_array[cpu->node_id][i];
>> - }
>> + return vcpu_assoc;
>> +}
>> +
>> +int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> + int offset, PowerPCCPU *cpu)
>> +{
>> + g_autofree uint32_t *vcpu_assoc = NULL;
>> + uint vcpu_assoc_size;
>>
>> - vcpu_assoc[vcpu_assoc_size - 1] = cpu_to_be32(index);
>> + vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, cpu, &vcpu_assoc_size);
>>
>> /* Advertise NUMA via ibm,associativity */
>> return fdt_setprop(fdt, offset, "ibm,associativity",
>> - vcpu_assoc, sizeof(vcpu_assoc));
>> + vcpu_assoc, vcpu_assoc_size);> }
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper
2020-09-04 1:04 ` [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper Daniel Henrique Barboza
2020-09-04 4:10 ` David Gibson
@ 2020-09-04 10:02 ` Greg Kurz
2020-09-04 10:19 ` Daniel Henrique Barboza
1 sibling, 1 reply; 10+ messages in thread
From: Greg Kurz @ 2020-09-04 10:02 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, david
On Thu, 3 Sep 2020 22:04:38 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> The work to be done in h_home_node_associativity() intersects
> with what is already done in spapr_numa_fixup_cpu_dt(). This
> patch creates a new helper, spapr_numa_get_vcpu_assoc(), to
> be used for both spapr_numa_fixup_cpu_dt() and
> h_home_node_associativity().
>
> While we're at it, use memcpy() instead of loop assignment
> to created the returned array.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
Hi Daniel,
A few comments below.
> hw/ppc/spapr_numa.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 368c1a494d..980a6488bf 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -71,13 +71,15 @@ void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
> sizeof(spapr->numa_assoc_array[nodeid]))));
> }
>
> -int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> - int offset, PowerPCCPU *cpu)
> +static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
> + PowerPCCPU *cpu,
> + uint *vcpu_assoc_size)
> {
> - uint vcpu_assoc_size = NUMA_ASSOC_SIZE + 1;
> - uint32_t vcpu_assoc[vcpu_assoc_size];
> + uint32_t *vcpu_assoc = NULL;
You don't need to initialize this pointer since it is assigned a value
unconditionally just below.
> int index = spapr_get_vcpu_id(cpu);
> - int i;
> +
> + *vcpu_assoc_size = (NUMA_ASSOC_SIZE + 1) * sizeof(uint32_t);
It's a bit weird to return something that is definitely a compile
time constant by reference... What about introducing a macro ?
#define VCPU_NUMA_ASSOC_SIZE (NUMA_ASSOC_SIZE + 1)
> + vcpu_assoc = g_malloc(*vcpu_assoc_size);
>
vcpu_assoc = g_new(uint32_t, VCPU_NUMA_ASSOC_SIZE);
> /*
> * VCPUs have an extra 'cpu_id' value in ibm,associativity
> @@ -86,16 +88,24 @@ int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> * cpu_id last.
> */
> vcpu_assoc[0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS + 1);
> + memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
> + MAX_DISTANCE_REF_POINTS);
> + vcpu_assoc[MAX_DISTANCE_REF_POINTS + 1] = cpu_to_be32(index);
>
memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
(VPCU_ASSOC_SIZE - 2) * sizeof(uint32_t));
vcpu_assoc[VCPU_ASSOC_SIZE - 1] = cpu_to_be32(index);
I personally find more clear than using MAX_DISTANCE_REF_POINTS in an array
that was just allocated with NUMA_ASSOC_SIZE... one has to check spapr.h
to see that NUMA_ASSOC_SIZE == MAX_DISTANCE_REF_POINTS + 1
> - for (i = 1; i <= MAX_DISTANCE_REF_POINTS; i++) {
> - vcpu_assoc[i] = spapr->numa_assoc_array[cpu->node_id][i];
> - }
> + return vcpu_assoc;
> +}
> +
> +int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
> + int offset, PowerPCCPU *cpu)
> +{
> + g_autofree uint32_t *vcpu_assoc = NULL;
> + uint vcpu_assoc_size;
>
> - vcpu_assoc[vcpu_assoc_size - 1] = cpu_to_be32(index);
> + vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, cpu, &vcpu_assoc_size);
>
> /* Advertise NUMA via ibm,associativity */
> return fdt_setprop(fdt, offset, "ibm,associativity",
> - vcpu_assoc, sizeof(vcpu_assoc));
> + vcpu_assoc, vcpu_assoc_size);
return fdt_setprop(fdt, offset, "ibm,associativity",
vcpu_assoc, VCPU_NUMA_ASSOC_SIZE * sizeof(uint32_t));
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper
2020-09-04 10:02 ` Greg Kurz
@ 2020-09-04 10:19 ` Daniel Henrique Barboza
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Henrique Barboza @ 2020-09-04 10:19 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-ppc, qemu-devel, david
On 9/4/20 7:02 AM, Greg Kurz wrote:
> On Thu, 3 Sep 2020 22:04:38 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>
>> The work to be done in h_home_node_associativity() intersects
>> with what is already done in spapr_numa_fixup_cpu_dt(). This
>> patch creates a new helper, spapr_numa_get_vcpu_assoc(), to
>> be used for both spapr_numa_fixup_cpu_dt() and
>> h_home_node_associativity().
>>
>> While we're at it, use memcpy() instead of loop assignment
>> to created the returned array.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>
> Hi Daniel,
>
> A few comments below.
>
>> hw/ppc/spapr_numa.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 368c1a494d..980a6488bf 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -71,13 +71,15 @@ void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
>> sizeof(spapr->numa_assoc_array[nodeid]))));
>> }
>>
>> -int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> - int offset, PowerPCCPU *cpu)
>> +static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
>> + PowerPCCPU *cpu,
>> + uint *vcpu_assoc_size)
>> {
>> - uint vcpu_assoc_size = NUMA_ASSOC_SIZE + 1;
>> - uint32_t vcpu_assoc[vcpu_assoc_size];
>> + uint32_t *vcpu_assoc = NULL;
>
> You don't need to initialize this pointer since it is assigned a value
> unconditionally just below.
>
>> int index = spapr_get_vcpu_id(cpu);
>> - int i;
>> +
>> + *vcpu_assoc_size = (NUMA_ASSOC_SIZE + 1) * sizeof(uint32_t);
>
> It's a bit weird to return something that is definitely a compile
> time constant by reference... What about introducing a macro ?
>
> #define VCPU_NUMA_ASSOC_SIZE (NUMA_ASSOC_SIZE + 1)
>
>> + vcpu_assoc = g_malloc(*vcpu_assoc_size);
>>
>
> vcpu_assoc = g_new(uint32_t, VCPU_NUMA_ASSOC_SIZE);
>
>> /*
>> * VCPUs have an extra 'cpu_id' value in ibm,associativity
>> @@ -86,16 +88,24 @@ int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> * cpu_id last.
>> */
>> vcpu_assoc[0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS + 1);
>> + memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
>> + MAX_DISTANCE_REF_POINTS);
>> + vcpu_assoc[MAX_DISTANCE_REF_POINTS + 1] = cpu_to_be32(index);
>>
>
> memcpy(vcpu_assoc + 1, spapr->numa_assoc_array[cpu->node_id],
> (VPCU_ASSOC_SIZE - 2) * sizeof(uint32_t));
> vcpu_assoc[VCPU_ASSOC_SIZE - 1] = cpu_to_be32(index);
>
> I personally find more clear than using MAX_DISTANCE_REF_POINTS in an array
> that was just allocated with NUMA_ASSOC_SIZE... one has to check spapr.h
> to see that NUMA_ASSOC_SIZE == MAX_DISTANCE_REF_POINTS + 1
That all makes sense to me. I'll introduce a VCPU_ASSOC_SIZE in spapr_numa.h
and use it when operating the associativity for vcpus, both in this patch
and also in patch 3.
Thanks,
DHB
>
>> - for (i = 1; i <= MAX_DISTANCE_REF_POINTS; i++) {
>> - vcpu_assoc[i] = spapr->numa_assoc_array[cpu->node_id][i];
>> - }
>> + return vcpu_assoc;
>> +}
>> +
>> +int spapr_numa_fixup_cpu_dt(SpaprMachineState *spapr, void *fdt,
>> + int offset, PowerPCCPU *cpu)
>> +{
>> + g_autofree uint32_t *vcpu_assoc = NULL;
>> + uint vcpu_assoc_size;
>>
>> - vcpu_assoc[vcpu_assoc_size - 1] = cpu_to_be32(index);
>> + vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, cpu, &vcpu_assoc_size);
>>
>> /* Advertise NUMA via ibm,associativity */
>> return fdt_setprop(fdt, offset, "ibm,associativity",
>> - vcpu_assoc, sizeof(vcpu_assoc));
>> + vcpu_assoc, vcpu_assoc_size);
>
> return fdt_setprop(fdt, offset, "ibm,associativity",
> vcpu_assoc, VCPU_NUMA_ASSOC_SIZE * sizeof(uint32_t));
>
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 3/3] spapr_numa: use spapr_numa_get_vcpu_assoc() in home_node hcall
2020-09-04 1:04 [PATCH v4 0/3] pseries NUMA distance rework Daniel Henrique Barboza
2020-09-04 1:04 ` [PATCH v4 1/3] spapr: move h_home_node_associativity to spapr_numa.c Daniel Henrique Barboza
2020-09-04 1:04 ` [PATCH v4 2/3] spapr_numa: create a vcpu associativity helper Daniel Henrique Barboza
@ 2020-09-04 1:04 ` Daniel Henrique Barboza
2020-09-04 10:33 ` Greg Kurz
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Henrique Barboza @ 2020-09-04 1:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, david
The current implementation of h_home_node_associativity hard codes
the values of associativity domains of the vcpus. Let's make
it consider the values already initialized in spapr->numa_assoc_array,
via the spapr_numa_get_vcpu_assoc() helper.
We want to set it and forget it, and for that we also need to
assert that we don't overflow the registers of the hypercall.
From R4 to R9 we can squeeze in 12 associativity domains, so
let's assert that MAX_DISTANCE_REF_POINTS isn't greater
than that.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr_numa.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 980a6488bf..0a7e07fe60 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -181,10 +181,12 @@ static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
target_ulong opcode,
target_ulong *args)
{
+ g_autofree uint32_t *vcpu_assoc = NULL;
target_ulong flags = args[0];
target_ulong procno = args[1];
PowerPCCPU *tcpu;
- int idx;
+ uint vcpu_assoc_size;
+ int idx, assoc_idx;
/* only support procno from H_REGISTER_VPA */
if (flags != 0x1) {
@@ -196,16 +198,31 @@ static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
return H_P2;
}
- /* sequence is the same as in the "ibm,associativity" property */
+ /*
+ * Given that we want to be flexible with the sizes and indexes,
+ * we must consider that there is a hard limit of how many
+ * associativities domain we can fit in R4 up to R9, which
+ * would be 12. Assert and bail if that's not the case.
+ */
+ G_STATIC_ASSERT(MAX_DISTANCE_REF_POINTS <= 12);
+
+ vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, tcpu, &vcpu_assoc_size);
+ vcpu_assoc_size /= sizeof(uint32_t);
+ /* assoc_idx starts at 1 to skip associativity size */
+ assoc_idx = 1;
- idx = 0;
#define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
((uint64_t)(b) & 0xffffffff))
- args[idx++] = ASSOCIATIVITY(0, 0);
- args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
- args[idx++] = ASSOCIATIVITY(procno, -1);
- for ( ; idx < 6; idx++) {
- args[idx] = -1;
+
+ for (idx = 0; idx < 6; idx++) {
+ int32_t a, b;
+
+ a = assoc_idx < vcpu_assoc_size ?
+ be32_to_cpu(vcpu_assoc[assoc_idx++]) : -1;
+ b = assoc_idx < vcpu_assoc_size ?
+ be32_to_cpu(vcpu_assoc[assoc_idx++]) : -1;
+
+ args[idx] = ASSOCIATIVITY(a, b);
}
#undef ASSOCIATIVITY
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 3/3] spapr_numa: use spapr_numa_get_vcpu_assoc() in home_node hcall
2020-09-04 1:04 ` [PATCH v4 3/3] spapr_numa: use spapr_numa_get_vcpu_assoc() in home_node hcall Daniel Henrique Barboza
@ 2020-09-04 10:33 ` Greg Kurz
0 siblings, 0 replies; 10+ messages in thread
From: Greg Kurz @ 2020-09-04 10:33 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, david
On Thu, 3 Sep 2020 22:04:39 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> The current implementation of h_home_node_associativity hard codes
> the values of associativity domains of the vcpus. Let's make
> it consider the values already initialized in spapr->numa_assoc_array,
> via the spapr_numa_get_vcpu_assoc() helper.
>
> We want to set it and forget it, and for that we also need to
> assert that we don't overflow the registers of the hypercall.
> From R4 to R9 we can squeeze in 12 associativity domains, so
> let's assert that MAX_DISTANCE_REF_POINTS isn't greater
> than that.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/spapr_numa.c | 33 +++++++++++++++++++++++++--------
> 1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 980a6488bf..0a7e07fe60 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -181,10 +181,12 @@ static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
> target_ulong opcode,
> target_ulong *args)
> {
> + g_autofree uint32_t *vcpu_assoc = NULL;
> target_ulong flags = args[0];
> target_ulong procno = args[1];
> PowerPCCPU *tcpu;
> - int idx;
> + uint vcpu_assoc_size;
> + int idx, assoc_idx;
>
> /* only support procno from H_REGISTER_VPA */
> if (flags != 0x1) {
> @@ -196,16 +198,31 @@ static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
> return H_P2;
> }
>
> - /* sequence is the same as in the "ibm,associativity" property */
> + /*
> + * Given that we want to be flexible with the sizes and indexes,
> + * we must consider that there is a hard limit of how many
> + * associativities domain we can fit in R4 up to R9, which
> + * would be 12. Assert and bail if that's not the case.
> + */
> + G_STATIC_ASSERT(MAX_DISTANCE_REF_POINTS <= 12);
> +
> + vcpu_assoc = spapr_numa_get_vcpu_assoc(spapr, tcpu, &vcpu_assoc_size);
> + vcpu_assoc_size /= sizeof(uint32_t);
Using vcpu_assoc_size both as a size-in-bytes and a number of elements in
the array is gross... Anyway since this should go away if you introduce
a macro as suggested in the previous patch.
> + /* assoc_idx starts at 1 to skip associativity size */
> + assoc_idx = 1;
>
> - idx = 0;
> #define ASSOCIATIVITY(a, b) (((uint64_t)(a) << 32) | \
> ((uint64_t)(b) & 0xffffffff))
> - args[idx++] = ASSOCIATIVITY(0, 0);
> - args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> - args[idx++] = ASSOCIATIVITY(procno, -1);
> - for ( ; idx < 6; idx++) {
> - args[idx] = -1;
> +
> + for (idx = 0; idx < 6; idx++) {
> + int32_t a, b;
> +
> + a = assoc_idx < vcpu_assoc_size ?
> + be32_to_cpu(vcpu_assoc[assoc_idx++]) : -1;
> + b = assoc_idx < vcpu_assoc_size ?
> + be32_to_cpu(vcpu_assoc[assoc_idx++]) : -1;
> +
> + args[idx] = ASSOCIATIVITY(a, b);
> }
Ouch this change is really giving me a headache... I understand that
tcpu->node_id and procno are now being read from vcpu_assoc[] but
it's hard to check what vcpu_assoc[assoc_idx++] points to, especially
with the ternary operator... Honestly, I'd rather keep that loop
unrolled with comments telling what's being read.
> #undef ASSOCIATIVITY
>
^ permalink raw reply [flat|nested] 10+ messages in thread