[RFC] acpi: add reset register to fadt

About 2 years ago, Reza Jelveh submitted essentially this same patch:
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05832.html

It adds the reset register defined in ACPI 2.0 to the x86 FADT, which fixes rebooting for Darwin/OS X/macOS guests.

I'm trying to revive this as part of an effort to get recent versions of OS X/macOS working in Qemu/KVM with OVMF and without the need for out-of-tree patches. I've adapted the original patch so it cleanly applies on current master. The unaddressed issues last time around were:

 1. Does this work with a range of Windows versions?

I've tested this new version of the patch with KVM from Ubuntu's 4.4.0-51-generic kernel, using the following guests:

 * Windows XP SP2 and SP3, x86. "pc" (FX440) machine, IDE disk, SeaBIOS.
 * Windows 7 SP 1, x86. "pc" (FX440) machine, AHCI disk, SeaBIOS.
 * Windows 10 1607, x86-64, q35 machine, AHCI disk, OVMF from 2017-01-17 EDK2 master.
 * OS X 10.9 to 10.11, macOS 10.12; patched OVMF [1]

In all cases I went through a fresh install, checked basic functionality, and rebooting. I noticed nothing different in any of the Windows VMs, and of course the Darwin-based ones no longer hang when attempting to reboot.


 2. The reset register is defined in ACPI 2.0, this still advertises 1.0.

I have to admit, I know very little about how ACPI works, so I'm not sure I got this right, but: I subsequently added a line
    rsdp->revision = 0x02;
to build_rsdp() in acpi-build.c, then booted the aforementioned Windows VMs, and the macOS 10.12 one with this change. I noticed no change or ill effect whatsoever.

I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?


Additionally, please suggest any further guest OSes and configurations I should test which seem likely to exhibit regressions with this change.

(First post to this list - apologies if I messed up any of the conventions! Thanks, phil)

[1] EDK2 fork with OS X/macOS compatibility patches: https://github.com/pmj/edk2/tree/mac-patches

---
 hw/i386/acpi-build.c        | 5 +++++
 include/hw/acpi/acpi-defs.h | 2 ++
 2 files changed, 7 insertions(+)

On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> About 2 years ago, Reza Jelveh submitted essentially this same patch:
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05832.html
> 
> It adds the reset register defined in ACPI 2.0 to the x86 FADT, which fixes rebooting for Darwin/OS X/macOS guests.
> 
> I'm trying to revive this as part of an effort to get recent versions of OS X/macOS working in Qemu/KVM with OVMF and without the need for out-of-tree patches. I've adapted the original patch so it cleanly applies on current master. The unaddressed issues last time around were:
> 
>  1. Does this work with a range of Windows versions?
> 
> I've tested this new version of the patch with KVM from Ubuntu's 4.4.0-51-generic kernel, using the following guests:
> 
>  * Windows XP SP2 and SP3, x86. "pc" (FX440) machine, IDE disk, SeaBIOS.
>  * Windows 7 SP 1, x86. "pc" (FX440) machine, AHCI disk, SeaBIOS.
>  * Windows 10 1607, x86-64, q35 machine, AHCI disk, OVMF from 2017-01-17 EDK2 master.
>  * OS X 10.9 to 10.11, macOS 10.12; patched OVMF [1]
> 
> In all cases I went through a fresh install, checked basic functionality, and rebooting. I noticed nothing different in any of the Windows VMs, and of course the Darwin-based ones no longer hang when attempting to reboot.
> 
> 
>  2. The reset register is defined in ACPI 2.0, this still advertises 1.0.
> 
> I have to admit, I know very little about how ACPI works, so I'm not sure I got this right, but: I subsequently added a line
>     rsdp->revision = 0x02;
> to build_rsdp() in acpi-build.c, then booted the aforementioned Windows VMs, and the macOS 10.12 one with this change. I noticed no change or ill effect whatsoever.

For RSDP revision, I don't think we need to bother with it. People do
mix revisions of tables in practice.

I think what's important is the Fadt format revision. That one was 1 for 1.0b and 3 for 2.0.

See page 112, Table 5-5 Fixed ACPI Description Table Format in acpi spec
1.0b.

Now look at page 110 in spec 2.0, this time
"Table 5-8 Fixed ACPI Description Table (FADT) Format".

You will see that FADT revision is now 3, and there are a bunch of new
fields after the reset register. I think you have to fill them in,
you probably can get away with copying the non-extended values.




> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?

I would say an option to make FADT use rev 3 format would be a good
idea.

I'd make it the default if XP survives.

> 
> Additionally, please suggest any further guest OSes and configurations I should test which seem likely to exhibit regressions with this change.

Pls also try XP on q35. Paolo explained how on the thread you link to.

> 
> (First post to this list - apologies if I messed up any of the conventions! Thanks, phil)
> 
> [1] EDK2 fork with OS X/macOS compatibility patches: https://github.com/pmj/edk2/tree/mac-patches

It looks ok - you need to add your S.O.B (and maybe the original one)
to certify the origin of the patch. And you want to wrap lines around
80 characters.

> ---
>  hw/i386/acpi-build.c        | 5 +++++
>  include/hw/acpi/acpi-defs.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0c8912f..93781a0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -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_val = 0xf;
> +    fadt->reset_reg.address_space_id   = 1;
> +    fadt->reset_reg.register_bit_width = 8;
> +    fadt->reset_reg.address            = ICH9_RST_CNT_IOPORT;
>      /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>       * For more than 8 CPUs, "Clustered Logical" mode has to be used
>       */

So this works for PC too because RCR_IOPORT has the same value.
I think QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT)
and a comment would be helpful here.


> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 4cc3630..5755570 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -140,6 +140,8 @@ struct AcpiFadtDescriptorRev1
>      uint8_t  reserved4a;             /* Reserved */
>      uint8_t  reserved4b;             /* Reserved */
>      uint32_t flags;
> +    Acpi20GenericAddress reset_reg;
> +    uint8_t reset_val;
>  } QEMU_PACKED;
>  typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;

I think you want a new AcpiFadtDescriptorRev3 structure.

> -- 
> 2.3.2 (Apple Git-55)

On Wed, 18 Jan 2017 18:30:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
[...]

> > I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?  
> 
> I would say an option to make FADT use rev 3 format would be a good
> idea.
> 
> I'd make it the default if XP survives.
if XP and legacy linux survive,
I'd skip adding option as probably there won't be any users,
in unlikely case such user surfaces we always can add option later
but we can't do it other way around (i.e. take it away).

On 18 January 2017 at 17:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> I think what's important is the Fadt format revision. That one was 1 for 1.0b and 3 for 2.0.
>
> See page 112, Table 5-5 Fixed ACPI Description Table Format in acpi spec
> 1.0b.
>
> Now look at page 110 in spec 2.0, this time
> "Table 5-8 Fixed ACPI Description Table (FADT) Format".
>
> You will see that FADT revision is now 3, and there are a bunch of new
> fields after the reset register. I think you have to fill them in,
> you probably can get away with copying the non-extended values.


I've just spent a few hours fiddling around with this, and the results
aren't good. WinXP and 7 seem absolutely fine with a rev3 (or rev5)
FADT, presumably because they ignore it. macOS 10.12 hangs on boot
with a fully filled out r3/5 FADT. If I skip filling out the
Xdsdt/Xfacs addresses and just leave them at 0 it boots OK. Windows 10
hangs or bluescreens ("ACPI BIOS ERROR") in either case. (I tested
UEFI boot with OVMF, did not try legacy BIOS.)

Just in case I'd messed up the code, I also tried this FADT patch
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02216.html
from some months ago which evidently never got accepted. It yielded
the same result as my independently developed implementation.

I don't really know enough about ACPI to have an intuition for what to
try next. Unfortunately, macOS's ACPI table parser does not appear to
be in any of the open source bits of the xnu kernel, otherwise I'd be
looking there for hints.

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.

Thanks,
Phil

On Thu, 19 Jan 2017 19:09:47 +0100
Phil Dennis-Jordan <phil@philjordan.eu> wrote:

> On 18 January 2017 at 17:30, Michael S. Tsirkin <mst@redhat.com> wrote:
> > I think what's important is the Fadt format revision. That one was 1 for 1.0b and 3 for 2.0.
> >
> > See page 112, Table 5-5 Fixed ACPI Description Table Format in acpi spec
> > 1.0b.
> >
> > Now look at page 110 in spec 2.0, this time
> > "Table 5-8 Fixed ACPI Description Table (FADT) Format".
> >
> > You will see that FADT revision is now 3, and there are a bunch of new
> > fields after the reset register. I think you have to fill them in,
> > you probably can get away with copying the non-extended values.  
> 
> 
> I've just spent a few hours fiddling around with this, and the results
> aren't good. WinXP and 7 seem absolutely fine with a rev3 (or rev5)
> FADT, presumably because they ignore it. macOS 10.12 hangs on boot
> with a fully filled out r3/5 FADT. If I skip filling out the
> Xdsdt/Xfacs addresses and just leave them at 0 it boots OK. Windows 10
> hangs or bluescreens ("ACPI BIOS ERROR") in either case. (I tested
> UEFI boot with OVMF, did not try legacy BIOS.)
> 
> Just in case I'd messed up the code, I also tried this FADT patch
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02216.html
> from some months ago which evidently never got accepted. It yielded
> the same result as my independently developed implementation.
patch looks a bit wrong in filling x_fields, pls see spec first
before setting them.

> 
> I don't really know enough about ACPI to have an intuition for what to
> try next. Unfortunately, macOS's ACPI table parser does not appear to
> be in any of the open source bits of the xnu kernel, otherwise I'd be
> looking there for hints.
> 
> 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.

X_FOO is typically is used for addresses above 4G and seabios
always allocates tables below 4G so you don't need to set
X_FOO variants unless you have to for some reason.

Read FADT fields description that you are modifying/adding
in respective ACPI spec http://www.uefi.org/acpi/specs

> 
> Thanks,
> Phil

On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 18 Jan 2017 18:30:59 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> [...]
>
>> > I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
>>
>> I would say an option to make FADT use rev 3 format would be a good
>> idea.
>>
>> I'd make it the default if XP survives.
> if XP and legacy linux survive,
> I'd skip adding option as probably there won't be any users,
> in unlikely case such user surfaces we always can add option later
> but we can't do it other way around (i.e. take it away).

I have now finally solved the mystery of why my FADT patch has been
going so disastrously wrong - I've now got working code, but I'd
appreciate some guidance on the best way to structure a patch to
minimise further back-and forth. The culprit turned out to be OVMF,
specifically 2 bugs/shortcomings:

1. It completely gives up on parsing Qemu's ACPI tables if more than
one "add pointer" linker command points to the same table. In this
case, if you add a command for both the DSDT and X_DSDT fields of the
FADT, it aborts completely and uses fallback tables. (The following
InstallAcpiTable call fails if called twice with the same table type.)
https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518

2. After applying all the linker commands, it goes and rewrites part
of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
fields - and it always sets one of them to 0. Which one depends on
whether the DSDT is above the 4G barrier:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650

Both of these are easily fixed, and I will submit a corresponding patch to EDK2.

With that fixed, the rest of the FADT provided by Qemu is accepted by
OVMF and the operating systems. On the Qemu side, it does mean we'll
need to still retain the ACPI 1.0 tables for backwards compatibility.

Q1: How should the option be structured and named? Should the FADT
revision be selectable via a sub-option on -machine? Or as a
standalone option? Something else?


Q2: To avoid any more confusion, I'd appreciate
confirmation/clarification on the X_ and non-X FADT fields in the case
where 32-bit pointers suffice.

Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.

Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.

Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
"This is a required field" for both variants.

Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
is not supported, this field contains zero." - I understand this to
mean that when the register block IS supported and 32-bit, both
variants must be filled.

In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.


I'll come up with a revised patch in the next few days.

Thanks for your help and patience so far, everyone!

On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
> On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Wed, 18 Jan 2017 18:30:59 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> > [...]
> >
> >> > I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
> >>
> >> I would say an option to make FADT use rev 3 format would be a good
> >> idea.
> >>
> >> I'd make it the default if XP survives.
> > if XP and legacy linux survive,
> > I'd skip adding option as probably there won't be any users,
> > in unlikely case such user surfaces we always can add option later
> > but we can't do it other way around (i.e. take it away).
> 
> I have now finally solved the mystery of why my FADT patch has been
> going so disastrously wrong - I've now got working code, but I'd
> appreciate some guidance on the best way to structure a patch to
> minimise further back-and forth.

+ lersek

> The culprit turned out to be OVMF,
> specifically 2 bugs/shortcomings:
> 
> 1. It completely gives up on parsing Qemu's ACPI tables if more than
> one "add pointer" linker command points to the same table. In this
> case, if you add a command for both the DSDT and X_DSDT fields of the
> FADT, it aborts completely and uses fallback tables. (The following
> InstallAcpiTable call fails if called twice with the same table type.)
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
> 
> 2. After applying all the linker commands, it goes and rewrites part
> of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
> fields - and it always sets one of them to 0. Which one depends on
> whether the DSDT is above the 4G barrier:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
> 
> Both of these are easily fixed, and I will submit a corresponding patch to EDK2.
> 
> With that fixed, the rest of the FADT provided by Qemu is accepted by
> OVMF and the operating systems. On the Qemu side, it does mean we'll
> need to still retain the ACPI 1.0 tables for backwards compatibility.
> 
> Q1: How should the option be structured and named? Should the FADT
> revision be selectable via a sub-option on -machine? Or as a
> standalone option? Something else?
> 
> 
> Q2: To avoid any more confusion, I'd appreciate
> confirmation/clarification on the X_ and non-X FADT fields in the case
> where 32-bit pointers suffice.
> 
> Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
> 
> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
> 
> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
> "This is a required field" for both variants.
> 
> Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
> is not supported, this field contains zero." - I understand this to
> mean that when the register block IS supported and 32-bit, both
> variants must be filled.
> 
> In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
> 
> 
> I'll come up with a revised patch in the next few days.
> 
> Thanks for your help and patience so far, everyone!

On Tue, 31 Jan 2017 16:58:22 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
> > On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:  
> > > On Wed, 18 Jan 2017 18:30:59 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >  
> > >> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:  
> > > [...]
> > >  
> > >> > I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?  
> > >>
> > >> I would say an option to make FADT use rev 3 format would be a good
> > >> idea.
> > >>
> > >> I'd make it the default if XP survives.  
> > > if XP and legacy linux survive,
> > > I'd skip adding option as probably there won't be any users,
> > > in unlikely case such user surfaces we always can add option later
> > > but we can't do it other way around (i.e. take it away).  
> > 
> > I have now finally solved the mystery of why my FADT patch has been
> > going so disastrously wrong - I've now got working code, but I'd
> > appreciate some guidance on the best way to structure a patch to
> > minimise further back-and forth.  
> 
> + lersek
> 
> > The culprit turned out to be OVMF,
> > specifically 2 bugs/shortcomings:
> > 
> > 1. It completely gives up on parsing Qemu's ACPI tables if more than
> > one "add pointer" linker command points to the same table. In this
> > case, if you add a command for both the DSDT and X_DSDT fields of the
> > FADT, it aborts completely and uses fallback tables. (The following
> > InstallAcpiTable call fails if called twice with the same table type.)
> > https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
> > 
> > 2. After applying all the linker commands, it goes and rewrites part
> > of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
> > fields - and it always sets one of them to 0. Which one depends on
> > whether the DSDT is above the 4G barrier:
> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
> > 
> > Both of these are easily fixed, and I will submit a corresponding patch to EDK2.
> > 
> > With that fixed, the rest of the FADT provided by Qemu is accepted by
> > OVMF and the operating systems. On the Qemu side, it does mean we'll
> > need to still retain the ACPI 1.0 tables for backwards compatibility.
> > 
> > Q1: How should the option be structured and named? Should the FADT
> > revision be selectable via a sub-option on -machine? Or as a
> > standalone option? Something else?
WinXP is main reason why we are using 1.0 revisions,
but if bumping revision doesn't affect it or later versions
and linux kernel (ancient/contemporary) boots fine,
I wouldn't bother with yet another option.


> > Q2: To avoid any more confusion, I'd appreciate
> > confirmation/clarification on the X_ and non-X FADT fields in the case
> > where 32-bit pointers suffice.
> > 
> > Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
spec doesn't say that X_DSDT is optional and Windows requires it if field is present.

> > 
> > Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
> > 
> > Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
> > "This is a required field" for both variants.
> > 
> > Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
> > is not supported, this field contains zero." - I understand this to
> > mean that when the register block IS supported and 32-bit, both
> > variants must be filled.
> > 
> > In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
> > 
> > 
> > I'll come up with a revised patch in the next few days.
> > 
> > Thanks for your help and patience so far, everyone!

On Tue, 31 Jan 2017 at 16:41, Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 31 Jan 2017 16:58:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
> > > On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com>
> wrote:
> > > > On Wed, 18 Jan 2017 18:30:59 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > >> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> > > > [...]
> > > >
> > > >> > I suspect more might be involved in enabling ACPI 2.0, and it
> should probably be an option so as to avoid regressions. I don't know what
> the best approach would be for this, so comments welcome. Should adding the
> reset register to the FADT also be configurable?
> > > >>
> > > >> I would say an option to make FADT use rev 3 format would be a good
> > > >> idea.
> > > >>
> > > >> I'd make it the default if XP survives.
> > > > if XP and legacy linux survive,
> > > > I'd skip adding option as probably there won't be any users,
> > > > in unlikely case such user surfaces we always can add option later
> > > > but we can't do it other way around (i.e. take it away).
> > >
> > > I have now finally solved the mystery of why my FADT patch has been
> > > going so disastrously wrong - I've now got working code, but I'd
> > > appreciate some guidance on the best way to structure a patch to
> > > minimise further back-and forth.
> >
> > + lersek
> >
> > > The culprit turned out to be OVMF,
> > > specifically 2 bugs/shortcomings:
> > >
> > > 1. It completely gives up on parsing Qemu's ACPI tables if more than
> > > one "add pointer" linker command points to the same table. In this
> > > case, if you add a command for both the DSDT and X_DSDT fields of the
> > > FADT, it aborts completely and uses fallback tables. (The following
> > > InstallAcpiTable call fails if called twice with the same table type.)
> > >
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
> > >
> > > 2. After applying all the linker commands, it goes and rewrites part
> > > of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
> > > fields - and it always sets one of them to 0. Which one depends on
> > > whether the DSDT is above the 4G barrier:
> > >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
> > >
> > > Both of these are easily fixed, and I will submit a corresponding
> patch to EDK2.
> > >
> > > With that fixed, the rest of the FADT provided by Qemu is accepted by
> > > OVMF and the operating systems. On the Qemu side, it does mean we'll
> > > need to still retain the ACPI 1.0 tables for backwards compatibility.
> > >
> > > Q1: How should the option be structured and named? Should the FADT
> > > revision be selectable via a sub-option on -machine? Or as a
> > > standalone option? Something else?
> WinXP is main reason why we are using 1.0 revisions,
> but if bumping revision doesn't affect it or later versions
> and linux kernel (ancient/contemporary) boots fine,
> I wouldn't bother with yet another option.
>

XP is fine. My concern is that setups with OVMF+Windows (10 confirmed, but
probably 7/8 too) will suddenly bluescreen on boot because unpatched OVMF
delivers a non-compliant FADT.


> > > Q2: To avoid any more confusion, I'd appreciate
> > > confirmation/clarification on the X_ and non-X FADT fields in the case
> > > where 32-bit pointers suffice.
> > >
> > > Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
> spec doesn't say that X_DSDT is optional and Windows requires it if field
> is present.
>
> > >
> > > Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
> > >
> > > Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
> > > "This is a required field" for both variants.
> > >
> > > Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
> > > is not supported, this field contains zero." - I understand this to
> > > mean that when the register block IS supported and 32-bit, both
> > > variants must be filled.
> > >
> > > In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
> > >
> > >
> > > I'll come up with a revised patch in the next few days.
> > >
> > > Thanks for your help and patience so far, everyone!
>
>

On Tue, 31 Jan 2017 16:04:58 +0000
Phil Dennis-Jordan <phil@philjordan.eu> wrote:

> On Tue, 31 Jan 2017 at 16:41, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Tue, 31 Jan 2017 16:58:22 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >  
> > > On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:  
> > > > On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com>  
> > wrote:  
> > > > > On Wed, 18 Jan 2017 18:30:59 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >  
> > > > >> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:  
> > > > > [...]
> > > > >  
> > > > >> > I suspect more might be involved in enabling ACPI 2.0, and it  
> > should probably be an option so as to avoid regressions. I don't know what
> > the best approach would be for this, so comments welcome. Should adding the
> > reset register to the FADT also be configurable?  
> > > > >>
> > > > >> I would say an option to make FADT use rev 3 format would be a good
> > > > >> idea.
> > > > >>
> > > > >> I'd make it the default if XP survives.  
> > > > > if XP and legacy linux survive,
> > > > > I'd skip adding option as probably there won't be any users,
> > > > > in unlikely case such user surfaces we always can add option later
> > > > > but we can't do it other way around (i.e. take it away).  
> > > >
> > > > I have now finally solved the mystery of why my FADT patch has been
> > > > going so disastrously wrong - I've now got working code, but I'd
> > > > appreciate some guidance on the best way to structure a patch to
> > > > minimise further back-and forth.  
> > >
> > > + lersek
> > >  
> > > > The culprit turned out to be OVMF,
> > > > specifically 2 bugs/shortcomings:
> > > >
> > > > 1. It completely gives up on parsing Qemu's ACPI tables if more than
> > > > one "add pointer" linker command points to the same table. In this
> > > > case, if you add a command for both the DSDT and X_DSDT fields of the
> > > > FADT, it aborts completely and uses fallback tables. (The following
> > > > InstallAcpiTable call fails if called twice with the same table type.)
> > > >  
> > https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518  
> > > >
> > > > 2. After applying all the linker commands, it goes and rewrites part
> > > > of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
> > > > fields - and it always sets one of them to 0. Which one depends on
> > > > whether the DSDT is above the 4G barrier:
> > > >  
> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650  
> > > >
> > > > Both of these are easily fixed, and I will submit a corresponding  
> > patch to EDK2.  
> > > >
> > > > With that fixed, the rest of the FADT provided by Qemu is accepted by
> > > > OVMF and the operating systems. On the Qemu side, it does mean we'll
> > > > need to still retain the ACPI 1.0 tables for backwards compatibility.
> > > >
> > > > Q1: How should the option be structured and named? Should the FADT
> > > > revision be selectable via a sub-option on -machine? Or as a
> > > > standalone option? Something else?  
> > WinXP is main reason why we are using 1.0 revisions,
> > but if bumping revision doesn't affect it or later versions
> > and linux kernel (ancient/contemporary) boots fine,
> > I wouldn't bother with yet another option.
> >  
> 
> XP is fine. My concern is that setups with OVMF+Windows (10 confirmed, but
> probably 7/8 too) will suddenly bluescreen on boot because unpatched OVMF
> delivers a non-compliant FADT.
Does it work with SeaBIOS?
Wrt QVMF lets waits for Laszlo's reply,
may he could offer a way around issue.

> 
> 
> > > > Q2: To avoid any more confusion, I'd appreciate
> > > > confirmation/clarification on the X_ and non-X FADT fields in the case
> > > > where 32-bit pointers suffice.
> > > >
> > > > Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.  
> > spec doesn't say that X_DSDT is optional and Windows requires it if field
> > is present.
> >  
> > > >
> > > > Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
> > > >
> > > > Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
> > > > "This is a required field" for both variants.
> > > >
> > > > Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
> > > > is not supported, this field contains zero." - I understand this to
> > > > mean that when the register block IS supported and 32-bit, both
> > > > variants must be filled.
> > > >
> > > > In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
> > > >
> > > >
> > > > I'll come up with a revised patch in the next few days.
> > > >
> > > > Thanks for your help and patience so far, everyone!  
> >
> >

On 01/31/17 15:58, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>> On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
>>> On Wed, 18 Jan 2017 18:30:59 +0200
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
>>> [...]
>>>
>>>>> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
>>>>
>>>> I would say an option to make FADT use rev 3 format would be a good
>>>> idea.
>>>>
>>>> I'd make it the default if XP survives.
>>> if XP and legacy linux survive,
>>> I'd skip adding option as probably there won't be any users,
>>> in unlikely case such user surfaces we always can add option later
>>> but we can't do it other way around (i.e. take it away).
>>
>> I have now finally solved the mystery of why my FADT patch has been
>> going so disastrously wrong - I've now got working code, but I'd
>> appreciate some guidance on the best way to structure a patch to
>> minimise further back-and forth.
> 
> + lersek
> 
>> The culprit turned out to be OVMF,
>> specifically 2 bugs/shortcomings:
>>
>> 1. It completely gives up on parsing Qemu's ACPI tables if more than
>> one "add pointer" linker command points to the same table. In this
>> case, if you add a command for both the DSDT and X_DSDT fields of the
>> FADT, it aborts completely and uses fallback tables. (The following
>> InstallAcpiTable call fails if called twice with the same table type.)
>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518

Unlike SeaBIOS, OVMF can only use EFI_ACPI_TABLE_PROTOCOL to install
ACPI tables.

EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs a *copy* of the
table that is passed in. In addition, EFI_ACPI_TABLE_PROTOCOL (whose
implementation is in generic edk2 code, not in OVMF platform code) has
built-in "smarts" for handling and linking together the various specific
tables defined by the ACPI spec.

The way we were able to make EFI_ACPI_TABLE_PROTOCOL cooperate with
QEMU's linker/loader is a two-phase process.

In the first phase, the fw_cfg blobs are allocated, downloaded, linked
together, and checksummed, in AcpiNVS type memory, as instructed by the
linker/loader script.

In the second phase, all ADD_POINTER commands are processed again, and
the targets of those pointers are investigated for ACPI SDT headers.
Every such pointer target that looks like an ACPI SDT header is passed
to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(); except root tables like
RSDT etc. This member function handles the actual copying, installation,
and cross-table linking (to the extent specified in the ACPI spec).

Additionally, for each allocated  / downloaded fw_cfg blob, the second
phase tracks if there is at least one pointer into the blob that does
*not* point to an ACPI SDT header. If that's the case, then it implies
the presence of some non-ACPI-table-buffer within the blob (that is
referenced by some other field elsewhere). In turn such fw_cfg blobs are
not freed at the end of the whole process. (If an fw_cfg blob turns out
to host *only* ACPI tables -- which were then all copied for
installation, see above -- then the blob is released in the end.)

Finally, EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() may return
EFI_ACCESS_DENIED (quoting the UEFI spec):

"The table signature matches a table already present in the system and
platform policy does not allow duplicate tables of this type."

(And see edk2 commit 5966402ed51c, "MdeModulePkg/IntelFrameworkModulePkg
ACPI: Follow the new UEFI 2.4a spec to return EFI_ACCESS_DENIED for
duplicated FADT, FACS or DSDT installation.", 2014-05-06.)


... BTW, we discussed the question of multiply-pointed-to tables before,
in connection with XSDT support. Under "XSDT support", I mean an XSDT
built by QEMU explicitly (because EFI_ACPI_TABLE_PROTOCOL handles both
XSDT and RSDT internally, automatically). Clearly, if some ACPI table is
referenced from QEMU's RSDT and XSDT both, that immediately triggers the
same problem.

Looking at my older notes, I find two references:

Message-Id: <20150827205706-mutt-send-email-mst@redhat.com>
URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html

Message-Id: <20151228141917-mutt-send-email-mst@redhat.com>
URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg04636.html

Looking further at my notes, I find the following idea:

    in the second pass only, maintain a global rbtree that contains
    VOID* objects, pointing to actual physcal addresses in the relocated
    blobs that have already been passed to InstallAcpiTable. This will
    ensure that no address is passed to InstallAcpiTable twice

IOW, it's simple memoization for ADD_POINTER targets (in absolute
guest-phys addr space) so that duplicate
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() can be avoided.

If this feature is important, I can file an upstream OVMF bug for it,
and work on it. (I already have another open BZ for the linker/loader
anyway, <https://bugzilla.tianocore.org/show_bug.cgi?id=359>.)

Please confirm if this is necessary.


>>
>> 2. After applying all the linker commands, it goes and rewrites part
>> of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
>> fields - and it always sets one of them to 0. Which one depends on
>> whether the DSDT is above the 4G barrier:
>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650

Yes. This is precisely what I meant above with:

    EFI_ACPI_TABLE_PROTOCOL (whose implementation is in generic edk2
    code, not in OVMF platform code) has built-in "smarts" for handling
    and linking together the various specific tables defined by the
    ACPI spec.

>>
>> Both of these are easily fixed, and I will submit a corresponding patch to EDK2.

What easy fixes do you have in mind?

For problem (1), simply ignoring EFI_ACCESS_DENIED from
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() is not right. Unless we can
ensure in a future-proof way that an error code gets returned for *only
one* error scenario and nothing else, suppressing the error code is
risky. I'd much rather prevent a function call (with memoization) that
we know in advance will fail.

For symptom (2), I'm unsure if we can call that a problem at all. You'd
have to prove that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" violates
the ACPI spec (or that it's within the spec, but works sub-optimally),
when it ensures exclusivity between FADT.DSDT and FADT.X_DSDT.

The ACPI 6.1 spec says,

- DSDT: [...] If the X_DSDT field contains a non-zero value then this
  field must be zero.
- X_DSDT: [...] If the DSDT field contains a non-zero value then this
  field must be zero.

Therefore the EFI_ACPI_TABLE_PROTOCOL implementation in edk2 seems
conformant & correct to me.

Related edk2 commits:

- the one that added the code you reference above:

f9bbb8d9c3f0 ("MdeModulePkg: AcpiTableDxe: make 4 GB table allocation
limit optional", 2016-02-17)

- the earlier, similar one, that enforces exclusivity between
FADT.FIRMWARE_CTRL and FADT.X_FIRMWARE_CTRL:

f798e8bff773 ("MdeModulePkg: Acpi: enforce exclusion between
FirmwareCtrl and XFirmwareCtrl", 2015-01-26)

>>
>> With that fixed, the rest of the FADT provided by Qemu is accepted by
>> OVMF and the operating systems. On the Qemu side, it does mean we'll
>> need to still retain the ACPI 1.0 tables for backwards compatibility.
>>
>> Q1: How should the option be structured and named? Should the FADT
>> revision be selectable via a sub-option on -machine? Or as a
>> standalone option? Something else?

No input from me on this one.

>> Q2: To avoid any more confusion, I'd appreciate
>> confirmation/clarification on the X_ and non-X FADT fields in the case
>> where 32-bit pointers suffice.
>>
>> Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.

I disagree. The 6.1 release of the ACPI spec requires exclusivity.

The changelog at the top of the spec lists:

Revision    Change Description                         Affected Sections
----------  -----------------------------------------  -----------------
6.0 Errata  1393 In FADT: if X_DSDT field is           Table 5-34
            non-zero, DSDT field should be ignored or
            deprecated

The number "1393" is most likely the Mantis ticket number:

https://mantis.uefi.org/mantis/view.php?id=1393

(Unfortunately, I don't have the necessary credentials to verify if this
Mantis ticket actually corresponds to this change. I could do that for
Platform Init or UEFI spec items -- without quoting any specifics --,
but apparently my ASWG membership level isn't high enough for this one.)

>> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.

Sounds good.

>>
>> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>> "This is a required field" for both variants.

Hmmm, I don't think so (again, from ACPI 6.1):

- PM1a_EVT_BLK: [...] This is a required field. If the X_PM1a_CNT_BLK
  field contains a non-zero value then this field must be zero.
- X_PM1a_EVT_BLK: [...] This is a required field. If the PM1a_EVT_BLK
  field contains a non-zero value then this field must be zero.

(Not checking the rest.)

>>
>> Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
>> is not supported, this field contains zero." - I understand this to
>> mean that when the register block IS supported and 32-bit, both
>> variants must be filled.
>>
>> In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
>>
>>
>> I'll come up with a revised patch in the next few days.
>>
>> Thanks for your help and patience so far, everyone!

In summary, here's my opinion:

- I think that setting both FADT.DSDT and FADT.X_DSDT to nonzero values
simultaneously would break the ACPI 6.1 spec (update originating from
Mantis #1393, most likely)

- "MdeModulePkg/Universal/Acpi/AcpiTableDxe", implementing
EFI_ACPI_TABLE_PROTOCOL in edk2, appears to conform to this requirement
specifically

- if remedying symptom (1) were only necessary for setting both DSDT and
X_DSDT -- which, I claim, should not be done --, then I'd like to
postpone the above memoization indefinitely. It's too complex to be
added speculatively.

Thank you,
Laszlo

On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
> The ACPI 6.1 spec says,
> 
> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>   field must be zero.
> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>   field must be zero.

But that's only 6.1. 6.0 and earlier did not say this.
The errata they wanted to address was:
1393 In FADT: if X_DSDT field is non-zero, DSDT
field should be ignored or deprecated

I would class this as a spec bug.

On 01/31/17 19:17, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
>> The ACPI 6.1 spec says,
>>
>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>   field must be zero.
>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>   field must be zero.
> 
> But that's only 6.1. 6.0 and earlier did not say this.
> The errata they wanted to address was:
> 1393 In FADT: if X_DSDT field is non-zero, DSDT
> field should be ignored or deprecated
> 
> I would class this as a spec bug.
> 

Process-wise, that's not a bad idea; it could be the only way (or the
best way) to argue for a corresponding change in edk2's
EFI_ACPI_TABLE_PROTOCOL implementation
(MdeModulePkg/Universal/Acpi/AcpiTableDxe).

Do you want to raise this on the ASWG list? I vaguely recall that you
subscribed; if not, I think you should be able to, as a Red Hatter
<http://members.uefi.org/kmembership_info/person_signup/>.

(I'd like to avoid being the middle man.)

Hm... It seems that the "Adopter Membership" is free, which could be
appropriate for individual observers:

http://uefi.org/join
http://members.uefi.org/home/

(Should Phil consider it.)

Thanks
Laszlo

On Tue, 31 Jan 2017 20:17:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
> > The ACPI 6.1 spec says,
> > 
> > - DSDT: [...] If the X_DSDT field contains a non-zero value then this
> >   field must be zero.
> > - X_DSDT: [...] If the DSDT field contains a non-zero value then this
> >   field must be zero.  
> 
> But that's only 6.1. 6.0 and earlier did not say this.
> The errata they wanted to address was:
> 1393 In FADT: if X_DSDT field is non-zero, DSDT
> field should be ignored or deprecated
> 
> I would class this as a spec bug.
> 

The same applies to X_PM1a_EVT_BLK and co,
for example 5.1 spec "This is a required
field."

And looks like Windows implemented it as mandatory
to boot perhaps to be compatible with 5.1 and earlier
specs.

It appears fw would be forced to fill fields depending
on table revision.

On 02/01/17 12:37, Igor Mammedov wrote:
> On Tue, 31 Jan 2017 20:17:02 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
>>> The ACPI 6.1 spec says,
>>>
>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>   field must be zero.
>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>   field must be zero.  
>>
>> But that's only 6.1. 6.0 and earlier did not say this.
>> The errata they wanted to address was:
>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>> field should be ignored or deprecated
>>
>> I would class this as a spec bug.
>>
> 
> The same applies to X_PM1a_EVT_BLK and co,
> for example 5.1 spec "This is a required
> field."
> 
> And looks like Windows implemented it as mandatory
> to boot perhaps to be compatible with 5.1 and earlier
> specs.
> 
> It appears fw would be forced to fill fields depending
> on table revision.

Sounds like a valid point.

I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
former says:

- FADT Major Version: 5; Major Version of this FADT structure, [...]
- DSDT: Physical memory address (0-4 GB) of the DSDT.
- X_DSDT: 64bit physical address of the DSDT.

the latter says:

- FADT Major Version: 6; Major Version of this FADT structure, [...]

- DSDT: Physical memory address of the DSDT. If the X_DSDT field
  contains a non-zero value then this field must be zero.

- X_DSDT: Extended physical address of the DSDT. If the DSDT field
  contains a non-zero value then this field must be zero.

I will ask on edk2-devel whether the
"MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
way to accommodate this.

Thanks
Laszlo

On 02/01/17 13:52, Laszlo Ersek wrote:
> On 02/01/17 12:37, Igor Mammedov wrote:
>> On Tue, 31 Jan 2017 20:17:02 +0200
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
>>>> The ACPI 6.1 spec says,
>>>>
>>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>>   field must be zero.
>>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>>   field must be zero.  
>>>
>>> But that's only 6.1. 6.0 and earlier did not say this.
>>> The errata they wanted to address was:
>>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>>> field should be ignored or deprecated
>>>
>>> I would class this as a spec bug.
>>>
>>
>> The same applies to X_PM1a_EVT_BLK and co,
>> for example 5.1 spec "This is a required
>> field."
>>
>> And looks like Windows implemented it as mandatory
>> to boot perhaps to be compatible with 5.1 and earlier
>> specs.
>>
>> It appears fw would be forced to fill fields depending
>> on table revision.
> 
> Sounds like a valid point.
> 
> I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
> former says:
> 
> - FADT Major Version: 5; Major Version of this FADT structure, [...]
> - DSDT: Physical memory address (0-4 GB) of the DSDT.
> - X_DSDT: 64bit physical address of the DSDT.
> 
> the latter says:
> 
> - FADT Major Version: 6; Major Version of this FADT structure, [...]
> 
> - DSDT: Physical memory address of the DSDT. If the X_DSDT field
>   contains a non-zero value then this field must be zero.
> 
> - X_DSDT: Extended physical address of the DSDT. If the DSDT field
>   contains a non-zero value then this field must be zero.
> 
> I will ask on edk2-devel whether the
> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
> way to accommodate this.

Sigh, this looks nasty.

Considering specifically the DSDT <-> X_DSDT question, Mantis ticket
#1393 (which requires the mutual exclusion) went into 5.1B. In version
5.1A, the mutual exclusion is not required.

Unfortuantely, the FADT Major.Minor version, as reported through the
bytes at offsets 8 and 131 decimal in the table, is "5.1" for *both*
5.1A and 5.1B. In other words, looking at just Major.Minor, it cannot be
determined with full precision whether the DSDT and X_DSDT fields should
be exclusive or not. :/

Laszlo

On Wed, Feb 01, 2017 at 12:37:39PM +0100, Igor Mammedov wrote:
> On Tue, 31 Jan 2017 20:17:02 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
> > > The ACPI 6.1 spec says,
> > > 
> > > - DSDT: [...] If the X_DSDT field contains a non-zero value then this
> > >   field must be zero.
> > > - X_DSDT: [...] If the DSDT field contains a non-zero value then this
> > >   field must be zero.  
> > 
> > But that's only 6.1. 6.0 and earlier did not say this.
> > The errata they wanted to address was:
> > 1393 In FADT: if X_DSDT field is non-zero, DSDT
> > field should be ignored or deprecated
> > 
> > I would class this as a spec bug.
> > 
> 
> The same applies to X_PM1a_EVT_BLK and co,
> for example 5.1 spec "This is a required
> field."
> 
> And looks like Windows implemented it as mandatory
> to boot perhaps to be compatible with 5.1 and earlier
> specs.
> 
> It appears fw would be forced to fill fields depending
> on table revision.

Or just do what every hardware vendor does:
1. write according to spec
2. test a bunch of OSes, fix crashes

On Wed, 1 Feb 2017 14:03:52 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 02/01/17 13:52, Laszlo Ersek wrote:
> > On 02/01/17 12:37, Igor Mammedov wrote:  
> >> On Tue, 31 Jan 2017 20:17:02 +0200
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>  
> >>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:  
> >>>> The ACPI 6.1 spec says,
> >>>>
> >>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
> >>>>   field must be zero.
> >>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
> >>>>   field must be zero.    
> >>>
> >>> But that's only 6.1. 6.0 and earlier did not say this.
> >>> The errata they wanted to address was:
> >>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
> >>> field should be ignored or deprecated
> >>>
> >>> I would class this as a spec bug.
> >>>  
> >>
> >> The same applies to X_PM1a_EVT_BLK and co,
> >> for example 5.1 spec "This is a required
> >> field."
> >>
> >> And looks like Windows implemented it as mandatory
> >> to boot perhaps to be compatible with 5.1 and earlier
> >> specs.
> >>
> >> It appears fw would be forced to fill fields depending
> >> on table revision.  
> > 
> > Sounds like a valid point.
> > 
> > I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
> > former says:
> > 
> > - FADT Major Version: 5; Major Version of this FADT structure, [...]
> > - DSDT: Physical memory address (0-4 GB) of the DSDT.
> > - X_DSDT: 64bit physical address of the DSDT.
> > 
> > the latter says:
> > 
> > - FADT Major Version: 6; Major Version of this FADT structure, [...]
> > 
> > - DSDT: Physical memory address of the DSDT. If the X_DSDT field
> >   contains a non-zero value then this field must be zero.
> > 
> > - X_DSDT: Extended physical address of the DSDT. If the DSDT field
> >   contains a non-zero value then this field must be zero.
> > 
> > I will ask on edk2-devel whether the
> > "MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
> > way to accommodate this.  
> 
> Sigh, this looks nasty.
> 
> Considering specifically the DSDT <-> X_DSDT question, Mantis ticket
> #1393 (which requires the mutual exclusion) went into 5.1B. In version
> 5.1A, the mutual exclusion is not required.
> 
> Unfortuantely, the FADT Major.Minor version, as reported through the
> bytes at offsets 8 and 131 decimal in the table, is "5.1" for *both*
> 5.1A and 5.1B. In other words, looking at just Major.Minor, it cannot be
> determined with full precision whether the DSDT and X_DSDT fields should
> be exclusive or not. :/
The same applies to 6.0 vs 6.0A 

> 
> Laszlo
> 
>

On 02/01/17 16:16, Igor Mammedov wrote:
> On Wed, 1 Feb 2017 14:03:52 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 02/01/17 13:52, Laszlo Ersek wrote:
>>> On 02/01/17 12:37, Igor Mammedov wrote:  
>>>> On Tue, 31 Jan 2017 20:17:02 +0200
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>  
>>>>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:  
>>>>>> The ACPI 6.1 spec says,
>>>>>>
>>>>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>>>>   field must be zero.
>>>>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>>>>   field must be zero.    
>>>>>
>>>>> But that's only 6.1. 6.0 and earlier did not say this.
>>>>> The errata they wanted to address was:
>>>>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>>>>> field should be ignored or deprecated
>>>>>
>>>>> I would class this as a spec bug.
>>>>>  
>>>>
>>>> The same applies to X_PM1a_EVT_BLK and co,
>>>> for example 5.1 spec "This is a required
>>>> field."
>>>>
>>>> And looks like Windows implemented it as mandatory
>>>> to boot perhaps to be compatible with 5.1 and earlier
>>>> specs.
>>>>
>>>> It appears fw would be forced to fill fields depending
>>>> on table revision.  
>>>
>>> Sounds like a valid point.
>>>
>>> I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
>>> former says:
>>>
>>> - FADT Major Version: 5; Major Version of this FADT structure, [...]
>>> - DSDT: Physical memory address (0-4 GB) of the DSDT.
>>> - X_DSDT: 64bit physical address of the DSDT.
>>>
>>> the latter says:
>>>
>>> - FADT Major Version: 6; Major Version of this FADT structure, [...]
>>>
>>> - DSDT: Physical memory address of the DSDT. If the X_DSDT field
>>>   contains a non-zero value then this field must be zero.
>>>
>>> - X_DSDT: Extended physical address of the DSDT. If the DSDT field
>>>   contains a non-zero value then this field must be zero.
>>>
>>> I will ask on edk2-devel whether the
>>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
>>> way to accommodate this.  
>>
>> Sigh, this looks nasty.
>>
>> Considering specifically the DSDT <-> X_DSDT question, Mantis ticket
>> #1393 (which requires the mutual exclusion) went into 5.1B. In version
>> 5.1A, the mutual exclusion is not required.
>>
>> Unfortuantely, the FADT Major.Minor version, as reported through the
>> bytes at offsets 8 and 131 decimal in the table, is "5.1" for *both*
>> 5.1A and 5.1B. In other words, looking at just Major.Minor, it cannot be
>> determined with full precision whether the DSDT and X_DSDT fields should
>> be exclusive or not. :/
> The same applies to 6.0 vs 6.0A 

Thanks for the info; I've updated the patch!

Laszlo

On Wed, Feb 01, 2017 at 05:03:38PM +0100, Laszlo Ersek wrote:
> On 02/01/17 16:16, Igor Mammedov wrote:
> > On Wed, 1 Feb 2017 14:03:52 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> >> On 02/01/17 13:52, Laszlo Ersek wrote:
> >>> On 02/01/17 12:37, Igor Mammedov wrote:  
> >>>> On Tue, 31 Jan 2017 20:17:02 +0200
> >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>>  
> >>>>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:  
> >>>>>> The ACPI 6.1 spec says,
> >>>>>>
> >>>>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
> >>>>>>   field must be zero.
> >>>>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
> >>>>>>   field must be zero.    
> >>>>>
> >>>>> But that's only 6.1. 6.0 and earlier did not say this.
> >>>>> The errata they wanted to address was:
> >>>>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
> >>>>> field should be ignored or deprecated
> >>>>>
> >>>>> I would class this as a spec bug.
> >>>>>  
> >>>>
> >>>> The same applies to X_PM1a_EVT_BLK and co,
> >>>> for example 5.1 spec "This is a required
> >>>> field."
> >>>>
> >>>> And looks like Windows implemented it as mandatory
> >>>> to boot perhaps to be compatible with 5.1 and earlier
> >>>> specs.
> >>>>
> >>>> It appears fw would be forced to fill fields depending
> >>>> on table revision.  
> >>>
> >>> Sounds like a valid point.
> >>>
> >>> I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
> >>> former says:
> >>>
> >>> - FADT Major Version: 5; Major Version of this FADT structure, [...]
> >>> - DSDT: Physical memory address (0-4 GB) of the DSDT.
> >>> - X_DSDT: 64bit physical address of the DSDT.
> >>>
> >>> the latter says:
> >>>
> >>> - FADT Major Version: 6; Major Version of this FADT structure, [...]
> >>>
> >>> - DSDT: Physical memory address of the DSDT. If the X_DSDT field
> >>>   contains a non-zero value then this field must be zero.
> >>>
> >>> - X_DSDT: Extended physical address of the DSDT. If the DSDT field
> >>>   contains a non-zero value then this field must be zero.
> >>>
> >>> I will ask on edk2-devel whether the
> >>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
> >>> way to accommodate this.  
> >>
> >> Sigh, this looks nasty.
> >>
> >> Considering specifically the DSDT <-> X_DSDT question, Mantis ticket
> >> #1393 (which requires the mutual exclusion) went into 5.1B. In version
> >> 5.1A, the mutual exclusion is not required.
> >>
> >> Unfortuantely, the FADT Major.Minor version, as reported through the
> >> bytes at offsets 8 and 131 decimal in the table, is "5.1" for *both*
> >> 5.1A and 5.1B. In other words, looking at just Major.Minor, it cannot be
> >> determined with full precision whether the DSDT and X_DSDT fields should
> >> be exclusive or not. :/
> > The same applies to 6.0 vs 6.0A 
> 
> Thanks for the info; I've updated the patch!
> 
> Laszlo

Same applies for firmware control. There, the difference would be
between 3.0 and 4.0 where they made the incompatible change.

On 02/01/17 17:17, Michael S. Tsirkin wrote:
> On Wed, Feb 01, 2017 at 05:03:38PM +0100, Laszlo Ersek wrote:
>> On 02/01/17 16:16, Igor Mammedov wrote:
>>> On Wed, 1 Feb 2017 14:03:52 +0100
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>>> On 02/01/17 13:52, Laszlo Ersek wrote:
>>>>> On 02/01/17 12:37, Igor Mammedov wrote:  
>>>>>> On Tue, 31 Jan 2017 20:17:02 +0200
>>>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>>  
>>>>>>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:  
>>>>>>>> The ACPI 6.1 spec says,
>>>>>>>>
>>>>>>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>>>>>>   field must be zero.
>>>>>>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>>>>>>   field must be zero.    
>>>>>>>
>>>>>>> But that's only 6.1. 6.0 and earlier did not say this.
>>>>>>> The errata they wanted to address was:
>>>>>>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>>>>>>> field should be ignored or deprecated
>>>>>>>
>>>>>>> I would class this as a spec bug.
>>>>>>>  
>>>>>>
>>>>>> The same applies to X_PM1a_EVT_BLK and co,
>>>>>> for example 5.1 spec "This is a required
>>>>>> field."
>>>>>>
>>>>>> And looks like Windows implemented it as mandatory
>>>>>> to boot perhaps to be compatible with 5.1 and earlier
>>>>>> specs.
>>>>>>
>>>>>> It appears fw would be forced to fill fields depending
>>>>>> on table revision.  
>>>>>
>>>>> Sounds like a valid point.
>>>>>
>>>>> I compared the FADT defintion between ACPI 5.1 and ACPI 6.1. Indeed, the
>>>>> former says:
>>>>>
>>>>> - FADT Major Version: 5; Major Version of this FADT structure, [...]
>>>>> - DSDT: Physical memory address (0-4 GB) of the DSDT.
>>>>> - X_DSDT: 64bit physical address of the DSDT.
>>>>>
>>>>> the latter says:
>>>>>
>>>>> - FADT Major Version: 6; Major Version of this FADT structure, [...]
>>>>>
>>>>> - DSDT: Physical memory address of the DSDT. If the X_DSDT field
>>>>>   contains a non-zero value then this field must be zero.
>>>>>
>>>>> - X_DSDT: Extended physical address of the DSDT. If the DSDT field
>>>>>   contains a non-zero value then this field must be zero.
>>>>>
>>>>> I will ask on edk2-devel whether the
>>>>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" maintainers can think of a
>>>>> way to accommodate this.  
>>>>
>>>> Sigh, this looks nasty.
>>>>
>>>> Considering specifically the DSDT <-> X_DSDT question, Mantis ticket
>>>> #1393 (which requires the mutual exclusion) went into 5.1B. In version
>>>> 5.1A, the mutual exclusion is not required.
>>>>
>>>> Unfortuantely, the FADT Major.Minor version, as reported through the
>>>> bytes at offsets 8 and 131 decimal in the table, is "5.1" for *both*
>>>> 5.1A and 5.1B. In other words, looking at just Major.Minor, it cannot be
>>>> determined with full precision whether the DSDT and X_DSDT fields should
>>>> be exclusive or not. :/
>>> The same applies to 6.0 vs 6.0A 
>>
>> Thanks for the info; I've updated the patch!
>>
>> Laszlo
> 
> Same applies for firmware control. There, the difference would be
> between 3.0 and 4.0 where they made the incompatible change.
> 

Let's see first how the DSDT / X_DSDT patch fares...

On Tue, Jan 31, 2017 at 04:04:58PM +0000, Phil Dennis-Jordan wrote:
> 
> On Tue, 31 Jan 2017 at 16:41, Igor Mammedov <imammedo@redhat.com> wrote:
> 
>     On Tue, 31 Jan 2017 16:58:22 +0200
>     "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>     > On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>     > > On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
>     > > > On Wed, 18 Jan 2017 18:30:59 +0200
>     > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>     > > >
>     > > >> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
>     > > > [...]
>     > > >
>     > > >> > I suspect more might be involved in enabling ACPI 2.0, and it
>     should probably be an option so as to avoid regressions. I don't know what
>     the best approach would be for this, so comments welcome. Should adding the
>     reset register to the FADT also be configurable?
>     > > >>
>     > > >> I would say an option to make FADT use rev 3 format would be a good
>     > > >> idea.
>     > > >>
>     > > >> I'd make it the default if XP survives.
>     > > > if XP and legacy linux survive,
>     > > > I'd skip adding option as probably there won't be any users,
>     > > > in unlikely case such user surfaces we always can add option later
>     > > > but we can't do it other way around (i.e. take it away).
>     > >
>     > > I have now finally solved the mystery of why my FADT patch has been
>     > > going so disastrously wrong - I've now got working code, but I'd
>     > > appreciate some guidance on the best way to structure a patch to
>     > > minimise further back-and forth.
>     >
>     > + lersek
>     >
>     > > The culprit turned out to be OVMF,
>     > > specifically 2 bugs/shortcomings:
>     > >
>     > > 1. It completely gives up on parsing Qemu's ACPI tables if more than
>     > > one "add pointer" linker command points to the same table. In this
>     > > case, if you add a command for both the DSDT and X_DSDT fields of the
>     > > FADT, it aborts completely and uses fallback tables. (The following
>     > > InstallAcpiTable call fails if called twice with the same table type.)
>     > > https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/
>     QemuFwCfgAcpi.c#L518
>     > >
>     > > 2. After applying all the linker commands, it goes and rewrites part
>     > > of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
>     > > fields - and it always sets one of them to 0. Which one depends on
>     > > whether the DSDT is above the 4G barrier:
>     > > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/
>     Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
>     > >
>     > > Both of these are easily fixed, and I will submit a corresponding patch
>     to EDK2.
>     > >
>     > > With that fixed, the rest of the FADT provided by Qemu is accepted by
>     > > OVMF and the operating systems. On the Qemu side, it does mean we'll
>     > > need to still retain the ACPI 1.0 tables for backwards compatibility.
>     > >
>     > > Q1: How should the option be structured and named? Should the FADT
>     > > revision be selectable via a sub-option on -machine? Or as a
>     > > standalone option? Something else?
>     WinXP is main reason why we are using 1.0 revisions,
>     but if bumping revision doesn't affect it or later versions
>     and linux kernel (ancient/contemporary) boots fine,
>     I wouldn't bother with yet another option.
> 
> 
> 
> XP is fine. My concern is that setups with OVMF+Windows (10 confirmed, but
> probably 7/8 too) will suddenly bluescreen on boot because unpatched OVMF
> delivers a non-compliant FADT.

So for that, there might be a work around if you reorder the
tables in the file.


> 
> 
>     > > Q2: To avoid any more confusion, I'd appreciate
>     > > confirmation/clarification on the X_ and non-X FADT fields in the case
>     > > where 32-bit pointers suffice.
>     > >
>     > > Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
>     spec doesn't say that X_DSDT is optional and Windows requires it if field
>     is present.
> 
>     > >
>     > > Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
>     > >
>     > > Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>     > > "This is a required field" for both variants.
>     > >
>     > > Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
>     > > is not supported, this field contains zero." - I understand this to
>     > > mean that when the register block IS supported and 32-bit, both
>     > > variants must be filled.
>     > >
>     > > In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
>     > >
>     > >
>     > > I'll come up with a revised patch in the next few days.
>     > >
>     > > Thanks for your help and patience so far, everyone!
> 
>

Thanks for the in-depth reply - apologies for abandoning the thread
for a few days. Looks like a bunch of things have already been hashed
out, but I have a few more comments:

On 31 January 2017 at 17:28, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/31/17 15:58, Michael S. Tsirkin wrote:
>> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>>> On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
>>>> On Wed, 18 Jan 2017 18:30:59 +0200
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>
>>>>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
>>>> [...]
>>>>
>>>>>> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
>>>>>
>>>>> I would say an option to make FADT use rev 3 format would be a good
>>>>> idea.
>>>>>
>>>>> I'd make it the default if XP survives.
>>>> if XP and legacy linux survive,
>>>> I'd skip adding option as probably there won't be any users,
>>>> in unlikely case such user surfaces we always can add option later
>>>> but we can't do it other way around (i.e. take it away).
>>>
>>> I have now finally solved the mystery of why my FADT patch has been
>>> going so disastrously wrong - I've now got working code, but I'd
>>> appreciate some guidance on the best way to structure a patch to
>>> minimise further back-and forth.
>>
>> + lersek
>>
>>> The culprit turned out to be OVMF,
>>> specifically 2 bugs/shortcomings:
>>>
>>> 1. It completely gives up on parsing Qemu's ACPI tables if more than
>>> one "add pointer" linker command points to the same table. In this
>>> case, if you add a command for both the DSDT and X_DSDT fields of the
>>> FADT, it aborts completely and uses fallback tables. (The following
>>> InstallAcpiTable call fails if called twice with the same table type.)
>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
>
> Unlike SeaBIOS, OVMF can only use EFI_ACPI_TABLE_PROTOCOL to install
> ACPI tables.
>
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs a *copy* of the
> table that is passed in. In addition, EFI_ACPI_TABLE_PROTOCOL (whose
> implementation is in generic edk2 code, not in OVMF platform code) has
> built-in "smarts" for handling and linking together the various specific
> tables defined by the ACPI spec.
>
> The way we were able to make EFI_ACPI_TABLE_PROTOCOL cooperate with
> QEMU's linker/loader is a two-phase process.
>
> In the first phase, the fw_cfg blobs are allocated, downloaded, linked
> together, and checksummed, in AcpiNVS type memory, as instructed by the
> linker/loader script.
>
> In the second phase, all ADD_POINTER commands are processed again, and
> the targets of those pointers are investigated for ACPI SDT headers.
> Every such pointer target that looks like an ACPI SDT header is passed
> to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(); except root tables like
> RSDT etc. This member function handles the actual copying, installation,
> and cross-table linking (to the extent specified in the ACPI spec).
>
> Additionally, for each allocated  / downloaded fw_cfg blob, the second
> phase tracks if there is at least one pointer into the blob that does
> *not* point to an ACPI SDT header. If that's the case, then it implies
> the presence of some non-ACPI-table-buffer within the blob (that is
> referenced by some other field elsewhere). In turn such fw_cfg blobs are
> not freed at the end of the whole process. (If an fw_cfg blob turns out
> to host *only* ACPI tables -- which were then all copied for
> installation, see above -- then the blob is released in the end.)
>
> Finally, EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() may return
> EFI_ACCESS_DENIED (quoting the UEFI spec):
>
> "The table signature matches a table already present in the system and
> platform policy does not allow duplicate tables of this type."
>
> (And see edk2 commit 5966402ed51c, "MdeModulePkg/IntelFrameworkModulePkg
> ACPI: Follow the new UEFI 2.4a spec to return EFI_ACCESS_DENIED for
> duplicated FADT, FACS or DSDT installation.", 2014-05-06.)
>
>
> ... BTW, we discussed the question of multiply-pointed-to tables before,
> in connection with XSDT support. Under "XSDT support", I mean an XSDT
> built by QEMU explicitly (because EFI_ACPI_TABLE_PROTOCOL handles both
> XSDT and RSDT internally, automatically). Clearly, if some ACPI table is
> referenced from QEMU's RSDT and XSDT both, that immediately triggers the
> same problem.
>
> Looking at my older notes, I find two references:
>
> Message-Id: <20150827205706-mutt-send-email-mst@redhat.com>
> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html
>
> Message-Id: <20151228141917-mutt-send-email-mst@redhat.com>
> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg04636.html
>
> Looking further at my notes, I find the following idea:
>
>     in the second pass only, maintain a global rbtree that contains
>     VOID* objects, pointing to actual physcal addresses in the relocated
>     blobs that have already been passed to InstallAcpiTable. This will
>     ensure that no address is passed to InstallAcpiTable twice
>
> IOW, it's simple memoization for ADD_POINTER targets (in absolute
> guest-phys addr space) so that duplicate
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() can be avoided.
>
> If this feature is important, I can file an upstream OVMF bug for it,
> and work on it. (I already have another open BZ for the linker/loader
> anyway, <https://bugzilla.tianocore.org/show_bug.cgi?id=359>.)
>
> Please confirm if this is necessary.

That sounds good. Some background on the specific issue I'm trying to solve:

 - OS X/macOS guests currently can't reboot with upstream Qemu, as it
expects the reset register to be advertised by the FADT, but x86 Qemu
currently only publishes a rev1 FADT, which lacks said register spec.
(OVMF still requires a large array of patches to boot OS X/macOS,
which is a separate issue.)
 - I therefore would like to update Qemu such that it publishes a rev3
(ACPI 2.0) or newer FADT, including the reset register.
 - Windows 10 appears to require both DSDT AND X_DSDT to be filled for
rev3-rev4 FADTs, else it won't boot at all. (Not sure about 5+)
 - Guest OS backwards compatibility without a flag is desirable, and
ACPI 1.0 era guest OSes will not try to find the DSDT via X_DSDT, so
we need to fill both.
 - This should work with both SeaBIOS and OVMF, so Qemu needs to set
up linker commands for both DSDT and X_DSDT, and OVMF itself needs to
produce a FADT with both fields filled.

Your EDK2 patch fixes the problem with values OVMF writes to
DSDT/X_DSDT, but the issue of it refusing Qemu's linker commands for
those two still needs to be solved so that SeaBIOS can boot a wide
range of OSes.

I'd be happy to give writing the memoising patch a go myself if that
helps. Looks like setting up an ORDERED_COLLECTION during phase 2
might be the simplest solution?

>>> 2. After applying all the linker commands, it goes and rewrites part
>>> of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
>>> fields - and it always sets one of them to 0. Which one depends on
>>> whether the DSDT is above the 4G barrier:
>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
>
> Yes. This is precisely what I meant above with:
>
>     EFI_ACPI_TABLE_PROTOCOL (whose implementation is in generic edk2
>     code, not in OVMF platform code) has built-in "smarts" for handling
>     and linking together the various specific tables defined by the
>     ACPI spec.
>
>>>
>>> Both of these are easily fixed, and I will submit a corresponding patch to EDK2.
>
> What easy fixes do you have in mind?
>
> For problem (1), simply ignoring EFI_ACCESS_DENIED from
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() is not right. Unless we can
> ensure in a future-proof way that an error code gets returned for *only
> one* error scenario and nothing else, suppressing the error code is
> risky. I'd much rather prevent a function call (with memoization) that
> we know in advance will fail.

That's been my prototype workaround - the memoisation should be pretty
straightforward too though.

> For symptom (2), I'm unsure if we can call that a problem at all. You'd
> have to prove that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" violates
> the ACPI spec (or that it's within the spec, but works sub-optimally),
> when it ensures exclusivity between FADT.DSDT and FADT.X_DSDT.
>
> The ACPI 6.1 spec says,
>
> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>   field must be zero.
> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>   field must be zero.
>
> Therefore the EFI_ACPI_TABLE_PROTOCOL implementation in edk2 seems
> conformant & correct to me.
>
> Related edk2 commits:
>
> - the one that added the code you reference above:
>
> f9bbb8d9c3f0 ("MdeModulePkg: AcpiTableDxe: make 4 GB table allocation
> limit optional", 2016-02-17)
>
> - the earlier, similar one, that enforces exclusivity between
> FADT.FIRMWARE_CTRL and FADT.X_FIRMWARE_CTRL:
>
> f798e8bff773 ("MdeModulePkg: Acpi: enforce exclusion between
> FirmwareCtrl and XFirmwareCtrl", 2015-01-26)

Between you you've already established the apparent revision-dependent
rules on whether or not DSDT and X_DSDT are mutually exclusive, so I
have nothing to add there, other than that Windows 10 fails to boot if
they're not both filled for rev3 and rev4 FADTs. (I'm so far having
some issues with producing a rev5 FADT it likes, regardless of DSDT
pointers.)

The X_FIRMWARE_CTRL vs FIRMWARE_CTRL situation appears to me to be
slightly different. They're already explicitly mutually exclusive from
ACPI 4.0 onwards, not 5.1. Note however, that it's not specified that
one should be preferred over the other in the sub-4G case, not even in
6.1. Note also that both ACPI 3 and 4 specify FADT revision 4, but
only 4.0 makes the fields mutually exclusive.

I therefore think the safest behaviour for the FIRMWARE_CTRL fields,
also with a view to backwards compatibility, is to always use the
32-bit variant where possible, and the X_ field only if a 64-bit
pointer is necessary. I have thus far encountered no issues with
booting real OSes using this policy.

>>> With that fixed, the rest of the FADT provided by Qemu is accepted by
>>> OVMF and the operating systems. On the Qemu side, it does mean we'll
>>> need to still retain the ACPI 1.0 tables for backwards compatibility.
>>>
>>> Q1: How should the option be structured and named? Should the FADT
>>> revision be selectable via a sub-option on -machine? Or as a
>>> standalone option? Something else?
>
> No input from me on this one.
>
>>> Q2: To avoid any more confusion, I'd appreciate
>>> confirmation/clarification on the X_ and non-X FADT fields in the case
>>> where 32-bit pointers suffice.
>>>
>>> Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
>
> I disagree. The 6.1 release of the ACPI spec requires exclusivity.
>
> The changelog at the top of the spec lists:
>
> Revision    Change Description                         Affected Sections
> ----------  -----------------------------------------  -----------------
> 6.0 Errata  1393 In FADT: if X_DSDT field is           Table 5-34
>             non-zero, DSDT field should be ignored or
>             deprecated
>
> The number "1393" is most likely the Mantis ticket number:
>
> https://mantis.uefi.org/mantis/view.php?id=1393
>
> (Unfortunately, I don't have the necessary credentials to verify if this
> Mantis ticket actually corresponds to this change. I could do that for
> Platform Init or UEFI spec items -- without quoting any specifics --,
> but apparently my ASWG membership level isn't high enough for this one.)
>
>>> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
>
> Sounds good.
>
>>>
>>> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>>> "This is a required field" for both variants.
>
> Hmmm, I don't think so (again, from ACPI 6.1):
>
> - PM1a_EVT_BLK: [...] This is a required field. If the X_PM1a_CNT_BLK
>   field contains a non-zero value then this field must be zero.
> - X_PM1a_EVT_BLK: [...] This is a required field. If the PM1a_EVT_BLK
>   field contains a non-zero value then this field must be zero.
>
> (Not checking the rest.)
>
>>>
>>> Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
>>> is not supported, this field contains zero." - I understand this to
>>> mean that when the register block IS supported and 32-bit, both
>>> variants must be filled.
>>>
>>> In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
>>>
>>>
>>> I'll come up with a revised patch in the next few days.
>>>
>>> Thanks for your help and patience so far, everyone!
>
> In summary, here's my opinion:
>
> - I think that setting both FADT.DSDT and FADT.X_DSDT to nonzero values
> simultaneously would break the ACPI 6.1 spec (update originating from
> Mantis #1393, most likely)
>
> - "MdeModulePkg/Universal/Acpi/AcpiTableDxe", implementing
> EFI_ACPI_TABLE_PROTOCOL in edk2, appears to conform to this requirement
> specifically
>
> - if remedying symptom (1) were only necessary for setting both DSDT and
> X_DSDT -- which, I claim, should not be done --, then I'd like to
> postpone the above memoization indefinitely. It's too complex to be
> added speculatively.
>
> Thank you,
> Laszlo

On 31 January 2017 at 20:08, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/31/17 19:17, Michael S. Tsirkin wrote:
>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
>>> The ACPI 6.1 spec says,
>>>
>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>   field must be zero.
>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>   field must be zero.
>>
>> But that's only 6.1. 6.0 and earlier did not say this.
>> The errata they wanted to address was:
>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>> field should be ignored or deprecated
>>
>> I would class this as a spec bug.
>>
>
> Process-wise, that's not a bad idea; it could be the only way (or the
> best way) to argue for a corresponding change in edk2's
> EFI_ACPI_TABLE_PROTOCOL implementation
> (MdeModulePkg/Universal/Acpi/AcpiTableDxe).
>
> Do you want to raise this on the ASWG list? I vaguely recall that you
> subscribed; if not, I think you should be able to, as a Red Hatter
> <http://members.uefi.org/kmembership_info/person_signup/>.
>
> (I'd like to avoid being the middle man.)
>
> Hm... It seems that the "Adopter Membership" is free, which could be
> appropriate for individual observers:
>
> http://uefi.org/join
> http://members.uefi.org/home/
>
> (Should Phil consider it.)

To be honest, I have no idea - does the revelation of 5.1b's
introduction of the mutual exclusivity, and the fact that you've
written up an edk2 patch change anything? If my signing up for
membership will help with resolving the problem, I'm happy to do it,
I'm just lacking the context to know if this is the case - please let
me know.

Thanks,
Phil

On 02/06/17 17:44, Phil Dennis-Jordan wrote:
> On 31 January 2017 at 20:08, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/31/17 19:17, Michael S. Tsirkin wrote:
>>> On Tue, Jan 31, 2017 at 05:28:57PM +0100, Laszlo Ersek wrote:
>>>> The ACPI 6.1 spec says,
>>>>
>>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>>   field must be zero.
>>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>>   field must be zero.
>>>
>>> But that's only 6.1. 6.0 and earlier did not say this.
>>> The errata they wanted to address was:
>>> 1393 In FADT: if X_DSDT field is non-zero, DSDT
>>> field should be ignored or deprecated
>>>
>>> I would class this as a spec bug.
>>>
>>
>> Process-wise, that's not a bad idea; it could be the only way (or the
>> best way) to argue for a corresponding change in edk2's
>> EFI_ACPI_TABLE_PROTOCOL implementation
>> (MdeModulePkg/Universal/Acpi/AcpiTableDxe).
>>
>> Do you want to raise this on the ASWG list? I vaguely recall that you
>> subscribed; if not, I think you should be able to, as a Red Hatter
>> <http://members.uefi.org/kmembership_info/person_signup/>.
>>
>> (I'd like to avoid being the middle man.)
>>
>> Hm... It seems that the "Adopter Membership" is free, which could be
>> appropriate for individual observers:
>>
>> http://uefi.org/join
>> http://members.uefi.org/home/
>>
>> (Should Phil consider it.)
> 
> To be honest, I have no idea - does the revelation of 5.1b's
> introduction of the mutual exclusivity, and the fact that you've
> written up an edk2 patch change anything? If my signing up for
> membership will help with resolving the problem, I'm happy to do it,
> I'm just lacking the context to know if this is the case - please let
> me know.

I'm neither encouraging you to, nor discouraging you from, joining the
ASWG :) I just wanted to share the details that I managed to find,
should you want to join the ASWG in order to (co-)champion the question
(with Michael).

For example, elsewhere you mention that Windows 10 "insist[s] on both
DSDT and X_DSDT" [1]. That is somewhat in conflict with the most recent
spec requirements that both fields be exclusive... If you've seen this
happen first hand, that can be a strong argument to make.

[1] https://lists.01.org/pipermail/edk2-devel/2017-February/007072.html

Thanks,
Laszlo

On 02/06/17 17:30, Phil Dennis-Jordan wrote:
> Thanks for the in-depth reply - apologies for abandoning the thread
> for a few days. Looks like a bunch of things have already been hashed
> out, but I have a few more comments:

Vice versa... Sorry about responding a bit late. Yesterday I severely
overworked myself and for all of today I've been struggling with a
terrible headache. Having had trouble at forming coherent thoughts, I've
sort of procrastinated following up here.

> On 31 January 2017 at 17:28, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/31/17 15:58, Michael S. Tsirkin wrote:
>>> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>>>> On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>> On Wed, 18 Jan 2017 18:30:59 +0200
>>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>
>>>>>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
>>>>> [...]
>>>>>
>>>>>>> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
>>>>>>
>>>>>> I would say an option to make FADT use rev 3 format would be a good
>>>>>> idea.
>>>>>>
>>>>>> I'd make it the default if XP survives.
>>>>> if XP and legacy linux survive,
>>>>> I'd skip adding option as probably there won't be any users,
>>>>> in unlikely case such user surfaces we always can add option later
>>>>> but we can't do it other way around (i.e. take it away).
>>>>
>>>> I have now finally solved the mystery of why my FADT patch has been
>>>> going so disastrously wrong - I've now got working code, but I'd
>>>> appreciate some guidance on the best way to structure a patch to
>>>> minimise further back-and forth.
>>>
>>> + lersek
>>>
>>>> The culprit turned out to be OVMF,
>>>> specifically 2 bugs/shortcomings:
>>>>
>>>> 1. It completely gives up on parsing Qemu's ACPI tables if more than
>>>> one "add pointer" linker command points to the same table. In this
>>>> case, if you add a command for both the DSDT and X_DSDT fields of the
>>>> FADT, it aborts completely and uses fallback tables. (The following
>>>> InstallAcpiTable call fails if called twice with the same table type.)
>>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
>>
>> Unlike SeaBIOS, OVMF can only use EFI_ACPI_TABLE_PROTOCOL to install
>> ACPI tables.
>>
>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs a *copy* of the
>> table that is passed in. In addition, EFI_ACPI_TABLE_PROTOCOL (whose
>> implementation is in generic edk2 code, not in OVMF platform code) has
>> built-in "smarts" for handling and linking together the various specific
>> tables defined by the ACPI spec.
>>
>> The way we were able to make EFI_ACPI_TABLE_PROTOCOL cooperate with
>> QEMU's linker/loader is a two-phase process.
>>
>> In the first phase, the fw_cfg blobs are allocated, downloaded, linked
>> together, and checksummed, in AcpiNVS type memory, as instructed by the
>> linker/loader script.
>>
>> In the second phase, all ADD_POINTER commands are processed again, and
>> the targets of those pointers are investigated for ACPI SDT headers.
>> Every such pointer target that looks like an ACPI SDT header is passed
>> to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(); except root tables like
>> RSDT etc. This member function handles the actual copying, installation,
>> and cross-table linking (to the extent specified in the ACPI spec).
>>
>> Additionally, for each allocated  / downloaded fw_cfg blob, the second
>> phase tracks if there is at least one pointer into the blob that does
>> *not* point to an ACPI SDT header. If that's the case, then it implies
>> the presence of some non-ACPI-table-buffer within the blob (that is
>> referenced by some other field elsewhere). In turn such fw_cfg blobs are
>> not freed at the end of the whole process. (If an fw_cfg blob turns out
>> to host *only* ACPI tables -- which were then all copied for
>> installation, see above -- then the blob is released in the end.)
>>
>> Finally, EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() may return
>> EFI_ACCESS_DENIED (quoting the UEFI spec):
>>
>> "The table signature matches a table already present in the system and
>> platform policy does not allow duplicate tables of this type."
>>
>> (And see edk2 commit 5966402ed51c, "MdeModulePkg/IntelFrameworkModulePkg
>> ACPI: Follow the new UEFI 2.4a spec to return EFI_ACCESS_DENIED for
>> duplicated FADT, FACS or DSDT installation.", 2014-05-06.)
>>
>>
>> ... BTW, we discussed the question of multiply-pointed-to tables before,
>> in connection with XSDT support. Under "XSDT support", I mean an XSDT
>> built by QEMU explicitly (because EFI_ACPI_TABLE_PROTOCOL handles both
>> XSDT and RSDT internally, automatically). Clearly, if some ACPI table is
>> referenced from QEMU's RSDT and XSDT both, that immediately triggers the
>> same problem.
>>
>> Looking at my older notes, I find two references:
>>
>> Message-Id: <20150827205706-mutt-send-email-mst@redhat.com>
>> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html
>>
>> Message-Id: <20151228141917-mutt-send-email-mst@redhat.com>
>> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg04636.html
>>
>> Looking further at my notes, I find the following idea:
>>
>>     in the second pass only, maintain a global rbtree that contains
>>     VOID* objects, pointing to actual physcal addresses in the relocated
>>     blobs that have already been passed to InstallAcpiTable. This will
>>     ensure that no address is passed to InstallAcpiTable twice
>>
>> IOW, it's simple memoization for ADD_POINTER targets (in absolute
>> guest-phys addr space) so that duplicate
>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() can be avoided.
>>
>> If this feature is important, I can file an upstream OVMF bug for it,
>> and work on it. (I already have another open BZ for the linker/loader
>> anyway, <https://bugzilla.tianocore.org/show_bug.cgi?id=359>.)
>>
>> Please confirm if this is necessary.
> 
> That sounds good. Some background on the specific issue I'm trying to solve:
> 
>  - OS X/macOS guests currently can't reboot with upstream Qemu, as it
> expects the reset register to be advertised by the FADT, but x86 Qemu
> currently only publishes a rev1 FADT, which lacks said register spec.
> (OVMF still requires a large array of patches to boot OS X/macOS,
> which is a separate issue.)
>  - I therefore would like to update Qemu such that it publishes a rev3
> (ACPI 2.0) or newer FADT, including the reset register.
>  - Windows 10 appears to require both DSDT AND X_DSDT to be filled for
> rev3-rev4 FADTs, else it won't boot at all. (Not sure about 5+)
>  - Guest OS backwards compatibility without a flag is desirable, and
> ACPI 1.0 era guest OSes will not try to find the DSDT via X_DSDT, so
> we need to fill both.
>  - This should work with both SeaBIOS and OVMF, so Qemu needs to set
> up linker commands for both DSDT and X_DSDT, and OVMF itself needs to
> produce a FADT with both fields filled.
> 

I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=368>.

> Your EDK2 patch

For the record:
[1] https://lists.01.org/pipermail/edk2-devel/2017-February/007072.html

> fixes the problem with values OVMF writes to
> DSDT/X_DSDT, but the issue of it refusing Qemu's linker commands for
> those two still needs to be solved so that SeaBIOS can boot a wide
> range of OSes.
> 
> I'd be happy to give writing the memoising patch a go myself if that
> helps. Looks like setting up an ORDERED_COLLECTION during phase 2
> might be the simplest solution?

Thanks for the offer. :)

* If you'd like to help me with my load and reach a good "development
  throughput", then I prefer to write the patch myself. On this
  *specific* occasion, I think it will be faster.

  I intend to send an OVMF series this week that addresses both
  TianoCore#368 (see above) and TianoCore#359 too (mentioned earlier).
  Both are for OvmfPkg/AcpiPlatformDxe, and it makes sense to round
  them up.

  I plan to CC the QEMU stakeholders as appropriate. Your feedback
  would be greatly appreciated; the same way as [1] is very helpful
  (thanks again for it!)

* If your main interest is rather to get into OVMF development, then I
  positively welcome that, and encourage you to send the patch.
  Completing the patch will likely take longer (the edk2 coding style
  is... arcane... and your reviewer is somewhat pedantic :)), but if
  you plan to get into OVMF development, then it's worth both our times.

(The above should not be misunderstood as "Laszlo doesn't value one-off
contributions" -- it really depends on feature size and area. Hence the
emphasis on "specific" above.)

Your call :) If possible, please let me know it tomorrow.

> 
>>>> 2. After applying all the linker commands, it goes and rewrites part
>>>> of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
>>>> fields - and it always sets one of them to 0. Which one depends on
>>>> whether the DSDT is above the 4G barrier:
>>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
>>
>> Yes. This is precisely what I meant above with:
>>
>>     EFI_ACPI_TABLE_PROTOCOL (whose implementation is in generic edk2
>>     code, not in OVMF platform code) has built-in "smarts" for handling
>>     and linking together the various specific tables defined by the
>>     ACPI spec.
>>
>>>>
>>>> Both of these are easily fixed, and I will submit a corresponding patch to EDK2.
>>
>> What easy fixes do you have in mind?
>>
>> For problem (1), simply ignoring EFI_ACCESS_DENIED from
>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() is not right. Unless we can
>> ensure in a future-proof way that an error code gets returned for *only
>> one* error scenario and nothing else, suppressing the error code is
>> risky. I'd much rather prevent a function call (with memoization) that
>> we know in advance will fail.
> 
> That's been my prototype workaround - the memoisation should be pretty
> straightforward too though.

It's also more correct for ACPI table *types* that are permitted to have
several (different!) instances installed. For those table types, passing
the exact same table to InstallAcpiTable() might succeed -- and that
would be even worse. I commented on this in TianoCore#368.

> 
>> For symptom (2), I'm unsure if we can call that a problem at all. You'd
>> have to prove that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" violates
>> the ACPI spec (or that it's within the spec, but works sub-optimally),
>> when it ensures exclusivity between FADT.DSDT and FADT.X_DSDT.
>>
>> The ACPI 6.1 spec says,
>>
>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>   field must be zero.
>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>   field must be zero.
>>
>> Therefore the EFI_ACPI_TABLE_PROTOCOL implementation in edk2 seems
>> conformant & correct to me.
>>
>> Related edk2 commits:
>>
>> - the one that added the code you reference above:
>>
>> f9bbb8d9c3f0 ("MdeModulePkg: AcpiTableDxe: make 4 GB table allocation
>> limit optional", 2016-02-17)
>>
>> - the earlier, similar one, that enforces exclusivity between
>> FADT.FIRMWARE_CTRL and FADT.X_FIRMWARE_CTRL:
>>
>> f798e8bff773 ("MdeModulePkg: Acpi: enforce exclusion between
>> FirmwareCtrl and XFirmwareCtrl", 2015-01-26)
> 
> Between you you've already established the apparent revision-dependent
> rules on whether or not DSDT and X_DSDT are mutually exclusive, so I
> have nothing to add there, other than that Windows 10 fails to boot if
> they're not both filled for rev3 and rev4 FADTs. (I'm so far having
> some issues with producing a rev5 FADT it likes, regardless of DSDT
> pointers.)

I think it's prudent to stick with as low a version as possible.

> 
> The X_FIRMWARE_CTRL vs FIRMWARE_CTRL situation appears to me to be
> slightly different. They're already explicitly mutually exclusive from
> ACPI 4.0 onwards, not 5.1. Note however, that it's not specified that
> one should be preferred over the other in the sub-4G case, not even in
> 6.1. Note also that both ACPI 3 and 4 specify FADT revision 4, but
> only 4.0 makes the fields mutually exclusive.
> 
> I therefore think the safest behaviour for the FIRMWARE_CTRL fields,
> also with a view to backwards compatibility, is to always use the
> 32-bit variant where possible, and the X_ field only if a 64-bit
> pointer is necessary. I have thus far encountered no issues with
> booting real OSes using this policy.

Sounds good to me.

And, I think the edk2 code already does this. Commit f798e8bff773
("MdeModulePkg: Acpi: enforce exclusion between FirmwareCtrl and
XFirmwareCtrl", 2015-01-26) is one half of the puzzle.

The other half is that MdeModulePkg/Universal/Acpi/AcpiTableDxe, as
built into OVMF (Ia32 and X64), keeps the tables under 4GB. So in
practice it's always the 32-bit FIRMWARE_CTRL field that gets set.

Thanks!
Laszlo

> 
>>>> With that fixed, the rest of the FADT provided by Qemu is accepted by
>>>> OVMF and the operating systems. On the Qemu side, it does mean we'll
>>>> need to still retain the ACPI 1.0 tables for backwards compatibility.
>>>>
>>>> Q1: How should the option be structured and named? Should the FADT
>>>> revision be selectable via a sub-option on -machine? Or as a
>>>> standalone option? Something else?
>>
>> No input from me on this one.
>>
>>>> Q2: To avoid any more confusion, I'd appreciate
>>>> confirmation/clarification on the X_ and non-X FADT fields in the case
>>>> where 32-bit pointers suffice.
>>>>
>>>> Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
>>
>> I disagree. The 6.1 release of the ACPI spec requires exclusivity.
>>
>> The changelog at the top of the spec lists:
>>
>> Revision    Change Description                         Affected Sections
>> ----------  -----------------------------------------  -----------------
>> 6.0 Errata  1393 In FADT: if X_DSDT field is           Table 5-34
>>             non-zero, DSDT field should be ignored or
>>             deprecated
>>
>> The number "1393" is most likely the Mantis ticket number:
>>
>> https://mantis.uefi.org/mantis/view.php?id=1393
>>
>> (Unfortunately, I don't have the necessary credentials to verify if this
>> Mantis ticket actually corresponds to this change. I could do that for
>> Platform Init or UEFI spec items -- without quoting any specifics --,
>> but apparently my ASWG membership level isn't high enough for this one.)
>>
>>>> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
>>
>> Sounds good.
>>
>>>>
>>>> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>>>> "This is a required field" for both variants.
>>
>> Hmmm, I don't think so (again, from ACPI 6.1):
>>
>> - PM1a_EVT_BLK: [...] This is a required field. If the X_PM1a_CNT_BLK
>>   field contains a non-zero value then this field must be zero.
>> - X_PM1a_EVT_BLK: [...] This is a required field. If the PM1a_EVT_BLK
>>   field contains a non-zero value then this field must be zero.
>>
>> (Not checking the rest.)
>>
>>>>
>>>> Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
>>>> is not supported, this field contains zero." - I understand this to
>>>> mean that when the register block IS supported and 32-bit, both
>>>> variants must be filled.
>>>>
>>>> In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
>>>>
>>>>
>>>> I'll come up with a revised patch in the next few days.
>>>>
>>>> Thanks for your help and patience so far, everyone!
>>
>> In summary, here's my opinion:
>>
>> - I think that setting both FADT.DSDT and FADT.X_DSDT to nonzero values
>> simultaneously would break the ACPI 6.1 spec (update originating from
>> Mantis #1393, most likely)
>>
>> - "MdeModulePkg/Universal/Acpi/AcpiTableDxe", implementing
>> EFI_ACPI_TABLE_PROTOCOL in edk2, appears to conform to this requirement
>> specifically
>>
>> - if remedying symptom (1) were only necessary for setting both DSDT and
>> X_DSDT -- which, I claim, should not be done --, then I'd like to
>> postpone the above memoization indefinitely. It's too complex to be
>> added speculatively.
>>
>> Thank you,
>> Laszlo
>

On 7 February 2017 at 20:54, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/06/17 17:30, Phil Dennis-Jordan wrote:
>> Thanks for the in-depth reply - apologies for abandoning the thread
>> for a few days. Looks like a bunch of things have already been hashed
>> out, but I have a few more comments:
>
> Vice versa... Sorry about responding a bit late. Yesterday I severely
> overworked myself and for all of today I've been struggling with a
> terrible headache. Having had trouble at forming coherent thoughts, I've
> sort of procrastinated following up here.

No worries, I can't spend time every day on this either.

>> On 31 January 2017 at 17:28, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 01/31/17 15:58, Michael S. Tsirkin wrote:
>>>> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>>>>> On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
>>>>>> On Wed, 18 Jan 2017 18:30:59 +0200
>>>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>>
>>>>>>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
>>>>>> [...]
>>>>>>
>>>>>>>> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?
>>>>>>>
>>>>>>> I would say an option to make FADT use rev 3 format would be a good
>>>>>>> idea.
>>>>>>>
>>>>>>> I'd make it the default if XP survives.
>>>>>> if XP and legacy linux survive,
>>>>>> I'd skip adding option as probably there won't be any users,
>>>>>> in unlikely case such user surfaces we always can add option later
>>>>>> but we can't do it other way around (i.e. take it away).
>>>>>
>>>>> I have now finally solved the mystery of why my FADT patch has been
>>>>> going so disastrously wrong - I've now got working code, but I'd
>>>>> appreciate some guidance on the best way to structure a patch to
>>>>> minimise further back-and forth.
>>>>
>>>> + lersek
>>>>
>>>>> The culprit turned out to be OVMF,
>>>>> specifically 2 bugs/shortcomings:
>>>>>
>>>>> 1. It completely gives up on parsing Qemu's ACPI tables if more than
>>>>> one "add pointer" linker command points to the same table. In this
>>>>> case, if you add a command for both the DSDT and X_DSDT fields of the
>>>>> FADT, it aborts completely and uses fallback tables. (The following
>>>>> InstallAcpiTable call fails if called twice with the same table type.)
>>>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518
>>>
>>> Unlike SeaBIOS, OVMF can only use EFI_ACPI_TABLE_PROTOCOL to install
>>> ACPI tables.
>>>
>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs a *copy* of the
>>> table that is passed in. In addition, EFI_ACPI_TABLE_PROTOCOL (whose
>>> implementation is in generic edk2 code, not in OVMF platform code) has
>>> built-in "smarts" for handling and linking together the various specific
>>> tables defined by the ACPI spec.
>>>
>>> The way we were able to make EFI_ACPI_TABLE_PROTOCOL cooperate with
>>> QEMU's linker/loader is a two-phase process.
>>>
>>> In the first phase, the fw_cfg blobs are allocated, downloaded, linked
>>> together, and checksummed, in AcpiNVS type memory, as instructed by the
>>> linker/loader script.
>>>
>>> In the second phase, all ADD_POINTER commands are processed again, and
>>> the targets of those pointers are investigated for ACPI SDT headers.
>>> Every such pointer target that looks like an ACPI SDT header is passed
>>> to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(); except root tables like
>>> RSDT etc. This member function handles the actual copying, installation,
>>> and cross-table linking (to the extent specified in the ACPI spec).
>>>
>>> Additionally, for each allocated  / downloaded fw_cfg blob, the second
>>> phase tracks if there is at least one pointer into the blob that does
>>> *not* point to an ACPI SDT header. If that's the case, then it implies
>>> the presence of some non-ACPI-table-buffer within the blob (that is
>>> referenced by some other field elsewhere). In turn such fw_cfg blobs are
>>> not freed at the end of the whole process. (If an fw_cfg blob turns out
>>> to host *only* ACPI tables -- which were then all copied for
>>> installation, see above -- then the blob is released in the end.)
>>>
>>> Finally, EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() may return
>>> EFI_ACCESS_DENIED (quoting the UEFI spec):
>>>
>>> "The table signature matches a table already present in the system and
>>> platform policy does not allow duplicate tables of this type."
>>>
>>> (And see edk2 commit 5966402ed51c, "MdeModulePkg/IntelFrameworkModulePkg
>>> ACPI: Follow the new UEFI 2.4a spec to return EFI_ACCESS_DENIED for
>>> duplicated FADT, FACS or DSDT installation.", 2014-05-06.)
>>>
>>>
>>> ... BTW, we discussed the question of multiply-pointed-to tables before,
>>> in connection with XSDT support. Under "XSDT support", I mean an XSDT
>>> built by QEMU explicitly (because EFI_ACPI_TABLE_PROTOCOL handles both
>>> XSDT and RSDT internally, automatically). Clearly, if some ACPI table is
>>> referenced from QEMU's RSDT and XSDT both, that immediately triggers the
>>> same problem.
>>>
>>> Looking at my older notes, I find two references:
>>>
>>> Message-Id: <20150827205706-mutt-send-email-mst@redhat.com>
>>> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html
>>>
>>> Message-Id: <20151228141917-mutt-send-email-mst@redhat.com>
>>> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg04636.html
>>>
>>> Looking further at my notes, I find the following idea:
>>>
>>>     in the second pass only, maintain a global rbtree that contains
>>>     VOID* objects, pointing to actual physcal addresses in the relocated
>>>     blobs that have already been passed to InstallAcpiTable. This will
>>>     ensure that no address is passed to InstallAcpiTable twice
>>>
>>> IOW, it's simple memoization for ADD_POINTER targets (in absolute
>>> guest-phys addr space) so that duplicate
>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() can be avoided.
>>>
>>> If this feature is important, I can file an upstream OVMF bug for it,
>>> and work on it. (I already have another open BZ for the linker/loader
>>> anyway, <https://bugzilla.tianocore.org/show_bug.cgi?id=359>.)
>>>
>>> Please confirm if this is necessary.
>>
>> That sounds good. Some background on the specific issue I'm trying to solve:
>>
>>  - OS X/macOS guests currently can't reboot with upstream Qemu, as it
>> expects the reset register to be advertised by the FADT, but x86 Qemu
>> currently only publishes a rev1 FADT, which lacks said register spec.
>> (OVMF still requires a large array of patches to boot OS X/macOS,
>> which is a separate issue.)
>>  - I therefore would like to update Qemu such that it publishes a rev3
>> (ACPI 2.0) or newer FADT, including the reset register.
>>  - Windows 10 appears to require both DSDT AND X_DSDT to be filled for
>> rev3-rev4 FADTs, else it won't boot at all. (Not sure about 5+)
>>  - Guest OS backwards compatibility without a flag is desirable, and
>> ACPI 1.0 era guest OSes will not try to find the DSDT via X_DSDT, so
>> we need to fill both.
>>  - This should work with both SeaBIOS and OVMF, so Qemu needs to set
>> up linker commands for both DSDT and X_DSDT, and OVMF itself needs to
>> produce a FADT with both fields filled.
>>
>
> I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=368>.

That looks nice and thorough.

>> Your EDK2 patch
>
> For the record:
> [1] https://lists.01.org/pipermail/edk2-devel/2017-February/007072.html
>
>> fixes the problem with values OVMF writes to
>> DSDT/X_DSDT, but the issue of it refusing Qemu's linker commands for
>> those two still needs to be solved so that SeaBIOS can boot a wide
>> range of OSes.
>>
>> I'd be happy to give writing the memoising patch a go myself if that
>> helps. Looks like setting up an ORDERED_COLLECTION during phase 2
>> might be the simplest solution?
>
> Thanks for the offer. :)
>
> * If you'd like to help me with my load and reach a good "development
>   throughput", then I prefer to write the patch myself. On this
>   *specific* occasion, I think it will be faster.
>
>   I intend to send an OVMF series this week that addresses both
>   TianoCore#368 (see above) and TianoCore#359 too (mentioned earlier).
>   Both are for OvmfPkg/AcpiPlatformDxe, and it makes sense to round
>   them up.
>
>   I plan to CC the QEMU stakeholders as appropriate. Your feedback
>   would be greatly appreciated; the same way as [1] is very helpful
>   (thanks again for it!)
>
> * If your main interest is rather to get into OVMF development, then I
>   positively welcome that, and encourage you to send the patch.
>   Completing the patch will likely take longer (the edk2 coding style
>   is... arcane... and your reviewer is somewhat pedantic :)), but if
>   you plan to get into OVMF development, then it's worth both our times.
>
> (The above should not be misunderstood as "Laszlo doesn't value one-off
> contributions" -- it really depends on feature size and area. Hence the
> emphasis on "specific" above.)
>
> Your call :) If possible, please let me know it tomorrow.

I actually ended up writing said pointer memoisation patch yesterday,
and it appears to work fine in initial tests. I still need to fully
work my way through the edk2 coding and contribution guidelines, so
I'll need to re-visit that patch with a fine tooth comb before
submitting it. In any case, here it is so far:
https://github.com/pmj/edk2/commit/58e0510c6da62d5a985b97e9bff84bc53442d3fe

I am intending to submit more patches to edk2 over time - like this
one, they'll mostly be based on Reza Jelveh's GSoC project from a few
years ago. Some of his work on getting OS X/macOS guests working in
Qemu/OVMF have got upstreamed, most of it has not. I'm hoping I can
improve the ratio a little.

I've also written an EFI framebuffer driver for the VMware virtual
SVGA adapter in Qemu, which again I need to tidy up to conform with
the coding conventions before submitting. (The only advantage of this
vs. QXL or virtio-gpu being that there are OSes, including OSX/macOS
for which there exist drivers for neither QXL nor virtio-gpu.)

So I guess I may as well get some practice. :-)


I anticipate submitting the memoisation patch for review on Thursday
or Friday as I'll be out tomorrow.

>>>>> 2. After applying all the linker commands, it goes and rewrites part
>>>>> of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
>>>>> fields - and it always sets one of them to 0. Which one depends on
>>>>> whether the DSDT is above the 4G barrier:
>>>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650
>>>
>>> Yes. This is precisely what I meant above with:
>>>
>>>     EFI_ACPI_TABLE_PROTOCOL (whose implementation is in generic edk2
>>>     code, not in OVMF platform code) has built-in "smarts" for handling
>>>     and linking together the various specific tables defined by the
>>>     ACPI spec.
>>>
>>>>>
>>>>> Both of these are easily fixed, and I will submit a corresponding patch to EDK2.
>>>
>>> What easy fixes do you have in mind?
>>>
>>> For problem (1), simply ignoring EFI_ACCESS_DENIED from
>>> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() is not right. Unless we can
>>> ensure in a future-proof way that an error code gets returned for *only
>>> one* error scenario and nothing else, suppressing the error code is
>>> risky. I'd much rather prevent a function call (with memoization) that
>>> we know in advance will fail.
>>
>> That's been my prototype workaround - the memoisation should be pretty
>> straightforward too though.
>
> It's also more correct for ACPI table *types* that are permitted to have
> several (different!) instances installed. For those table types, passing
> the exact same table to InstallAcpiTable() might succeed -- and that
> would be even worse. I commented on this in TianoCore#368.

Yes, good point.

>>> For symptom (2), I'm unsure if we can call that a problem at all. You'd
>>> have to prove that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" violates
>>> the ACPI spec (or that it's within the spec, but works sub-optimally),
>>> when it ensures exclusivity between FADT.DSDT and FADT.X_DSDT.
>>>
>>> The ACPI 6.1 spec says,
>>>
>>> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>>>   field must be zero.
>>> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>>>   field must be zero.
>>>
>>> Therefore the EFI_ACPI_TABLE_PROTOCOL implementation in edk2 seems
>>> conformant & correct to me.
>>>
>>> Related edk2 commits:
>>>
>>> - the one that added the code you reference above:
>>>
>>> f9bbb8d9c3f0 ("MdeModulePkg: AcpiTableDxe: make 4 GB table allocation
>>> limit optional", 2016-02-17)
>>>
>>> - the earlier, similar one, that enforces exclusivity between
>>> FADT.FIRMWARE_CTRL and FADT.X_FIRMWARE_CTRL:
>>>
>>> f798e8bff773 ("MdeModulePkg: Acpi: enforce exclusion between
>>> FirmwareCtrl and XFirmwareCtrl", 2015-01-26)
>>
>> Between you you've already established the apparent revision-dependent
>> rules on whether or not DSDT and X_DSDT are mutually exclusive, so I
>> have nothing to add there, other than that Windows 10 fails to boot if
>> they're not both filled for rev3 and rev4 FADTs. (I'm so far having
>> some issues with producing a rev5 FADT it likes, regardless of DSDT
>> pointers.)
>
> I think it's prudent to stick with as low a version as possible.

I've got a Qemu patch for a changing the FADT from rev1 to rev3 (ACPI
2.0); I'll post that Thursday or Friday as well assuming all the guest
OSes I can throw at it are happy.

>> The X_FIRMWARE_CTRL vs FIRMWARE_CTRL situation appears to me to be
>> slightly different. They're already explicitly mutually exclusive from
>> ACPI 4.0 onwards, not 5.1. Note however, that it's not specified that
>> one should be preferred over the other in the sub-4G case, not even in
>> 6.1. Note also that both ACPI 3 and 4 specify FADT revision 4, but
>> only 4.0 makes the fields mutually exclusive.
>>
>> I therefore think the safest behaviour for the FIRMWARE_CTRL fields,
>> also with a view to backwards compatibility, is to always use the
>> 32-bit variant where possible, and the X_ field only if a 64-bit
>> pointer is necessary. I have thus far encountered no issues with
>> booting real OSes using this policy.
>
> Sounds good to me.
>
> And, I think the edk2 code already does this. Commit f798e8bff773
> ("MdeModulePkg: Acpi: enforce exclusion between FirmwareCtrl and
> XFirmwareCtrl", 2015-01-26) is one half of the puzzle.
>
> The other half is that MdeModulePkg/Universal/Acpi/AcpiTableDxe, as
> built into OVMF (Ia32 and X64), keeps the tables under 4GB. So in
> practice it's always the 32-bit FIRMWARE_CTRL field that gets set.

I haven't reviewed the corresponding code, but I certainly haven't run
into any issues with its behaviour in practice so far.

>>>>> With that fixed, the rest of the FADT provided by Qemu is accepted by
>>>>> OVMF and the operating systems. On the Qemu side, it does mean we'll
>>>>> need to still retain the ACPI 1.0 tables for backwards compatibility.
>>>>>
>>>>> Q1: How should the option be structured and named? Should the FADT
>>>>> revision be selectable via a sub-option on -machine? Or as a
>>>>> standalone option? Something else?
>>>
>>> No input from me on this one.
>>>
>>>>> Q2: To avoid any more confusion, I'd appreciate
>>>>> confirmation/clarification on the X_ and non-X FADT fields in the case
>>>>> where 32-bit pointers suffice.
>>>>>
>>>>> Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.
>>>
>>> I disagree. The 6.1 release of the ACPI spec requires exclusivity.
>>>
>>> The changelog at the top of the spec lists:
>>>
>>> Revision    Change Description                         Affected Sections
>>> ----------  -----------------------------------------  -----------------
>>> 6.0 Errata  1393 In FADT: if X_DSDT field is           Table 5-34
>>>             non-zero, DSDT field should be ignored or
>>>             deprecated
>>>
>>> The number "1393" is most likely the Mantis ticket number:
>>>
>>> https://mantis.uefi.org/mantis/view.php?id=1393
>>>
>>> (Unfortunately, I don't have the necessary credentials to verify if this
>>> Mantis ticket actually corresponds to this change. I could do that for
>>> Platform Init or UEFI spec items -- without quoting any specifics --,
>>> but apparently my ASWG membership level isn't high enough for this one.)
>>>
>>>>> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
>>>
>>> Sounds good.
>>>
>>>>>
>>>>> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>>>>> "This is a required field" for both variants.
>>>
>>> Hmmm, I don't think so (again, from ACPI 6.1):
>>>
>>> - PM1a_EVT_BLK: [...] This is a required field. If the X_PM1a_CNT_BLK
>>>   field contains a non-zero value then this field must be zero.
>>> - X_PM1a_EVT_BLK: [...] This is a required field. If the PM1a_EVT_BLK
>>>   field contains a non-zero value then this field must be zero.
>>>
>>> (Not checking the rest.)
>>>
>>>>>
>>>>> Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
>>>>> is not supported, this field contains zero." - I understand this to
>>>>> mean that when the register block IS supported and 32-bit, both
>>>>> variants must be filled.
>>>>>
>>>>> In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.
>>>>>
>>>>>
>>>>> I'll come up with a revised patch in the next few days.
>>>>>
>>>>> Thanks for your help and patience so far, everyone!
>>>
>>> In summary, here's my opinion:
>>>
>>> - I think that setting both FADT.DSDT and FADT.X_DSDT to nonzero values
>>> simultaneously would break the ACPI 6.1 spec (update originating from
>>> Mantis #1393, most likely)
>>>
>>> - "MdeModulePkg/Universal/Acpi/AcpiTableDxe", implementing
>>> EFI_ACPI_TABLE_PROTOCOL in edk2, appears to conform to this requirement
>>> specifically
>>>
>>> - if remedying symptom (1) were only necessary for setting both DSDT and
>>> X_DSDT -- which, I claim, should not be done --, then I'd like to
>>> postpone the above memoization indefinitely. It's too complex to be
>>> added speculatively.
>>>
>>> Thank you,
>>> Laszlo
>>
>

On 02/07/17 22:02, Phil Dennis-Jordan wrote:
> On 7 February 2017 at 20:54, Laszlo Ersek <lersek@redhat.com> wrote:

>> I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=368>.
> 
> That looks nice and thorough.
> 
>>> Your EDK2 patch
>>
>> For the record:
>> [1] https://lists.01.org/pipermail/edk2-devel/2017-February/007072.html
>>
>>> fixes the problem with values OVMF writes to
>>> DSDT/X_DSDT, but the issue of it refusing Qemu's linker commands for
>>> those two still needs to be solved so that SeaBIOS can boot a wide
>>> range of OSes.
>>>
>>> I'd be happy to give writing the memoising patch a go myself if that
>>> helps. Looks like setting up an ORDERED_COLLECTION during phase 2
>>> might be the simplest solution?
>>
>> Thanks for the offer. :)
>>
>> * If you'd like to help me with my load and reach a good "development
>>   throughput", then I prefer to write the patch myself. On this
>>   *specific* occasion, I think it will be faster.
>>
>>   I intend to send an OVMF series this week that addresses both
>>   TianoCore#368 (see above) and TianoCore#359 too (mentioned earlier).
>>   Both are for OvmfPkg/AcpiPlatformDxe, and it makes sense to round
>>   them up.
>>
>>   I plan to CC the QEMU stakeholders as appropriate. Your feedback
>>   would be greatly appreciated; the same way as [1] is very helpful
>>   (thanks again for it!)
>>
>> * If your main interest is rather to get into OVMF development, then I
>>   positively welcome that, and encourage you to send the patch.
>>   Completing the patch will likely take longer (the edk2 coding style
>>   is... arcane... and your reviewer is somewhat pedantic :)), but if
>>   you plan to get into OVMF development, then it's worth both our times.
>>
>> (The above should not be misunderstood as "Laszlo doesn't value one-off
>> contributions" -- it really depends on feature size and area. Hence the
>> emphasis on "specific" above.)
>>
>> Your call :) If possible, please let me know it tomorrow.
> 
> I actually ended up writing said pointer memoisation patch yesterday,
> and it appears to work fine in initial tests. I still need to fully
> work my way through the edk2 coding and contribution guidelines, so
> I'll need to re-visit that patch with a fine tooth comb before
> submitting it. In any case, here it is so far:
> https://github.com/pmj/edk2/commit/58e0510c6da62d5a985b97e9bff84bc53442d3fe
> 
> I am intending to submit more patches to edk2 over time - like this
> one, they'll mostly be based on Reza Jelveh's GSoC project from a few
> years ago. Some of his work on getting OS X/macOS guests working in
> Qemu/OVMF have got upstreamed, most of it has not. I'm hoping I can
> improve the ratio a little.
> 
> I've also written an EFI framebuffer driver for the VMware virtual
> SVGA adapter in Qemu, which again I need to tidy up to conform with
> the coding conventions before submitting. (The only advantage of this
> vs. QXL or virtio-gpu being that there are OSes, including OSX/macOS
> for which there exist drivers for neither QXL nor virtio-gpu.)
> 
> So I guess I may as well get some practice. :-)
> 
> 
> I anticipate submitting the memoisation patch for review on Thursday
> or Friday as I'll be out tomorrow.

Sounds good to me!

Thanks,
Laszlo