Re: [Qemu-devel] [PATCH for-3.0 v2] pc: acpi: fix memory hotplug regression by reducing stub SRAT entry size

[Eduardo Habkost]

On Mon, Jul 30, 2018 at 11:41:41AM +0200, Igor Mammedov wrote:
> Commit 848a1cc1e (hw/acpi-build: build SRAT memory affinity structures for 
> DIMM devices)
> broke the first dimm hotplug in following cases:
> 
>  1: there is no coldplugged dimm in the last numa node
>     but there is a coldplugged dimm in another node
> 
>   -m 4096,slots=4,maxmem=32G               \
>   -object memory-backend-ram,id=m0,size=2G \
>   -device pc-dimm,memdev=m0,node=0         \
>   -numa node,nodeid=0                      \
>   -numa node,nodeid=1
> 
>  2: if order of dimms on CLI is:
>        1st plugged dimm in node1
>        2nd plugged dimm in node0
> 
>   -m 4096,slots=4,maxmem=32G               \
>   -object memory-backend-ram,size=2G,id=m0 \
>   -device pc-dimm,memdev=m0,node=1         \
>   -object memory-backend-ram,id=m1,size=2G \
>   -device pc-dimm,memdev=m1,node=0         \
>   -numa node,nodeid=0                      \
>   -numa node,nodeid=1
> 
> (qemu) object_add memory-backend-ram,id=m2,size=1G
> (qemu) device_add pc-dimm,memdev=m2,node=0
> 
> the first DIMM hotplug to any node except the last one
> fails (Windows is unable to online it).
> 
> Length reduction of stub hotplug memory SRAT entry,
> fixes issue for some reason.
> 

I'm really bothered by the lack of automated testing for all
these NUMA/ACPI corner cases.

This looks like a good candidate for an avocado_qemu test case.
Can you show pseudo-code of how exactly the bug fix could be
verified, so we can try to write a test case?


> RHBZ: 1609234

I suggest including full URL of bug report[1].  I doubt anybody
outside Red Hat knows what "RHBZ" means.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1609234

> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> NOTE TO MAINTAINER: UPDATE REFERENCE APCI TABLE BLOBS
> 
> v2:
>  fixup examples in commit message
>  (they were in wrong order and with duplicate memdev=m1)
> ---
>  hw/i386/acpi-build.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9e8350c..b52fdb2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2269,7 +2269,16 @@ static void build_srat_hotpluggable_memory(GArray 
> *table_data, uint64_t base,
>          numamem = acpi_data_push(table_data, sizeof *numamem);
>  
>          if (!info) {
> -            build_srat_memory(numamem, cur, end - cur, default_node,
> +            /*
> +             * 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;
>          }
> @@ -2402,14 +2411,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> MachineState *machine)
>          build_srat_memory(numamem, 0, 0, 0, MEM_AFFINITY_NOFLAGS);
>      }
>  
> -    /*
> -     * 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.
> -     * Memory devices may override proximity set by this entry,
> -     * providing _PXM method if necessary.
> -     */
>      if (hotplugabble_address_space_size) {
>          build_srat_hotpluggable_memory(table_data, 
> machine->device_memory->base,
>                                         hotplugabble_address_space_size,
> -- 
> 2.7.4
> 

-- 
Eduardo