Re: [Qemu-devel] [PATCH v2 1/4] pc: append ssdt-misc.dsl to the DSDT

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

On Wed, Dec 24, 2014 at 05:07:35PM +0100, Paolo Bonzini wrote:
> This part of the ACPI tables can vary in size across machine types, but
> does not depend on the command-line.  It is an SSDT just because it is
> the same for i440fx and Q35, and making it an SSDT made the code a bit
> simpler.  However, it also complicates backwards compatibility, so
> merge it with the DSDT.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/acpi-build.c          |  55 +++++++++++++++++++++++++++---------------
>  tests/acpi-test-data/pc/DSDT  | Bin 3592 -> 3920 bytes
>  tests/acpi-test-data/pc/SSDT  | Bin 2279 -> 1951 bytes
>  tests/acpi-test-data/q35/DSDT | Bin 8182 -> 8510 bytes
>  tests/acpi-test-data/q35/SSDT | Bin 560 -> 232 bytes
>  5 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a4d0c0c..e723fe1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1061,26 +1061,18 @@ static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
>      }
>  }
>  
> +#define SSDT_MISC_SIZE (sizeof(ssdt_misc_aml) - sizeof(AcpiTableHeader))
> +
>  static void
> -build_ssdt(GArray *table_data, GArray *linker,
> -           AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> -           PcPciInfo *pci, PcGuestInfo *guest_info)
> +fill_ssdt_misc(uint8_t *dest, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> +               PcPciInfo *pci)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      uint32_t nr_mem = machine->ram_slots;
> -    unsigned acpi_cpus = guest_info->apic_id_limit;
> -    int ssdt_start = table_data->len;
>      uint8_t *ssdt_ptr;
> -    int i;
> -
> -    /* The current AML generator can cover the APIC ID range [0..255],
> -     * inclusive, for VCPU hotplug. */
> -    QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> -    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
>  
>      /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
> -    ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
> -    memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
> +    ssdt_ptr = g_memdup(ssdp_misc_aml, sizeof(ssdp_misc_aml));
>      if (pm->s3_disabled) {
>          ssdt_ptr[acpi_s3_name[0]] = 'X';
>      }
> @@ -1099,6 +1091,27 @@ build_ssdt(GArray *table_data, GArray *linker,
>      ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml),
>                        ssdt_mctrl_nr_slots[0], 32, nr_mem);
>  
> +    memcpy(dest, ssdt_ptr + sizeof(AcpiTableHeader), SSDT_MISC_SIZE);
> +    g_free(ssdt_ptr);
> +}

We are trying to avoid low-level memory manipulation
around acpi generation.
Maybe you can rewrite it to push the data into GArray?
After all, that's what rest of the code uses.


> +
> +static void
> +build_ssdt(GArray *table_data, GArray *linker,
> +           AcpiCpuInfo *cpu, AcpiPmInfo *pm, PcGuestInfo *guest_info)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    uint32_t nr_mem = machine->ram_slots;
> +    unsigned acpi_cpus = guest_info->apic_id_limit;
> +    int ssdt_start = table_data->len;
> +    int i;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    /* The current AML generator can cover the APIC ID range [0..255],
> +     * inclusive, for VCPU hotplug. */
> +    QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> +    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> +
>      {
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
> @@ -1423,18 +1436,21 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>  }
>  
>  static void
> -build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
> +build_dsdt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
> +           AcpiMiscInfo *misc, PcPciInfo *pci)
>  {
>      AcpiTableHeader *dsdt;
> +    size_t size;
>  
>      assert(misc->dsdt_code && misc->dsdt_size);
>  
> -    dsdt = acpi_data_push(table_data, misc->dsdt_size);
> +    size = misc->dsdt_size + SSDT_MISC_SIZE;
> +    dsdt = acpi_data_push(table_data, size);
>      memcpy(dsdt, misc->dsdt_code, misc->dsdt_size);
> +    fill_ssdt_misc(((uint8_t *)dsdt) + misc->dsdt_size, pm, misc, pci);

I prefer avoiding pointer math above.
If doing pushes here, I would do:

AcpiTableHeader *dsdt = acpi_data_push(table_data, misc->dsdt_size);
AcpiTableHeader *ssdt = acpi_data_push(table_data, SSDT_MISC_SIZE);



>  
>      memset(dsdt, 0, sizeof *dsdt);
> -    build_header(linker, table_data, dsdt, "DSDT",
> -                 misc->dsdt_size, 1);
> +    build_header(linker, table_data, dsdt, "DSDT", size, 1);

You can just save the size at start of function, then replace size
with
    table_data->len - dsdt_start
like we do e.g. for dmar.

>  }
>  
>  /* Build final rsdt table */
> @@ -1591,7 +1607,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>  
>      /* DSDT is pointed to by FADT */
>      dsdt = tables->table_data->len;
> -    build_dsdt(tables->table_data, tables->linker, &misc);
> +    build_dsdt(tables->table_data, tables->linker, &pm, &misc, &pci);
>  
>      /* Count the size of the DSDT and SSDT, we will need it for legacy
>       * sizing of ACPI tables.
> @@ -1604,8 +1620,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>  
>      ssdt = tables->table_data->len;
>      acpi_add_table(table_offsets, tables->table_data);
> -    build_ssdt(tables->table_data, tables->linker, &cpu, &pm, &misc, &pci,
> -               guest_info);
> +    build_ssdt(tables->table_data, tables->linker, &cpu, &pm, guest_info);
>      aml_len += tables->table_data->len - ssdt;
>  
>      acpi_add_table(table_offsets, tables->table_data);
> diff --git a/tests/acpi-test-data/pc/DSDT b/tests/acpi-test-data/pc/DSDT
> index ee9cc6781cea3a9515b9a176eea3459f8e4d8655..9bcc9ba9540f768afeba95231229e093b538d020 100644
> GIT binary patch
> delta 350
> zcmZ9Iy-ve06os!tsH~fW{wM<jVrL=|uu~eV7&WPhO+iYfn~||AQ-KMIk(B|YyZ}$o
> zSKx_S?34xWbk6y%j<5W3@O_Ax&W!2;u=g)qN6X%1cMe=7nnD2JRtX9>o7I}DbVg`V
> zs;M6!x3nD_i2uRlZ;)q2>Dr)oWV=b9(4gZpX6s3x(t!Kuq1U>zr9<uNc{o4bA$>t=
> zBonEJR6XaY#LRHIlv#8w@|z?{QhSilCIF}&m6(>yIaWOdzeX7z?~^t|SU6IDblz%c
> z;L~rg8}F5wG7czHH+3A|mg>r|#k~tSjY{2*5XbgT`#bCbUR{KqoaV(=`c~fAdRfSA
> Ky+iFg4*UTB4N6r2
> 
> delta 19
> acmca0*CE5@66_Mf!N<VBn6i=UA1?qiYXuDe
> 
> diff --git a/tests/acpi-test-data/pc/SSDT b/tests/acpi-test-data/pc/SSDT
> index 558e4c85b031ec081045aec6cc382a680e9c6009..0dd46c68d186f6cb1fb7ee5ad935945fed4f31e8 100644
> GIT binary patch
> delta 23
> ecmaDZIG>*@IM^j*K05;g<F$!g(wlP`tJncn1_s0c
> 
> delta 352
> zcmZ9Iu};G<5Qfho6w66lQe{A5Wnm%_uu~cfEH$l(O+iYf%gERjF%(!kAszsb@&Y^o
> zkJGoHoD&xC^#A{N`tRho{yGxIWOmH~SnoybFUQ++;5soF0sto^G2CpvzPV0kC<vtn
> zqd?Gn`zVnAgYn-W&nAUygWW^wMstjVj?Wm?qdCC=_k}0C#+~AlP&0ZK&X2axoTDD8
> zM42I$&$-ZYW;tG}ta~N%>m*51I!GED0F;kfP7CH7E1!a2qb-m3(AXg?9I4(ruNBnr
> z;n$)c_litg2ehr~Dh_W7Z53PGxq#!SFi}^C3%Zm3hTX%;MZ#0le9}sv<So^!iE8T|
> MYW6trdGN6F1Ki?DumAu6
> 
> diff --git a/tests/acpi-test-data/q35/DSDT b/tests/acpi-test-data/q35/DSDT
> index ef0c75f4236de3e6f727e21991533f2bf8279dee..c30ec0462acee10c55e1035ba0e6798a0739dc35 100644
> GIT binary patch
> delta 350
> zcmZ9Iu};G<5QfhXD$7ZstunA7MkW#gE6_F;7&WP}O+iYf%gET30Toyoc>$2}0=xk5
> zK+KGkbIJlY{r~@+exLkx^fnNg&D`NO0K0t^pIh_JXl)8yCl(_B_#h=QS}fkbxlSi2
> z2&ERgq2NTDp+No*Mt_4mD~xZ0&JJ~>-NRlhU<|z8#~u#^2%_W;<{mYx$I*0m73~1^
> zP$kOtv3$sdmJ`SERAudRnO`JHlF~uax)7ke({gN>cdWb*e~orL-bCyAF!Q8(Zaq~{
> z!%I+$jd<N;+IgT|RabFzY4n+6vuhu492(ng3vohs((kYvc=VB2QRBQ=%7Ek*)vH1+
> K>pg1T@!$uPnoErU
> 
> delta 19
> acmdnz^v#~jCD<k8n>+&p<FAceZ{z_=+y_Mf
> 
> diff --git a/tests/acpi-test-data/q35/SSDT b/tests/acpi-test-data/q35/SSDT
> index 4e45510181c4038505ae1815b76799a2cd0e8808..3700bcc31e1469d4b9aafbf34963d937200a6f15 100644
> GIT binary patch
> delta 22
> dcmdnM@`8~oIM^lR1p@;Equ)d>>B+f_hXG9s2A2Q;
> 
> delta 351
> zcmZ9Iu};G<5QfhsRF;zhtui37vM}KY*eQ(#mYUSWrXZ!#Wn}EiRA6PQ`T`*30eFLU
> z;01UKo`P~tS>UGs|G(4klfT$|AgoQDYyq&(xj2{&w<o}jv={-vij=}=y<UEElZ;Ui
> zN-TCm!HG6Qf%+c|{{~r_7rqVlj;I^$3HI6nW8n2E_IMyb5G8kpGeXVkc{DlRLVJN`
> zpry8bET1!B)Yx&n(rM>fWj9jFgbq^Hg#hKFQKN!+$I7Sh*BHm+eYCC*Q%|bb_M(Cs
> z-hx`}#ydr7oCn5Lbrnaqg>jlq?|r~=P}puqh)cSY{)Roki;qN0kFtCt1Cn=CuXDYr
> M_o#WtgZbgd4{V`Jg8%>k

Binary diffs break whenever I rebase, no need to include them -
just remind in commit log that they need to be updated.


> -- 
> 1.8.3.1
> 
>