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

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/16/2015 04:25 PM, Igor Mammedov wrote:
> On Wed, 16 Dec 2015 15:25:46 +0200
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
>
>> On 12/10/2015 01:41 AM, Igor Mammedov wrote:
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>    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 <imammedo@redhat.com>
>>> + *
>>> + * 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
>>>
>>
>