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

[Verma, Vishal L]

On Thu, 2020-05-28 at 13:19 +0200, Igor Mammedov wrote:

[..]
> > @@ -1334,6 +1335,31 @@ static void nvdimm_build_ssdt(GArray *table_offsets, 
> > GArray *table_data,
> >      free_aml_allocator();
> >  }
> >  
> > +void *nvdimm_build_srat(GArray *table_data)
> > +{
> > +    AcpiSratMemoryAffinity *numamem = NULL;
> > +    GSList *device_list = nvdimm_get_device_list();
> > +
> > +    for (; device_list; device_list = device_list->next) {
> > +        DeviceState *dev = device_list->data;
> I'd use Object here with OBJECT() cast and drop casts beolw in property 
> getters
> 
Done, that makes it much cleaner.

> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 2e15f6848e..1461d8a718 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2428,6 +2428,16 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >                                MEM_AFFINITY_ENABLED);
> >          }
> >      }
> > +
> > +    if (machine->nvdimms_state->is_enabled) {
> > +        void *ret;
> > +
> > +        ret = nvdimm_build_srat(table_data);
> > +        if (ret != NULL) {
> > +            numamem = ret;
> > +        }
> 
> why do we need return value here and a test condition and assign 'ret' to 
> numamem?

Ah I thought numamem was propagated through the different parts of the
build_srat flow, but I misread. You're right it is not needed, removing.