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

[Greg Kurz]

On Wed, 19 Dec 2018 17:25:04 +0100
Laurent Vivier <address@hidden> wrote:

> On 18/12/2018 17:10, Greg Kurz wrote:
> > On Tue, 18 Dec 2018 14:18:29 +0100
> > Laurent Vivier <address@hidden> 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 <address@hidden>
> >> ---
> >>
> >> 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