hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr.h | 1 + 2 files changed, 40 insertions(+)
H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
designation associated with the identifier input parameter.
Remove the warning message from the kernel:
VPHN is not supported. Disabling polling..
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
"[PULL 00/27] ppc-for-4.0 queue 20181213"
hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr.h | 1 +
2 files changed, 40 insertions(+)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 78fecc8fe9..454ec594fd 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) == 0) {
+ return H_PARAMETER;
+ }
+
+ 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
On Mon, Dec 17, 2018 at 03:00:55PM +0100, Laurent Vivier wrote:
> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
> designation associated with the identifier input parameter.
>
> Remove the warning message from the kernel:
> VPHN is not supported. Disabling polling..
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
From the looks of PAPR, I suspect this call isn't of much use outside
PowerVM guests, though it probably wouldn't do any harm.
BenH, Paulus, any thoughts?
One nit in implementation: if you implement this hcall, it's supposed
to be advertised by adding hcall-vphn to ibm,hypertas-functions.
> ---
> Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
> "[PULL 00/27] ppc-for-4.0 queue 20181213"
>
> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr.h | 1 +
> 2 files changed, 40 insertions(+)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 78fecc8fe9..454ec594fd 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) == 0) {
> + return H_PARAMETER;
> + }
> +
> + 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
--
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
On 18/12/2018 05:29, David Gibson wrote:
> On Mon, Dec 17, 2018 at 03:00:55PM +0100, Laurent Vivier wrote:
>> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
>> designation associated with the identifier input parameter.
>>
>> Remove the warning message from the kernel:
>> VPHN is not supported. Disabling polling..
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>
> From the looks of PAPR, I suspect this call isn't of much use outside
> PowerVM guests, though it probably wouldn't do any harm.
This call is used by the kernel to get the node id of a CPU on hotplug
and fixes a crash when we hotplug a CPU in a memory-less/CPU-less node
where this information is missing (not initialized from the device-tree).
> BenH, Paulus, any thoughts?
>
> One nit in implementation: if you implement this hcall, it's supposed
> to be advertised by adding hcall-vphn to ibm,hypertas-functions.
ok in v2.
Thanks,
Laurent
>> ---
>> Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
>> "[PULL 00/27] ppc-for-4.0 queue 20181213"
>>
>> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr.h | 1 +
>> 2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 78fecc8fe9..454ec594fd 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) == 0) {
>> + return H_PARAMETER;
>> + }
>> +
>> + 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
>
On Tue, 18 Dec 2018 08:50:00 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> On 18/12/2018 05:29, David Gibson wrote:
> > On Mon, Dec 17, 2018 at 03:00:55PM +0100, Laurent Vivier wrote:
> >> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
> >> designation associated with the identifier input parameter.
> >>
> >> Remove the warning message from the kernel:
> >> VPHN is not supported. Disabling polling..
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >
> > From the looks of PAPR, I suspect this call isn't of much use outside
> > PowerVM guests, though it probably wouldn't do any harm.
>
> This call is used by the kernel to get the node id of a CPU on hotplug
> and fixes a crash when we hotplug a CPU in a memory-less/CPU-less node
> where this information is missing (not initialized from the device-tree).
>
So this patch isn't just about removing the warning message from the kernel
but about fixing an actual crash ?
I ask because if it's only about the warning, why does the kernel call
H_HOME_NODE_ASSOCIATIVITY when hcall-vphn isn't advertised ? Especially,
the polling for topology changes is only started if hcall-vphn is present:
if (firmware_has_feature(FW_FEATURE_VPHN) &&
lppaca_shared_proc(get_lppaca())) {
if (!vphn_enabled) {
vphn_enabled = 1;
setup_cpu_associativity_change_counters();
timer_setup(&topology_timer, topology_timer_fn,
TIMER_DEFERRABLE);
reset_topology_timer();
}
}
It thus seems wrong to emit the "Disable polling.." warning for something
that was never enabled in the first place, doesn't it ?
On the other hand, if this really needed to avoid a crash, I guess you
should provide some more details.
> > BenH, Paulus, any thoughts?
> >
> > One nit in implementation: if you implement this hcall, it's supposed
> > to be advertised by adding hcall-vphn to ibm,hypertas-functions.
> ok in v2.
>
> Thanks,
> Laurent
>
> >> ---
> >> Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
> >> "[PULL 00/27] ppc-for-4.0 queue 20181213"
> >>
> >> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
> >> include/hw/ppc/spapr.h | 1 +
> >> 2 files changed, 40 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 78fecc8fe9..454ec594fd 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) == 0) {
> >> + return H_PARAMETER;
> >> + }
LoPAPR says that the guest can pass exactly 0x1 or 0x2 in flags. The
above check should then rather be flags == 0x1.
Also, even if linux only seems to call this with 0x1, this is a
limitation from a LoPAPR standpoint. Not sure H_PARAMETER is the
appropriate return value if flags is 0x2 since the guest did
nothing wrong... I'd rather return H_FUNCTION in this case.
> >> +
> >> + 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
> >
>
>
On 18/12/2018 10:23, Greg Kurz wrote:
> On Tue, 18 Dec 2018 08:50:00 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
>
>> On 18/12/2018 05:29, David Gibson wrote:
>>> On Mon, Dec 17, 2018 at 03:00:55PM +0100, Laurent Vivier wrote:
>>>> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
>>>> designation associated with the identifier input parameter.
>>>>
>>>> Remove the warning message from the kernel:
>>>> VPHN is not supported. Disabling polling..
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>
>>> From the looks of PAPR, I suspect this call isn't of much use outside
>>> PowerVM guests, though it probably wouldn't do any harm.
>>
>> This call is used by the kernel to get the node id of a CPU on hotplug
>> and fixes a crash when we hotplug a CPU in a memory-less/CPU-less node
>> where this information is missing (not initialized from the device-tree).
>>
>
> So this patch isn't just about removing the warning message from the kernel
> but about fixing an actual crash ?
Yes, I updated the message but sent the wrong e-mail.
> I ask because if it's only about the warning, why does the kernel call
> H_HOME_NODE_ASSOCIATIVITY when hcall-vphn isn't advertised ? Especially,
> the polling for topology changes is only started if hcall-vphn is present:
>
> if (firmware_has_feature(FW_FEATURE_VPHN) &&
> lppaca_shared_proc(get_lppaca())) {
> if (!vphn_enabled) {
> vphn_enabled = 1;
> setup_cpu_associativity_change_counters();
> timer_setup(&topology_timer, topology_timer_fn,
> TIMER_DEFERRABLE);
> reset_topology_timer();
> }
> }
>
> It thus seems wrong to emit the "Disable polling.." warning for something
> that was never enabled in the first place, doesn't it ?
It's unconditionally called from find_and_online_cpu_nid() that is used
to plug a CPU in a node that is not already online.
> On the other hand, if this really needed to avoid a crash, I guess you
> should provide some more details.
I agree.
>>> BenH, Paulus, any thoughts?
>>>
>>> One nit in implementation: if you implement this hcall, it's supposed
>>> to be advertised by adding hcall-vphn to ibm,hypertas-functions.
>> ok in v2.
>>
>> Thanks,
>> Laurent
>>
>>>> ---
>>>> Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
>>>> "[PULL 00/27] ppc-for-4.0 queue 20181213"
>>>>
>>>> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>> include/hw/ppc/spapr.h | 1 +
>>>> 2 files changed, 40 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>>> index 78fecc8fe9..454ec594fd 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) == 0) {
>>>> + return H_PARAMETER;
>>>> + }
>
> LoPAPR says that the guest can pass exactly 0x1 or 0x2 in flags. The
> above check should then rather be flags == 0x1.
>
ok
> Also, even if linux only seems to call this with 0x1, this is a
> limitation from a LoPAPR standpoint. Not sure H_PARAMETER is the
> appropriate return value if flags is 0x2 since the guest did
> nothing wrong... I'd rather return H_FUNCTION in this case.
The doc says:
H_Function: The function is not supported
H_Parameter: Unsupported flag parameter value
in that case function is supported but not the flag, so I think
H_PARAMETER is a better choice.
Thanks,
Laurent
On Tue, 18 Dec 2018 11:00:01 +0100
Laurent Vivier <lvivier@redhat.com> wrote:
> On 18/12/2018 10:23, Greg Kurz wrote:
> > On Tue, 18 Dec 2018 08:50:00 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >
> >> On 18/12/2018 05:29, David Gibson wrote:
> >>> On Mon, Dec 17, 2018 at 03:00:55PM +0100, Laurent Vivier wrote:
> >>>> H_HOME_NODE_ASSOCIATIVITY H-Call returns the associativity domain
> >>>> designation associated with the identifier input parameter.
> >>>>
> >>>> Remove the warning message from the kernel:
> >>>> VPHN is not supported. Disabling polling..
> >>>>
> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>
> >>> From the looks of PAPR, I suspect this call isn't of much use outside
> >>> PowerVM guests, though it probably wouldn't do any harm.
> >>
> >> This call is used by the kernel to get the node id of a CPU on hotplug
> >> and fixes a crash when we hotplug a CPU in a memory-less/CPU-less node
> >> where this information is missing (not initialized from the device-tree).
> >>
> >
> > So this patch isn't just about removing the warning message from the kernel
> > but about fixing an actual crash ?
>
> Yes, I updated the message but sent the wrong e-mail.
>
> > I ask because if it's only about the warning, why does the kernel call
> > H_HOME_NODE_ASSOCIATIVITY when hcall-vphn isn't advertised ? Especially,
> > the polling for topology changes is only started if hcall-vphn is present:
> >
> > if (firmware_has_feature(FW_FEATURE_VPHN) &&
> > lppaca_shared_proc(get_lppaca())) {
> > if (!vphn_enabled) {
> > vphn_enabled = 1;
> > setup_cpu_associativity_change_counters();
> > timer_setup(&topology_timer, topology_timer_fn,
> > TIMER_DEFERRABLE);
> > reset_topology_timer();
> > }
> > }
> >
> > It thus seems wrong to emit the "Disable polling.." warning for something
> > that was never enabled in the first place, doesn't it ?
>
> It's unconditionally called from find_and_online_cpu_nid() that is used
> to plug a CPU in a node that is not already online.
>
> > On the other hand, if this really needed to avoid a crash, I guess you
> > should provide some more details.
>
> I agree.
>
> >>> BenH, Paulus, any thoughts?
> >>>
> >>> One nit in implementation: if you implement this hcall, it's supposed
> >>> to be advertised by adding hcall-vphn to ibm,hypertas-functions.
> >> ok in v2.
> >>
> >> Thanks,
> >> Laurent
> >>
> >>>> ---
> >>>> Based-on: <20181213040126.6768-1-david@gibson.dropbear.id.au>
> >>>> "[PULL 00/27] ppc-for-4.0 queue 20181213"
> >>>>
> >>>> hw/ppc/spapr_hcall.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>> include/hw/ppc/spapr.h | 1 +
> >>>> 2 files changed, 40 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >>>> index 78fecc8fe9..454ec594fd 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) == 0) {
> >>>> + return H_PARAMETER;
> >>>> + }
> >
> > LoPAPR says that the guest can pass exactly 0x1 or 0x2 in flags. The
> > above check should then rather be flags == 0x1.
> >
>
> ok
>
> > Also, even if linux only seems to call this with 0x1, this is a
> > limitation from a LoPAPR standpoint. Not sure H_PARAMETER is the
> > appropriate return value if flags is 0x2 since the guest did
> > nothing wrong... I'd rather return H_FUNCTION in this case.
>
> The doc says:
>
> H_Function: The function is not supported
> H_Parameter: Unsupported flag parameter value
>
> in that case function is supported but not the flag, so I think
> H_PARAMETER is a better choice.
>
Well... neither LoPAPR, nor IBM confidential PAPR+ do say anything
about partial support for this hcall. If the guest was to use the
flags == 0x2 variant, eg, some closed-source OS supporting PAPR,
it could be legitimately confused to get an H_PARAMETER error when
passing supposedly valid parameters... how to cope with that ?
On the other hand, if QEMU cannot honor the flags == 0x2 variant
and returns H_FUNCTION then the OS can recover since it is
required by the specification.
But I don't really care for now, and we can talk about this later
if I'm assigned a BZ from the people who run such OS in KVM guests ;)
> Thanks,
> Laurent
>
On 18/12/2018 11:50, Greg Kurz wrote: > On Tue, 18 Dec 2018 11:00:01 +0100 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 18/12/2018 10:23, Greg Kurz wrote: >>> On Tue, 18 Dec 2018 08:50:00 +0100 ... >>> Also, even if linux only seems to call this with 0x1, this is a >>> limitation from a LoPAPR standpoint. Not sure H_PARAMETER is the >>> appropriate return value if flags is 0x2 since the guest did >>> nothing wrong... I'd rather return H_FUNCTION in this case. >> >> The doc says: >> >> H_Function: The function is not supported >> H_Parameter: Unsupported flag parameter value >> >> in that case function is supported but not the flag, so I think >> H_PARAMETER is a better choice. >> > > Well... neither LoPAPR, nor IBM confidential PAPR+ do say anything > about partial support for this hcall. If the guest was to use the > flags == 0x2 variant, eg, some closed-source OS supporting PAPR, > it could be legitimately confused to get an H_PARAMETER error when > passing supposedly valid parameters... how to cope with that ? > On the other hand, if QEMU cannot honor the flags == 0x2 variant > and returns H_FUNCTION then the OS can recover since it is > required by the specification. You have certainly access to more information than I have, so I'll change H_PARAMETER to H_FUNCTION. Thanks, Laurent
© 2016 - 2024 Red Hat, Inc.