Re: [PATCH 2/3] hw/acpi-build: account for NVDIMM numa nodes in SRAT

[Verma, Vishal L]

On Thu, 2020-05-21 at 17:16 +0200, Igor Mammedov wrote:

Hi Igor, Thanks for the review.

[..]
> >  
> > @@ -2429,6 +2430,25 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >                                MEM_AFFINITY_ENABLED);
> >          }
> >      }
> > +
> > +    if (machine->nvdimms_state->is_enabled) {
> > +        GSList *device_list = nvdimm_get_device_list();
> > +
> > +        for (; device_list; device_list = device_list->next) {
> > +            DeviceState *dev = device_list->data;
> > +            int node = object_property_get_int(OBJECT(dev), 
> > PC_DIMM_NODE_PROP,
> > +                                               NULL);
> > +            uint64_t addr = object_property_get_uint(OBJECT(dev),
> > +                                                     PC_DIMM_ADDR_PROP, 
> > NULL);
> > +            uint64_t size = object_property_get_uint(OBJECT(dev),
> > +                                                     PC_DIMM_SIZE_PROP, 
> > NULL);
> > +
> suggest to use error_abort in getters

Yep, fixed in v2.

> 
> > +            numamem = acpi_data_push(table_data, sizeof *numamem);
> > +            build_srat_memory(numamem, addr, size, node,
> > +                              MEM_AFFINITY_ENABLED | 
> > MEM_AFFINITY_NON_VOLATILE);
> > +        }
> who is in charge of freeing device_list ?

Thanks, also fixed in v2.

> 
> > +    }
> 
> There is ARM version of build_srat(),
> I suggest to put this NVDIMM specific part in helper function within 
> hw/acpi/nvdimm.c
> and use it from both build_srat() functions.

Splitting the work out into a helper function in nvdimm.c does make
sense, and I've done that. However, looking at the arm version of
build_srat and generally in virt-acpi-build.c, I don't see any NVDIMM
support, so unless I'm mistaken, it wouldn't make sense to actually call
this from the arm version of build_srat.

I'll send a v2 with the above fixes.

> 
> > +
> >      slots = (table_data->len - numa_start) / sizeof *numamem;
> >      for (; slots < pcms->numa_nodes + 2; slots++) {
> >          numamem = acpi_data_push(table_data, sizeof *numamem);