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

[Greg Kurz]

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 ?

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: <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.

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  
> >   
> 
>