Re: [Qemu-ppc] [PATCH] spapr: Add H-Call H_HOME_NODE_ASSOCIATIVITY

[Laurent Vivier]

On 18/12/2018 10:23, Greg Kurz wrote:
> On Tue, 18 Dec 2018 08:50:00 +0100
> Laurent Vivier <address@hidden> 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 <address@hidden>  
>>>
>>> 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: <address@hidden>
>>>>           "[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