Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 26 Jan 2017 14:43:04 +0100
Phil Dennis-Jordan <phil@philjordan.eu> wrote:

> On 23 January 2017 at 12:12, Igor Mammedov <imammedo@redhat.com> wrote:
> >> For reference, my approach to filling out the Xdsdt/Xfacs fields in
> >> build_fadt() is essentially the same as for the 32-bit variants from
> >> rev1:
> >>
> >> unsigned xfacs_offset = (char *)&fadt->Xfacs - table_data->data;
> >> bios_linker_loader_add_pointer(linker,
> >>         ACPI_BUILD_TABLE_FILE, xfacs_offset, sizeof(fadt->Xfacs),
> >>         ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> >>
> >> Suggestions welcome.  
> > You are not supposed to fill in the same values in X_FOO as in FOO.
> > Spec says that only either one of above should be set.  
> 
> Thanks for clarifying that. I'd been looking at the ACPI 2.0 spec,
> which isn't clear on this point - the 5.0 one unambiguously agrees
> with you.
> 
> I've tested the FADT change through again. Leaving Xdsdt and Xfacs as
> 0 in the Rev3 does indeed allow macOS to boot, as previously reported.
> Rebooting also works, which is the whole point of the patch. I'm still
> no further with getting Windows 10 past the "ACPI BIOS ERROR"
> bluescreen, however, even after some more experimenting with the
> various fields.
> I've tried a Rev5 FADT too, the Win10 bootloader just hangs in this
> cace, I don't even get a bluescreen. macOS is still happy.
> I've dumped the ACPI tables that OVMF ultimately exports with an EFI
> shell utility, and it all looks ok to me there; I also added various
> debug output to OVMF's ACPI code, and again everything looks pretty
> sane there. I guess my problem is I don't know what I'm looking for as
> I haven't found a way for Windows to tell me exactly what it doesn't
> like.
> 
> I'm basically stumped on the Windows 10 issue, but I'd still like to
> fix macOS guest rebooting. Would a patch be acceptable that introduces
> a Rev3 FADT option? (or Rev5 - this'll be much less code as the struct
> already exists) If so, what nature should the option take?
> 
> For reference, my latest FADT Rev3 prototype patch follows, in case
> anyone can spot some obvious problem. (I've kept this as compact as
> possible as opposed to optimising code quality for upstreaming.) The
> Rev5 FADT is identical except it uses sizeof(*fadt) and of course 5 as
> the revision.
> 
> ---
>  hw/i386/acpi-build.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1c928ab..19dde8b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -271,7 +271,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
>  }
> 
>  /* Load chipset information in FADT */
> -static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
> +static void fadt_setup(AcpiFadtDescriptorRev5_1 *fadt, AcpiPmInfo *pm)
>  {
>      fadt->model = 1;
>      fadt->reserved1 = 0;
> @@ -296,6 +296,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1
> *fadt, AcpiPmInfo *pm)
>                                (1 << ACPI_FADT_F_SLP_BUTTON) |
>                                (1 << ACPI_FADT_F_RTC_S4));
>      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> +    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> +    fadt->reset_value = 0xf;
> +    fadt->reset_register.space_id   = 1;
> +    fadt->reset_register.bit_width = 8;
> +    fadt->reset_register.address            = ICH9_RST_CNT_IOPORT;
cpu_to_le64

+ specify explicitly all fields of GAS structure: reset_register

+ is reset_register applicable only to q35 or to both machine types?
If it's only q35, you should do above only for it.

>      /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>       * For more than 8 CPUs, "Clustered Logical" mode has to be used
>       */
> @@ -303,6 +308,15 @@ static void fadt_setup(AcpiFadtDescriptorRev1
> *fadt, AcpiPmInfo *pm)
>          fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
>      }
>      fadt->century = RTC_CENTURY;
> +
> +    fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
> +    fadt->xpm1a_event_block.space_id = 1;
> +    fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x04);
> +    fadt->xpm1a_control_block.space_id = 1;
> +    fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x08);
> +    fadt->xpm_timer_block.space_id = 1;
> +    fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
> +    fadt->xgpe0_block.space_id = 1;
ditto: wrt all fields of xxx

>  }
> 
> 
> @@ -312,7 +326,7 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> AcpiPmInfo *pm,
>             unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
>             const char *oem_id, const char *oem_table_id)
>  {
> -    AcpiFadtDescriptorRev1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
> +    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, 244);
>      unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
>      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> 
> @@ -328,7 +342,7 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> AcpiPmInfo *pm,
>          ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
xdsdt field is mandatory per 5.1 spec, but I don't see you adding pointer to it.

> 
>      build_header(linker, table_data,
> -                 (void *)fadt, "FACP", sizeof(*fadt), 1, oem_id, oem_table_id);
> +                 (void *)fadt, "FACP", 244, 3, oem_id, oem_table_id);
>  }
> 
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> --