[Qemu-devel] [PATCH v2 00/10] generalize build_fadt()

[Qemu-devel] [PATCH v2 00/10] generalize build_fadt()

[Igor Mammedov]

v2:
  * fix typo in "acpi: remove unused acpi-dsdt.aml"
  * split ACPI_PORT_SMI_CMD into separate cleanup patch
  * s/pm1_/pm1a_/, s/c2_latency/plvl2_lat/, s/c3_latency/plvl3_lat/
    and fix conflicts in followup patches caused by renaming
  * conditional FIRMWARE_CTRL, DSDT patching is introduced in
    the patch that makes build_fadt() generic instead of earlier
    "pc: acpi: isolate FADT specific data into AcpiFadtData structure"
  * update comment to mention that build_fadt() supports 5.1 revision
  


Series first cleanups ACPI code around build_fadt() and then
converts current packed structure approach to a build_append_FOO()
API, getting rid of error prone explicit endianness conversions
in code and making build_fadt() look more like APCI table declaration
from spec, which should be easier to read/maintain. After that
build_fadt() becomes generic enough that we could drop ARM specific
implementation and reuse generic build_fadt(), reducing code
duplication.

PS: tested only x86 which has make check coverage,
    ARM was only slightly tested. 

git tree for testing:
  https://github.com/imammedo/qemu.git fadt_refactoring_v2

CC: "Michael S. Tsirkin" <mst@redhat.com> 
CC: Shannon Zhao <zhaoshenglong@huawei.com> 
CC: Auger Eric <eric.auger@redhat.com>
CC: qemu-devel@nongnu.org 
CC: qemu-arm@nongnu.org 

Igor Mammedov (10):
  acpi: remove unused acpi-dsdt.aml
  pc: replace pm object initialization with one-liner in
    acpi_get_pm_info()
  acpi: reuse AcpiGenericAddress instead of Acpi20GenericAddress
  acpi: add build_append_gas() helper for Generic Address Structure
  acpi: move ACPI_PORT_SMI_CMD define to header it belongs to
  pc: acpi: isolate FADT specific data into AcpiFadtData structure
  pc: acpi: use build_append_foo() API to construct FADT
  acpi: move build_fadt() from i386 specific to generic ACPI source
  virt_arm: acpi: reuse common build_fadt()
  tests: acpi: don't read all fields in test_acpi_fadt_table()

 include/hw/acpi/acpi-defs.h | 136 ++++++++----------------------
 include/hw/acpi/aml-build.h |  23 ++++++
 include/hw/isa/apm.h        |   3 +
 Makefile                    |   1 -
 hw/acpi/aml-build.c         | 140 +++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    |  39 ++++-----
 hw/i386/acpi-build.c        | 196 ++++++++++++++------------------------------
 hw/isa/apm.c                |   1 -
 pc-bios/acpi-dsdt.aml       | Bin 4405 -> 0 bytes
 tests/bios-tables-test.c    |  82 ++++--------------
 10 files changed, 294 insertions(+), 327 deletions(-)
 delete mode 100644 pc-bios/acpi-dsdt.aml

-- 
2.7.4

[Qemu-devel] [PATCH v2 01/10] acpi: remove unused acpi-dsdt.aml

[Igor Mammedov]

SeaBIOS blob which is currently shipped with QEMU
doesn't need acpi-dsdt.aml nor is able to use it
and code that loaded it in QEMU was removed by
(commit 9fb7aaaf4c "pc: drop external DSDT loading")
in 2013.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 Makefile              |   1 -
 pc-bios/acpi-dsdt.aml | Bin 4405 -> 0 bytes
 2 files changed, 1 deletion(-)
 delete mode 100644 pc-bios/acpi-dsdt.aml

diff --git a/Makefile b/Makefile
index 90e05ac..f3a4cf9 100644
--- a/Makefile
+++ b/Makefile
@@ -648,7 +648,6 @@ bepo    cz
 ifdef INSTALL_BLOBS
 BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
 vgabios-stdvga.bin vgabios-vmware.bin vgabios-qxl.bin vgabios-virtio.bin \
-acpi-dsdt.aml \
 ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin QEMU,cgthree.bin \
 pxe-e1000.rom pxe-eepro100.rom pxe-ne2k_pci.rom \
 pxe-pcnet.rom pxe-rtl8139.rom pxe-virtio.rom \
diff --git a/pc-bios/acpi-dsdt.aml b/pc-bios/acpi-dsdt.aml
deleted file mode 100644
index 558c10f51ccbbf9ec2f47a4a998a7055059a8963..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 4405
zcmb7{O>f)C8OI;KNTx=#Ov$uk$9XY?b_=xIjb0q@_EJP5WkrsxFrpHqP*76NE}&CE
zqzSOzpn&5RO*WTe*G<u*ve0?^5!z#q`3UV-*rJF}QJ-hVlxK_$u;qa>|Kam8znLL9
z<A?s>dJ#bTx_LkF0GjuGY(WhGo!+3koGWcQ9rFPU5B+94((<~g4WH$C9dAv`{m^gT
zZEJrW$A5|A$IoMJl)(Ns&a3@V^7|L@K9JFq{e&^9IOQm8M#H0x!0S}3=w`>a8*i9l
zMGe0XR&=-HYff+FBQoL^UcVI<n+xnWFBU<!u}c6mx@m3g#6Gb#3)?l@pr*I@_{5&;
z#Tgm?=lKNy@f?7`Y?dceyma7Ch?1^<&1QdpC#wIrasZas-`*<LS>@%=&fLZ0Lo8-=
z2|2%0`vJHO7J2;;UQ)-|gCMNeL^TdtX>}ZQ>$N1Pgb_W)N-Ls=$)m@-itRY~WHYgk
zgX+BqrWEW?)FoC3!tE_lT@6}k^@E_hy_E!2ipVPzkypAAJ^BL$Apdw8JA0Oxf|hkN
zXbsXi&~OfL^l_GN27^7ov3&C`59aWhLwfmMtLJY9eLvcCx1(^-fP`A&gqlWQ#LS5&
z_E*O-9LM?DYzmXYSH~mx^T>vO|2H#*DO<8=Sc*kf_+yTS?9DqcX}p{p+4*D-kG#yi
zb|d180Xv{$XK)dCIxy@<o~j1#hl>PNAErQ+8n3KJVco~H$7En{Z-6#s#%p5=&X1+|
z>%skMU4%Dqj4^z*PT^<HPDV28n4R#f8{BTI;^{1=ethujk0=Ux0^Ga?3*DgA)8G>@
zyarVauZe}V<Kx}wZd^0cwM;RGM?dcmJR}qgKaWeEhGmVdw6z2haP%@Q?MLtk^y~o)
zk3PQD^ylV=;pX_@&&QKH#t?&sUZ29JSeA7h*5T1l_HN&uJ1#AsceGfh3=SFYnmfKX
ze-#(NT@&+50P!S?b2^3B<~-vDTWf3I8Q&RTwzap$TO7yo4fv_alm4<B4CYDAc_<p8
z?+N9w#kTgj@ws7H<wNe@GQHb-)pT?+n)o23J)-e_Uzii)!~m=8@Gv_Rrgkn2)8}z;
zg5DcPKhZIcg>jsYb+#sOA%+7j58pBiUkMT(uE)EZc=I=hhhb|Mzl_$me4&!?nw8e>
zrn?k)tz9iS(8fRw?snkyc5t5KZ$5k#v#U?yN%1MgInZJNd^U)+Nr_tgvleDJyAxVO
zPWv%F!Kn)RgVL?v9+vVZzHHF#-SR=yHLN$FWK%oSQ8ZIwpzxryXxg(Ge)CX;b46Zg
zSP;*+ADX6;JTX4^)VU|xo+|Q8P4P9NjA+U|QIaS2hTGy7TG*Z{@=Q$);fbc)6D4`3
zS@1I<Y`Lcir;Oax6rO44QOcYd?wR%=!#z{ejPOi5k5cB$Dx6vFnVM!*PLwj|g2K7L
zJyXsFl@q1RX(^nR!fC0TC}mFkpyCLnoH>Ovr*fi{Ihn%A6i%jcqLexDH;OrNO!%zi
z70$fMiBjgo54$v<w!&$voG4|^MTHap`xqyk&qb9JrOa7SI137ALFGg#b1o^IOA6<b
z%863ubQDfU;dE3^lrkr7#-#ZyDx5`?6Q#^qQaDQrXG!HmDRV9>oXZO5vdW24=5!TK
zSK)M3PLwj|io&^~aIUDFC}qx7g>zNmTva(y%AB|xl-BJ9h4X^SiBjfVQ#jWY&NY=2
zrOdgmaIPzy>nbNone(E;c~RlKsB)r|IX4te+zyF%j(;^bR8EvK=Ou;nlEQgO<wPlS
zURF3SE1Z{APLwj|6@~MP!g)pIL@9G#RXDFIoL5y&l!9~k>_^uO`jgU*EWn+e7WD5_
zEWB0eR-;?pa+f=I@RvWyJ!OYu+`;CiEbnf2?s)wi8uTm00?U7yg&aRY9KcIzV;Q`6
zCiz!mc9@K*KBea2QFnpf=yXS7<8GMt+Vm$6i>qw;%L3#K{9WM*1%OT{c#v2UJ3Z<I
zb<ZtEekX+AQJo#~mL-1Dm{OOxz7U1|P<uHRy}+$`zeDY(*_-FG<L2rIXRk`xt2}!Z
z`$y-TG<((k{_NG^(H^mT=dv^X|43hx(${$U+PU<#_oT0#ruWaM$J5Rarmsus>pXq^
zT>AQZ($|Maw@suE&!;y<`g94=kqD<e-Q4HhET3#QFUFX<icK`TPP;%`LHD{B>@_qz
zV0*#s-WcMfm}eH?9)hk>GJY{)I`G1PBt~VzbmU(20$kE(UXx6W8+$q?xy%b%yZW%q
z{)wleKHuy9jcpE}*<9c)y5YFHS*&<~ODl{%!&5$OWOp*J;^)+h)37m&ChTd<7T}A0
zZU426&7a``5jTMQ$<uue9!|<%ACDd;4|&&Pn6TrAT5quPt5|z&@sb%&VyBmj+Chtt
z+hW5DI+aRg8*mW1l?u2kQL9pg2WMw1+E%*`w$|W**tBCmxpiGQZHeN#C{81NEYv5W
U_=PAMqG*c36NN8|mMC`Mf01wzJpcdz

-- 
2.7.4

[Qemu-devel] [PATCH v2 02/10] pc: replace pm object initialization with one-liner in acpi_get_pm_info()

[Igor Mammedov]

next patch will need it before it gets to piix4/lpc branches
that initializes 'obj' now.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/acpi-build.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index deb440f..b85fefe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -128,7 +128,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
 {
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
-    Object *obj = NULL;
+    Object *obj = piix ? piix : lpc;
     QObject *o;
 
     pm->force_rev1_fadt = false;
@@ -138,7 +138,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     if (piix) {
         /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
         pm->force_rev1_fadt = true;
-        obj = piix;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
         pm->pcihp_io_base =
             object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
@@ -146,7 +145,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
             object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
     }
     if (lpc) {
-        obj = lpc;
         pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
     assert(obj);
-- 
2.7.4

[Qemu-devel] [PATCH v2 03/10] acpi: reuse AcpiGenericAddress instead of Acpi20GenericAddress

[Igor Mammedov]

Drop duplicate in form of Acpi20GenericAddress and reuse
AcpiGenericAddress.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/acpi/acpi-defs.h | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 80c8099..9942bc5 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -40,18 +40,6 @@ enum {
     ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
 };
 
-/*
- * ACPI 2.0 Generic Address Space definition.
- */
-struct Acpi20GenericAddress {
-    uint8_t  address_space_id;
-    uint8_t  register_bit_width;
-    uint8_t  register_bit_offset;
-    uint8_t  reserved;
-    uint64_t address;
-} QEMU_PACKED;
-typedef struct Acpi20GenericAddress Acpi20GenericAddress;
-
 struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
     uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
     uint8_t  checksum;               /* To make sum of struct == 0 */
@@ -167,7 +155,8 @@ struct AcpiGenericAddress {
     uint8_t space_id;        /* Address space where struct or register exists */
     uint8_t bit_width;       /* Size in bits of given register */
     uint8_t bit_offset;      /* Bit offset within the register */
-    uint8_t access_width;    /* Minimum Access size (ACPI 3.0) */
+    uint8_t access_width;    /* ACPI 3.0: Minimum Access size (ACPI 3.0),
+                                ACPI 2.0: Reserved, Table 5-1 */
     uint64_t address;        /* 64-bit address of struct or register */
 } QEMU_PACKED;
 
@@ -456,7 +445,7 @@ typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
 struct Acpi20Hpet {
     ACPI_TABLE_HEADER_DEF                    /* ACPI common table header */
     uint32_t           timer_block_id;
-    Acpi20GenericAddress addr;
+    struct AcpiGenericAddress addr;
     uint8_t            hpet_number;
     uint16_t           min_tick;
     uint8_t            page_protect;
-- 
2.7.4

[Qemu-devel] [PATCH v2 04/10] acpi: add build_append_gas() helper for Generic Address Structure

[Igor Mammedov]

it will help to add Generic Address Structure to ACPI tables
without using packed C structures and avoid endianness
issues as API doesn't need an explicit conversion.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/acpi/aml-build.h | 20 ++++++++++++++++++++
 hw/acpi/aml-build.c         | 16 ++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 88d0738..8692ccc 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -78,6 +78,15 @@ typedef enum {
 } AmlUpdateRule;
 
 typedef enum {
+    AML_AS_SYSTEM_MEMORY = 0X00,
+    AML_AS_SYSTEM_IO = 0X01,
+    AML_AS_PCI_CONFIG = 0X02,
+    AML_AS_EMBEDDED_CTRL = 0X03,
+    AML_AS_SMBUS = 0X04,
+    AML_AS_FFH = 0X7F,
+} AmlAddressSpace;
+
+typedef enum {
     AML_SYSTEM_MEMORY = 0X00,
     AML_SYSTEM_IO = 0X01,
     AML_PCI_CONFIG = 0X02,
@@ -389,6 +398,17 @@ int
 build_append_named_dword(GArray *array, const char *name_format, ...)
 GCC_FMT_ATTR(2, 3);
 
+void build_append_gas(GArray *table, AmlAddressSpace as,
+                      uint8_t bit_width, uint8_t bit_offset,
+                      uint8_t access_width, uint64_t address);
+
+static inline void
+build_append_gas_from_struct(GArray *table, const struct AcpiGenericAddress *s)
+{
+    build_append_gas(table, s->space_id, s->bit_width, s->bit_offset,
+                     s->access_width, s->address);
+}
+
 void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
 
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 36a6cc4..3fef5f6 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -258,6 +258,22 @@ static void build_append_int(GArray *table, uint64_t value)
     }
 }
 
+/* Generic Address Structure (GAS)
+ * ACPI 2.0/3.0: 5.2.3.1 Generic Address Structure
+ * 2.0 compat note:
+ *    @access_width must be 0, see ACPI 2.0:Table 5-1
+ */
+void build_append_gas(GArray *table, AmlAddressSpace as,
+                      uint8_t bit_width, uint8_t bit_offset,
+                      uint8_t access_width, uint64_t address)
+{
+    build_append_int_noprefix(table, as, 1);
+    build_append_int_noprefix(table, bit_width, 1);
+    build_append_int_noprefix(table, bit_offset, 1);
+    build_append_int_noprefix(table, access_width, 1);
+    build_append_int_noprefix(table, address, 8);
+}
+
 /*
  * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
  * and return the offset to 0x00000000 for runtime patching.
-- 
2.7.4

[Qemu-devel] [PATCH v2 05/10] acpi: move ACPI_PORT_SMI_CMD define to header it belongs to

[Igor Mammedov]

ACPI_PORT_SMI_CMD is alias for APM_CNT_IOPORT,
so make it really one instead of duplicating its value.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/isa/apm.h | 3 +++
 hw/i386/acpi-build.c | 2 --
 hw/isa/apm.c         | 1 -
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/isa/apm.h b/include/hw/isa/apm.h
index 4839ff1..b7098bf 100644
--- a/include/hw/isa/apm.h
+++ b/include/hw/isa/apm.h
@@ -5,6 +5,9 @@
 #include "hw/hw.h"
 #include "exec/memory.h"
 
+#define APM_CNT_IOPORT  0xb2
+#define ACPI_PORT_SMI_CMD APM_CNT_IOPORT
+
 typedef void (*apm_ctrl_changed_t)(uint32_t val, void *arg);
 
 typedef struct APMState {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b85fefe..699f3a0 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -255,8 +255,6 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
                                                NULL));
 }
 
-#define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is APM_CNT_IOPORT */
-
 static void acpi_align_size(GArray *blob, unsigned align)
 {
     /* Align size to multiple of given size. This reduces the chance
diff --git a/hw/isa/apm.c b/hw/isa/apm.c
index e232b0d..c3101ef 100644
--- a/hw/isa/apm.c
+++ b/hw/isa/apm.c
@@ -34,7 +34,6 @@
 #endif
 
 /* fixed I/O location */
-#define APM_CNT_IOPORT  0xb2
 #define APM_STS_IOPORT  0xb3
 
 static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 05/10] acpi: move ACPI_PORT_SMI_CMD define to header it belongs to

[Auger Eric]

Hi,

On 28/02/18 15:23, Igor Mammedov wrote:
> ACPI_PORT_SMI_CMD is alias for APM_CNT_IOPORT,
> so make it really one instead of duplicating its value.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/isa/apm.h | 3 +++
>  hw/i386/acpi-build.c | 2 --
>  hw/isa/apm.c         | 1 -
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/isa/apm.h b/include/hw/isa/apm.h
> index 4839ff1..b7098bf 100644
> --- a/include/hw/isa/apm.h
> +++ b/include/hw/isa/apm.h
> @@ -5,6 +5,9 @@
>  #include "hw/hw.h"
>  #include "exec/memory.h"
>  
> +#define APM_CNT_IOPORT  0xb2
> +#define ACPI_PORT_SMI_CMD APM_CNT_IOPORT
> +
>  typedef void (*apm_ctrl_changed_t)(uint32_t val, void *arg);
>  
>  typedef struct APMState {
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b85fefe..699f3a0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -255,8 +255,6 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>                                                 NULL));
>  }
>  
> -#define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is APM_CNT_IOPORT */
> -
>  static void acpi_align_size(GArray *blob, unsigned align)
>  {
>      /* Align size to multiple of given size. This reduces the chance
> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
> index e232b0d..c3101ef 100644
> --- a/hw/isa/apm.c
> +++ b/hw/isa/apm.c
> @@ -34,7 +34,6 @@
>  #endif
>  
>  /* fixed I/O location */
> -#define APM_CNT_IOPORT  0xb2
>  #define APM_STS_IOPORT  0xb3
>  
>  static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
> 

[Qemu-devel] [PATCH v2 06/10] pc: acpi: isolate FADT specific data into AcpiFadtData structure

[Igor Mammedov]

move FADT data initialization out of fadt_setup() into dedicated
init_fadt_data() that will set common for pc/q35 values in
AcpiFadtData structure and acpi_get_pm_info() will complement
it with pc/q35 specific values initialization.

That will allow to get rid of fadt_setup() and generalize
build_fadt() so it could be easily extended for rev5 and
reused by ARM target.

While at it also move facs/dsdt/xdsdt offsets from build_fadt()
arg list into AcpiFadtData, as they belong to the same dataset.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
changes requested by Auger Eric <eric.auger@redhat.com>
  - s/pm1_/pm1a_/
  - s/c2_latency/plvl2_lat/
  - s/c3_latency/plvl3_lat/
  - cleanup ACPI_PORT_SMI_CMD TODO, move  it to hw/isa/apm.h
  - move
       if (f->facs_tbl_offset) / if (f->dsdt_tbl_offset)
           bios_linker_loader_add_pointer(...)
    into the patch that makes build_fadt() generic
---
 include/hw/acpi/acpi-defs.h |  28 +++++++
 hw/i386/acpi-build.c        | 190 ++++++++++++++++++++++++--------------------
 2 files changed, 130 insertions(+), 88 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 9942bc5..3fb0ace 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -175,6 +175,34 @@ struct AcpiFadtDescriptorRev5_1 {
 
 typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
 
+typedef struct AcpiFadtData {
+    struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
+    struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
+    struct AcpiGenericAddress pm_tmr;    /* PM_TMR_BLK */
+    struct AcpiGenericAddress gpe0_blk;  /* GPE0_BLK */
+    struct AcpiGenericAddress reset_reg; /* RESET_REG */
+    uint8_t reset_val;         /* RESET_VALUE */
+    uint8_t  rev;              /* Revision */
+    uint32_t flags;            /* Flags */
+    uint32_t smi_cmd;          /* SMI_CMD */
+    uint16_t sci_int;          /* SCI_INT */
+    uint8_t  int_model;        /* INT_MODEL */
+    uint8_t  acpi_enable_cmd;  /* ACPI_ENABLE */
+    uint8_t  acpi_disable_cmd; /* ACPI_DISABLE */
+    uint8_t  rtc_century;      /* CENTURY */
+    uint16_t plvl2_lat;        /* P_LVL2_LAT */
+    uint16_t plvl3_lat;        /* P_LVL3_LAT */
+
+    /*
+     * respective tables offsets within ACPI_BUILD_TABLE_FILE,
+     * NULL if table doesn't exist (in that case field's value
+     * won't be patched by linker and will be kept set to 0)
+     */
+    unsigned *facs_tbl_offset; /* FACS offset in */
+    unsigned *dsdt_tbl_offset;
+    unsigned *xdsdt_tbl_offset;
+} AcpiFadtData;
+
 #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
 #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 699f3a0..1f88ed1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -91,17 +91,11 @@ typedef struct AcpiMcfgInfo {
 } AcpiMcfgInfo;
 
 typedef struct AcpiPmInfo {
-    bool force_rev1_fadt;
     bool s3_disabled;
     bool s4_disabled;
     bool pcihp_bridge_en;
     uint8_t s4_val;
-    uint16_t sci_int;
-    uint8_t acpi_enable_cmd;
-    uint8_t acpi_disable_cmd;
-    uint32_t gpe0_blk;
-    uint32_t gpe0_blk_len;
-    uint32_t io_base;
+    AcpiFadtData fadt;
     uint16_t cpu_hp_io_base;
     uint16_t pcihp_io_base;
     uint16_t pcihp_io_len;
@@ -124,20 +118,59 @@ typedef struct AcpiBuildPciBusHotplugState {
     bool pcihp_bridge_en;
 } AcpiBuildPciBusHotplugState;
 
+static void init_common_fadt_data(Object *o, AcpiFadtData *data)
+{
+    uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
+    AmlAddressSpace as = AML_AS_SYSTEM_IO;
+    AcpiFadtData fadt = {
+        .rev = 3,
+        .flags =
+            (1 << ACPI_FADT_F_WBINVD) |
+            (1 << ACPI_FADT_F_PROC_C1) |
+            (1 << ACPI_FADT_F_SLP_BUTTON) |
+            (1 << ACPI_FADT_F_RTC_S4) |
+            (1 << ACPI_FADT_F_USE_PLATFORM_CLOCK) |
+            /* APIC destination mode ("Flat Logical") has an upper limit of 8
+             * CPUs for more than 8 CPUs, "Clustered Logical" mode has to be
+             * used
+             */
+            ((max_cpus > 8) ? (1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL) : 0),
+        .int_model = 1 /* Multiple APIC */,
+        .rtc_century = RTC_CENTURY,
+        .plvl2_lat = 0xfff /* C2 state not supported */,
+        .plvl3_lat = 0xfff /* C3 state not supported */,
+        .smi_cmd = ACPI_PORT_SMI_CMD,
+        .sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
+        .acpi_enable_cmd =
+            object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL),
+        .acpi_disable_cmd =
+            object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL),
+        .pm1a_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
+        .pm1a_cnt = { .space_id = as, .bit_width = 2 * 8,
+                      .address = io + 0x04 },
+        .pm_tmr = { .space_id = as, .bit_width = 4 * 8, .address = io + 0x08 },
+        .gpe0_blk = { .space_id = as, .bit_width =
+            object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK_LEN, NULL) * 8,
+            .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
+        },
+    };
+    *data = fadt;
+}
+
 static void acpi_get_pm_info(AcpiPmInfo *pm)
 {
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     Object *obj = piix ? piix : lpc;
     QObject *o;
-
-    pm->force_rev1_fadt = false;
     pm->cpu_hp_io_base = 0;
     pm->pcihp_io_base = 0;
     pm->pcihp_io_len = 0;
+
+    init_common_fadt_data(obj, &pm->fadt);
     if (piix) {
         /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
-        pm->force_rev1_fadt = true;
+        pm->fadt.rev = 1;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
         pm->pcihp_io_base =
             object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
@@ -145,10 +178,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
             object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
     }
     if (lpc) {
+        struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
+            .bit_width = 8, .address = ICH9_RST_CNT_IOPORT };
+        pm->fadt.reset_reg = r;
+        pm->fadt.reset_val = 0xf;
+        pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
         pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
     assert(obj);
 
+    /* The above need not be conditional on machine type because the reset port
+     * happens to be the same on PIIX (pc) and ICH9 (q35). */
+    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
+
     /* Fill in optional s3/s4 related properties */
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
     if (o) {
@@ -172,22 +214,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     }
     qobject_decref(o);
 
-    /* Fill in mandatory properties */
-    pm->sci_int = object_property_get_uint(obj, ACPI_PM_PROP_SCI_INT, NULL);
-
-    pm->acpi_enable_cmd = object_property_get_uint(obj,
-                                                   ACPI_PM_PROP_ACPI_ENABLE_CMD,
-                                                   NULL);
-    pm->acpi_disable_cmd =
-        object_property_get_uint(obj,
-                                 ACPI_PM_PROP_ACPI_DISABLE_CMD,
-                                 NULL);
-    pm->io_base = object_property_get_uint(obj, ACPI_PM_PROP_PM_IO_BASE,
-                                           NULL);
-    pm->gpe0_blk = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK,
-                                            NULL);
-    pm->gpe0_blk_len = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
-                                                NULL);
     pm->pcihp_bridge_en =
         object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
                                  NULL);
@@ -273,73 +299,53 @@ build_facs(GArray *table_data, BIOSLinker *linker)
 }
 
 /* Load chipset information in FADT */
-static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
+static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
 {
-    fadt->model = 1;
+    fadt->model = f.int_model;
     fadt->reserved1 = 0;
-    fadt->sci_int = cpu_to_le16(pm->sci_int);
-    fadt->smi_cmd = cpu_to_le32(ACPI_PORT_SMI_CMD);
-    fadt->acpi_enable = pm->acpi_enable_cmd;
-    fadt->acpi_disable = pm->acpi_disable_cmd;
+    fadt->sci_int = cpu_to_le16(f.sci_int);
+    fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
+    fadt->acpi_enable = f.acpi_enable_cmd;
+    fadt->acpi_disable = f.acpi_disable_cmd;
     /* EVT, CNT, TMR offset matches hw/acpi/core.c */
-    fadt->pm1a_evt_blk = cpu_to_le32(pm->io_base);
-    fadt->pm1a_cnt_blk = cpu_to_le32(pm->io_base + 0x04);
-    fadt->pm_tmr_blk = cpu_to_le32(pm->io_base + 0x08);
-    fadt->gpe0_blk = cpu_to_le32(pm->gpe0_blk);
+    fadt->pm1a_evt_blk = cpu_to_le32(f.pm1a_evt.address);
+    fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1a_cnt.address);
+    fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
+    fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
     /* EVT, CNT, TMR length matches hw/acpi/core.c */
-    fadt->pm1_evt_len = 4;
-    fadt->pm1_cnt_len = 2;
-    fadt->pm_tmr_len = 4;
-    fadt->gpe0_blk_len = pm->gpe0_blk_len;
-    fadt->plvl2_lat = cpu_to_le16(0xfff); /* C2 state not supported */
-    fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
-    fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
-                              (1 << ACPI_FADT_F_PROC_C1) |
-                              (1 << ACPI_FADT_F_SLP_BUTTON) |
-                              (1 << ACPI_FADT_F_RTC_S4));
-    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
-    /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
-     * For more than 8 CPUs, "Clustered Logical" mode has to be used
-     */
-    if (max_cpus > 8) {
-        fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
-    }
-    fadt->century = RTC_CENTURY;
-    if (pm->force_rev1_fadt) {
+    fadt->pm1_evt_len = f.pm1a_evt.bit_width / 8;
+    fadt->pm1_cnt_len = f.pm1a_cnt.bit_width / 8;
+    fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
+    fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
+    fadt->plvl2_lat = cpu_to_le16(f.plvl2_lat);
+    fadt->plvl3_lat = cpu_to_le16(f.plvl3_lat);
+    fadt->flags = cpu_to_le32(f.flags);
+    fadt->century = f.rtc_century;
+    if (f.rev == 1) {
         return;
     }
 
-    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
-    fadt->reset_value = 0xf;
-    fadt->reset_register.space_id = AML_SYSTEM_IO;
-    fadt->reset_register.bit_width = 8;
-    fadt->reset_register.address = cpu_to_le64(ICH9_RST_CNT_IOPORT);
-    /* The above need not be conditional on machine type because the reset port
-     * happens to be the same on PIIX (pc) and ICH9 (q35). */
-    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
+    fadt->reset_value = f.reset_val;
+    fadt->reset_register = f.reset_reg;
+    fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
 
-    fadt->xpm1a_event_block.space_id = AML_SYSTEM_IO;
-    fadt->xpm1a_event_block.bit_width = fadt->pm1_evt_len * 8;
-    fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
+    fadt->xpm1a_event_block = f.pm1a_evt;
+    fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1a_evt.address);
 
-    fadt->xpm1a_control_block.space_id = AML_SYSTEM_IO;
-    fadt->xpm1a_control_block.bit_width = fadt->pm1_cnt_len * 8;
-    fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x4);
+    fadt->xpm1a_control_block = f.pm1a_cnt;
+    fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1a_cnt.address);
 
-    fadt->xpm_timer_block.space_id = AML_SYSTEM_IO;
-    fadt->xpm_timer_block.bit_width = fadt->pm_tmr_len * 8;
-    fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x8);
+    fadt->xpm_timer_block = f.pm_tmr;
+    fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
 
-    fadt->xgpe0_block.space_id = AML_SYSTEM_IO;
-    fadt->xgpe0_block.bit_width = pm->gpe0_blk_len * 8;
-    fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
+    fadt->xgpe0_block = f.gpe0_blk;
+    fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
 }
 
 
 /* FADT */
 static void
-build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
-           unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
+build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
            const char *oem_id, const char *oem_table_id)
 {
     AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data, sizeof(*fadt));
@@ -347,29 +353,29 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
     unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
     unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
     int fadt_size = sizeof(*fadt);
-    int rev = 3;
 
     /* FACS address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
-        ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
+        ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
 
     /* DSDT address to be filled by Guest linker */
-    fadt_setup(fadt, pm);
+    fadt_setup(fadt, *f);
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
-        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
-    if (pm->force_rev1_fadt) {
-        rev = 1;
+        ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
+
+    if (f->rev == 1) {
         fadt_size = offsetof(typeof(*fadt), reset_register);
-    } else {
+    } else if (f->xdsdt_tbl_offset) {
         bios_linker_loader_add_pointer(linker,
             ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
-            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
+            ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
     }
 
     build_header(linker, table_data,
-                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
+                (void *)fadt, "FACP", fadt_size, f->rev,
+                oem_id, oem_table_id);
 }
 
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
@@ -2049,7 +2055,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
     crs = aml_resource_template();
     aml_append(crs,
-        aml_io(AML_DECODE16, pm->gpe0_blk, pm->gpe0_blk, 1, pm->gpe0_blk_len)
+        aml_io(
+               AML_DECODE16,
+               pm->fadt.gpe0_blk.address,
+               pm->fadt.gpe0_blk.address,
+               1,
+               pm->fadt.gpe0_blk.bit_width / 8)
     );
     aml_append(dev, aml_name_decl("_CRS", crs));
     aml_append(scope, dev);
@@ -2696,7 +2707,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     /* ACPI tables pointed to by RSDT */
     fadt = tables_blob->len;
     acpi_add_table(table_offsets, tables_blob);
-    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
+    pm.fadt.facs_tbl_offset = &facs;
+    pm.fadt.dsdt_tbl_offset = &dsdt;
+    pm.fadt.xdsdt_tbl_offset = &dsdt;
+    build_fadt(tables_blob, tables->linker, &pm.fadt,
                slic_oem.id, slic_oem.table_id);
     aml_len += tables_blob->len - fadt;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 06/10] pc: acpi: isolate FADT specific data into AcpiFadtData structure

[Auger Eric]

Hi Igor,

On 28/02/18 15:23, Igor Mammedov wrote:
> move FADT data initialization out of fadt_setup() into dedicated
> init_fadt_data() that will set common for pc/q35 values in
> AcpiFadtData structure and acpi_get_pm_info() will complement
> it with pc/q35 specific values initialization.
> 
> That will allow to get rid of fadt_setup() and generalize
> build_fadt() so it could be easily extended for rev5 and
> reused by ARM target.
> 
> While at it also move facs/dsdt/xdsdt offsets from build_fadt()
> arg list into AcpiFadtData, as they belong to the same dataset.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v2:
> changes requested by Auger Eric <eric.auger@redhat.com>
suggested ;-)

>   - s/pm1_/pm1a_/
>   - s/c2_latency/plvl2_lat/
>   - s/c3_latency/plvl3_lat/
>   - cleanup ACPI_PORT_SMI_CMD TODO, move  it to hw/isa/apm.h
>   - move
>        if (f->facs_tbl_offset) / if (f->dsdt_tbl_offset)
>            bios_linker_loader_add_pointer(...)
>     into the patch that makes build_fadt() generic
> ---
>  include/hw/acpi/acpi-defs.h |  28 +++++++
>  hw/i386/acpi-build.c        | 190 ++++++++++++++++++++++++--------------------
>  2 files changed, 130 insertions(+), 88 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 9942bc5..3fb0ace 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -175,6 +175,34 @@ struct AcpiFadtDescriptorRev5_1 {
>  
>  typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
>  
> +typedef struct AcpiFadtData {
> +    struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
> +    struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
> +    struct AcpiGenericAddress pm_tmr;    /* PM_TMR_BLK */
> +    struct AcpiGenericAddress gpe0_blk;  /* GPE0_BLK */
> +    struct AcpiGenericAddress reset_reg; /* RESET_REG */
> +    uint8_t reset_val;         /* RESET_VALUE */
> +    uint8_t  rev;              /* Revision */
> +    uint32_t flags;            /* Flags */
> +    uint32_t smi_cmd;          /* SMI_CMD */
> +    uint16_t sci_int;          /* SCI_INT */
> +    uint8_t  int_model;        /* INT_MODEL */
> +    uint8_t  acpi_enable_cmd;  /* ACPI_ENABLE */
> +    uint8_t  acpi_disable_cmd; /* ACPI_DISABLE */
> +    uint8_t  rtc_century;      /* CENTURY */
> +    uint16_t plvl2_lat;        /* P_LVL2_LAT */
> +    uint16_t plvl3_lat;        /* P_LVL3_LAT */
> +
> +    /*
> +     * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> +     * NULL if table doesn't exist (in that case field's value
> +     * won't be patched by linker and will be kept set to 0)
> +     */
> +    unsigned *facs_tbl_offset; /* FACS offset in */
> +    unsigned *dsdt_tbl_offset;
> +    unsigned *xdsdt_tbl_offset;
> +} AcpiFadtData;
> +
>  #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
>  #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 699f3a0..1f88ed1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -91,17 +91,11 @@ typedef struct AcpiMcfgInfo {
>  } AcpiMcfgInfo;
>  
>  typedef struct AcpiPmInfo {
> -    bool force_rev1_fadt;
>      bool s3_disabled;
>      bool s4_disabled;
>      bool pcihp_bridge_en;
>      uint8_t s4_val;
> -    uint16_t sci_int;
> -    uint8_t acpi_enable_cmd;
> -    uint8_t acpi_disable_cmd;
> -    uint32_t gpe0_blk;
> -    uint32_t gpe0_blk_len;
> -    uint32_t io_base;
> +    AcpiFadtData fadt;
>      uint16_t cpu_hp_io_base;
>      uint16_t pcihp_io_base;
>      uint16_t pcihp_io_len;
> @@ -124,20 +118,59 @@ typedef struct AcpiBuildPciBusHotplugState {
>      bool pcihp_bridge_en;
>  } AcpiBuildPciBusHotplugState;
>  
> +static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> +{
> +    uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> +    AmlAddressSpace as = AML_AS_SYSTEM_IO;
> +    AcpiFadtData fadt = {
> +        .rev = 3,
> +        .flags =
> +            (1 << ACPI_FADT_F_WBINVD) |
> +            (1 << ACPI_FADT_F_PROC_C1) |
> +            (1 << ACPI_FADT_F_SLP_BUTTON) |
> +            (1 << ACPI_FADT_F_RTC_S4) |
> +            (1 << ACPI_FADT_F_USE_PLATFORM_CLOCK) |
> +            /* APIC destination mode ("Flat Logical") has an upper limit of 8
> +             * CPUs for more than 8 CPUs, "Clustered Logical" mode has to be
> +             * used
> +             */
> +            ((max_cpus > 8) ? (1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL) : 0),
> +        .int_model = 1 /* Multiple APIC */,
> +        .rtc_century = RTC_CENTURY,
> +        .plvl2_lat = 0xfff /* C2 state not supported */,
> +        .plvl3_lat = 0xfff /* C3 state not supported */,
> +        .smi_cmd = ACPI_PORT_SMI_CMD,
> +        .sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
> +        .acpi_enable_cmd =
> +            object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL),
> +        .acpi_disable_cmd =
> +            object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL),
> +        .pm1a_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
> +        .pm1a_cnt = { .space_id = as, .bit_width = 2 * 8,
> +                      .address = io + 0x04 },
> +        .pm_tmr = { .space_id = as, .bit_width = 4 * 8, .address = io + 0x08 },
> +        .gpe0_blk = { .space_id = as, .bit_width =
> +            object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK_LEN, NULL) * 8,
> +            .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
> +        },
> +    };
> +    *data = fadt;
> +}
> +
>  static void acpi_get_pm_info(AcpiPmInfo *pm)
>  {
>      Object *piix = piix4_pm_find();
>      Object *lpc = ich9_lpc_find();
>      Object *obj = piix ? piix : lpc;
>      QObject *o;
> -
> -    pm->force_rev1_fadt = false;
>      pm->cpu_hp_io_base = 0;
>      pm->pcihp_io_base = 0;
>      pm->pcihp_io_len = 0;
> +
> +    init_common_fadt_data(obj, &pm->fadt);
>      if (piix) {
>          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> -        pm->force_rev1_fadt = true;
> +        pm->fadt.rev = 1;
>          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>          pm->pcihp_io_base =
>              object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> @@ -145,10 +178,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>              object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>      }
>      if (lpc) {
> +        struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> +            .bit_width = 8, .address = ICH9_RST_CNT_IOPORT };
> +        pm->fadt.reset_reg = r;
> +        pm->fadt.reset_val = 0xf;
> +        pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
>          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
>      }
>      assert(obj);
>  
> +    /* The above need not be conditional on machine type because the reset port
> +     * happens to be the same on PIIX (pc) and ICH9 (q35). */
> +    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> +
>      /* Fill in optional s3/s4 related properties */
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>      if (o) {
> @@ -172,22 +214,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      }
>      qobject_decref(o);
>  
> -    /* Fill in mandatory properties */
> -    pm->sci_int = object_property_get_uint(obj, ACPI_PM_PROP_SCI_INT, NULL);
> -
> -    pm->acpi_enable_cmd = object_property_get_uint(obj,
> -                                                   ACPI_PM_PROP_ACPI_ENABLE_CMD,
> -                                                   NULL);
> -    pm->acpi_disable_cmd =
> -        object_property_get_uint(obj,
> -                                 ACPI_PM_PROP_ACPI_DISABLE_CMD,
> -                                 NULL);
> -    pm->io_base = object_property_get_uint(obj, ACPI_PM_PROP_PM_IO_BASE,
> -                                           NULL);
> -    pm->gpe0_blk = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK,
> -                                            NULL);
> -    pm->gpe0_blk_len = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> -                                                NULL);
>      pm->pcihp_bridge_en =
>          object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
>                                   NULL);
> @@ -273,73 +299,53 @@ build_facs(GArray *table_data, BIOSLinker *linker)
>  }
>  
>  /* Load chipset information in FADT */
> -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
>  {
> -    fadt->model = 1;
> +    fadt->model = f.int_model;
>      fadt->reserved1 = 0;
> -    fadt->sci_int = cpu_to_le16(pm->sci_int);
> -    fadt->smi_cmd = cpu_to_le32(ACPI_PORT_SMI_CMD);
> -    fadt->acpi_enable = pm->acpi_enable_cmd;
> -    fadt->acpi_disable = pm->acpi_disable_cmd;
> +    fadt->sci_int = cpu_to_le16(f.sci_int);
> +    fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
> +    fadt->acpi_enable = f.acpi_enable_cmd;
> +    fadt->acpi_disable = f.acpi_disable_cmd;
>      /* EVT, CNT, TMR offset matches hw/acpi/core.c */
> -    fadt->pm1a_evt_blk = cpu_to_le32(pm->io_base);
> -    fadt->pm1a_cnt_blk = cpu_to_le32(pm->io_base + 0x04);
> -    fadt->pm_tmr_blk = cpu_to_le32(pm->io_base + 0x08);
> -    fadt->gpe0_blk = cpu_to_le32(pm->gpe0_blk);
> +    fadt->pm1a_evt_blk = cpu_to_le32(f.pm1a_evt.address);
> +    fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1a_cnt.address);
> +    fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
> +    fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
>      /* EVT, CNT, TMR length matches hw/acpi/core.c */
> -    fadt->pm1_evt_len = 4;
> -    fadt->pm1_cnt_len = 2;
> -    fadt->pm_tmr_len = 4;
> -    fadt->gpe0_blk_len = pm->gpe0_blk_len;
> -    fadt->plvl2_lat = cpu_to_le16(0xfff); /* C2 state not supported */
> -    fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> -    fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> -                              (1 << ACPI_FADT_F_PROC_C1) |
> -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
> -                              (1 << ACPI_FADT_F_RTC_S4));
> -    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> -    /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> -     * For more than 8 CPUs, "Clustered Logical" mode has to be used
> -     */
> -    if (max_cpus > 8) {
> -        fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> -    }
> -    fadt->century = RTC_CENTURY;
> -    if (pm->force_rev1_fadt) {
> +    fadt->pm1_evt_len = f.pm1a_evt.bit_width / 8;
> +    fadt->pm1_cnt_len = f.pm1a_cnt.bit_width / 8;
> +    fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
> +    fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
> +    fadt->plvl2_lat = cpu_to_le16(f.plvl2_lat);
> +    fadt->plvl3_lat = cpu_to_le16(f.plvl3_lat);
> +    fadt->flags = cpu_to_le32(f.flags);
> +    fadt->century = f.rtc_century;
> +    if (f.rev == 1) {
>          return;
>      }
>  
> -    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> -    fadt->reset_value = 0xf;
> -    fadt->reset_register.space_id = AML_SYSTEM_IO;
> -    fadt->reset_register.bit_width = 8;
> -    fadt->reset_register.address = cpu_to_le64(ICH9_RST_CNT_IOPORT);
> -    /* The above need not be conditional on machine type because the reset port
> -     * happens to be the same on PIIX (pc) and ICH9 (q35). */
> -    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> +    fadt->reset_value = f.reset_val;
> +    fadt->reset_register = f.reset_reg;
> +    fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
>  
> -    fadt->xpm1a_event_block.space_id = AML_SYSTEM_IO;
> -    fadt->xpm1a_event_block.bit_width = fadt->pm1_evt_len * 8;
> -    fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
> +    fadt->xpm1a_event_block = f.pm1a_evt;
> +    fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1a_evt.address);
>  
> -    fadt->xpm1a_control_block.space_id = AML_SYSTEM_IO;
> -    fadt->xpm1a_control_block.bit_width = fadt->pm1_cnt_len * 8;
> -    fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x4);
> +    fadt->xpm1a_control_block = f.pm1a_cnt;
> +    fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1a_cnt.address);
>  
> -    fadt->xpm_timer_block.space_id = AML_SYSTEM_IO;
> -    fadt->xpm_timer_block.bit_width = fadt->pm_tmr_len * 8;
> -    fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x8);
> +    fadt->xpm_timer_block = f.pm_tmr;
> +    fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
>  
> -    fadt->xgpe0_block.space_id = AML_SYSTEM_IO;
> -    fadt->xgpe0_block.bit_width = pm->gpe0_blk_len * 8;
> -    fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
> +    fadt->xgpe0_block = f.gpe0_blk;
> +    fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
>  }
>  
>  
>  /* FADT */
>  static void
> -build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> -           unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> +build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
>             const char *oem_id, const char *oem_table_id)
>  {
>      AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> @@ -347,29 +353,29 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
>      unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
>      int fadt_size = sizeof(*fadt);
> -    int rev = 3;
>  
>      /* FACS address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
> -        ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> +        ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
>  
>      /* DSDT address to be filled by Guest linker */
> -    fadt_setup(fadt, pm);
> +    fadt_setup(fadt, *f);
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> -    if (pm->force_rev1_fadt) {
> -        rev = 1;
> +        ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
> +
> +    if (f->rev == 1) {
>          fadt_size = offsetof(typeof(*fadt), reset_register);
> -    } else {
> +    } else if (f->xdsdt_tbl_offset) {
I fail to understand why you added this check in this patch on not in
[PATCH v2 08/10] but anyway

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
>          bios_linker_loader_add_pointer(linker,
>              ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> -            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> +            ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
>      }
>  
>      build_header(linker, table_data,
> -                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> +                (void *)fadt, "FACP", fadt_size, f->rev,
> +                oem_id, oem_table_id);
>  }
>  
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> @@ -2049,7 +2055,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>      crs = aml_resource_template();
>      aml_append(crs,
> -        aml_io(AML_DECODE16, pm->gpe0_blk, pm->gpe0_blk, 1, pm->gpe0_blk_len)
> +        aml_io(
> +               AML_DECODE16,
> +               pm->fadt.gpe0_blk.address,
> +               pm->fadt.gpe0_blk.address,
> +               1,
> +               pm->fadt.gpe0_blk.bit_width / 8)
>      );
>      aml_append(dev, aml_name_decl("_CRS", crs));
>      aml_append(scope, dev);
> @@ -2696,7 +2707,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      /* ACPI tables pointed to by RSDT */
>      fadt = tables_blob->len;
>      acpi_add_table(table_offsets, tables_blob);
> -    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> +    pm.fadt.facs_tbl_offset = &facs;
> +    pm.fadt.dsdt_tbl_offset = &dsdt;
> +    pm.fadt.xdsdt_tbl_offset = &dsdt;
> +    build_fadt(tables_blob, tables->linker, &pm.fadt,
>                 slic_oem.id, slic_oem.table_id);
>      aml_len += tables_blob->len - fadt;
>  
> 

Re: [Qemu-devel] [PATCH v2 06/10] pc: acpi: isolate FADT specific data into AcpiFadtData structure

[Igor Mammedov]

On Thu, 1 Mar 2018 09:43:35 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Igor,
> 
> On 28/02/18 15:23, Igor Mammedov wrote:
> > move FADT data initialization out of fadt_setup() into dedicated
> > init_fadt_data() that will set common for pc/q35 values in
> > AcpiFadtData structure and acpi_get_pm_info() will complement
> > it with pc/q35 specific values initialization.
> > 
> > That will allow to get rid of fadt_setup() and generalize
> > build_fadt() so it could be easily extended for rev5 and
> > reused by ARM target.
> > 
> > While at it also move facs/dsdt/xdsdt offsets from build_fadt()
> > arg list into AcpiFadtData, as they belong to the same dataset.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> > ---
> > v2:
> > changes requested by Auger Eric <eric.auger@redhat.com>  
> suggested ;-)
> 
> >   - s/pm1_/pm1a_/
> >   - s/c2_latency/plvl2_lat/
> >   - s/c3_latency/plvl3_lat/
> >   - cleanup ACPI_PORT_SMI_CMD TODO, move  it to hw/isa/apm.h
> >   - move
> >        if (f->facs_tbl_offset) / if (f->dsdt_tbl_offset)
> >            bios_linker_loader_add_pointer(...)
> >     into the patch that makes build_fadt() generic
> > ---
> >  include/hw/acpi/acpi-defs.h |  28 +++++++
> >  hw/i386/acpi-build.c        | 190 ++++++++++++++++++++++++--------------------
> >  2 files changed, 130 insertions(+), 88 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 9942bc5..3fb0ace 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -175,6 +175,34 @@ struct AcpiFadtDescriptorRev5_1 {
> >  
> >  typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
> >  
> > +typedef struct AcpiFadtData {
> > +    struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
> > +    struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
> > +    struct AcpiGenericAddress pm_tmr;    /* PM_TMR_BLK */
> > +    struct AcpiGenericAddress gpe0_blk;  /* GPE0_BLK */
> > +    struct AcpiGenericAddress reset_reg; /* RESET_REG */
> > +    uint8_t reset_val;         /* RESET_VALUE */
> > +    uint8_t  rev;              /* Revision */
> > +    uint32_t flags;            /* Flags */
> > +    uint32_t smi_cmd;          /* SMI_CMD */
> > +    uint16_t sci_int;          /* SCI_INT */
> > +    uint8_t  int_model;        /* INT_MODEL */
> > +    uint8_t  acpi_enable_cmd;  /* ACPI_ENABLE */
> > +    uint8_t  acpi_disable_cmd; /* ACPI_DISABLE */
> > +    uint8_t  rtc_century;      /* CENTURY */
> > +    uint16_t plvl2_lat;        /* P_LVL2_LAT */
> > +    uint16_t plvl3_lat;        /* P_LVL3_LAT */
> > +
> > +    /*
> > +     * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> > +     * NULL if table doesn't exist (in that case field's value
> > +     * won't be patched by linker and will be kept set to 0)
> > +     */
> > +    unsigned *facs_tbl_offset; /* FACS offset in */
> > +    unsigned *dsdt_tbl_offset;
> > +    unsigned *xdsdt_tbl_offset;
> > +} AcpiFadtData;
> > +
> >  #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
> >  #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
> >  
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 699f3a0..1f88ed1 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -91,17 +91,11 @@ typedef struct AcpiMcfgInfo {
> >  } AcpiMcfgInfo;
> >  
> >  typedef struct AcpiPmInfo {
> > -    bool force_rev1_fadt;
> >      bool s3_disabled;
> >      bool s4_disabled;
> >      bool pcihp_bridge_en;
> >      uint8_t s4_val;
> > -    uint16_t sci_int;
> > -    uint8_t acpi_enable_cmd;
> > -    uint8_t acpi_disable_cmd;
> > -    uint32_t gpe0_blk;
> > -    uint32_t gpe0_blk_len;
> > -    uint32_t io_base;
> > +    AcpiFadtData fadt;
> >      uint16_t cpu_hp_io_base;
> >      uint16_t pcihp_io_base;
> >      uint16_t pcihp_io_len;
> > @@ -124,20 +118,59 @@ typedef struct AcpiBuildPciBusHotplugState {
> >      bool pcihp_bridge_en;
> >  } AcpiBuildPciBusHotplugState;
> >  
> > +static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> > +{
> > +    uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> > +    AmlAddressSpace as = AML_AS_SYSTEM_IO;
> > +    AcpiFadtData fadt = {
> > +        .rev = 3,
> > +        .flags =
> > +            (1 << ACPI_FADT_F_WBINVD) |
> > +            (1 << ACPI_FADT_F_PROC_C1) |
> > +            (1 << ACPI_FADT_F_SLP_BUTTON) |
> > +            (1 << ACPI_FADT_F_RTC_S4) |
> > +            (1 << ACPI_FADT_F_USE_PLATFORM_CLOCK) |
> > +            /* APIC destination mode ("Flat Logical") has an upper limit of 8
> > +             * CPUs for more than 8 CPUs, "Clustered Logical" mode has to be
> > +             * used
> > +             */
> > +            ((max_cpus > 8) ? (1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL) : 0),
> > +        .int_model = 1 /* Multiple APIC */,
> > +        .rtc_century = RTC_CENTURY,
> > +        .plvl2_lat = 0xfff /* C2 state not supported */,
> > +        .plvl3_lat = 0xfff /* C3 state not supported */,
> > +        .smi_cmd = ACPI_PORT_SMI_CMD,
> > +        .sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
> > +        .acpi_enable_cmd =
> > +            object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL),
> > +        .acpi_disable_cmd =
> > +            object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL),
> > +        .pm1a_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
> > +        .pm1a_cnt = { .space_id = as, .bit_width = 2 * 8,
> > +                      .address = io + 0x04 },
> > +        .pm_tmr = { .space_id = as, .bit_width = 4 * 8, .address = io + 0x08 },
> > +        .gpe0_blk = { .space_id = as, .bit_width =
> > +            object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK_LEN, NULL) * 8,
> > +            .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
> > +        },
> > +    };
> > +    *data = fadt;
> > +}
> > +
> >  static void acpi_get_pm_info(AcpiPmInfo *pm)
> >  {
> >      Object *piix = piix4_pm_find();
> >      Object *lpc = ich9_lpc_find();
> >      Object *obj = piix ? piix : lpc;
> >      QObject *o;
> > -
> > -    pm->force_rev1_fadt = false;
> >      pm->cpu_hp_io_base = 0;
> >      pm->pcihp_io_base = 0;
> >      pm->pcihp_io_len = 0;
> > +
> > +    init_common_fadt_data(obj, &pm->fadt);
> >      if (piix) {
> >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> > -        pm->force_rev1_fadt = true;
> > +        pm->fadt.rev = 1;
> >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> >          pm->pcihp_io_base =
> >              object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > @@ -145,10 +178,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >              object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >      }
> >      if (lpc) {
> > +        struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > +            .bit_width = 8, .address = ICH9_RST_CNT_IOPORT };
> > +        pm->fadt.reset_reg = r;
> > +        pm->fadt.reset_val = 0xf;
> > +        pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> >      }
> >      assert(obj);
> >  
> > +    /* The above need not be conditional on machine type because the reset port
> > +     * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > +    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> > +
> >      /* Fill in optional s3/s4 related properties */
> >      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
> >      if (o) {
> > @@ -172,22 +214,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >      }
> >      qobject_decref(o);
> >  
> > -    /* Fill in mandatory properties */
> > -    pm->sci_int = object_property_get_uint(obj, ACPI_PM_PROP_SCI_INT, NULL);
> > -
> > -    pm->acpi_enable_cmd = object_property_get_uint(obj,
> > -                                                   ACPI_PM_PROP_ACPI_ENABLE_CMD,
> > -                                                   NULL);
> > -    pm->acpi_disable_cmd =
> > -        object_property_get_uint(obj,
> > -                                 ACPI_PM_PROP_ACPI_DISABLE_CMD,
> > -                                 NULL);
> > -    pm->io_base = object_property_get_uint(obj, ACPI_PM_PROP_PM_IO_BASE,
> > -                                           NULL);
> > -    pm->gpe0_blk = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK,
> > -                                            NULL);
> > -    pm->gpe0_blk_len = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
> > -                                                NULL);
> >      pm->pcihp_bridge_en =
> >          object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> >                                   NULL);
> > @@ -273,73 +299,53 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> >  }
> >  
> >  /* Load chipset information in FADT */
> > -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
> >  {
> > -    fadt->model = 1;
> > +    fadt->model = f.int_model;
> >      fadt->reserved1 = 0;
> > -    fadt->sci_int = cpu_to_le16(pm->sci_int);
> > -    fadt->smi_cmd = cpu_to_le32(ACPI_PORT_SMI_CMD);
> > -    fadt->acpi_enable = pm->acpi_enable_cmd;
> > -    fadt->acpi_disable = pm->acpi_disable_cmd;
> > +    fadt->sci_int = cpu_to_le16(f.sci_int);
> > +    fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
> > +    fadt->acpi_enable = f.acpi_enable_cmd;
> > +    fadt->acpi_disable = f.acpi_disable_cmd;
> >      /* EVT, CNT, TMR offset matches hw/acpi/core.c */
> > -    fadt->pm1a_evt_blk = cpu_to_le32(pm->io_base);
> > -    fadt->pm1a_cnt_blk = cpu_to_le32(pm->io_base + 0x04);
> > -    fadt->pm_tmr_blk = cpu_to_le32(pm->io_base + 0x08);
> > -    fadt->gpe0_blk = cpu_to_le32(pm->gpe0_blk);
> > +    fadt->pm1a_evt_blk = cpu_to_le32(f.pm1a_evt.address);
> > +    fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1a_cnt.address);
> > +    fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
> > +    fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
> >      /* EVT, CNT, TMR length matches hw/acpi/core.c */
> > -    fadt->pm1_evt_len = 4;
> > -    fadt->pm1_cnt_len = 2;
> > -    fadt->pm_tmr_len = 4;
> > -    fadt->gpe0_blk_len = pm->gpe0_blk_len;
> > -    fadt->plvl2_lat = cpu_to_le16(0xfff); /* C2 state not supported */
> > -    fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> > -    fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> > -                              (1 << ACPI_FADT_F_PROC_C1) |
> > -                              (1 << ACPI_FADT_F_SLP_BUTTON) |
> > -                              (1 << ACPI_FADT_F_RTC_S4));
> > -    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> > -    /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> > -     * For more than 8 CPUs, "Clustered Logical" mode has to be used
> > -     */
> > -    if (max_cpus > 8) {
> > -        fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > -    }
> > -    fadt->century = RTC_CENTURY;
> > -    if (pm->force_rev1_fadt) {
> > +    fadt->pm1_evt_len = f.pm1a_evt.bit_width / 8;
> > +    fadt->pm1_cnt_len = f.pm1a_cnt.bit_width / 8;
> > +    fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
> > +    fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
> > +    fadt->plvl2_lat = cpu_to_le16(f.plvl2_lat);
> > +    fadt->plvl3_lat = cpu_to_le16(f.plvl3_lat);
> > +    fadt->flags = cpu_to_le32(f.flags);
> > +    fadt->century = f.rtc_century;
> > +    if (f.rev == 1) {
> >          return;
> >      }
> >  
> > -    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > -    fadt->reset_value = 0xf;
> > -    fadt->reset_register.space_id = AML_SYSTEM_IO;
> > -    fadt->reset_register.bit_width = 8;
> > -    fadt->reset_register.address = cpu_to_le64(ICH9_RST_CNT_IOPORT);
> > -    /* The above need not be conditional on machine type because the reset port
> > -     * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > -    QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> > +    fadt->reset_value = f.reset_val;
> > +    fadt->reset_register = f.reset_reg;
> > +    fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
> >  
> > -    fadt->xpm1a_event_block.space_id = AML_SYSTEM_IO;
> > -    fadt->xpm1a_event_block.bit_width = fadt->pm1_evt_len * 8;
> > -    fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
> > +    fadt->xpm1a_event_block = f.pm1a_evt;
> > +    fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1a_evt.address);
> >  
> > -    fadt->xpm1a_control_block.space_id = AML_SYSTEM_IO;
> > -    fadt->xpm1a_control_block.bit_width = fadt->pm1_cnt_len * 8;
> > -    fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x4);
> > +    fadt->xpm1a_control_block = f.pm1a_cnt;
> > +    fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1a_cnt.address);
> >  
> > -    fadt->xpm_timer_block.space_id = AML_SYSTEM_IO;
> > -    fadt->xpm_timer_block.bit_width = fadt->pm_tmr_len * 8;
> > -    fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x8);
> > +    fadt->xpm_timer_block = f.pm_tmr;
> > +    fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
> >  
> > -    fadt->xgpe0_block.space_id = AML_SYSTEM_IO;
> > -    fadt->xgpe0_block.bit_width = pm->gpe0_blk_len * 8;
> > -    fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
> > +    fadt->xgpe0_block = f.gpe0_blk;
> > +    fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
> >  }
> >  
> >  
> >  /* FADT */
> >  static void
> > -build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > -           unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> > +build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
> >             const char *oem_id, const char *oem_table_id)
> >  {
> >      AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> > @@ -347,29 +353,29 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> >      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> >      unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
> >      int fadt_size = sizeof(*fadt);
> > -    int rev = 3;
> >  
> >      /* FACS address to be filled by Guest linker */
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
> > -        ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> > +        ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
> >  
> >      /* DSDT address to be filled by Guest linker */
> > -    fadt_setup(fadt, pm);
> > +    fadt_setup(fadt, *f);
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > -    if (pm->force_rev1_fadt) {
> > -        rev = 1;
> > +        ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
> > +
> > +    if (f->rev == 1) {
> >          fadt_size = offsetof(typeof(*fadt), reset_register);
> > -    } else {
> > +    } else if (f->xdsdt_tbl_offset) {  
> I fail to understand why you added this check in this patch on not in
> [PATCH v2 08/10] but anyway
Yep, it's not related to this patch and should be in 8/10.
if I respin series, I'll move it there.

(condition is there is mostly for spec compliance (i.e. optional field)
but it is always true in current code, though if we ever make it NULL
in future it won't break and will still work as expected without
fixing build_fadt())

> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks!

> 
> Thanks
> 
> Eric
> >          bios_linker_loader_add_pointer(linker,
> >              ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> > -            ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > +            ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
> >      }
> >  
> >      build_header(linker, table_data,
> > -                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> > +                (void *)fadt, "FACP", fadt_size, f->rev,
> > +                oem_id, oem_table_id);
> >  }
> >  
> >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > @@ -2049,7 +2055,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> >      crs = aml_resource_template();
> >      aml_append(crs,
> > -        aml_io(AML_DECODE16, pm->gpe0_blk, pm->gpe0_blk, 1, pm->gpe0_blk_len)
> > +        aml_io(
> > +               AML_DECODE16,
> > +               pm->fadt.gpe0_blk.address,
> > +               pm->fadt.gpe0_blk.address,
> > +               1,
> > +               pm->fadt.gpe0_blk.bit_width / 8)
> >      );
> >      aml_append(dev, aml_name_decl("_CRS", crs));
> >      aml_append(scope, dev);
> > @@ -2696,7 +2707,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> >      /* ACPI tables pointed to by RSDT */
> >      fadt = tables_blob->len;
> >      acpi_add_table(table_offsets, tables_blob);
> > -    build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> > +    pm.fadt.facs_tbl_offset = &facs;
> > +    pm.fadt.dsdt_tbl_offset = &dsdt;
> > +    pm.fadt.xdsdt_tbl_offset = &dsdt;
> > +    build_fadt(tables_blob, tables->linker, &pm.fadt,
> >                 slic_oem.id, slic_oem.table_id);
> >      aml_len += tables_blob->len - fadt;
> >  
> >   

[Qemu-devel] [PATCH v2 07/10] pc: acpi: use build_append_foo() API to construct FADT

[Igor Mammedov]

build_append_foo() API doesn't need explicit endianness
conversions which eliminates a source of errors and
it makes build_fadt() look like declarative definition of
FADT table in ACPI spec, which makes it easy to review.
Also it allows easily extending FADT to support other
revisions which will be used by follow up patches
where build_fadt() will be reused for ARM target.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 hw/i386/acpi-build.c | 146 +++++++++++++++++++++++++++++----------------------
 1 file changed, 84 insertions(+), 62 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1f88ed1..d1b387e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -298,84 +298,106 @@ build_facs(GArray *table_data, BIOSLinker *linker)
     facs->length = cpu_to_le32(sizeof(*facs));
 }
 
-/* Load chipset information in FADT */
-static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
-{
-    fadt->model = f.int_model;
-    fadt->reserved1 = 0;
-    fadt->sci_int = cpu_to_le16(f.sci_int);
-    fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
-    fadt->acpi_enable = f.acpi_enable_cmd;
-    fadt->acpi_disable = f.acpi_disable_cmd;
-    /* EVT, CNT, TMR offset matches hw/acpi/core.c */
-    fadt->pm1a_evt_blk = cpu_to_le32(f.pm1a_evt.address);
-    fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1a_cnt.address);
-    fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
-    fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
-    /* EVT, CNT, TMR length matches hw/acpi/core.c */
-    fadt->pm1_evt_len = f.pm1a_evt.bit_width / 8;
-    fadt->pm1_cnt_len = f.pm1a_cnt.bit_width / 8;
-    fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
-    fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
-    fadt->plvl2_lat = cpu_to_le16(f.plvl2_lat);
-    fadt->plvl3_lat = cpu_to_le16(f.plvl3_lat);
-    fadt->flags = cpu_to_le32(f.flags);
-    fadt->century = f.rtc_century;
-    if (f.rev == 1) {
-        return;
-    }
-
-    fadt->reset_value = f.reset_val;
-    fadt->reset_register = f.reset_reg;
-    fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
-
-    fadt->xpm1a_event_block = f.pm1a_evt;
-    fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1a_evt.address);
-
-    fadt->xpm1a_control_block = f.pm1a_cnt;
-    fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1a_cnt.address);
-
-    fadt->xpm_timer_block = f.pm_tmr;
-    fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
-
-    fadt->xgpe0_block = f.gpe0_blk;
-    fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
-}
-
-
 /* FADT */
 static void
-build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
+build_fadt(GArray *tbl, BIOSLinker *linker, AcpiFadtData *f,
            const char *oem_id, const char *oem_table_id)
 {
-    AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data, sizeof(*fadt));
-    unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
-    unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
-    unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
-    int fadt_size = sizeof(*fadt);
+    int off;
+    int fadt_start = tbl->len;
 
-    /* FACS address to be filled by Guest linker */
+    acpi_data_push(tbl, sizeof(AcpiTableHeader));
+
+    /* FACS address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 4); /* FIRMWARE_CTRL */
     bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
+        ACPI_BUILD_TABLE_FILE, off, 4,
         ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
 
-    /* DSDT address to be filled by Guest linker */
-    fadt_setup(fadt, *f);
+    /* DSDT address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 4); /* DSDT */
     bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
+        ACPI_BUILD_TABLE_FILE, off, 4,
         ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
 
+    /* ACPI1.0: INT_MODEL, ACPI2.0+: Reserved */
+    build_append_int_noprefix(tbl, f->int_model /* Multiple APIC */, 1);
+    /* Preferred_PM_Profile */
+    build_append_int_noprefix(tbl, 0 /* Unspecified */, 1);
+    build_append_int_noprefix(tbl, f->sci_int, 2); /* SCI_INT */
+    build_append_int_noprefix(tbl, f->smi_cmd, 4); /* SMI_CMD */
+    build_append_int_noprefix(tbl, f->acpi_enable_cmd, 1); /* ACPI_ENABLE */
+    build_append_int_noprefix(tbl, f->acpi_disable_cmd, 1); /* ACPI_DISABLE */
+    build_append_int_noprefix(tbl, 0 /* not supported */, 1); /* S4BIOS_REQ */
+    /* ACPI1.0: Reserved, ACPI2.0+: PSTATE_CNT */
+    build_append_int_noprefix(tbl, 0, 1);
+    build_append_int_noprefix(tbl, f->pm1a_evt.address, 4); /* PM1a_EVT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM1b_EVT_BLK */
+    build_append_int_noprefix(tbl, f->pm1a_cnt.address, 4); /* PM1a_CNT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM1b_CNT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM2_CNT_BLK */
+    build_append_int_noprefix(tbl, f->pm_tmr.address, 4); /* PM_TMR_BLK */
+    build_append_int_noprefix(tbl, f->gpe0_blk.address, 4); /* GPE0_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* GPE1_BLK */
+    /* PM1_EVT_LEN */
+    build_append_int_noprefix(tbl, f->pm1a_evt.bit_width / 8, 1);
+    /* PM1_CNT_LEN */
+    build_append_int_noprefix(tbl, f->pm1a_cnt.bit_width / 8, 1);
+    build_append_int_noprefix(tbl, 0, 1); /* PM2_CNT_LEN */
+    build_append_int_noprefix(tbl, f->pm_tmr.bit_width / 8, 1); /* PM_TMR_LEN */
+    /* GPE0_BLK_LEN */
+    build_append_int_noprefix(tbl, f->gpe0_blk.bit_width / 8, 1);
+    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BLK_LEN */
+    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BASE */
+    build_append_int_noprefix(tbl, 0, 1); /* CST_CNT */
+    build_append_int_noprefix(tbl, f->plvl2_lat, 2); /* P_LVL2_LAT */
+    build_append_int_noprefix(tbl, f->plvl3_lat, 2); /* P_LVL3_LAT */
+    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_SIZE */
+    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_STRIDE */
+    build_append_int_noprefix(tbl, 0, 1); /* DUTY_OFFSET */
+    build_append_int_noprefix(tbl, 0, 1); /* DUTY_WIDTH */
+    build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
+    build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
+    build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
+    build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+    build_append_int_noprefix(tbl, 0, 1); /* Reserved */
+    build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
+
     if (f->rev == 1) {
-        fadt_size = offsetof(typeof(*fadt), reset_register);
-    } else if (f->xdsdt_tbl_offset) {
+        goto build_hdr;
+    }
+
+    build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
+    build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
+    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
+    build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
+
+    /* XDSDT address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 8); /* X_DSDT */
+    if (f->xdsdt_tbl_offset) {
         bios_linker_loader_add_pointer(linker,
-            ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
+            ACPI_BUILD_TABLE_FILE, off, 8,
             ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
     }
 
-    build_header(linker, table_data,
-                (void *)fadt, "FACP", fadt_size, f->rev,
-                oem_id, oem_table_id);
+    build_append_gas_from_struct(tbl, &f->pm1a_evt); /* X_PM1a_EVT_BLK */
+    /* X_PM1b_EVT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    build_append_gas_from_struct(tbl, &f->pm1a_cnt); /* X_PM1a_CNT_BLK */
+    /* X_PM1b_CNT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    /* X_PM2_CNT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    build_append_gas_from_struct(tbl, &f->pm_tmr); /* X_PM_TMR_BLK */
+    build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
+
+build_hdr:
+    build_header(linker, tbl, (void *)(tbl->data + fadt_start),
+                 "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
 }
 
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.7.4

[Qemu-devel] [PATCH v2 08/10] acpi: move build_fadt() from i386 specific to generic ACPI source

[Igor Mammedov]

It will be extended and reused by follow up patch for ARM target.

PS:
Since it's generic function now, don't patch FIRMWARE_CTRL, DSDT
fields if they don't point to tables since platform might not
provide them and use X_ variants instead if applicable.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
v2:
  - conditional FIRMWARE_CTRL, DSDT patching is introduced in this patch
    instead of earlier "pc: acpi: isolate FADT specific data into AcpiFadtData structure"
    as it better fits generalizing nature of this patch.
    (Auger Eric <eric.auger@redhat.com>)
---
 include/hw/acpi/aml-build.h |   3 ++
 hw/acpi/aml-build.c         | 105 ++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    |   6 +--
 hw/i386/acpi-build.c        | 102 ------------------------------------------
 4 files changed, 111 insertions(+), 105 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 8692ccc..6c36903 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -413,4 +413,7 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
 
 void build_slit(GArray *table_data, BIOSLinker *linker);
+
+void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
+                const char *oem_id, const char *oem_table_id);
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 3fef5f6..8f45298 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1678,3 +1678,108 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
                  "SLIT",
                  table_data->len - slit_start, 1, NULL, NULL);
 }
+
+/* build rev1/rev3 FADT */
+void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
+                const char *oem_id, const char *oem_table_id)
+{
+    int off;
+    int fadt_start = tbl->len;
+
+    acpi_data_push(tbl, sizeof(AcpiTableHeader));
+
+    /* FACS address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 4); /* FIRMWARE_CTRL */
+    if (f->facs_tbl_offset) { /* don't patch if not supported by platform */
+        bios_linker_loader_add_pointer(linker,
+            ACPI_BUILD_TABLE_FILE, off, 4,
+            ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
+    }
+
+    /* DSDT address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 4); /* DSDT */
+    if (f->dsdt_tbl_offset) { /* don't patch if not supported by platform */
+        bios_linker_loader_add_pointer(linker,
+            ACPI_BUILD_TABLE_FILE, off, 4,
+            ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
+    }
+
+    /* ACPI1.0: INT_MODEL, ACPI2.0+: Reserved */
+    build_append_int_noprefix(tbl, f->int_model /* Multiple APIC */, 1);
+    /* Preferred_PM_Profile */
+    build_append_int_noprefix(tbl, 0 /* Unspecified */, 1);
+    build_append_int_noprefix(tbl, f->sci_int, 2); /* SCI_INT */
+    build_append_int_noprefix(tbl, f->smi_cmd, 4); /* SMI_CMD */
+    build_append_int_noprefix(tbl, f->acpi_enable_cmd, 1); /* ACPI_ENABLE */
+    build_append_int_noprefix(tbl, f->acpi_disable_cmd, 1); /* ACPI_DISABLE */
+    build_append_int_noprefix(tbl, 0 /* not supported */, 1); /* S4BIOS_REQ */
+    /* ACPI1.0: Reserved, ACPI2.0+: PSTATE_CNT */
+    build_append_int_noprefix(tbl, 0, 1);
+    build_append_int_noprefix(tbl, f->pm1a_evt.address, 4); /* PM1a_EVT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM1b_EVT_BLK */
+    build_append_int_noprefix(tbl, f->pm1a_cnt.address, 4); /* PM1a_CNT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM1b_CNT_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* PM2_CNT_BLK */
+    build_append_int_noprefix(tbl, f->pm_tmr.address, 4); /* PM_TMR_BLK */
+    build_append_int_noprefix(tbl, f->gpe0_blk.address, 4); /* GPE0_BLK */
+    build_append_int_noprefix(tbl, 0, 4); /* GPE1_BLK */
+    /* PM1_EVT_LEN */
+    build_append_int_noprefix(tbl, f->pm1a_evt.bit_width / 8, 1);
+    /* PM1_CNT_LEN */
+    build_append_int_noprefix(tbl, f->pm1a_cnt.bit_width / 8, 1);
+    build_append_int_noprefix(tbl, 0, 1); /* PM2_CNT_LEN */
+    build_append_int_noprefix(tbl, f->pm_tmr.bit_width / 8, 1); /* PM_TMR_LEN */
+    /* GPE0_BLK_LEN */
+    build_append_int_noprefix(tbl, f->gpe0_blk.bit_width / 8, 1);
+    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BLK_LEN */
+    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BASE */
+    build_append_int_noprefix(tbl, 0, 1); /* CST_CNT */
+    build_append_int_noprefix(tbl, f->plvl2_lat, 2); /* P_LVL2_LAT */
+    build_append_int_noprefix(tbl, f->plvl3_lat, 2); /* P_LVL3_LAT */
+    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_SIZE */
+    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_STRIDE */
+    build_append_int_noprefix(tbl, 0, 1); /* DUTY_OFFSET */
+    build_append_int_noprefix(tbl, 0, 1); /* DUTY_WIDTH */
+    build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
+    build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
+    build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
+    build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+    build_append_int_noprefix(tbl, 0, 1); /* Reserved */
+    build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
+
+    if (f->rev == 1) {
+        goto build_hdr;
+    }
+
+    build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
+    build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
+    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
+    build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
+
+    /* XDSDT address to be filled by Guest linker at runtime */
+    off = tbl->len;
+    build_append_int_noprefix(tbl, 0, 8); /* X_DSDT */
+    if (f->xdsdt_tbl_offset) {
+        bios_linker_loader_add_pointer(linker,
+            ACPI_BUILD_TABLE_FILE, off, 8,
+            ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
+    }
+
+    build_append_gas_from_struct(tbl, &f->pm1a_evt); /* X_PM1a_EVT_BLK */
+    /* X_PM1b_EVT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    build_append_gas_from_struct(tbl, &f->pm1a_cnt); /* X_PM1a_CNT_BLK */
+    /* X_PM1b_CNT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    /* X_PM2_CNT_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    build_append_gas_from_struct(tbl, &f->pm_tmr); /* X_PM_TMR_BLK */
+    build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
+
+build_hdr:
+    build_header(linker, tbl, (void *)(tbl->data + fadt_start),
+                 "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
+}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f7fa795..b644da9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -651,8 +651,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 }
 
 /* FADT */
-static void build_fadt(GArray *table_data, BIOSLinker *linker,
-                       VirtMachineState *vms, unsigned dsdt_tbl_offset)
+static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
+                            VirtMachineState *vms, unsigned dsdt_tbl_offset)
 {
     int fadt_start = table_data->len;
     AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
@@ -761,7 +761,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
 
     /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
     acpi_add_table(table_offsets, tables_blob);
-    build_fadt(tables_blob, tables->linker, vms, dsdt);
+    build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
 
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d1b387e..ebde2cd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -298,108 +298,6 @@ build_facs(GArray *table_data, BIOSLinker *linker)
     facs->length = cpu_to_le32(sizeof(*facs));
 }
 
-/* FADT */
-static void
-build_fadt(GArray *tbl, BIOSLinker *linker, AcpiFadtData *f,
-           const char *oem_id, const char *oem_table_id)
-{
-    int off;
-    int fadt_start = tbl->len;
-
-    acpi_data_push(tbl, sizeof(AcpiTableHeader));
-
-    /* FACS address to be filled by Guest linker at runtime */
-    off = tbl->len;
-    build_append_int_noprefix(tbl, 0, 4); /* FIRMWARE_CTRL */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_TABLE_FILE, off, 4,
-        ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
-
-    /* DSDT address to be filled by Guest linker at runtime */
-    off = tbl->len;
-    build_append_int_noprefix(tbl, 0, 4); /* DSDT */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_TABLE_FILE, off, 4,
-        ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
-
-    /* ACPI1.0: INT_MODEL, ACPI2.0+: Reserved */
-    build_append_int_noprefix(tbl, f->int_model /* Multiple APIC */, 1);
-    /* Preferred_PM_Profile */
-    build_append_int_noprefix(tbl, 0 /* Unspecified */, 1);
-    build_append_int_noprefix(tbl, f->sci_int, 2); /* SCI_INT */
-    build_append_int_noprefix(tbl, f->smi_cmd, 4); /* SMI_CMD */
-    build_append_int_noprefix(tbl, f->acpi_enable_cmd, 1); /* ACPI_ENABLE */
-    build_append_int_noprefix(tbl, f->acpi_disable_cmd, 1); /* ACPI_DISABLE */
-    build_append_int_noprefix(tbl, 0 /* not supported */, 1); /* S4BIOS_REQ */
-    /* ACPI1.0: Reserved, ACPI2.0+: PSTATE_CNT */
-    build_append_int_noprefix(tbl, 0, 1);
-    build_append_int_noprefix(tbl, f->pm1a_evt.address, 4); /* PM1a_EVT_BLK */
-    build_append_int_noprefix(tbl, 0, 4); /* PM1b_EVT_BLK */
-    build_append_int_noprefix(tbl, f->pm1a_cnt.address, 4); /* PM1a_CNT_BLK */
-    build_append_int_noprefix(tbl, 0, 4); /* PM1b_CNT_BLK */
-    build_append_int_noprefix(tbl, 0, 4); /* PM2_CNT_BLK */
-    build_append_int_noprefix(tbl, f->pm_tmr.address, 4); /* PM_TMR_BLK */
-    build_append_int_noprefix(tbl, f->gpe0_blk.address, 4); /* GPE0_BLK */
-    build_append_int_noprefix(tbl, 0, 4); /* GPE1_BLK */
-    /* PM1_EVT_LEN */
-    build_append_int_noprefix(tbl, f->pm1a_evt.bit_width / 8, 1);
-    /* PM1_CNT_LEN */
-    build_append_int_noprefix(tbl, f->pm1a_cnt.bit_width / 8, 1);
-    build_append_int_noprefix(tbl, 0, 1); /* PM2_CNT_LEN */
-    build_append_int_noprefix(tbl, f->pm_tmr.bit_width / 8, 1); /* PM_TMR_LEN */
-    /* GPE0_BLK_LEN */
-    build_append_int_noprefix(tbl, f->gpe0_blk.bit_width / 8, 1);
-    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BLK_LEN */
-    build_append_int_noprefix(tbl, 0, 1); /* GPE1_BASE */
-    build_append_int_noprefix(tbl, 0, 1); /* CST_CNT */
-    build_append_int_noprefix(tbl, f->plvl2_lat, 2); /* P_LVL2_LAT */
-    build_append_int_noprefix(tbl, f->plvl3_lat, 2); /* P_LVL3_LAT */
-    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_SIZE */
-    build_append_int_noprefix(tbl, 0, 2); /* FLUSH_STRIDE */
-    build_append_int_noprefix(tbl, 0, 1); /* DUTY_OFFSET */
-    build_append_int_noprefix(tbl, 0, 1); /* DUTY_WIDTH */
-    build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
-    build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
-    build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
-    build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
-    build_append_int_noprefix(tbl, 0, 1); /* Reserved */
-    build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
-
-    if (f->rev == 1) {
-        goto build_hdr;
-    }
-
-    build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
-    build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
-    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
-    build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
-
-    /* XDSDT address to be filled by Guest linker at runtime */
-    off = tbl->len;
-    build_append_int_noprefix(tbl, 0, 8); /* X_DSDT */
-    if (f->xdsdt_tbl_offset) {
-        bios_linker_loader_add_pointer(linker,
-            ACPI_BUILD_TABLE_FILE, off, 8,
-            ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
-    }
-
-    build_append_gas_from_struct(tbl, &f->pm1a_evt); /* X_PM1a_EVT_BLK */
-    /* X_PM1b_EVT_BLK */
-    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
-    build_append_gas_from_struct(tbl, &f->pm1a_cnt); /* X_PM1a_CNT_BLK */
-    /* X_PM1b_CNT_BLK */
-    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
-    /* X_PM2_CNT_BLK */
-    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
-    build_append_gas_from_struct(tbl, &f->pm_tmr); /* X_PM_TMR_BLK */
-    build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
-    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
-
-build_hdr:
-    build_header(linker, tbl, (void *)(tbl->data + fadt_start),
-                 "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
-}
-
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry)
 {
-- 
2.7.4

[Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt()

[Igor Mammedov]

Extend generic build_fadt() to support rev5.1 FADT
and reuse it for 'virt' board, it would allow to
phase out usage of AcpiFadtDescriptorRev5_1 and
later ACPI_FADT_COMMON_DEF.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - update comment to mention that build_fadt() supports 5.1 revision
---
 include/hw/acpi/acpi-defs.h | 12 ++----------
 hw/acpi/aml-build.c         | 23 +++++++++++++++++++++--
 hw/arm/virt-acpi-build.c    | 33 ++++++++++++---------------------
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 3fb0ace..fe15e95 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 {
 } QEMU_PACKED;
 typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
 
-struct AcpiFadtDescriptorRev5_1 {
-    ACPI_FADT_COMMON_DEF
-    /* 64-bit Sleep Control register (ACPI 5.0) */
-    struct AcpiGenericAddress sleep_control;
-    /* 64-bit Sleep Status register (ACPI 5.0) */
-    struct AcpiGenericAddress sleep_status;
-} QEMU_PACKED;
-
-typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
-
 typedef struct AcpiFadtData {
     struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
     struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
@@ -192,6 +182,8 @@ typedef struct AcpiFadtData {
     uint8_t  rtc_century;      /* CENTURY */
     uint16_t plvl2_lat;        /* P_LVL2_LAT */
     uint16_t plvl3_lat;        /* P_LVL3_LAT */
+    uint16_t arm_boot_arch;    /* ARM_BOOT_ARCH */
+    uint8_t minor_ver;         /* FADT Minor Version */
 
     /*
      * respective tables offsets within ACPI_BUILD_TABLE_FILE,
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 8f45298..3fa557c 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1679,7 +1679,7 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
                  table_data->len - slit_start, 1, NULL, NULL);
 }
 
-/* build rev1/rev3 FADT */
+/* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
 {
@@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
 
     build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
     build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
-    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
+    /* Since ACPI 5.1 */
+    if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {
+        build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */
+        /* FADT Minor Version */
+        build_append_int_noprefix(tbl, f->minor_ver, 1);
+    } else {
+        build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
+    }
     build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
 
     /* XDSDT address to be filled by Guest linker at runtime */
@@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
     build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
     build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
 
+    if (f->rev <= 4) {
+        goto build_hdr;
+    }
+
+    /* SLEEP_CONTROL_REG */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+    /* SLEEP_STATUS_REG */
+    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+
+    /* TODO: extra fields need to be added to support revisions above rev5 */
+    assert(f->rev == 5);
+
 build_hdr:
     build_header(linker, tbl, (void *)(tbl->data + fadt_start),
                  "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b644da9..c7c6a57 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
                             VirtMachineState *vms, unsigned dsdt_tbl_offset)
 {
-    int fadt_start = table_data->len;
-    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
-    unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
-    uint16_t bootflags;
+    /* ACPI v5.1 */
+    AcpiFadtData fadt = {
+        .rev = 5,
+        .minor_ver = 1,
+        .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
+        .xdsdt_tbl_offset = &dsdt_tbl_offset,
+    };
 
     switch (vms->psci_conduit) {
     case QEMU_PSCI_CONDUIT_DISABLED:
-        bootflags = 0;
+        fadt.arm_boot_arch = 0;
         break;
     case QEMU_PSCI_CONDUIT_HVC:
-        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC;
+        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
+                             ACPI_FADT_ARM_PSCI_USE_HVC;
         break;
     case QEMU_PSCI_CONDUIT_SMC:
-        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT;
+        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
         break;
     default:
         g_assert_not_reached();
     }
 
-    /* Hardware Reduced = 1 and use PSCI 0.2+ */
-    fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
-    fadt->arm_boot_flags = cpu_to_le16(bootflags);
-
-    /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
-    fadt->minor_revision = 0x1;
-
-    /* DSDT address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
-        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
-
-    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
-                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
+    build_fadt(table_data, linker, &fadt, NULL, NULL);
 }
 
 /* DSDT */
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt()

[Auger Eric]

Hi,
On 28/02/18 15:23, Igor Mammedov wrote:
> Extend generic build_fadt() to support rev5.1 FADT
> and reuse it for 'virt' board, it would allow to
> phase out usage of AcpiFadtDescriptorRev5_1 and
> later ACPI_FADT_COMMON_DEF.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - update comment to mention that build_fadt() supports 5.1 revision
> ---
>  include/hw/acpi/acpi-defs.h | 12 ++----------
>  hw/acpi/aml-build.c         | 23 +++++++++++++++++++++--
>  hw/arm/virt-acpi-build.c    | 33 ++++++++++++---------------------
>  3 files changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 3fb0ace..fe15e95 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 {
>  } QEMU_PACKED;
>  typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
>  
> -struct AcpiFadtDescriptorRev5_1 {
> -    ACPI_FADT_COMMON_DEF
> -    /* 64-bit Sleep Control register (ACPI 5.0) */
> -    struct AcpiGenericAddress sleep_control;
> -    /* 64-bit Sleep Status register (ACPI 5.0) */
> -    struct AcpiGenericAddress sleep_status;
> -} QEMU_PACKED;
> -
> -typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
> -
>  typedef struct AcpiFadtData {
>      struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
>      struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
> @@ -192,6 +182,8 @@ typedef struct AcpiFadtData {
>      uint8_t  rtc_century;      /* CENTURY */
>      uint16_t plvl2_lat;        /* P_LVL2_LAT */
>      uint16_t plvl3_lat;        /* P_LVL3_LAT */
> +    uint16_t arm_boot_arch;    /* ARM_BOOT_ARCH */
> +    uint8_t minor_ver;         /* FADT Minor Version */
>  
>      /*
>       * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8f45298..3fa557c 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1679,7 +1679,7 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
>                   table_data->len - slit_start, 1, NULL, NULL);
>  }
>  
> -/* build rev1/rev3 FADT */
> +/* build rev1/rev3/rev5.1 FADT */
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id)
>  {
> @@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>  
>      build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
>      build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
> +    /* Since ACPI 5.1 */
> +    if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {
> +        build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */
> +        /* FADT Minor Version */
> +        build_append_int_noprefix(tbl, f->minor_ver, 1);
> +    } else {
> +        build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
> +    }
>      build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
>  
>      /* XDSDT address to be filled by Guest linker at runtime */
> @@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>      build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
>      build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
>  
> +    if (f->rev <= 4) {
> +        goto build_hdr;
> +    }
> +
> +    /* SLEEP_CONTROL_REG */
> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> +    /* SLEEP_STATUS_REG */
> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> +
> +    /* TODO: extra fields need to be added to support revisions above rev5 */
> +    assert(f->rev == 5);
My previous comment about asserting here if f->rev != 5 and previous
(f->rev >= 6) check was skipped but that's not a big deal.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> +
>  build_hdr:
>      build_header(linker, tbl, (void *)(tbl->data + fadt_start),
>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b644da9..c7c6a57 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>                              VirtMachineState *vms, unsigned dsdt_tbl_offset)
>  {
> -    int fadt_start = table_data->len;
> -    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> -    unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
> -    uint16_t bootflags;
> +    /* ACPI v5.1 */
> +    AcpiFadtData fadt = {
> +        .rev = 5,
> +        .minor_ver = 1,
> +        .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
> +        .xdsdt_tbl_offset = &dsdt_tbl_offset,
> +    };
>  
>      switch (vms->psci_conduit) {
>      case QEMU_PSCI_CONDUIT_DISABLED:
> -        bootflags = 0;
> +        fadt.arm_boot_arch = 0;
>          break;
>      case QEMU_PSCI_CONDUIT_HVC:
> -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC;
> +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
> +                             ACPI_FADT_ARM_PSCI_USE_HVC;
>          break;
>      case QEMU_PSCI_CONDUIT_SMC:
> -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT;
> +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
>          break;
>      default:
>          g_assert_not_reached();
>      }
>  
> -    /* Hardware Reduced = 1 and use PSCI 0.2+ */
> -    fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
> -    fadt->arm_boot_flags = cpu_to_le16(bootflags);
> -
> -    /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
> -    fadt->minor_revision = 0x1;
> -
> -    /* DSDT address to be filled by Guest linker */
> -    bios_linker_loader_add_pointer(linker,
> -        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> -
> -    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
> -                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
> +    build_fadt(table_data, linker, &fadt, NULL, NULL);
>  }
>  
>  /* DSDT */
> 

Re: [Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt()

[Igor Mammedov]

On Thu, 1 Mar 2018 09:51:14 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi,
> On 28/02/18 15:23, Igor Mammedov wrote:
> > Extend generic build_fadt() to support rev5.1 FADT
> > and reuse it for 'virt' board, it would allow to
> > phase out usage of AcpiFadtDescriptorRev5_1 and
> > later ACPI_FADT_COMMON_DEF.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   - update comment to mention that build_fadt() supports 5.1 revision
> > ---
> >  include/hw/acpi/acpi-defs.h | 12 ++----------
> >  hw/acpi/aml-build.c         | 23 +++++++++++++++++++++--
> >  hw/arm/virt-acpi-build.c    | 33 ++++++++++++---------------------
> >  3 files changed, 35 insertions(+), 33 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 3fb0ace..fe15e95 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 {
> >  } QEMU_PACKED;
> >  typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
> >  
> > -struct AcpiFadtDescriptorRev5_1 {
> > -    ACPI_FADT_COMMON_DEF
> > -    /* 64-bit Sleep Control register (ACPI 5.0) */
> > -    struct AcpiGenericAddress sleep_control;
> > -    /* 64-bit Sleep Status register (ACPI 5.0) */
> > -    struct AcpiGenericAddress sleep_status;
> > -} QEMU_PACKED;
> > -
> > -typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
> > -
> >  typedef struct AcpiFadtData {
> >      struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
> >      struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
> > @@ -192,6 +182,8 @@ typedef struct AcpiFadtData {
> >      uint8_t  rtc_century;      /* CENTURY */
> >      uint16_t plvl2_lat;        /* P_LVL2_LAT */
> >      uint16_t plvl3_lat;        /* P_LVL3_LAT */
> > +    uint16_t arm_boot_arch;    /* ARM_BOOT_ARCH */
> > +    uint8_t minor_ver;         /* FADT Minor Version */
> >  
> >      /*
> >       * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 8f45298..3fa557c 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1679,7 +1679,7 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
> >                   table_data->len - slit_start, 1, NULL, NULL);
> >  }
> >  
> > -/* build rev1/rev3 FADT */
> > +/* build rev1/rev3/rev5.1 FADT */
> >  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >                  const char *oem_id, const char *oem_table_id)
> >  {
> > @@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >  
> >      build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
> >      build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
> > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
> > +    /* Since ACPI 5.1 */
> > +    if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {
> > +        build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */
> > +        /* FADT Minor Version */
> > +        build_append_int_noprefix(tbl, f->minor_ver, 1);
> > +    } else {
> > +        build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
> > +    }
> >      build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
> >  
> >      /* XDSDT address to be filled by Guest linker at runtime */
> > @@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >      build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
> >      build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
> >  
> > +    if (f->rev <= 4) {
> > +        goto build_hdr;
> > +    }
> > +
> > +    /* SLEEP_CONTROL_REG */
> > +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> > +    /* SLEEP_STATUS_REG */
> > +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> > +
> > +    /* TODO: extra fields need to be added to support revisions above rev5 */
> > +    assert(f->rev == 5);  
> My previous comment about asserting here if f->rev != 5 and previous
> (f->rev >= 6) check was skipped but that's not a big deal.
I've thought that I've replied to comment,
maybe I just didn't get meaning behind question?
So I'll try to explain again.

If we reach this point rev must be 5, so f->rev >= 6 would be confusing
at this point. When v6 support is added this assert should be moved after
v6 stuff and condition is amended to f->rev == 6.

 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks!

> 
> Thanks
> 
> Eric
> > +
> >  build_hdr:
> >      build_header(linker, tbl, (void *)(tbl->data + fadt_start),
> >                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index b644da9..c7c6a57 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >  static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> >                              VirtMachineState *vms, unsigned dsdt_tbl_offset)
> >  {
> > -    int fadt_start = table_data->len;
> > -    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> > -    unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
> > -    uint16_t bootflags;
> > +    /* ACPI v5.1 */
> > +    AcpiFadtData fadt = {
> > +        .rev = 5,
> > +        .minor_ver = 1,
> > +        .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
> > +        .xdsdt_tbl_offset = &dsdt_tbl_offset,
> > +    };
> >  
> >      switch (vms->psci_conduit) {
> >      case QEMU_PSCI_CONDUIT_DISABLED:
> > -        bootflags = 0;
> > +        fadt.arm_boot_arch = 0;
> >          break;
> >      case QEMU_PSCI_CONDUIT_HVC:
> > -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC;
> > +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
> > +                             ACPI_FADT_ARM_PSCI_USE_HVC;
> >          break;
> >      case QEMU_PSCI_CONDUIT_SMC:
> > -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT;
> > +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
> >          break;
> >      default:
> >          g_assert_not_reached();
> >      }
> >  
> > -    /* Hardware Reduced = 1 and use PSCI 0.2+ */
> > -    fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
> > -    fadt->arm_boot_flags = cpu_to_le16(bootflags);
> > -
> > -    /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
> > -    fadt->minor_revision = 0x1;
> > -
> > -    /* DSDT address to be filled by Guest linker */
> > -    bios_linker_loader_add_pointer(linker,
> > -        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> > -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > -
> > -    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
> > -                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
> > +    build_fadt(table_data, linker, &fadt, NULL, NULL);
> >  }
> >  
> >  /* DSDT */
> >   

Re: [Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt()

[Auger Eric]

On 01/03/18 10:21, Igor Mammedov wrote:
> On Thu, 1 Mar 2018 09:51:14 +0100
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi,
>> On 28/02/18 15:23, Igor Mammedov wrote:
>>> Extend generic build_fadt() to support rev5.1 FADT
>>> and reuse it for 'virt' board, it would allow to
>>> phase out usage of AcpiFadtDescriptorRev5_1 and
>>> later ACPI_FADT_COMMON_DEF.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v2:
>>>   - update comment to mention that build_fadt() supports 5.1 revision
>>> ---
>>>  include/hw/acpi/acpi-defs.h | 12 ++----------
>>>  hw/acpi/aml-build.c         | 23 +++++++++++++++++++++--
>>>  hw/arm/virt-acpi-build.c    | 33 ++++++++++++---------------------
>>>  3 files changed, 35 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>>> index 3fb0ace..fe15e95 100644
>>> --- a/include/hw/acpi/acpi-defs.h
>>> +++ b/include/hw/acpi/acpi-defs.h
>>> @@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 {
>>>  } QEMU_PACKED;
>>>  typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
>>>  
>>> -struct AcpiFadtDescriptorRev5_1 {
>>> -    ACPI_FADT_COMMON_DEF
>>> -    /* 64-bit Sleep Control register (ACPI 5.0) */
>>> -    struct AcpiGenericAddress sleep_control;
>>> -    /* 64-bit Sleep Status register (ACPI 5.0) */
>>> -    struct AcpiGenericAddress sleep_status;
>>> -} QEMU_PACKED;
>>> -
>>> -typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
>>> -
>>>  typedef struct AcpiFadtData {
>>>      struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
>>>      struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
>>> @@ -192,6 +182,8 @@ typedef struct AcpiFadtData {
>>>      uint8_t  rtc_century;      /* CENTURY */
>>>      uint16_t plvl2_lat;        /* P_LVL2_LAT */
>>>      uint16_t plvl3_lat;        /* P_LVL3_LAT */
>>> +    uint16_t arm_boot_arch;    /* ARM_BOOT_ARCH */
>>> +    uint8_t minor_ver;         /* FADT Minor Version */
>>>  
>>>      /*
>>>       * respective tables offsets within ACPI_BUILD_TABLE_FILE,
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index 8f45298..3fa557c 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -1679,7 +1679,7 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
>>>                   table_data->len - slit_start, 1, NULL, NULL);
>>>  }
>>>  
>>> -/* build rev1/rev3 FADT */
>>> +/* build rev1/rev3/rev5.1 FADT */
>>>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>                  const char *oem_id, const char *oem_table_id)
>>>  {
>>> @@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>  
>>>      build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
>>>      build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
>>> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
I meant we could have assert(f->rev < 6) here
>>> +    /* Since ACPI 5.1 */
>>> +    if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {
and remove (f->rev >= 6) as in any case rev6 is not supported
(Hypervisor Vendor Identity field (8 byte) not duly added).

Or do I miss something?

Thanks

Eric
>>> +        build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */
>>> +        /* FADT Minor Version */
>>> +        build_append_int_noprefix(tbl, f->minor_ver, 1);
>>> +    } else {
>>> +        build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
>>> +    }
>>>      build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
>>>  
>>>      /* XDSDT address to be filled by Guest linker at runtime */
>>> @@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>>      build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
>>>      build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
>>>  
>>> +    if (f->rev <= 4) {
>>> +        goto build_hdr;
>>> +    }
>>> +
>>> +    /* SLEEP_CONTROL_REG */
>>> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
>>> +    /* SLEEP_STATUS_REG */
>>> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
>>> +
>>> +    /* TODO: extra fields need to be added to support revisions above rev5 */
>>> +    assert(f->rev == 5);  
>> My previous comment about asserting here if f->rev != 5 and previous
>> (f->rev >= 6) check was skipped but that's not a big deal.
> I've thought that I've replied to comment,
> maybe I just didn't get meaning behind question?
> So I'll try to explain again.
> 
> If we reach this point rev must be 5, so f->rev >= 6 would be confusing
> at this point. When v6 support is added this assert should be moved after
> v6 stuff and condition is amended to f->rev == 6.
> 
>  
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Thanks!
> 
>>
>> Thanks
>>
>> Eric
>>> +
>>>  build_hdr:
>>>      build_header(linker, tbl, (void *)(tbl->data + fadt_start),
>>>                   "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index b644da9..c7c6a57 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>  static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>>>                              VirtMachineState *vms, unsigned dsdt_tbl_offset)
>>>  {
>>> -    int fadt_start = table_data->len;
>>> -    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
>>> -    unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
>>> -    uint16_t bootflags;
>>> +    /* ACPI v5.1 */
>>> +    AcpiFadtData fadt = {
>>> +        .rev = 5,
>>> +        .minor_ver = 1,
>>> +        .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
>>> +        .xdsdt_tbl_offset = &dsdt_tbl_offset,
>>> +    };
>>>  
>>>      switch (vms->psci_conduit) {
>>>      case QEMU_PSCI_CONDUIT_DISABLED:
>>> -        bootflags = 0;
>>> +        fadt.arm_boot_arch = 0;
>>>          break;
>>>      case QEMU_PSCI_CONDUIT_HVC:
>>> -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC;
>>> +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
>>> +                             ACPI_FADT_ARM_PSCI_USE_HVC;
>>>          break;
>>>      case QEMU_PSCI_CONDUIT_SMC:
>>> -        bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT;
>>> +        fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
>>>          break;
>>>      default:
>>>          g_assert_not_reached();
>>>      }
>>>  
>>> -    /* Hardware Reduced = 1 and use PSCI 0.2+ */
>>> -    fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
>>> -    fadt->arm_boot_flags = cpu_to_le16(bootflags);
>>> -
>>> -    /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
>>> -    fadt->minor_revision = 0x1;
>>> -
>>> -    /* DSDT address to be filled by Guest linker */
>>> -    bios_linker_loader_add_pointer(linker,
>>> -        ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
>>> -        ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>>> -
>>> -    build_header(linker, table_data, (void *)(table_data->data + fadt_start),
>>> -                 "FACP", table_data->len - fadt_start, 5, NULL, NULL);
>>> +    build_fadt(table_data, linker, &fadt, NULL, NULL);
>>>  }
>>>  
>>>  /* DSDT */
>>>   
> 

Re: [Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt()

[Igor Mammedov]

On Thu, 1 Mar 2018 10:38:52 +0100
Auger Eric <eric.auger@redhat.com> wrote:

[...]
> >>>  {
> >>> @@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >>>  
> >>>      build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
> >>>      build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
> >>> -    build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */  
> I meant we could have assert(f->rev < 6) here
> >>> +    /* Since ACPI 5.1 */
> >>> +    if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {  
> and remove (f->rev >= 6) as in any case rev6 is not supported
> (Hypervisor Vendor Identity field (8 byte) not duly added).
> 
> Or do I miss something?
Nope, you are right.
I've missed 'code above' part and didn't get suggestion the first time.

The reason why I didn't assert here is to keep this part
(in the middle of table) working correctly without need
to touch it when later revisions will add new fields at
the end of table. So all we would have to do is to add
new code at the end and update assert that sets revision limit.

condition here essentially implements specs, i.e. 
from 5.1 onward reserved space should be used for now defined fields.

We might add similar conditions, for other 'Reserved' fields,
in the future if new revisions start to use them.

> Thanks
> 
> Eric
> >>> +        build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */
> >>> +        /* FADT Minor Version */
> >>> +        build_append_int_noprefix(tbl, f->minor_ver, 1);
> >>> +    } else {
> >>> +        build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
> >>> +    }
> >>>      build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
> >>>  
> >>>      /* XDSDT address to be filled by Guest linker at runtime */
> >>> @@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >>>      build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
> >>>      build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
> >>>  
> >>> +    if (f->rev <= 4) {
> >>> +        goto build_hdr;
> >>> +    }
> >>> +
> >>> +    /* SLEEP_CONTROL_REG */
> >>> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> >>> +    /* SLEEP_STATUS_REG */
> >>> +    build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
> >>> +
> >>> +    /* TODO: extra fields need to be added to support revisions above rev5 */
> >>> +    assert(f->rev == 5);    
> >> My previous comment about asserting here if f->rev != 5 and previous
> >> (f->rev >= 6) check was skipped but that's not a big deal.  
> > I've thought that I've replied to comment,
> > maybe I just didn't get meaning behind question?
> > So I'll try to explain again.
> > 
> > If we reach this point rev must be 5, so f->rev >= 6 would be confusing
> > at this point. When v6 support is added this assert should be moved after
> > v6 stuff and condition is amended to f->rev == 6.
> > 
> >    
> >> Reviewed-by: Eric Auger <eric.auger@redhat.com>  
> > Thanks!
> >   
> >>
> >> Thanks
> >>
> >> Eric  
[...]

[Qemu-devel] [PATCH v2 10/10] tests: acpi: don't read all fields in test_acpi_fadt_table()

[Igor Mammedov]

there is no point to read fields here but not actually
checking them so drop it and read only header + dsdt/facs
addresses since it's needed later to fetch that tables.

With this cleanup we can get rid of AcpiFadtDescriptorRev3/
ACPI_FADT_COMMON_DEF which have no users left.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/acpi-defs.h | 81 --------------------------------------------
 tests/bios-tables-test.c    | 82 ++++++++++-----------------------------------
 2 files changed, 18 insertions(+), 145 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index fe15e95..5955eb4 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -75,82 +75,6 @@ struct AcpiTableHeader {
 } QEMU_PACKED;
 typedef struct AcpiTableHeader AcpiTableHeader;
 
-/*
- * ACPI Fixed ACPI Description Table (FADT)
- */
-#define ACPI_FADT_COMMON_DEF /* FADT common definition */ \
-    ACPI_TABLE_HEADER_DEF    /* ACPI common table header */ \
-    uint32_t firmware_ctrl;  /* Physical address of FACS */ \
-    uint32_t dsdt;         /* Physical address of DSDT */ \
-    uint8_t  model;        /* System Interrupt Model */ \
-    uint8_t  reserved1;    /* Reserved */ \
-    uint16_t sci_int;      /* System vector of SCI interrupt */ \
-    uint32_t smi_cmd;      /* Port address of SMI command port */ \
-    uint8_t  acpi_enable;  /* Value to write to smi_cmd to enable ACPI */ \
-    uint8_t  acpi_disable; /* Value to write to smi_cmd to disable ACPI */ \
-    /* Value to write to SMI CMD to enter S4BIOS state */ \
-    uint8_t  S4bios_req; \
-    uint8_t  reserved2;    /* Reserved - must be zero */ \
-    /* Port address of Power Mgt 1a acpi_event Reg Blk */ \
-    uint32_t pm1a_evt_blk; \
-    /* Port address of Power Mgt 1b acpi_event Reg Blk */ \
-    uint32_t pm1b_evt_blk; \
-    uint32_t pm1a_cnt_blk; /* Port address of Power Mgt 1a Control Reg Blk */ \
-    uint32_t pm1b_cnt_blk; /* Port address of Power Mgt 1b Control Reg Blk */ \
-    uint32_t pm2_cnt_blk;  /* Port address of Power Mgt 2 Control Reg Blk */ \
-    uint32_t pm_tmr_blk;   /* Port address of Power Mgt Timer Ctrl Reg Blk */ \
-    /* Port addr of General Purpose acpi_event 0 Reg Blk */ \
-    uint32_t gpe0_blk; \
-    /* Port addr of General Purpose acpi_event 1 Reg Blk */ \
-    uint32_t gpe1_blk; \
-    uint8_t  pm1_evt_len;  /* Byte length of ports at pm1_x_evt_blk */ \
-    uint8_t  pm1_cnt_len;  /* Byte length of ports at pm1_x_cnt_blk */ \
-    uint8_t  pm2_cnt_len;  /* Byte Length of ports at pm2_cnt_blk */ \
-    uint8_t  pm_tmr_len;   /* Byte Length of ports at pm_tm_blk */ \
-    uint8_t  gpe0_blk_len; /* Byte Length of ports at gpe0_blk */ \
-    uint8_t  gpe1_blk_len; /* Byte Length of ports at gpe1_blk */ \
-    uint8_t  gpe1_base;    /* Offset in gpe model where gpe1 events start */ \
-    uint8_t  reserved3;    /* Reserved */ \
-    uint16_t plvl2_lat;    /* Worst case HW latency to enter/exit C2 state */ \
-    uint16_t plvl3_lat;    /* Worst case HW latency to enter/exit C3 state */ \
-    uint16_t flush_size;   /* Size of area read to flush caches */ \
-    uint16_t flush_stride; /* Stride used in flushing caches */ \
-    uint8_t  duty_offset;  /* Bit location of duty cycle field in p_cnt reg */ \
-    uint8_t  duty_width;   /* Bit width of duty cycle field in p_cnt reg */ \
-    uint8_t  day_alrm;     /* Index to day-of-month alarm in RTC CMOS RAM */ \
-    uint8_t  mon_alrm;     /* Index to month-of-year alarm in RTC CMOS RAM */ \
-    uint8_t  century;      /* Index to century in RTC CMOS RAM */ \
-    /* IA-PC Boot Architecture Flags (see below for individual flags) */ \
-    uint16_t boot_flags; \
-    uint8_t reserved;    /* Reserved, must be zero */ \
-    /* Miscellaneous flag bits (see below for individual flags) */ \
-    uint32_t flags; \
-    /* 64-bit address of the Reset register */ \
-    struct AcpiGenericAddress reset_register; \
-    /* Value to write to the reset_register port to reset the system */ \
-    uint8_t reset_value; \
-    /* ARM-Specific Boot Flags (see below for individual flags) (ACPI 5.1) */ \
-    uint16_t arm_boot_flags; \
-    uint8_t minor_revision;  /* FADT Minor Revision (ACPI 5.1) */ \
-    uint64_t x_facs;          /* 64-bit physical address of FACS */ \
-    uint64_t x_dsdt;          /* 64-bit physical address of DSDT */ \
-    /* 64-bit Extended Power Mgt 1a Event Reg Blk address */ \
-    struct AcpiGenericAddress xpm1a_event_block; \
-    /* 64-bit Extended Power Mgt 1b Event Reg Blk address */ \
-    struct AcpiGenericAddress xpm1b_event_block; \
-    /* 64-bit Extended Power Mgt 1a Control Reg Blk address */ \
-    struct AcpiGenericAddress xpm1a_control_block; \
-    /* 64-bit Extended Power Mgt 1b Control Reg Blk address */ \
-    struct AcpiGenericAddress xpm1b_control_block; \
-    /* 64-bit Extended Power Mgt 2 Control Reg Blk address */ \
-    struct AcpiGenericAddress xpm2_control_block; \
-    /* 64-bit Extended Power Mgt Timer Ctrl Reg Blk address */ \
-    struct AcpiGenericAddress xpm_timer_block; \
-    /* 64-bit Extended General Purpose Event 0 Reg Blk address */ \
-    struct AcpiGenericAddress xgpe0_block; \
-    /* 64-bit Extended General Purpose Event 1 Reg Blk address */ \
-    struct AcpiGenericAddress xgpe1_block; \
-
 struct AcpiGenericAddress {
     uint8_t space_id;        /* Address space where struct or register exists */
     uint8_t bit_width;       /* Size in bits of given register */
@@ -160,11 +84,6 @@ struct AcpiGenericAddress {
     uint64_t address;        /* 64-bit address of struct or register */
 } QEMU_PACKED;
 
-struct AcpiFadtDescriptorRev3 {
-    ACPI_FADT_COMMON_DEF
-} QEMU_PACKED;
-typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
-
 typedef struct AcpiFadtData {
     struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
     struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 65b271a..cd753ff 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -29,7 +29,8 @@ typedef struct {
     uint32_t rsdp_addr;
     AcpiRsdpDescriptor rsdp_table;
     AcpiRsdtDescriptorRev1 rsdt_table;
-    AcpiFadtDescriptorRev3 fadt_table;
+    uint32_t dsdt_addr;
+    uint32_t facs_addr;
     AcpiFacsDescriptorRev1 facs_table;
     uint32_t *rsdt_tables_addr;
     int rsdt_tables_nr;
@@ -127,71 +128,18 @@ static void test_acpi_rsdt_table(test_data *data)
     data->rsdt_tables_nr = tables_nr;
 }
 
-static void test_acpi_fadt_table(test_data *data)
+static void fadt_fetch_facs_and_dsdt_ptrs(test_data *data)
 {
-    AcpiFadtDescriptorRev3 *fadt_table = &data->fadt_table;
     uint32_t addr;
+    AcpiTableHeader hdr;
 
     /* FADT table comes first */
     addr = le32_to_cpu(data->rsdt_tables_addr[0]);
-    ACPI_READ_TABLE_HEADER(fadt_table, addr);
-
-    ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr);
-    ACPI_READ_FIELD(fadt_table->dsdt, addr);
-    ACPI_READ_FIELD(fadt_table->model, addr);
-    ACPI_READ_FIELD(fadt_table->reserved1, addr);
-    ACPI_READ_FIELD(fadt_table->sci_int, addr);
-    ACPI_READ_FIELD(fadt_table->smi_cmd, addr);
-    ACPI_READ_FIELD(fadt_table->acpi_enable, addr);
-    ACPI_READ_FIELD(fadt_table->acpi_disable, addr);
-    ACPI_READ_FIELD(fadt_table->S4bios_req, addr);
-    ACPI_READ_FIELD(fadt_table->reserved2, addr);
-    ACPI_READ_FIELD(fadt_table->pm1a_evt_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm1b_evt_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm1a_cnt_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm1b_cnt_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm2_cnt_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm_tmr_blk, addr);
-    ACPI_READ_FIELD(fadt_table->gpe0_blk, addr);
-    ACPI_READ_FIELD(fadt_table->gpe1_blk, addr);
-    ACPI_READ_FIELD(fadt_table->pm1_evt_len, addr);
-    ACPI_READ_FIELD(fadt_table->pm1_cnt_len, addr);
-    ACPI_READ_FIELD(fadt_table->pm2_cnt_len, addr);
-    ACPI_READ_FIELD(fadt_table->pm_tmr_len, addr);
-    ACPI_READ_FIELD(fadt_table->gpe0_blk_len, addr);
-    ACPI_READ_FIELD(fadt_table->gpe1_blk_len, addr);
-    ACPI_READ_FIELD(fadt_table->gpe1_base, addr);
-    ACPI_READ_FIELD(fadt_table->reserved3, addr);
-    ACPI_READ_FIELD(fadt_table->plvl2_lat, addr);
-    ACPI_READ_FIELD(fadt_table->plvl3_lat, addr);
-    ACPI_READ_FIELD(fadt_table->flush_size, addr);
-    ACPI_READ_FIELD(fadt_table->flush_stride, addr);
-    ACPI_READ_FIELD(fadt_table->duty_offset, addr);
-    ACPI_READ_FIELD(fadt_table->duty_width, addr);
-    ACPI_READ_FIELD(fadt_table->day_alrm, addr);
-    ACPI_READ_FIELD(fadt_table->mon_alrm, addr);
-    ACPI_READ_FIELD(fadt_table->century, addr);
-    ACPI_READ_FIELD(fadt_table->boot_flags, addr);
-    ACPI_READ_FIELD(fadt_table->reserved, addr);
-    ACPI_READ_FIELD(fadt_table->flags, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->reset_register, addr);
-    ACPI_READ_FIELD(fadt_table->reset_value, addr);
-    ACPI_READ_FIELD(fadt_table->arm_boot_flags, addr);
-    ACPI_READ_FIELD(fadt_table->minor_revision, addr);
-    ACPI_READ_FIELD(fadt_table->x_facs, addr);
-    ACPI_READ_FIELD(fadt_table->x_dsdt, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_event_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_event_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_control_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_control_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm2_control_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm_timer_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe0_block, addr);
-    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr);
-
-    ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
-    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table,
-                                 le32_to_cpu(fadt_table->length)));
+    ACPI_READ_TABLE_HEADER(&hdr, addr);
+    ACPI_ASSERT_CMP(hdr.signature, "FACP");
+
+    ACPI_READ_FIELD(data->facs_addr, addr);
+    ACPI_READ_FIELD(data->dsdt_addr, addr);
 }
 
 static void sanitize_fadt_ptrs(test_data *data)
@@ -206,6 +154,12 @@ static void sanitize_fadt_ptrs(test_data *data)
             continue;
         }
 
+        /* check original FADT checksum before sanitizing table */
+        g_assert(!(uint8_t)(
+            acpi_calc_checksum((uint8_t *)sdt, sizeof(AcpiTableHeader)) +
+            acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len)
+        ));
+
         /* sdt->aml field offset := spec offset - header size */
         memset(sdt->aml + 0, 0, 4); /* sanitize FIRMWARE_CTRL(36) ptr */
         memset(sdt->aml + 4, 0, 4); /* sanitize DSDT(40) ptr */
@@ -226,7 +180,7 @@ static void sanitize_fadt_ptrs(test_data *data)
 static void test_acpi_facs_table(test_data *data)
 {
     AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
-    uint32_t addr = le32_to_cpu(data->fadt_table.firmware_ctrl);
+    uint32_t addr = le32_to_cpu(data->facs_addr);
 
     ACPI_READ_FIELD(facs_table->signature, addr);
     ACPI_READ_FIELD(facs_table->length, addr);
@@ -265,7 +219,7 @@ static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr)
 static void test_acpi_dsdt_table(test_data *data)
 {
     AcpiSdtTable dsdt_table;
-    uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
+    uint32_t addr = le32_to_cpu(data->dsdt_addr);
 
     fetch_table(&dsdt_table, addr);
     ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
@@ -674,7 +628,7 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rsdp_address(data);
     test_acpi_rsdp_table(data);
     test_acpi_rsdt_table(data);
-    test_acpi_fadt_table(data);
+    fadt_fetch_facs_and_dsdt_ptrs(data);
     test_acpi_facs_table(data);
     test_acpi_dsdt_table(data);
     fetch_rsdt_referenced_tables(data);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 10/10] tests: acpi: don't read all fields in test_acpi_fadt_table()

[Auger Eric]

Hi Igor,

On 28/02/18 15:23, Igor Mammedov wrote:
> there is no point to read fields here but not actually
> checking them so drop it and read only header + dsdt/facs
> addresses since it's needed later to fetch that tables.
> 
> With this cleanup we can get rid of AcpiFadtDescriptorRev3/
> ACPI_FADT_COMMON_DEF which have no users left.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
My only comment is you could have removed include/hw/acpi/acpi-defs.h
defs in a separate subsequent patch after updating the test stuff. Besides,

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/acpi/acpi-defs.h | 81 --------------------------------------------
>  tests/bios-tables-test.c    | 82 ++++++++++-----------------------------------
>  2 files changed, 18 insertions(+), 145 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index fe15e95..5955eb4 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -75,82 +75,6 @@ struct AcpiTableHeader {
>  } QEMU_PACKED;
>  typedef struct AcpiTableHeader AcpiTableHeader;
>  
> -/*
> - * ACPI Fixed ACPI Description Table (FADT)
> - */
> -#define ACPI_FADT_COMMON_DEF /* FADT common definition */ \
> -    ACPI_TABLE_HEADER_DEF    /* ACPI common table header */ \
> -    uint32_t firmware_ctrl;  /* Physical address of FACS */ \
> -    uint32_t dsdt;         /* Physical address of DSDT */ \
> -    uint8_t  model;        /* System Interrupt Model */ \
> -    uint8_t  reserved1;    /* Reserved */ \
> -    uint16_t sci_int;      /* System vector of SCI interrupt */ \
> -    uint32_t smi_cmd;      /* Port address of SMI command port */ \
> -    uint8_t  acpi_enable;  /* Value to write to smi_cmd to enable ACPI */ \
> -    uint8_t  acpi_disable; /* Value to write to smi_cmd to disable ACPI */ \
> -    /* Value to write to SMI CMD to enter S4BIOS state */ \
> -    uint8_t  S4bios_req; \
> -    uint8_t  reserved2;    /* Reserved - must be zero */ \
> -    /* Port address of Power Mgt 1a acpi_event Reg Blk */ \
> -    uint32_t pm1a_evt_blk; \
> -    /* Port address of Power Mgt 1b acpi_event Reg Blk */ \
> -    uint32_t pm1b_evt_blk; \
> -    uint32_t pm1a_cnt_blk; /* Port address of Power Mgt 1a Control Reg Blk */ \
> -    uint32_t pm1b_cnt_blk; /* Port address of Power Mgt 1b Control Reg Blk */ \
> -    uint32_t pm2_cnt_blk;  /* Port address of Power Mgt 2 Control Reg Blk */ \
> -    uint32_t pm_tmr_blk;   /* Port address of Power Mgt Timer Ctrl Reg Blk */ \
> -    /* Port addr of General Purpose acpi_event 0 Reg Blk */ \
> -    uint32_t gpe0_blk; \
> -    /* Port addr of General Purpose acpi_event 1 Reg Blk */ \
> -    uint32_t gpe1_blk; \
> -    uint8_t  pm1_evt_len;  /* Byte length of ports at pm1_x_evt_blk */ \
> -    uint8_t  pm1_cnt_len;  /* Byte length of ports at pm1_x_cnt_blk */ \
> -    uint8_t  pm2_cnt_len;  /* Byte Length of ports at pm2_cnt_blk */ \
> -    uint8_t  pm_tmr_len;   /* Byte Length of ports at pm_tm_blk */ \
> -    uint8_t  gpe0_blk_len; /* Byte Length of ports at gpe0_blk */ \
> -    uint8_t  gpe1_blk_len; /* Byte Length of ports at gpe1_blk */ \
> -    uint8_t  gpe1_base;    /* Offset in gpe model where gpe1 events start */ \
> -    uint8_t  reserved3;    /* Reserved */ \
> -    uint16_t plvl2_lat;    /* Worst case HW latency to enter/exit C2 state */ \
> -    uint16_t plvl3_lat;    /* Worst case HW latency to enter/exit C3 state */ \
> -    uint16_t flush_size;   /* Size of area read to flush caches */ \
> -    uint16_t flush_stride; /* Stride used in flushing caches */ \
> -    uint8_t  duty_offset;  /* Bit location of duty cycle field in p_cnt reg */ \
> -    uint8_t  duty_width;   /* Bit width of duty cycle field in p_cnt reg */ \
> -    uint8_t  day_alrm;     /* Index to day-of-month alarm in RTC CMOS RAM */ \
> -    uint8_t  mon_alrm;     /* Index to month-of-year alarm in RTC CMOS RAM */ \
> -    uint8_t  century;      /* Index to century in RTC CMOS RAM */ \
> -    /* IA-PC Boot Architecture Flags (see below for individual flags) */ \
> -    uint16_t boot_flags; \
> -    uint8_t reserved;    /* Reserved, must be zero */ \
> -    /* Miscellaneous flag bits (see below for individual flags) */ \
> -    uint32_t flags; \
> -    /* 64-bit address of the Reset register */ \
> -    struct AcpiGenericAddress reset_register; \
> -    /* Value to write to the reset_register port to reset the system */ \
> -    uint8_t reset_value; \
> -    /* ARM-Specific Boot Flags (see below for individual flags) (ACPI 5.1) */ \
> -    uint16_t arm_boot_flags; \
> -    uint8_t minor_revision;  /* FADT Minor Revision (ACPI 5.1) */ \
> -    uint64_t x_facs;          /* 64-bit physical address of FACS */ \
> -    uint64_t x_dsdt;          /* 64-bit physical address of DSDT */ \
> -    /* 64-bit Extended Power Mgt 1a Event Reg Blk address */ \
> -    struct AcpiGenericAddress xpm1a_event_block; \
> -    /* 64-bit Extended Power Mgt 1b Event Reg Blk address */ \
> -    struct AcpiGenericAddress xpm1b_event_block; \
> -    /* 64-bit Extended Power Mgt 1a Control Reg Blk address */ \
> -    struct AcpiGenericAddress xpm1a_control_block; \
> -    /* 64-bit Extended Power Mgt 1b Control Reg Blk address */ \
> -    struct AcpiGenericAddress xpm1b_control_block; \
> -    /* 64-bit Extended Power Mgt 2 Control Reg Blk address */ \
> -    struct AcpiGenericAddress xpm2_control_block; \
> -    /* 64-bit Extended Power Mgt Timer Ctrl Reg Blk address */ \
> -    struct AcpiGenericAddress xpm_timer_block; \
> -    /* 64-bit Extended General Purpose Event 0 Reg Blk address */ \
> -    struct AcpiGenericAddress xgpe0_block; \
> -    /* 64-bit Extended General Purpose Event 1 Reg Blk address */ \
> -    struct AcpiGenericAddress xgpe1_block; \
> -
>  struct AcpiGenericAddress {
>      uint8_t space_id;        /* Address space where struct or register exists */
>      uint8_t bit_width;       /* Size in bits of given register */
> @@ -160,11 +84,6 @@ struct AcpiGenericAddress {
>      uint64_t address;        /* 64-bit address of struct or register */
>  } QEMU_PACKED;
>  
> -struct AcpiFadtDescriptorRev3 {
> -    ACPI_FADT_COMMON_DEF
> -} QEMU_PACKED;
> -typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
> -
>  typedef struct AcpiFadtData {
>      struct AcpiGenericAddress pm1a_cnt;   /* PM1a_CNT_BLK */
>      struct AcpiGenericAddress pm1a_evt;   /* PM1a_EVT_BLK */
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 65b271a..cd753ff 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -29,7 +29,8 @@ typedef struct {
>      uint32_t rsdp_addr;
>      AcpiRsdpDescriptor rsdp_table;
>      AcpiRsdtDescriptorRev1 rsdt_table;
> -    AcpiFadtDescriptorRev3 fadt_table;
> +    uint32_t dsdt_addr;
> +    uint32_t facs_addr;
>      AcpiFacsDescriptorRev1 facs_table;
>      uint32_t *rsdt_tables_addr;
>      int rsdt_tables_nr;
> @@ -127,71 +128,18 @@ static void test_acpi_rsdt_table(test_data *data)
>      data->rsdt_tables_nr = tables_nr;
>  }
>  
> -static void test_acpi_fadt_table(test_data *data)
> +static void fadt_fetch_facs_and_dsdt_ptrs(test_data *data)
>  {
> -    AcpiFadtDescriptorRev3 *fadt_table = &data->fadt_table;
>      uint32_t addr;
> +    AcpiTableHeader hdr;
>  
>      /* FADT table comes first */
>      addr = le32_to_cpu(data->rsdt_tables_addr[0]);
> -    ACPI_READ_TABLE_HEADER(fadt_table, addr);
> -
> -    ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr);
> -    ACPI_READ_FIELD(fadt_table->dsdt, addr);
> -    ACPI_READ_FIELD(fadt_table->model, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved1, addr);
> -    ACPI_READ_FIELD(fadt_table->sci_int, addr);
> -    ACPI_READ_FIELD(fadt_table->smi_cmd, addr);
> -    ACPI_READ_FIELD(fadt_table->acpi_enable, addr);
> -    ACPI_READ_FIELD(fadt_table->acpi_disable, addr);
> -    ACPI_READ_FIELD(fadt_table->S4bios_req, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved2, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1a_evt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1b_evt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1a_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1b_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm2_cnt_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm_tmr_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe0_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_blk, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1_evt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm1_cnt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm2_cnt_len, addr);
> -    ACPI_READ_FIELD(fadt_table->pm_tmr_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe0_blk_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_blk_len, addr);
> -    ACPI_READ_FIELD(fadt_table->gpe1_base, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved3, addr);
> -    ACPI_READ_FIELD(fadt_table->plvl2_lat, addr);
> -    ACPI_READ_FIELD(fadt_table->plvl3_lat, addr);
> -    ACPI_READ_FIELD(fadt_table->flush_size, addr);
> -    ACPI_READ_FIELD(fadt_table->flush_stride, addr);
> -    ACPI_READ_FIELD(fadt_table->duty_offset, addr);
> -    ACPI_READ_FIELD(fadt_table->duty_width, addr);
> -    ACPI_READ_FIELD(fadt_table->day_alrm, addr);
> -    ACPI_READ_FIELD(fadt_table->mon_alrm, addr);
> -    ACPI_READ_FIELD(fadt_table->century, addr);
> -    ACPI_READ_FIELD(fadt_table->boot_flags, addr);
> -    ACPI_READ_FIELD(fadt_table->reserved, addr);
> -    ACPI_READ_FIELD(fadt_table->flags, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->reset_register, addr);
> -    ACPI_READ_FIELD(fadt_table->reset_value, addr);
> -    ACPI_READ_FIELD(fadt_table->arm_boot_flags, addr);
> -    ACPI_READ_FIELD(fadt_table->minor_revision, addr);
> -    ACPI_READ_FIELD(fadt_table->x_facs, addr);
> -    ACPI_READ_FIELD(fadt_table->x_dsdt, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_event_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_event_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm2_control_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm_timer_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe0_block, addr);
> -    ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr);
> -
> -    ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
> -    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table,
> -                                 le32_to_cpu(fadt_table->length)));
> +    ACPI_READ_TABLE_HEADER(&hdr, addr);
> +    ACPI_ASSERT_CMP(hdr.signature, "FACP");
> +
> +    ACPI_READ_FIELD(data->facs_addr, addr);
> +    ACPI_READ_FIELD(data->dsdt_addr, addr);
>  }
>  
>  static void sanitize_fadt_ptrs(test_data *data)
> @@ -206,6 +154,12 @@ static void sanitize_fadt_ptrs(test_data *data)
>              continue;
>          }
>  
> +        /* check original FADT checksum before sanitizing table */
> +        g_assert(!(uint8_t)(
> +            acpi_calc_checksum((uint8_t *)sdt, sizeof(AcpiTableHeader)) +
> +            acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len)
> +        ));
> +
>          /* sdt->aml field offset := spec offset - header size */
>          memset(sdt->aml + 0, 0, 4); /* sanitize FIRMWARE_CTRL(36) ptr */
>          memset(sdt->aml + 4, 0, 4); /* sanitize DSDT(40) ptr */
> @@ -226,7 +180,7 @@ static void sanitize_fadt_ptrs(test_data *data)
>  static void test_acpi_facs_table(test_data *data)
>  {
>      AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
> -    uint32_t addr = le32_to_cpu(data->fadt_table.firmware_ctrl);
> +    uint32_t addr = le32_to_cpu(data->facs_addr);
>  
>      ACPI_READ_FIELD(facs_table->signature, addr);
>      ACPI_READ_FIELD(facs_table->length, addr);
> @@ -265,7 +219,7 @@ static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr)
>  static void test_acpi_dsdt_table(test_data *data)
>  {
>      AcpiSdtTable dsdt_table;
> -    uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
> +    uint32_t addr = le32_to_cpu(data->dsdt_addr);
>  
>      fetch_table(&dsdt_table, addr);
>      ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
> @@ -674,7 +628,7 @@ static void test_acpi_one(const char *params, test_data *data)
>      test_acpi_rsdp_address(data);
>      test_acpi_rsdp_table(data);
>      test_acpi_rsdt_table(data);
> -    test_acpi_fadt_table(data);
> +    fadt_fetch_facs_and_dsdt_ptrs(data);
>      test_acpi_facs_table(data);
>      test_acpi_dsdt_table(data);
>      fetch_rsdt_referenced_tables(data);
> 

Re: [Qemu-devel] [PATCH v2 00/10] generalize build_fadt()

[Auger Eric]

Hi,

On 28/02/18 15:23, Igor Mammedov wrote:
> 
> v2:
>   * fix typo in "acpi: remove unused acpi-dsdt.aml"
>   * split ACPI_PORT_SMI_CMD into separate cleanup patch
>   * s/pm1_/pm1a_/, s/c2_latency/plvl2_lat/, s/c3_latency/plvl3_lat/
>     and fix conflicts in followup patches caused by renaming
>   * conditional FIRMWARE_CTRL, DSDT patching is introduced in
>     the patch that makes build_fadt() generic instead of earlier
>     "pc: acpi: isolate FADT specific data into AcpiFadtData structure"
>   * update comment to mention that build_fadt() supports 5.1 revision
>   
> 
> 
> Series first cleanups ACPI code around build_fadt() and then
> converts current packed structure approach to a build_append_FOO()
> API, getting rid of error prone explicit endianness conversions
> in code and making build_fadt() look more like APCI table declaration
> from spec, which should be easier to read/maintain. After that
> build_fadt() becomes generic enough that we could drop ARM specific
> implementation and reuse generic build_fadt(), reducing code
> duplication.
> 
> PS: tested only x86 which has make check coverage,
>     ARM was only slightly tested. 

Tested-by: Eric Auger <eric.auger@redhat.com>
on AArch64 with a rhel guest.

Thanks

Eric

> 
> git tree for testing:
>   https://github.com/imammedo/qemu.git fadt_refactoring_v2
> 
> CC: "Michael S. Tsirkin" <mst@redhat.com> 
> CC: Shannon Zhao <zhaoshenglong@huawei.com> 
> CC: Auger Eric <eric.auger@redhat.com>
> CC: qemu-devel@nongnu.org 
> CC: qemu-arm@nongnu.org 
> 
> Igor Mammedov (10):
>   acpi: remove unused acpi-dsdt.aml
>   pc: replace pm object initialization with one-liner in
>     acpi_get_pm_info()
>   acpi: reuse AcpiGenericAddress instead of Acpi20GenericAddress
>   acpi: add build_append_gas() helper for Generic Address Structure
>   acpi: move ACPI_PORT_SMI_CMD define to header it belongs to
>   pc: acpi: isolate FADT specific data into AcpiFadtData structure
>   pc: acpi: use build_append_foo() API to construct FADT
>   acpi: move build_fadt() from i386 specific to generic ACPI source
>   virt_arm: acpi: reuse common build_fadt()
>   tests: acpi: don't read all fields in test_acpi_fadt_table()
> 
>  include/hw/acpi/acpi-defs.h | 136 ++++++++----------------------
>  include/hw/acpi/aml-build.h |  23 ++++++
>  include/hw/isa/apm.h        |   3 +
>  Makefile                    |   1 -
>  hw/acpi/aml-build.c         | 140 +++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    |  39 ++++-----
>  hw/i386/acpi-build.c        | 196 ++++++++++++++------------------------------
>  hw/isa/apm.c                |   1 -
>  pc-bios/acpi-dsdt.aml       | Bin 4405 -> 0 bytes
>  tests/bios-tables-test.c    |  82 ++++--------------
>  10 files changed, 294 insertions(+), 327 deletions(-)
>  delete mode 100644 pc-bios/acpi-dsdt.aml
>