Re: [Qemu-devel] [PATCH v3 47/52] pc: acpi-build: drop remaining ssdt_misc template and use acpi_def_block()

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 18, 2015 at 11:09:04AM +0100, Igor Mammedov wrote:
> On Tue, 17 Feb 2015 17:44:31 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Feb 09, 2015 at 10:54:09AM +0000, Igor Mammedov wrote:
> > > It completes dynamic SSDT generation and makes it
> > > independed of IASL binary blobs. It also hides
> > > from user all pointer arithmetic when building
> > > SSDT which makes resulting code a bit cleaner
> > > and concentrating only on composing ASL construct
> > > /i.e. a task build_ssdt() should be doing/.
> > > 
> > > Also it makes one binary blob less stored in QEMU
> > > source tree by removing need to keep and update
> > > hw/i386/ssdt-misc.hex.generated file here in total
> > > saving us ~430LOC.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > 
> > I see where we drop ssdt_misc here but I don't see
> > acpi_def_block anywhere.
> that's been introduced when conversion began in patch:
> [6/52] pc: acpi-build: use aml_def_block() for declaring SSDT table"
> 

OK, the description is confusing. just say dsl is now empty.


> > Also pls don't include generated files in patches,
> > they just make rebases painful.
> > just say in commit log they need to be updated.
> sure
> 
> > 
> > > ---
> > >  hw/i386/Makefile.objs           |   2 +-
> > >  hw/i386/acpi-build.c            |  12 --
> > >  hw/i386/ssdt-misc.dsl           |  21 ---
> > >  hw/i386/ssdt-misc.hex.generated | 399 ----------------------------------------
> > >  4 files changed, 1 insertion(+), 433 deletions(-)
> > >  delete mode 100644 hw/i386/ssdt-misc.dsl
> > >  delete mode 100644 hw/i386/ssdt-misc.hex.generated
> > > 
> > > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > > index 6c8705d..dc8c38a 100644
> > > --- a/hw/i386/Makefile.objs
> > > +++ b/hw/i386/Makefile.objs
> > > @@ -8,7 +8,7 @@ obj-$(CONFIG_XEN) += ../xenpv/ xen/
> > >  obj-y += kvmvapic.o
> > >  obj-y += acpi-build.o
> > >  hw/i386/acpi-build.o: hw/i386/acpi-build.c hw/i386/acpi-dsdt.hex \
> > > -	hw/i386/ssdt-misc.hex hw/i386/q35-acpi-dsdt.hex \
> > > +	hw/i386/q35-acpi-dsdt.hex \
> > >  	hw/i386/ssdt-tpm.hex
> > >  
> > >  iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index bead77e..14c1c7d 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -466,10 +466,6 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > >                   table_data->len - madt_start, 1);
> > >  }
> > >  
> > > -#define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */
> > > -#define ACPI_SSDT_HEADER_LENGTH 36
> > > -
> > > -#include "hw/i386/ssdt-misc.hex"
> > >  #include "hw/i386/ssdt-tpm.hex"
> > >  
> > >  /* Assign BSEL property to all buses.  In the future, this can be changed
> > > @@ -654,7 +650,6 @@ build_ssdt(Aml *table_data,
> > >      MachineState *machine = MACHINE(qdev_get_machine());
> > >      uint32_t nr_mem = machine->ram_slots;
> > >      unsigned acpi_cpus = guest_info->apic_id_limit;
> > > -    uint8_t *ssdt_ptr;
> > >      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> > >      int i;
> > >  
> > > @@ -668,13 +663,6 @@ build_ssdt(Aml *table_data,
> > >                           ACPI_BUILD_APPNAME4, 1,
> > >                           ACPI_BUILD_APPNAME4_HEX, 1);
> > >  
> > > -    /* Copy misc variables and patch values in the S3_ / S4_ / S5_ packages */
> > > -    acpi_data_push(ssdt->buf, sizeof(ssdp_misc_aml) - sizeof(AcpiTableHeader));
> > > -    ssdt_ptr = (uint8_t *)ssdt->buf->data;
> > > -    memcpy(ssdt_ptr + sizeof(AcpiTableHeader),
> > > -           ssdp_misc_aml + sizeof(AcpiTableHeader),
> > > -           sizeof(ssdp_misc_aml) - sizeof(AcpiTableHeader));
> > > -
> > >      scope = aml_scope("\\_SB.PCI0");
> > >      /* build PCI0._CRS */
> > >      crs = aml_resource_template();
> > 
> > So before this patch, we had two headers?
> > I don't see how this makes sense, and it does not
> > match commit log.
> nope, see
> [6/52] pc: acpi-build: use aml_def_block() for declaring SSDT table"
> where header from template is skipped and only content is copied

Pls squash this chunk there then.


> > 
> > 
> > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl
> > > deleted file mode 100644
> > > index 8d61f21..0000000
> > > --- a/hw/i386/ssdt-misc.dsl
> > > +++ /dev/null
> > > @@ -1,21 +0,0 @@
> > > -/*
> > > - * 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/>.
> > > - */
> > > -#include "hw/acpi/pc-hotplug.h"
> > > -
> > > -ACPI_EXTRACT_ALL_CODE ssdp_misc_aml
> > > -
> > > -DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
> > > -{
> > > -}
> > > diff --git a/hw/i386/ssdt-misc.hex.generated b/hw/i386/ssdt-misc.hex.generated
> > > deleted file mode 100644
> > > index cbcf61d..0000000
> > > --- a/hw/i386/ssdt-misc.hex.generated
> > > +++ /dev/null
> > > @@ -1,399 +0,0 @@
> > > -static unsigned char acpi_pci64_length[] = {
> > > -0x6f
> > > -};
> > > -static unsigned char acpi_s4_pkg[] = {
> > > -0x99
> > > -};
> > > -static unsigned char ssdt_mctrl_nr_slots[] = {
> > > -0x7d
> > > -};
> > > -static unsigned char acpi_s3_name[] = {
> > > -0x86
> > > -};
> > > -static unsigned char acpi_pci32_start[] = {
> > > -0x2f
> > > -};
> > > -static unsigned char acpi_pci64_valid[] = {
> > > -0x43
> > > -};
> > > -static unsigned char ssdp_misc_aml[] = {
> > > -0x53,
> > > -0x53,
> > > -0x44,
> > > -0x54,
> > > -0x6c,
> > > -0x1,
> > > -0x0,
> > > -0x0,
> > > -0x1,
> > > -0x3,
> > > -0x42,
> > > -0x58,
> > > -0x50,
> > > -0x43,
> > > -0x0,
> > > -0x0,
> > > -0x42,
> > > -0x58,
> > > -0x53,
> > > -0x53,
> > > -0x44,
> > > -0x54,
> > > -0x53,
> > > -0x55,
> > > -0x1,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x49,
> > > -0x4e,
> > > -0x54,
> > > -0x4c,
> > > -0x28,
> > > -0x8,
> > > -0x14,
> > > -0x20,
> > > -0x10,
> > > -0x4c,
> > > -0x5,
> > > -0x5c,
> > > -0x0,
> > > -0x8,
> > > -0x50,
> > > -0x30,
> > > -0x53,
> > > -0x5f,
> > > -0xc,
> > > -0x78,
> > > -0x56,
> > > -0x34,
> > > -0x12,
> > > -0x8,
> > > -0x50,
> > > -0x30,
> > > -0x45,
> > > -0x5f,
> > > -0xc,
> > > -0x78,
> > > -0x56,
> > > -0x34,
> > > -0x12,
> > > -0x8,
> > > -0x50,
> > > -0x31,
> > > -0x56,
> > > -0x5f,
> > > -0xa,
> > > -0x12,
> > > -0x8,
> > > -0x50,
> > > -0x31,
> > > -0x53,
> > > -0x5f,
> > > -0x11,
> > > -0xb,
> > > -0xa,
> > > -0x8,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x8,
> > > -0x50,
> > > -0x31,
> > > -0x45,
> > > -0x5f,
> > > -0x11,
> > > -0xb,
> > > -0xa,
> > > -0x8,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x8,
> > > -0x50,
> > > -0x31,
> > > -0x4c,
> > > -0x5f,
> > > -0x11,
> > > -0xb,
> > > -0xa,
> > > -0x8,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x8,
> > > -0x4d,
> > > -0x44,
> > > -0x4e,
> > > -0x52,
> > > -0xc,
> > > -0x78,
> > > -0x56,
> > > -0x34,
> > > -0x12,
> > > -0x10,
> > > -0x29,
> > > -0x5c,
> > > -0x0,
> > > -0x8,
> > > -0x5f,
> > > -0x53,
> > > -0x33,
> > > -0x5f,
> > > -0x12,
> > > -0x6,
> > > -0x4,
> > > -0x1,
> > > -0x1,
> > > -0x0,
> > > -0x0,
> > > -0x8,
> > > -0x5f,
> > > -0x53,
> > > -0x34,
> > > -0x5f,
> > > -0x12,
> > > -0x8,
> > > -0x4,
> > > -0xa,
> > > -0x2,
> > > -0xa,
> > > -0x2,
> > > -0x0,
> > > -0x0,
> > > -0x8,
> > > -0x5f,
> > > -0x53,
> > > -0x35,
> > > -0x5f,
> > > -0x12,
> > > -0x6,
> > > -0x4,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x10,
> > > -0x40,
> > > -0xc,
> > > -0x5c,
> > > -0x2f,
> > > -0x3,
> > > -0x5f,
> > > -0x53,
> > > -0x42,
> > > -0x5f,
> > > -0x50,
> > > -0x43,
> > > -0x49,
> > > -0x30,
> > > -0x49,
> > > -0x53,
> > > -0x41,
> > > -0x5f,
> > > -0x5b,
> > > -0x82,
> > > -0x4d,
> > > -0xa,
> > > -0x50,
> > > -0x45,
> > > -0x56,
> > > -0x54,
> > > -0x8,
> > > -0x5f,
> > > -0x48,
> > > -0x49,
> > > -0x44,
> > > -0xd,
> > > -0x51,
> > > -0x45,
> > > -0x4d,
> > > -0x55,
> > > -0x30,
> > > -0x30,
> > > -0x30,
> > > -0x31,
> > > -0x0,
> > > -0x8,
> > > -0x50,
> > > -0x45,
> > > -0x53,
> > > -0x54,
> > > -0xb,
> > > -0xff,
> > > -0xff,
> > > -0x5b,
> > > -0x80,
> > > -0x50,
> > > -0x45,
> > > -0x4f,
> > > -0x52,
> > > -0x1,
> > > -0x50,
> > > -0x45,
> > > -0x53,
> > > -0x54,
> > > -0x1,
> > > -0x5b,
> > > -0x81,
> > > -0xb,
> > > -0x50,
> > > -0x45,
> > > -0x4f,
> > > -0x52,
> > > -0x1,
> > > -0x50,
> > > -0x45,
> > > -0x50,
> > > -0x54,
> > > -0x8,
> > > -0x14,
> > > -0x18,
> > > -0x5f,
> > > -0x53,
> > > -0x54,
> > > -0x41,
> > > -0x0,
> > > -0x70,
> > > -0x50,
> > > -0x45,
> > > -0x53,
> > > -0x54,
> > > -0x60,
> > > -0xa0,
> > > -0x6,
> > > -0x93,
> > > -0x60,
> > > -0x0,
> > > -0xa4,
> > > -0x0,
> > > -0xa1,
> > > -0x4,
> > > -0xa4,
> > > -0xa,
> > > -0xf,
> > > -0x14,
> > > -0xe,
> > > -0x52,
> > > -0x44,
> > > -0x50,
> > > -0x54,
> > > -0x0,
> > > -0x70,
> > > -0x50,
> > > -0x45,
> > > -0x50,
> > > -0x54,
> > > -0x60,
> > > -0xa4,
> > > -0x60,
> > > -0x14,
> > > -0xc,
> > > -0x57,
> > > -0x52,
> > > -0x50,
> > > -0x54,
> > > -0x1,
> > > -0x70,
> > > -0x68,
> > > -0x50,
> > > -0x45,
> > > -0x50,
> > > -0x54,
> > > -0x8,
> > > -0x5f,
> > > -0x43,
> > > -0x52,
> > > -0x53,
> > > -0x11,
> > > -0xd,
> > > -0xa,
> > > -0xa,
> > > -0x47,
> > > -0x1,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x0,
> > > -0x1,
> > > -0x1,
> > > -0x79,
> > > -0x0,
> > > -0x8b,
> > > -0x5f,
> > > -0x43,
> > > -0x52,
> > > -0x53,
> > > -0xa,
> > > -0x2,
> > > -0x49,
> > > -0x4f,
> > > -0x4d,
> > > -0x4e,
> > > -0x8b,
> > > -0x5f,
> > > -0x43,
> > > -0x52,
> > > -0x53,
> > > -0xa,
> > > -0x4,
> > > -0x49,
> > > -0x4f,
> > > -0x4d,
> > > -0x58,
> > > -0x14,
> > > -0x18,
> > > -0x5f,
> > > -0x49,
> > > -0x4e,
> > > -0x49,
> > > -0x0,
> > > -0x70,
> > > -0x50,
> > > -0x45,
> > > -0x53,
> > > -0x54,
> > > -0x49,
> > > -0x4f,
> > > -0x4d,
> > > -0x4e,
> > > -0x70,
> > > -0x50,
> > > -0x45,
> > > -0x53,
> > > -0x54,
> > > -0x49,
> > > -0x4f,
> > > -0x4d,
> > > -0x58
> > > -};
> > > -static unsigned char ssdt_isa_pest[] = {
> > > -0xda
> > > -};
> > > -static unsigned char acpi_s4_name[] = {
> > > -0x92
> > > -};
> > > -static unsigned char acpi_pci64_start[] = {
> > > -0x4d
> > > -};
> > > -static unsigned char acpi_pci64_end[] = {
> > > -0x5e
> > > -};
> > > -static unsigned char acpi_pci32_end[] = {
> > > -0x39
> > > -};
> > > -- 
> > > 1.8.3.1