On Fri, 10 Aug 2018 16:06:57 +0200
Igor Mammedov <imamm...@redhat.com> wrote:
> Commit 848a1cc1e8b04 while introducing SRAT entries for DIMM and NVDIMM
> also introduced fake entries for gaps between memory devices, assuming
> that we need all possible range covered with SRAT entries.
> And it did it wrong since gap would overlap with preceeding entry.
> Reproduced with following CLI:
>
> -m 1G,slots=4,maxmem=8 \
> -object memory-backend-ram,size=1G,id=m0 \
> -device pc-dimm,memdev=m0,addr=0x101000000 \
> -object memory-backend-ram,size=1G,id=m1 \
> -device pc-dimm,memdev=m1
>
> However recent development (10efd7e108) showed that gap entries might
> be not need. And indeed testing with WS2008DC-WS2016DC guests range
> shows that memory hotplug works just fine without gap entries.
>
> So rather than fixing gap entry borders, just drop them altogether
> and simplify code around it.
>
> Spotted-by: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> There is no need to update reference blobs since gaps beween dimms
> aren't generated by any exsting test case.
>
> Considering issue is not visible by default lets just merge it into 3.1
> and stable 3.0.1
Pls ignore it for now I'll need to do more extensive testing with old kernels,
we might need these holes for old kernels or even new ones.
/me goes to read kernel code
/per spec possible to hotplug range could be in SRAT,
even though I don't like it bu we might end up with static
partitioning of hotplug area between nodes like on bare metal
to avoid chasing after unknown requirements from windows /
> ---
> hw/i386/acpi-build.c | 62
> +++++++++++++++++++---------------------------------
> 1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e1ee8ae..d3d690f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2254,48 +2254,16 @@ build_tpm2(GArray *table_data, BIOSLinker *linker,
> GArray *tcpalog)
> static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t base,
> uint64_t len, int default_node)
> {
> - MemoryDeviceInfoList *info_list = qmp_memory_device_list();
> MemoryDeviceInfoList *info;
> - MemoryDeviceInfo *mi;
> - PCDIMMDeviceInfo *di;
> - uint64_t end = base + len, cur, size;
> - bool is_nvdimm;
> AcpiSratMemoryAffinity *numamem;
> - MemoryAffinityFlags flags;
> + MemoryDeviceInfoList *info_list = qmp_memory_device_list();;
>
> - for (cur = base, info = info_list;
> - cur < end;
> - cur += size, info = info->next) {
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> -
> - if (!info) {
> - /*
> - * Entry is required for Windows to enable memory hotplug in OS
> - * and for Linux to enable SWIOTLB when booted with less than
> - * 4G of RAM. Windows works better if the entry sets proximity
> - * to the highest NUMA node in the machine at the end of the
> - * reserved space.
> - * Memory devices may override proximity set by this entry,
> - * providing _PXM method if necessary.
> - */
> - build_srat_memory(numamem, end - 1, 1, default_node,
> - MEM_AFFINITY_HOTPLUGGABLE |
> MEM_AFFINITY_ENABLED);
> - break;
> - }
> -
> - mi = info->value;
> - is_nvdimm = (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM);
> - di = !is_nvdimm ? mi->u.dimm.data : mi->u.nvdimm.data;
> -
> - if (cur < di->addr) {
> - build_srat_memory(numamem, cur, di->addr - cur, default_node,
> - MEM_AFFINITY_HOTPLUGGABLE |
> MEM_AFFINITY_ENABLED);
> - numamem = acpi_data_push(table_data, sizeof *numamem);
> - }
> -
> - size = di->size;
> + for (info = info_list; info != NULL; info = info->next) {
> + MemoryAffinityFlags flags = MEM_AFFINITY_ENABLED;
> + MemoryDeviceInfo *mi = info->value;
> + bool is_nvdimm = (mi->type == MEMORY_DEVICE_INFO_KIND_NVDIMM);
> + PCDIMMDeviceInfo *di = !is_nvdimm ? mi->u.dimm.data :
> mi->u.nvdimm.data;
>
> - flags = MEM_AFFINITY_ENABLED;
> if (di->hotpluggable) {
> flags |= MEM_AFFINITY_HOTPLUGGABLE;
> }
> @@ -2303,10 +2271,24 @@ static void build_srat_hotpluggable_memory(GArray
> *table_data, uint64_t base,
> flags |= MEM_AFFINITY_NON_VOLATILE;
> }
>
> - build_srat_memory(numamem, di->addr, size, di->node, flags);
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, di->addr, di->size, di->node, flags);
> }
> -
> qapi_free_MemoryDeviceInfoList(info_list);
> +
> + /*
> + * Entry is required for Windows to enable memory hotplug in OS
> + * and for Linux to enable SWIOTLB when booted with less than
> + * 4G of RAM. Windows works better if the entry sets proximity
> + * to the highest NUMA node in the machine at the end of the
> + * reserved space.
> + * Memory devices may override proximity set by this entry,
> + * providing _PXM method if necessary.
> + */
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, base + len - 1, 1, default_node,
> + MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
> +
> }
>
> static void
