Re: [Qemu-devel] [PATCH v8 02/11] numa: move numa global variable nb_numa_nodes into MachineState

[Tao Xu]

On 7/29/2019 9:09 PM, Igor Mammedov wrote:
> On Mon, 29 Jul 2019 14:31:18 +0800
> Tao Xu <address@hidden> wrote:
>
> > Add struct NumaState in MachineState and move existing numa global
> > nb_numa_nodes(renamed as "num_nodes") into NumaState. And add variable
> > numa_support into MachineClass to decide which submachines support NUMA.
> >
> > Suggested-by: Igor Mammedov <address@hidden>
> > Suggested-by: Eduardo Habkost <address@hidden>
> > Signed-off-by: Tao Xu <address@hidden>
> > ---
> >
> > Changes in v8:
> >      - Add check if numa->numa_state is NULL in pxb_dev_realize_common
> >      - Use nb_nodes in spapr_populate_memory() (Igor)
> > ---
> [...]
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 821f0d4a49..1c7c12c415 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -331,7 +331,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState 
> > *spapr)
> >               return ret;
> >           }
> >   
> > -        if (nb_numa_nodes > 1) {
> > +        if (ms->numa_state->num_nodes > 1) {
> >               ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu);
> >               if (ret < 0) {
> >                   return ret;
> > @@ -351,9 +351,9 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState 
> > *spapr)
> >   
> >   static hwaddr spapr_node0_size(MachineState *machine)
> >   {
> > -    if (nb_numa_nodes) {
> > +    if (machine->numa_state->num_nodes) {
> >           int i;
> > -        for (i = 0; i < nb_numa_nodes; ++i) {
> > +        for (i = 0; i < machine->numa_state->num_nodes; ++i) {
> >               if (numa_info[i].node_mem) {
> >                   return MIN(pow2floor(numa_info[i].node_mem),
> >                              machine->ram_size);
> > @@ -398,13 +398,14 @@ static int spapr_populate_memory(SpaprMachineState 
> > *spapr, void *fdt)
> >   {
> >       MachineState *machine = MACHINE(spapr);
> >       hwaddr mem_start, node_size;
> > -    int i, nb_nodes = nb_numa_nodes;
> > +    int i, nb_nodes = machine->numa_state->num_nodes;
> >       NodeInfo *nodes = numa_info;
> >       NodeInfo ramnode;
> >   
> >       /* No NUMA nodes, assume there is just one node with whole RAM */
> > -    if (!nb_numa_nodes) {
> > +    if (!nb_nodes) {
> >           nb_nodes = 1;
> > +        machine->numa_state->num_nodes = nb_nodes;
> You've not addressed a v7 comment
> On Tue, 23 Jul 2019 16:56:41 +0200
> Igor Mammedov <address@hidden> wrote:
>
> > I don't like user fixing up generic machine data that came from CLI
> > (or luck of such)
> [...]
> > I'd keep fixup local (i.e. using nb_nodes)
>
> i.e. do not modify machine->numa_state->num_nodes and just use value local
> like current code does.

But modify machine->numa_state->num_nodes can fix the issue:
spapr_populate_memory() creates a implicit node and info
temporarily but then spapr_validate_node_memory() will use
global nb_numa_nodes which is 0 and skip check.

Or if I should modify the check part, i.e.
static void spapr_validate_node_memory(MachineState *machine, Error **errp)
{
[...]
    for (i = 0; machine->numa_state->nodes[i] == NULL; i++) {
        if (machine->numa_state->nodes[i].node_mem % 
SPAPR_MEMORY_BLOCK_SIZE) {
            error_setg(errp,
                       "Node %d memory size 0x%" PRIx64
                       " is not aligned to %" PRIu64 " MiB",
                       i, machine->numa_state->nodes[i].node_mem,
                       SPAPR_MEMORY_BLOCK_SIZE / MiB);)

> >           ramnode.node_mem = machine->ram_size;
> >           nodes = &ramnode;
> >       }
> [...]