Re: [Qemu-devel] [PATCH] x86/acpi: build SRAT when memory hotplug is enabled

[Eduardo Habkost]

On Mon, Sep 04, 2017 at 09:38:47AM +0800, Dou Liyang wrote:
> Hi Eduardo, Thadeu,
> 
> At 09/02/2017 12:11 AM, Eduardo Habkost wrote:
> > On Fri, Sep 01, 2017 at 12:45:42PM -0300, Thadeu Lima de Souza Cascardo 
> > wrote:
> > > Linux uses SRAT to determine the maximum memory in a system, which is
> > > used to determine whether to use the swiotlb for IOMMU or not for a
> > > device that supports only 32 bits of addresses.
> > 
> > Do you have a pointer to the corresponding Linux code, for
> > reference?  Which SRAT entries Linux uses to make this decision?
> > 
> > > 
> > > When there is no NUMA configuration, qemu will not build SRAT. And when
> > > memory hotplug is done, some Linux device drivers start failing.
> > > 
> > > Tested by running with -m 512M,slots=8,maxmem=1G, adding the memory,
> > > putting that online and using the system. Without the patch, swiotlb is
> > > not used and ATA driver fails. With the patch, swiotlb is used, no
> > > driver failure is observed.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <address@hidden>
> > 
> > As far as I can see, this will only add APIC entries and a memory
> > affinity entry for the first 640KB (which would be obviously
> > wrong) if pcms->numa_nodes is 0.
> > 
> 
> In my opinion, this may also add the hotpluggable memory, and see the
> following commemts.
> 
>     /*
>      * 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) {
>         numamem = acpi_data_push(table_data, sizeof *numamem);
>         build_srat_memory(numamem, pcms->hotplug_memory.base,
>                           hotplugabble_address_space_size, pcms->numa_nodes
> - 1,
>                           MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
>     }

You are correct, I didn't see that part of the code.  If that's
the entry that's missing, the patch makes sense.  Thanks!

However, the resulting tables still don't look correct: it will
generate an entry assigned to NUMA node (uint32_t)-1 if no NUMA
nodes are configured elsewhere, some APIC entries, but no entries
for the rest of the memory.

Igor's suggestion to enable NUMA implicitly sounds safer to me.

> 
> 
> Thanks,
>       dou.
> 
> > Once we apply the "Fix SRAT memory building in case of node 0
> > without RAM" patch from Dou Liyang, no memory affinity entries
> > will be generated if pcms->numa_nodes is 0.  Would this cause the
> > problem to happen again?
> > 
> 
> 
> 
> > 
> > > ---
> > >  hw/i386/acpi-build.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 98dd424678..fb94249779 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -2645,6 +2645,9 @@ void acpi_build(AcpiBuildTables *tables, 
> > > MachineState *machine)
> > >      GArray *tables_blob = tables->table_data;
> > >      AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> > >      Object *vmgenid_dev;
> > > +    ram_addr_t hotplugabble_address_space_size =
> > > +        object_property_get_int(OBJECT(pcms), 
> > > PC_MACHINE_MEMHP_REGION_SIZE,
> > > +                                NULL);
> > > 
> > >      acpi_get_pm_info(&pm);
> > >      acpi_get_misc_info(&misc);
> > > @@ -2708,7 +2711,7 @@ void acpi_build(AcpiBuildTables *tables, 
> > > MachineState *machine)
> > >              build_tpm2(tables_blob, tables->linker);
> > >          }
> > >      }
> > > -    if (pcms->numa_nodes) {
> > > +    if (pcms->numa_nodes || hotplugabble_address_space_size) {
> > >          acpi_add_table(table_offsets, tables_blob);
> > >          build_srat(tables_blob, tables->linker, machine);
> > >          if (have_numa_distance) {
> > > --
> > > 2.11.0
> > > 
> > 
> 
> 

-- 
Eduardo