[Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Igor Mammedov]

w2k used to boot on QEMU until revision of FADT has
been bumped to rev3
(commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)

Keep PC machine at rev1 to remain compatible and Q35
at rev3 where w2k isn't supported anyway so OSX could
run as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
   - make pc rev1 and q35 rev3.
       "Michael S. Tsirkin" <mst@redhat.com>

PS:
Only compile tested.


CC: Programmingkid <programmingkidx@gmail.com>
CC: Phil Dennis-Jordan <lists@philjordan.eu>
CC: "Michael S. Tsirkin" <mst@redhat.com>

---
 hw/i386/acpi-build.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade..b9c245c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
 } AcpiMcfgInfo;
 
 typedef struct AcpiPmInfo {
+    bool force_rev1_fadt;
     bool s3_disabled;
     bool s4_disabled;
     bool pcihp_bridge_en;
@@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *obj = NULL;
     QObject *o;
 
+    pm->force_rev1_fadt = false;
     pm->cpu_hp_io_base = 0;
     pm->pcihp_io_base = 0;
     pm->pcihp_io_len = 0;
     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 =
@@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
         fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
     }
     fadt->century = RTC_CENTURY;
+    if (pm->force_rev1_fadt) {
+        return;
+    }
 
     fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
     fadt->reset_value = 0xf;
@@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
     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 rev = 3;
 
     /* FACS address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
@@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
     bios_linker_loader_add_pointer(linker,
         ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
         ACPI_BUILD_TABLE_FILE, dsdt_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);
+    if (pm->force_rev1_fadt) {
+        rev = 1;
+        fadt_size = offsetof(typeof(*fadt), reset_register);
+    } else {
+        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 *)fadt, "FACP", sizeof(*fadt), 3, oem_id, oem_table_id);
+                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
 }
 
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Programmingkid]

> On Jul 24, 2017, at 9:50 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> w2k used to boot on QEMU until revision of FADT has
> been bumped to rev3
> (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> 
> Keep PC machine at rev1 to remain compatible and Q35
> at rev3 where w2k isn't supported anyway so OSX could
> run as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - make pc rev1 and q35 rev3.
>       "Michael S. Tsirkin" <mst@redhat.com>
> 
> PS:
> Only compile tested.
> 
> 
> CC: Programmingkid <programmingkidx@gmail.com>
> CC: Phil Dennis-Jordan <lists@philjordan.eu>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> 
> ---
> hw/i386/acpi-build.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade..b9c245c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> } AcpiMcfgInfo;
> 
> typedef struct AcpiPmInfo {
> +    bool force_rev1_fadt;
>     bool s3_disabled;
>     bool s4_disabled;
>     bool pcihp_bridge_en;
> @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>     Object *obj = NULL;
>     QObject *o;
> 
> +    pm->force_rev1_fadt = false;
>     pm->cpu_hp_io_base = 0;
>     pm->pcihp_io_base = 0;
>     pm->pcihp_io_len = 0;
>     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 =
> @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>         fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
>     }
>     fadt->century = RTC_CENTURY;
> +    if (pm->force_rev1_fadt) {
> +        return;
> +    }
> 
>     fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
>     fadt->reset_value = 0xf;
> @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>     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 rev = 3;
> 
>     /* FACS address to be filled by Guest linker */
>     bios_linker_loader_add_pointer(linker,
> @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>     bios_linker_loader_add_pointer(linker,
>         ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>         ACPI_BUILD_TABLE_FILE, dsdt_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);
> +    if (pm->force_rev1_fadt) {
> +        rev = 1;
> +        fadt_size = offsetof(typeof(*fadt), reset_register);
> +    } else {
> +        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 *)fadt, "FACP", sizeof(*fadt), 3, oem_id, oem_table_id);
> +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> }
> 
> void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> -- 
> 2.7.4
> 

Just booted up Windows 2000 with your patch. Thank you.

Tested-By: John Arbuckle <programmingkidx@gmail.com>

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Michael S. Tsirkin]

On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:
> w2k used to boot on QEMU until revision of FADT has
> been bumped to rev3
> (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> 
> Keep PC machine at rev1 to remain compatible and Q35
> at rev3 where w2k isn't supported anyway so OSX could
> run as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>    - make pc rev1 and q35 rev3.
>        "Michael S. Tsirkin" <mst@redhat.com>
> 
> PS:
> Only compile tested.
> 
> 
> CC: Programmingkid <programmingkidx@gmail.com>
> CC: Phil Dennis-Jordan <lists@philjordan.eu>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> 
> ---
>  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade..b9c245c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
>  } AcpiMcfgInfo;
>  
>  typedef struct AcpiPmInfo {
> +    bool force_rev1_fadt;
>      bool s3_disabled;
>      bool s4_disabled;
>      bool pcihp_bridge_en;
> @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      Object *obj = NULL;
>      QObject *o;
>  
> +    pm->force_rev1_fadt = false;
>      pm->cpu_hp_io_base = 0;
>      pm->pcihp_io_base = 0;
>      pm->pcihp_io_len = 0;
>      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 =
> @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
>          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
>      }
>      fadt->century = RTC_CENTURY;
> +    if (pm->force_rev1_fadt) {
> +        return;
> +    }
>  
>      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
>      fadt->reset_value = 0xf;
> @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>      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 rev = 3;
>  
>      /* FACS address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
> @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
>      bios_linker_loader_add_pointer(linker,
>          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>          ACPI_BUILD_TABLE_FILE, dsdt_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);
> +    if (pm->force_rev1_fadt) {
> +        rev = 1;
> +        fadt_size = offsetof(typeof(*fadt), reset_register);

I don't really like it that we are using structs to format fields - I
would prefer build_append_int_noprefix based APIs - but since we do,
let's add a proper structure for FADT rev 1 instead of this hack.

While we are at it, maybe we should replace macro hacks like
ACPI_FADT_COMMON_DEF with simple sub-structures (unless you see a reason
not to).


> +    } else {
> +        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 *)fadt, "FACP", sizeof(*fadt), 3, oem_id, oem_table_id);
> +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
>  }
>  
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Igor Mammedov]

On Tue, 25 Jul 2017 16:36:06 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:
> > w2k used to boot on QEMU until revision of FADT has
> > been bumped to rev3
> > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> > 
> > Keep PC machine at rev1 to remain compatible and Q35
> > at rev3 where w2k isn't supported anyway so OSX could
> > run as well.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >    - make pc rev1 and q35 rev3.
> >        "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > PS:
> > Only compile tested.
> > 
> > 
> > CC: Programmingkid <programmingkidx@gmail.com>
> > CC: Phil Dennis-Jordan <lists@philjordan.eu>
> > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > ---
> >  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 6b7bade..b9c245c 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> >  } AcpiMcfgInfo;
> >  
> >  typedef struct AcpiPmInfo {
> > +    bool force_rev1_fadt;
> >      bool s3_disabled;
> >      bool s4_disabled;
> >      bool pcihp_bridge_en;
> > @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >      Object *obj = NULL;
> >      QObject *o;
> >  
> > +    pm->force_rev1_fadt = false;
> >      pm->cpu_hp_io_base = 0;
> >      pm->pcihp_io_base = 0;
> >      pm->pcihp_io_len = 0;
> >      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 =
> > @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> >          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> >      }
> >      fadt->century = RTC_CENTURY;
> > +    if (pm->force_rev1_fadt) {
> > +        return;
> > +    }
> >  
> >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> >      fadt->reset_value = 0xf;
> > @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> >      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 rev = 3;
> >  
> >      /* FACS address to be filled by Guest linker */
> >      bios_linker_loader_add_pointer(linker,
> > @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> >      bios_linker_loader_add_pointer(linker,
> >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> >          ACPI_BUILD_TABLE_FILE, dsdt_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);
> > +    if (pm->force_rev1_fadt) {
> > +        rev = 1;
> > +        fadt_size = offsetof(typeof(*fadt), reset_register);  
> 
> I don't really like it that we are using structs to format fields - I
> would prefer build_append_int_noprefix based APIs - but since we do,
> let's add a proper structure for FADT rev 1 instead of this hack.
I'll do it.

> 
> While we are at it, maybe we should replace macro hacks like
> ACPI_FADT_COMMON_DEF with simple sub-structures (unless you see a reason
> not to).
that would be rewriting every user that touches fields,
I'd ratter slowly /one table at time/ switch to build_append_int_noprefix() API
instead of amending legacy approach.

> 
> 
> > +    } else {
> > +        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 *)fadt, "FACP", sizeof(*fadt), 3, oem_id, oem_table_id);
> > +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> >  }
> >  
> >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > -- 
> > 2.7.4  
> 

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Michael S. Tsirkin]

On Wed, Jul 26, 2017 at 04:09:37PM +0200, Igor Mammedov wrote:
> On Tue, 25 Jul 2017 16:36:06 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:
> > > w2k used to boot on QEMU until revision of FADT has
> > > been bumped to rev3
> > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> > > 
> > > Keep PC machine at rev1 to remain compatible and Q35
> > > at rev3 where w2k isn't supported anyway so OSX could
> > > run as well.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v2:
> > >    - make pc rev1 and q35 rev3.
> > >        "Michael S. Tsirkin" <mst@redhat.com>
> > > 
> > > PS:
> > > Only compile tested.
> > > 
> > > 
> > > CC: Programmingkid <programmingkidx@gmail.com>
> > > CC: Phil Dennis-Jordan <lists@philjordan.eu>
> > > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > > 
> > > ---
> > >  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 6b7bade..b9c245c 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> > >  } AcpiMcfgInfo;
> > >  
> > >  typedef struct AcpiPmInfo {
> > > +    bool force_rev1_fadt;
> > >      bool s3_disabled;
> > >      bool s4_disabled;
> > >      bool pcihp_bridge_en;
> > > @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > >      Object *obj = NULL;
> > >      QObject *o;
> > >  
> > > +    pm->force_rev1_fadt = false;
> > >      pm->cpu_hp_io_base = 0;
> > >      pm->pcihp_io_base = 0;
> > >      pm->pcihp_io_len = 0;
> > >      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 =
> > > @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > >          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > >      }
> > >      fadt->century = RTC_CENTURY;
> > > +    if (pm->force_rev1_fadt) {
> > > +        return;
> > > +    }
> > >  
> > >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > >      fadt->reset_value = 0xf;
> > > @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > >      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 rev = 3;
> > >  
> > >      /* FACS address to be filled by Guest linker */
> > >      bios_linker_loader_add_pointer(linker,
> > > @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > >      bios_linker_loader_add_pointer(linker,
> > >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > >          ACPI_BUILD_TABLE_FILE, dsdt_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);
> > > +    if (pm->force_rev1_fadt) {
> > > +        rev = 1;
> > > +        fadt_size = offsetof(typeof(*fadt), reset_register);  
> > 
> > I don't really like it that we are using structs to format fields - I
> > would prefer build_append_int_noprefix based APIs - but since we do,
> > let's add a proper structure for FADT rev 1 instead of this hack.
> I'll do it.
> 
> > 
> > While we are at it, maybe we should replace macro hacks like
> > ACPI_FADT_COMMON_DEF with simple sub-structures (unless you see a reason
> > not to).
> that would be rewriting every user that touches fields,
> I'd ratter slowly /one table at time/ switch to build_append_int_noprefix() API
> instead of amending legacy approach.

Fine with me.

> > 
> > 
> > > +    } else {
> > > +        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 *)fadt, "FACP", sizeof(*fadt), 3, oem_id, oem_table_id);
> > > +                 (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> > >  }
> > >  
> > >  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > > -- 
> > > 2.7.4  
> > 

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Igor Mammedov]

On Wed, 26 Jul 2017 23:18:01 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 26, 2017 at 04:09:37PM +0200, Igor Mammedov wrote:
> > On Tue, 25 Jul 2017 16:36:06 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:  
> > > > w2k used to boot on QEMU until revision of FADT has
> > > > been bumped to rev3
> > > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> > > > 
> > > > Keep PC machine at rev1 to remain compatible and Q35
> > > > at rev3 where w2k isn't supported anyway so OSX could
> > > > run as well.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > v2:
> > > >    - make pc rev1 and q35 rev3.
> > > >        "Michael S. Tsirkin" <mst@redhat.com>
> > > > 
> > > > PS:
> > > > Only compile tested.
> > > > 
> > > > 
> > > > CC: Programmingkid <programmingkidx@gmail.com>
> > > > CC: Phil Dennis-Jordan <lists@philjordan.eu>
> > > > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > > > 
> > > > ---
> > > >  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
> > > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index 6b7bade..b9c245c 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> > > >  } AcpiMcfgInfo;
> > > >  
> > > >  typedef struct AcpiPmInfo {
> > > > +    bool force_rev1_fadt;
> > > >      bool s3_disabled;
> > > >      bool s4_disabled;
> > > >      bool pcihp_bridge_en;
> > > > @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > > >      Object *obj = NULL;
> > > >      QObject *o;
> > > >  
> > > > +    pm->force_rev1_fadt = false;
> > > >      pm->cpu_hp_io_base = 0;
> > > >      pm->pcihp_io_base = 0;
> > > >      pm->pcihp_io_len = 0;
> > > >      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 =
> > > > @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > > >          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > > >      }
> > > >      fadt->century = RTC_CENTURY;
> > > > +    if (pm->force_rev1_fadt) {
> > > > +        return;
> > > > +    }
> > > >  
> > > >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > > >      fadt->reset_value = 0xf;
> > > > @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > >      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 rev = 3;
> > > >  
> > > >      /* FACS address to be filled by Guest linker */
> > > >      bios_linker_loader_add_pointer(linker,
> > > > @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > >      bios_linker_loader_add_pointer(linker,
> > > >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > > >          ACPI_BUILD_TABLE_FILE, dsdt_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);
> > > > +    if (pm->force_rev1_fadt) {
> > > > +        rev = 1;
> > > > +        fadt_size = offsetof(typeof(*fadt), reset_register);    
> > > 
> > > I don't really like it that we are using structs to format fields - I
> > > would prefer build_append_int_noprefix based APIs - but since we do,
> > > let's add a proper structure for FADT rev 1 instead of this hack. 
 
Michael,

I've tried to switch FADT to build_append_int_noprefix APIs,
but it turned out into quite a bit of re-factoring including
clean-ups and adding new APIs to pack all the stuff in FADT.

It's too much for hard freeze, lets not rush it and go with
this patch for 2.10 to put out fire.

When 2.11 merge window is open I'll rebase/post FADTv1/3
conversion to build_append_int_noprefix API.

Re: [Qemu-devel] [PATCH v2] pc: acpi: force FADT rev1 for 440fx based machine types

[Michael S. Tsirkin]

On Fri, Jul 28, 2017 at 02:28:57PM +0200, Igor Mammedov wrote:
> On Wed, 26 Jul 2017 23:18:01 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 26, 2017 at 04:09:37PM +0200, Igor Mammedov wrote:
> > > On Tue, 25 Jul 2017 16:36:06 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Mon, Jul 24, 2017 at 03:50:20PM +0200, Igor Mammedov wrote:  
> > > > > w2k used to boot on QEMU until revision of FADT has
> > > > > been bumped to rev3
> > > > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.)
> > > > > 
> > > > > Keep PC machine at rev1 to remain compatible and Q35
> > > > > at rev3 where w2k isn't supported anyway so OSX could
> > > > > run as well.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > v2:
> > > > >    - make pc rev1 and q35 rev3.
> > > > >        "Michael S. Tsirkin" <mst@redhat.com>
> > > > > 
> > > > > PS:
> > > > > Only compile tested.
> > > > > 
> > > > > 
> > > > > CC: Programmingkid <programmingkidx@gmail.com>
> > > > > CC: Phil Dennis-Jordan <lists@philjordan.eu>
> > > > > CC: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > 
> > > > > ---
> > > > >  hw/i386/acpi-build.c | 22 ++++++++++++++++++----
> > > > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index 6b7bade..b9c245c 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -90,6 +90,7 @@ typedef struct AcpiMcfgInfo {
> > > > >  } AcpiMcfgInfo;
> > > > >  
> > > > >  typedef struct AcpiPmInfo {
> > > > > +    bool force_rev1_fadt;
> > > > >      bool s3_disabled;
> > > > >      bool s4_disabled;
> > > > >      bool pcihp_bridge_en;
> > > > > @@ -129,10 +130,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > > > >      Object *obj = NULL;
> > > > >      QObject *o;
> > > > >  
> > > > > +    pm->force_rev1_fadt = false;
> > > > >      pm->cpu_hp_io_base = 0;
> > > > >      pm->pcihp_io_base = 0;
> > > > >      pm->pcihp_io_len = 0;
> > > > >      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 =
> > > > > @@ -304,6 +308,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > > > >          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > > > >      }
> > > > >      fadt->century = RTC_CENTURY;
> > > > > +    if (pm->force_rev1_fadt) {
> > > > > +        return;
> > > > > +    }
> > > > >  
> > > > >      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > > > >      fadt->reset_value = 0xf;
> > > > > @@ -342,6 +349,8 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > > >      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 rev = 3;
> > > > >  
> > > > >      /* FACS address to be filled by Guest linker */
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > > @@ -353,12 +362,17 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > > >      bios_linker_loader_add_pointer(linker,
> > > > >          ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > > > >          ACPI_BUILD_TABLE_FILE, dsdt_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);
> > > > > +    if (pm->force_rev1_fadt) {
> > > > > +        rev = 1;
> > > > > +        fadt_size = offsetof(typeof(*fadt), reset_register);    
> > > > 
> > > > I don't really like it that we are using structs to format fields - I
> > > > would prefer build_append_int_noprefix based APIs - but since we do,
> > > > let's add a proper structure for FADT rev 1 instead of this hack. 
>  
> Michael,
> 
> I've tried to switch FADT to build_append_int_noprefix APIs,
> but it turned out into quite a bit of re-factoring including
> clean-ups and adding new APIs to pack all the stuff in FADT.
> 
> It's too much for hard freeze, lets not rush it and go with
> this patch for 2.10 to put out fire.
> 
> When 2.11 merge window is open I'll rebase/post FADTv1/3
> conversion to build_append_int_noprefix API.

Makes sense.