Re: [Qemu-devel] [PATCH 25/74] pc: acpi: memhp: prepare context in SSDT for moving memhp DSDT code

[Marcel Apfelbaum]

On 12/16/2015 04:25 PM, Igor Mammedov wrote:
> On Wed, 16 Dec 2015 15:25:46 +0200
> Marcel Apfelbaum <address@hidden> wrote:
>
> > On 12/10/2015 01:41 AM, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >    hw/acpi/Makefile.objs               |  2 +-
> > >    hw/acpi/memory_hotplug_acpi_table.c | 40
> > > +++++++++++++++++++++++++++++++++++++
> > > hw/i386/acpi-build.c                |  3 +++
> > > include/hw/acpi/memory_hotplug.h    |  4 ++++ 4 files changed, 48
> > > insertions(+), 1 deletion(-) create mode 100644
> > > hw/acpi/memory_hotplug_acpi_table.c
> > >
> > > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > > index 7d3230c..c04064e 100644
> > > --- a/hw/acpi/Makefile.objs
> > > +++ b/hw/acpi/Makefile.objs
> > > @@ -1,7 +1,7 @@
> > >    common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o pcihp.o
> > >    common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
> > >    common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> > > -common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> > > +common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> > > memory_hotplug_acpi_table.o common-obj-$(CONFIG_ACPI) +=
> > > acpi_interface.o common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> > >    common-obj-$(CONFIG_ACPI) += aml-build.o
> > > diff --git a/hw/acpi/memory_hotplug_acpi_table.c
> > > b/hw/acpi/memory_hotplug_acpi_table.c new file mode 100644
> > > index 0000000..25bbf5e
> > > --- /dev/null
> > > +++ b/hw/acpi/memory_hotplug_acpi_table.c
> > > @@ -0,0 +1,40 @@
> > > +/*
> > > + * Memory hotplug AML code of DSDT ACPI table
> >
> > You are advertising as part of the DSDT code, but you are putting in
> > SSDT, right? By the way, maybe the answer is clear, but why are you
> > moving it to SSDT?
> There could be only one instance of DSDT and I temporarily put
> converted DSDT bits in SSDT until conversion is complete.

I understand there can only be one DSDT table, but can't we have a "hybrid"
DSDT made from both asl files and C code?

> > Last thing, from this patch forward (until maybe the last one) make
> > check fails, right? (Specifically the acpi tests)
> yes, starting from this patch and upto 72nd acpi test fails since
> patches move DSDT bits into SSDT.

This can be a problem.
I am thinking of 2 solutions:
1. Disable the iasl tests on patch 1 and re-enable it on patch 73.
2. Go for a "hybrid" DSDT.

If (2) is too complicated, the implication is that all this series
must be taken atomically. Of course, (1) or some other idea is needed.

What do you think?

Thanks,
Marcel

> But patch 73, when precompiled DSDT is empty and provides only header,
> moves converted DSDT content out of SSDT into QEMU
> generated DSDT, and drops ASL template.
>
> > > + *
> > > + * Copyright (C) 2013 Red Hat Inc
> > > + *
> > > + * Author: Igor Mammedov <address@hidden>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > modify
> > > + * it under the terms of the GNU General Public License as
> > > published by
> > > + * the Free Software Foundation; either version 2 of the License,
> > > or
> > > + * (at your option) any later version.
> > > +
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > +
> > > + * You should have received a copy of the GNU General Public
> > > License along
> > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > + */
> >
> > Regarding the license, I was advised to just add a pointer to the
> > main license instead of copying it in each file, something like:
> >
> >      This work is licensed under the terms of the GNU GPL, version 2
> > or later. See the COPYING file in the top-level directory.
> >
> > Thanks,
> > Marcel
> >
> > > +
> > > +#include <stdbool.h>
> > > +#include "hw/acpi/memory_hotplug.h"
> > > +#include "include/hw/acpi/pc-hotplug.h"
> > > +#include "hw/boards.h"
> > > +
> > > +void build_memory_hotplug_aml(Aml *ctx, uint32_t nr_mem,
> > > +                              uint16_t io_base, uint16_t io_len)
> > > +{
> > > +    Aml *pci_scope;
> > > +    Aml *ctrl_dev;
> > > +
> > > +    /* scope for memory hotplug controller device node */
> > > +    pci_scope = aml_scope("_SB.PCI0");
> > > +    ctrl_dev = aml_scope(stringify(MEMORY_HOTPLUG_DEVICE));
> > > +    {
> > > +    }
> > > +    aml_append(pci_scope, ctrl_dev);
> > > +    aml_append(ctx, pci_scope);
> > > +}
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index bbd37e9..1b609e6 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -928,6 +928,9 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >        /* Reserve space for header */
> > >        acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > >
> > > +    build_memory_hotplug_aml(ssdt, nr_mem, pm->mem_hp_io_base,
> > > +                             pm->mem_hp_io_len);
> > > +
> > >        /* Extra PCI root buses are implemented  only for i440fx */
> > >        bus = find_i440fx();
> > >        if (bus) {
> > > diff --git a/include/hw/acpi/memory_hotplug.h
> > > b/include/hw/acpi/memory_hotplug.h index 1342adb..b6e9f50 100644
> > > --- a/include/hw/acpi/memory_hotplug.h
> > > +++ b/include/hw/acpi/memory_hotplug.h
> > > @@ -4,6 +4,7 @@
> > >    #include "hw/qdev-core.h"
> > >    #include "hw/acpi/acpi.h"
> > >    #include "migration/vmstate.h"
> > > +#include "hw/acpi/aml-build.h"
> > >
> > >    /**
> > >     * MemStatus:
> > > @@ -45,4 +46,7 @@ extern const VMStateDescription
> > > vmstate_memory_hotplug; vmstate_memory_hotplug, MemHotplugState)
> > >
> > >    void acpi_memory_ospm_status(MemHotplugState *mem_st,
> > > ACPIOSTInfoList ***list); +
> > > +void build_memory_hotplug_aml(Aml *ctx, uint32_t nr_mem,
> > > +                              uint16_t io_base, uint16_t io_len);
> > >    #endif