Message ID | 20181218131829.5135-1-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY | expand |
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
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
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
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
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(+)