* [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY
@ 2018-12-18 13:18 Laurent Vivier
2018-12-18 16:10 ` Greg Kurz
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Vivier @ 2018-12-18 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson, Greg Kurz, Laurent Vivier
H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
designation associated with the identifier input parameter
This fixes a crash when we try to hotplug a CPU in memory-less and
CPU-less numa node. In this case, the kernel tries to online the
node, but without the information provided by this h-call, the node id,
it cannot and the CPU is started while the node is not onlined.
It also removes the warning message from the kernel:
VPHN is not supported. Disabling polling..
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v2:
add hcall-vphn to ibm,hypertas-functions
correctly check flags
return H_FUNCTION rather than H_PARAMETER
update description
hw/ppc/spapr.c | 1 +
hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h | 1 +
3 files changed, 41 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7a0ab2da54..d961272c8a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1049,6 +1049,7 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
add_str(hypertas, "hcall-sprg0");
add_str(hypertas, "hcall-copy");
add_str(hypertas, "hcall-debug");
+ add_str(hypertas, "hcall-vphn");
add_str(qemu_hypertas, "hcall-memop1");
if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 78fecc8fe9..a48adb3cfb 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1663,6 +1663,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
return H_SUCCESS;
}
+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,
@@ -1864,6 +1899,10 @@ 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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b1a2515107..eb13e2b614 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -447,6 +447,7 @@ struct sPAPRMachineState {
#define H_GET_EM_PARMS 0x2B8
#define H_SET_MPP 0x2D0
#define H_GET_MPP 0x2D4
+#define H_HOME_NODE_ASSOCIATIVITY 0x2EC
#define H_XIRR_X 0x2FC
#define H_RANDOM 0x300
#define H_SET_MODE 0x31C
--
2.19.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY
2018-12-18 13:18 [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY Laurent Vivier
@ 2018-12-18 16:10 ` Greg Kurz
2018-12-19 16:25 ` Laurent Vivier
0 siblings, 1 reply; 4+ messages in thread
From: Greg Kurz @ 2018-12-18 16:10 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, qemu-ppc, David Gibson
On Tue, 18 Dec 2018 14:18:29 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
> designation associated with the identifier input parameter
>
> This fixes a crash when we try to hotplug a CPU in memory-less and
> CPU-less numa node. In this case, the kernel tries to online the
> node, but without the information provided by this h-call, the node id,
> it cannot and the CPU is started while the node is not onlined.
>
> It also removes the warning message from the kernel:
> VPHN is not supported. Disabling polling..
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>
> Notes:
> v2:
> add hcall-vphn to ibm,hypertas-functions
> correctly check flags
> return H_FUNCTION rather than H_PARAMETER
> update description
>
> hw/ppc/spapr.c | 1 +
> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 1 +
> 3 files changed, 41 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7a0ab2da54..d961272c8a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1049,6 +1049,7 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> add_str(hypertas, "hcall-sprg0");
> add_str(hypertas, "hcall-copy");
> add_str(hypertas, "hcall-debug");
> + add_str(hypertas, "hcall-vphn");
> add_str(qemu_hypertas, "hcall-memop1");
>
> if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 78fecc8fe9..a48adb3cfb 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1663,6 +1663,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> return H_SUCCESS;
> }
>
> +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))
It would be safer to have parens around a and b.
> + args[idx++] = ASSOCIATIVITY(0, 0);
> + args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> + args[idx++] = ASSOCIATIVITY(procno, -1);
The returned values are supposed to be a stream of mixed 16-bit and 32-bit
values, _insanely_ encoded as follows:
The high order bit of each 2 byte field is a length specifier:
1: The associativity domain number is contained in the low order 15 bits of the
field,
0: The associativity domain number is contained in the low order 15 bits of the
current field concatenated with the 16 bits of the next sequential field)
ie, usable values are either 31-bit, either 15-bit.
I don't see such an encoding taking place in the code... this may cause
problems if node_id >= 0x80000000: the guest will only get bits 30:16,
and consider bits 15:0 to belong to the next field, which may in turn
be considered as a 15-bit value or combined with the top 16-bits of
procno to form a bogus 31-bit value... :-\
I guess it is okay to go with the "long" variant for everyone since you
only have 5 of them, but you could also use the "short" variant when
values are < 0x10000.
And in any case, it seems we have to limit node ids and vcpu ids to be
31 bits if we're to support this interface.
> + 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,
> @@ -1864,6 +1899,10 @@ 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/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b1a2515107..eb13e2b614 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -447,6 +447,7 @@ struct sPAPRMachineState {
> #define H_GET_EM_PARMS 0x2B8
> #define H_SET_MPP 0x2D0
> #define H_GET_MPP 0x2D4
> +#define H_HOME_NODE_ASSOCIATIVITY 0x2EC
> #define H_XIRR_X 0x2FC
> #define H_RANDOM 0x300
> #define H_SET_MODE 0x31C
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY
2018-12-18 16:10 ` Greg Kurz
@ 2018-12-19 16:25 ` Laurent Vivier
2018-12-19 16:50 ` Greg Kurz
0 siblings, 1 reply; 4+ messages in thread
From: Laurent Vivier @ 2018-12-19 16:25 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, qemu-ppc, David Gibson
On 18/12/2018 17:10, Greg Kurz wrote:
> On Tue, 18 Dec 2018 14:18:29 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
>
>> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
>> designation associated with the identifier input parameter
>>
>> This fixes a crash when we try to hotplug a CPU in memory-less and
>> CPU-less numa node. In this case, the kernel tries to online the
>> node, but without the information provided by this h-call, the node id,
>> it cannot and the CPU is started while the node is not onlined.
>>
>> It also removes the warning message from the kernel:
>> VPHN is not supported. Disabling polling..
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>
>> Notes:
>> v2:
>> add hcall-vphn to ibm,hypertas-functions
>> correctly check flags
>> return H_FUNCTION rather than H_PARAMETER
>> update description
>>
>> hw/ppc/spapr.c | 1 +
>> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr.h | 1 +
>> 3 files changed, 41 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 7a0ab2da54..d961272c8a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1049,6 +1049,7 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
>> add_str(hypertas, "hcall-sprg0");
>> add_str(hypertas, "hcall-copy");
>> add_str(hypertas, "hcall-debug");
>> + add_str(hypertas, "hcall-vphn");
>> add_str(qemu_hypertas, "hcall-memop1");
>>
>> if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 78fecc8fe9..a48adb3cfb 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1663,6 +1663,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>> return H_SUCCESS;
>> }
>>
>> +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))
>
> It would be safer to have parens around a and b.
I agree
>> + args[idx++] = ASSOCIATIVITY(0, 0);
>> + args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
>> + args[idx++] = ASSOCIATIVITY(procno, -1);
>
> The returned values are supposed to be a stream of mixed 16-bit and 32-bit
> values, _insanely_ encoded as follows:
>
> The high order bit of each 2 byte field is a length specifier:
> 1: The associativity domain number is contained in the low order 15 bits of the
> field,
> 0: The associativity domain number is contained in the low order 15 bits of the
> current field concatenated with the 16 bits of the next sequential field)
>
> ie, usable values are either 31-bit, either 15-bit.
>
> I don't see such an encoding taking place in the code... this may cause
> problems if node_id >= 0x80000000: the guest will only get bits 30:16,
> and consider bits 15:0 to belong to the next field, which may in turn
> be considered as a 15-bit value or combined with the top 16-bits of
> procno to form a bogus 31-bit value... :-\
> I guess it is okay to go with the "long" variant for everyone since you
> only have 5 of them, but you could also use the "short" variant when
> values are < 0x10000.
>
> And in any case, it seems we have to limit node ids and vcpu ids to be
> 31 bits if we're to support this interface.
>From PowerPCCPU, node_id is an int32_t, vcpu_id is an "int", and node id
and cpu id cannot be negative, so the value encoded are already 31bit
values. Moreover, in NumaNodeOptions (see qapi/misc.json) nodeid and
cpus are uint16.
So I think we don't have to check the bit 31 of these values.
I don't want to manage the 15bit encoding because the list is hard coded
and I want to keep it simple.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY
2018-12-19 16:25 ` Laurent Vivier
@ 2018-12-19 16:50 ` Greg Kurz
0 siblings, 0 replies; 4+ messages in thread
From: Greg Kurz @ 2018-12-19 16:50 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-devel, qemu-ppc, David Gibson
On Wed, 19 Dec 2018 17:25:04 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> On 18/12/2018 17:10, Greg Kurz wrote:
> > On Tue, 18 Dec 2018 14:18:29 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >
> >> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
> >> designation associated with the identifier input parameter
> >>
> >> This fixes a crash when we try to hotplug a CPU in memory-less and
> >> CPU-less numa node. In this case, the kernel tries to online the
> >> node, but without the information provided by this h-call, the node id,
> >> it cannot and the CPU is started while the node is not onlined.
> >>
> >> It also removes the warning message from the kernel:
> >> VPHN is not supported. Disabling polling..
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>
> >> Notes:
> >> v2:
> >> add hcall-vphn to ibm,hypertas-functions
> >> correctly check flags
> >> return H_FUNCTION rather than H_PARAMETER
> >> update description
> >>
> >> hw/ppc/spapr.c | 1 +
> >> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
> >> include/hw/ppc/spapr.h | 1 +
> >> 3 files changed, 41 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 7a0ab2da54..d961272c8a 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1049,6 +1049,7 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
> >> add_str(hypertas, "hcall-sprg0");
> >> add_str(hypertas, "hcall-copy");
> >> add_str(hypertas, "hcall-debug");
> >> + add_str(hypertas, "hcall-vphn");
> >> add_str(qemu_hypertas, "hcall-memop1");
> >>
> >> if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 78fecc8fe9..a48adb3cfb 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -1663,6 +1663,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >> return H_SUCCESS;
> >> }
> >>
> >> +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))
> >
> > It would be safer to have parens around a and b.
>
> I agree
>
> >> + args[idx++] = ASSOCIATIVITY(0, 0);
> >> + args[idx++] = ASSOCIATIVITY(0, tcpu->node_id);
> >> + args[idx++] = ASSOCIATIVITY(procno, -1);
> >
> > The returned values are supposed to be a stream of mixed 16-bit and 32-bit
> > values, _insanely_ encoded as follows:
> >
> > The high order bit of each 2 byte field is a length specifier:
> > 1: The associativity domain number is contained in the low order 15 bits of the
> > field,
> > 0: The associativity domain number is contained in the low order 15 bits of the
> > current field concatenated with the 16 bits of the next sequential field)
> >
> > ie, usable values are either 31-bit, either 15-bit.
> >
> > I don't see such an encoding taking place in the code... this may cause
> > problems if node_id >= 0x80000000: the guest will only get bits 30:16,
> > and consider bits 15:0 to belong to the next field, which may in turn
> > be considered as a 15-bit value or combined with the top 16-bits of
> > procno to form a bogus 31-bit value... :-\
>
> > I guess it is okay to go with the "long" variant for everyone since you
> > only have 5 of them, but you could also use the "short" variant when
> > values are < 0x10000.
> >
> > And in any case, it seems we have to limit node ids and vcpu ids to be
> > 31 bits if we're to support this interface.
>
> From PowerPCCPU, node_id is an int32_t, vcpu_id is an "int", and node id
> and cpu id cannot be negative, so the value encoded are already 31bit
> values. Moreover, in NumaNodeOptions (see qapi/misc.json) nodeid and
> cpus are uint16.
>
> So I think we don't have to check the bit 31 of these values.
>
Indeed, it seems you're right.
> I don't want to manage the 15bit encoding because the list is hard coded
> and I want to keep it simple.
>
Fair enough.
> Thanks,
> Laurent
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-19 16:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18 13:18 [Qemu-devel] [PATCH v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY Laurent Vivier
2018-12-18 16:10 ` Greg Kurz
2018-12-19 16:25 ` Laurent Vivier
2018-12-19 16:50 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.