[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] hw/acpi-build: account for NVDIMM numa nodes in SRAT
From: |
Igor Mammedov |
Subject: |
Re: [PATCH 2/3] hw/acpi-build: account for NVDIMM numa nodes in SRAT |
Date: |
Thu, 28 May 2020 12:41:31 +0200 |
On Thu, 28 May 2020 01:24:42 +0000
"Verma, Vishal L" <vishal.l.verma@intel.com> wrote:
> 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.
perhaps you are lookin into old version on QEMU
current HEAD has followin snippet:
virt-acpi-build.c:
if (ms->nvdimms_state->is_enabled) {
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
ms->nvdimms_state, ms->ram_slots);
}
>
> 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);