Re: [Qemu-devel] [PATCH v2 30/31] pc: ACPI BIOS: reserve SRAT entry for hotplug mem hole

[Igor Mammedov]

On Wed, 21 May 2014 18:01:45 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, May 21, 2014 at 03:56:55PM +0200, Igor Mammedov wrote:
> > On Wed, 21 May 2014 15:44:07 +0300
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Wed, May 21, 2014 at 01:22:23PM +0200, Igor Mammedov wrote:
> > > > On Wed, 21 May 2014 11:05:58 +0300
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > 
> > > > > On Tue, May 20, 2014 at 05:15:33PM +0200, Igor Mammedov wrote:
> > > > > > Needed for Windows to use hotplugged memory device, otherwise
> > > > > > it complains that server is not configured for memory hotplug.
> > > > > > Tests shows that aftewards it uses dynamically provided
> > > > > > proximity value from _PXM() method if available.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > > ---
> > > > > >  hw/i386/acpi-build.c |   14 ++++++++++++++
> > > > > >  1 files changed, 14 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index 58e7306..97e3a82 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -1199,6 +1199,8 @@ build_srat(GArray *table_data, GArray *linker,
> > > > > >      uint64_t curnode;
> > > > > >      int srat_start, numa_start, slots;
> > > > > >      uint64_t mem_len, mem_base, next_base;
> > > > > > +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> > > > > > +    ram_addr_t hotplug_as_size = 
> > > > > > memory_region_size(&pcms->hotplug_memory);
> > > > > >  
> > > > > >      srat_start = table_data->len;
> > > > > >
> > > > > 
> > > > > Please don't abbreviate address space as "as". If you abbreviate as as
> > > > > as it can be misunderstood for the English as which stands for as.
> > > > > You see how confusing this can be :)
> > > > > How about hotplug_memory_max_size?
> > > > Ok, but ^^ is a bit ambiguous, hot about:
> > > > hotplugabble_address_space_size
> > > 
> > > Fine with me.
> > > 
> > > > > 
> > > > > Also, it would be a bit more elegant to make this a read-only property
> > > > > of the machine.
> > > > I'm not sure about exposing it as property since
> > > >   - it's internal to implementation and mgmt doesn't need to know about 
> > > > it at all
> > > >     so hiding it from QOM view might be a good idea
> > > >   - field is shared by PC machines and used only in PC code which runs 
> > > > only
> > > >     for a specific instance
> > > > So using property here loos like over-engineering for now.
> > > > 
> > > > However,
> > > > I don't feel strong about it and if you insist I'll make it a property.
> > > > 
> > > 
> > > If it's a property ACPI unit tests can poke at it.
> > > I do not feel strongly about it, it's just an idea.
> > np, I'll add it.
> > 
> > > And BTW, it would be nice to have some unit tests
> > > for the new functionality.
> > That's on my TODO list but after hot-add feature is complete.
> > Feature works but to make it more usable I need first to take
> > care about:
> >  - mgmt interface
> >  - e820/smbios integration
> 
> OK so you want to defer merging these bits until this integration is
> done? Maybe at least infrastructure patches (currently 1-28) can go in
> meanwhile?
nope,
it would be better to merge all patches in this series first and
then on top of it, I'll do HMP/QAPI + e802/smbios patches + tests

> 
> > > 
> > > > > > @@ -1263,6 +1265,18 @@ build_srat(GArray *table_data, GArray 
> > > > > > *linker,
> > > > > >          acpi_build_srat_memory(numamem, 0, 0, 0, 
> > > > > > MEM_AFFINITY_NOFLAGS);
> > > > > >      }
> > > > > >  
> > > > > > +    /*
> > > > > > +     * Fake entry required by Windows to enable memory hotplug in 
> > > > > > OS.
> > > > > > +     * Individual DIMM devices override proximity set here via 
> > > > > > _PXM method,
> > > > > > +     * which returns associated with it NUMA node id.
> > > > > > +     */
> > > > > > +    if (hotplug_as_size) {
> > > > > > +        numamem = acpi_data_push(table_data, sizeof *numamem);
> > > > > > +        acpi_build_srat_memory(numamem, pcms->hotplug_memory_base,
> > > > > > +                               hotplug_as_size, 0, 
> > > > > > MEM_AFFINITY_HOTPLUGGABLE |
> > > > > > +                               MEM_AFFINITY_ENABLED);
> > > > > > +    }
> > > > > > +
> > > > > >      build_header(linker, table_data,
> > > > > >                   (void *)(table_data->data + srat_start),
> > > > > >                   "SRAT",
> > > > > > -- 
> > > > > > 1.7.1
> > > > > 
> > > 
>