Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Zeng, Star]

Quoted from UEFI spec:
Configuration Table Groups
The GUID for a configuration table also defines a corresponding event group 
GUID with the same
value . If the data represented by a configuration table is changed,
InstallConfigurationTable() should be called. When
InstallConfigurationTable() is called, the corresponding event is signaled. 
When this
event is signaled, any components that cache information from the configuration 
table can optionally
update their cached state.
For example, EFI_ACPI_TABLE_GUID defines a configuration table for ACPI data. 
*When ACPI
data is changed, InstallConfigurationTable() is called*. During the execution of
InstallConfigurationTable(), a corresponding event group with
EFI_ACPI_TABLE_GUID is signaled, allowing an application to invalidate any 
cached ACPI
data.

The AcpiTableDxe in MdeModulePkg was ever updated to follow this rule to 
install configuration table for every data change.
But AcpiSupportDxe in IntelFrameworkModulePkg was not updated because intel 
framework spec has sentence below that has conflict with the UEFI spec.

Quoted from intel framework spec:
The PublishTables() function installs the ACPI tables for the versions that are 
specified in
Version. No tables are published for Version equal to EFI_ACPI_VERSION_NONE. 
*Once
published, tables will continue to be updated as tables are modified with
EFI_ACPI_SUPPORT_PROTOCOL.SetAcpiTable()*.


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Thursday, December 10, 2015 9:36 AM
To: Kinney, Michael D; Laszlo Ersek
Cc: Paolo Bonzini; Scott Duplichan; edk2-devel@lists.01.org; Zeng, Star
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Yes.

Regards,
Ray


-Original Message-
From: Kinney, Michael D
Sent: Thursday, December 10, 2015 9:33 AM
To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
Michael D <michael.d.kin...@intel.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
<star.z...@intel.com>
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Ray,

Do you prefer PCD solution?

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, December 9, 2015 4:45 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
> <ler...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan 
> <sc...@notabs.org>; edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; 
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot 
> hang issue
> 
> Mike,
> The UEFI Spec doesn't require implementation to call 
> InstallConfigurationTable() every time the ACPI table is updated through 
> EFI_ACPI_PROTOCOL.InstallAcpiTable().
> I checked the implementation:
> AcpiSupportDxe in IntelFrameworkModulePkg only calls 
> IntallConfigurationTable() in the first time InstallAcpiTable() is called.
> AcpiTableDxe in MdeModulePkg calls InstallConfigurationTable() every time 
> InstallAcpiTable() is called.
> 
> So we cannot depend on the event notification for EFI_ACPI_TABLE_GUID.
> 
> PI Spec defines a ACPI_SDT protocol (produced by AcpiTableDxe in 
> MdeModulePkg) which supports table installation notification. But it's 
> an optional protocol and not every platform includes AcpiTableDxe to produce 
> this protocol.
> Per my understanding, only EFI_ACPI_PROTOCOL is mandatory.
> 
> Regards,
> Ray
> 
> 
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, December 10, 2015 3:49 AM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; 
> Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan 
> <sc...@notabs.org>; edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; 
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot 
> hang issue
> 
> Ray,
> 
> The UEFI Specification 2.5 p. 136 - CreateEventEx() requires an event 
> group to be signaled every time a configuration table is installed or updated.
> 
> The RTC driver could create an event notification for the EFI_ACPI_TABLE_GUID 
> and cache FADT field in notification function.
> 
> Mike
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Wednesday, December 9, 2015 10:48 AM
> > To: Laszlo Ersek <ler...@redhat.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Paolo Bonzini 
> > <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> > edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Z

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Laszlo Ersek]

On 12/10/15 02:36, Ni, Ruiyu wrote:
> Yes.
> 
> Regards,
> Ray

To summarize the cases in a table:

  CENTURY field in FADT  ACPI & RTC   Win7 accesses  Linux accesses
 specs say
  -  ---  -  --
  0x00   RTC  offset 0x32nothing
 centenary(Win7 bug)
 feature
 unsupported

  0x01..0x7F validthe offset the offset
 offset   given  given

  0x80..0xFF invalid  nothingoffset & 0x7F
 offset  (Linux bug?)

(I'll note that Ray has documented the same earlier, just in a different
format.)

So we can say that there is no way a platform could disable the RTC
centenary feature in the FADT so that *both* Win7 and Linux would adhere
to it.

Bravo, OS designers!

I propose the following (more or less reiterating what Ray said
already):

Patch #1:

- introduce a new PCD in the .dec file, with the table above as
  documentation

- explain that disabling the RTC centenary feature is runtime OS
  dependent, and there is no universally working value

- Explain that it is the platform's responsibility to install a FADT
  that matches the PCD setting.

  If a platform *genuinely* has no support for the centenary feature,
  then it must decide if it wants to support Windows 7, vs. everything
  else, and it must set the PCD, *and* report the lack of said feature
  in the FADT, accordingly. (Ghost writes to 0x32 in the RTC driver are
  not acceptable -- see Scott's comments upthread.) The PCD can be set
  dynamically at each boot, if the platform can determine (possibly from
  explicit user configuration) which OS is going to be booted.

- The default value of the PCD should be 0x32: it matches most current
  PC hardware (it matches QEMU's x86 machine types as well -- see
  RTC_CENTURY), and it will work with both Win7 and Linux.

Patch #2:

- in the RTC driver, if the PCD is in the 0x01..0x7F range, inclusive,
  then store the century value at that offset; otherwise, don't store
  the century value to any offset.

- There is no need to ASSERT() in any case. I think it makes sense to
  enable a century-less platform to set the PCD to 0x80 *and* to
  populate its FADT directly from the PCD, if it knows it is about to
  boot Windows 7.

Notes related to QEMU / OVMF / ArmVirtQemu:

- The "virt" machine type of the arm / aarch64 QEMU targets does not
  include an RTC, and accordingly, the ArmVirtQemu build does not
  include the RTC driver from PcAtChipsetPkg, so there's nothing to do.

- The i440fx / Q35 machine types of the i386 / x86_64 QEMU targets
  emulate the century value at offset 0x32, so the default setting from
  the DEC file will just work.

- *However*, I think I noticed an independent bug in QEMU -- the century
  field is not set in the FADT (it is left at 0) in the i386 ACPI table
  generator. The consequence is that Win7 (incorrectly) falls back to
  the 0x32 offset (which happens to work on QEMU), while Win8 and Linux
  will be misled to think that the centenary feature is not supported.
  (Which might or might not be a grave issue; I don't know.)

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..c5e6c4b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
>  #include "sysemu/tpm_backend.h"
> +#include "hw/timer/mc146818rtc_regs.h"
>
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -334,6 +335,7 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, 
> AcpiPmInfo *pm)
>  if (max_cpus > 8) {
>  fadt->flags |= cpu_to_le32(1 << 
> ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
>  }
> +fadt->century = RTC_CENTURY;
>  }
>
>

Not sure if this fix should be restricted to new machine types.

Thanks
Laszlo


> 
> 
> -Original Message-
> From: Kinney, Michael D 
> Sent: Thursday, December 10, 2015 9:33 AM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
> Michael D <michael.d.kin...@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
> <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Ray,
> 
> Do you prefer PCD solution?
> 
> Mike
> 
>> -Original Message-
>> From: Ni, Ruiyu
>> Sent: Wednesday, December 9, 2015 4:45 PM
>> To: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Laszlo,
Star's mail tells us that if the ACPI table is changed, the configuration table 
should be installed. (AcpiSupportDxe in IntelFrameworkModulePkg seems to have 
bug which needs fix)
So I'd like to hook the ACPI table configuration installation and grabs the 
FADT.Century in the hook-handler.
if it's 0 - D, do nothing. (per RTC spec, 0 - D stores useful information 
defined by spec)
If it's E-7f, writ the century to CMOS
if it's 80 - FF, do nothing.

So that we needn't a PCD. PCD is good but with too many PCDs user can forget to 
configure them.

What's your opinion?

Regards,
Ray


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Thursday, December 10, 2015 7:04 PM
To: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star 
<star.z...@intel.com>; Scott Duplichan <sc...@notabs.org>
Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

On 12/10/15 02:36, Ni, Ruiyu wrote:
> Yes.
> 
> Regards,
> Ray

To summarize the cases in a table:

  CENTURY field in FADT  ACPI & RTC   Win7 accesses  Linux accesses
 specs say
  -  ---  -  --
  0x00   RTC  offset 0x32nothing
 centenary(Win7 bug)
 feature
 unsupported

  0x01..0x7F validthe offset the offset
 offset   given  given

  0x80..0xFF invalid  nothingoffset & 0x7F
 offset  (Linux bug?)

(I'll note that Ray has documented the same earlier, just in a different
format.)

So we can say that there is no way a platform could disable the RTC
centenary feature in the FADT so that *both* Win7 and Linux would adhere
to it.

Bravo, OS designers!

I propose the following (more or less reiterating what Ray said
already):

Patch #1:

- introduce a new PCD in the .dec file, with the table above as
  documentation

- explain that disabling the RTC centenary feature is runtime OS
  dependent, and there is no universally working value

- Explain that it is the platform's responsibility to install a FADT
  that matches the PCD setting.

  If a platform *genuinely* has no support for the centenary feature,
  then it must decide if it wants to support Windows 7, vs. everything
  else, and it must set the PCD, *and* report the lack of said feature
  in the FADT, accordingly. (Ghost writes to 0x32 in the RTC driver are
  not acceptable -- see Scott's comments upthread.) The PCD can be set
  dynamically at each boot, if the platform can determine (possibly from
  explicit user configuration) which OS is going to be booted.

- The default value of the PCD should be 0x32: it matches most current
  PC hardware (it matches QEMU's x86 machine types as well -- see
  RTC_CENTURY), and it will work with both Win7 and Linux.

Patch #2:

- in the RTC driver, if the PCD is in the 0x01..0x7F range, inclusive,
  then store the century value at that offset; otherwise, don't store
  the century value to any offset.

- There is no need to ASSERT() in any case. I think it makes sense to
  enable a century-less platform to set the PCD to 0x80 *and* to
  populate its FADT directly from the PCD, if it knows it is about to
  boot Windows 7.

Notes related to QEMU / OVMF / ArmVirtQemu:

- The "virt" machine type of the arm / aarch64 QEMU targets does not
  include an RTC, and accordingly, the ArmVirtQemu build does not
  include the RTC driver from PcAtChipsetPkg, so there's nothing to do.

- The i440fx / Q35 machine types of the i386 / x86_64 QEMU targets
  emulate the century value at offset 0x32, so the default setting from
  the DEC file will just work.

- *However*, I think I noticed an independent bug in QEMU -- the century
  field is not set in the FADT (it is left at 0) in the i386 ACPI table
  generator. The consequence is that Win7 (incorrectly) falls back to
  the 0x32 offset (which happens to work on QEMU), while Win8 and Linux
  will be misled to think that the centenary feature is not supported.
  (Which might or might not be a grave issue; I don't know.)

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..c5e6c4b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
>  #include "sysemu/tpm_backend.h"
> +#include "hw/timer/mc146818rtc_regs.h"
>
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -334,6 +335,7 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, 
> AcpiPmInfo *pm)
>  if (max_cpus > 8) {
>

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Laszlo Ersek]

On 12/10/15 12:22, Ni, Ruiyu wrote:
> Laszlo,
> Star's mail tells us that if the ACPI table is changed, the configuration 
> table should be installed. (AcpiSupportDxe in IntelFrameworkModulePkg seems 
> to have bug which needs fix)
> So I'd like to hook the ACPI table configuration installation and grabs the 
> FADT.Century in the hook-handler.
> if it's 0 - D, do nothing. (per RTC spec, 0 - D stores useful information 
> defined by spec)
> If it's E-7f, writ the century to CMOS
> if it's 80 - FF, do nothing.
> 
> So that we needn't a PCD. PCD is good but with too many PCDs user can forget 
> to configure them.
> 
> What's your opinion?

Works for me.

Let me explain why I "voted" against the configuration table callback
earlier.

The spec says, under "Configuration Table Groups", *first* paragrah:

The GUID for a configuration table also defines a corresponding
event group GUID with the same value. If the data represented by a
configuration table is changed, InstallConfigurationTable() should
be called. When InstallConfigurationTable() is called, the
corresponding event is signaled. When this event is signaled, any
components that cache information from the configuration table can
optionally update their cached state.

I remembered this. However, this paragraph didn't seem to apply to our
corner case, that is:
(1) a driver installs the FADT -- using the right protocol
(2) some time later, the driver locates the installed FADT,
starting from the RSDP that is linked directly into the system
table, then following pointers to the FADT
(3) changes data in the installed copy of the FADT.

The paragraph quoted above would not cover this corner case, because the
table being changed -- the FADT -- is *not* a configuration table in the
UEFI sense. It is not linked into the system table, with a GUID. The
RSDP is linked, but the above driver wouldn't change the RSDP.

The language above does say

  data represented by a configuration table

but it is not clear that in the case of ACPI tables (which are
recursively linked), the directly linked RSDP stands for the entire ACPI
table tree. In other words, that the "representation" is a logical one
-- it covers the entire data set.

So that's why I called such drivers -- poking at the installed FADT -
"technically correct".

However, I missed the second paragraph (and was reminded of it by Star's
email):

For example, EFI_ACPI_TABLE_GUID defines a configuration table for
ACPI data. When ACPI data is changed, InstallConfigurationTable()
is called. During the execution of InstallConfigurationTable(), a
corresponding event group with EFI_ACPI_TABLE_GUID is signaled,
allowing an application to invalidate any cached ACPI data.

(I also missed that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" was
already implementing this.)

The second paragraph makes it clear that the event group with
EFI_ACPI_TABLE_GUID stands for the full set of ACPI tables.

Furthermore, the second paragraph clarifies that the corner case driver
described above is not "technically correct"; it breaks the spec.
Consequently, if a single-instance ACPI table needs an update, then the
current instance must be first uninstalled, and a brand new instance
must be installed afresh.

Which in turn implies that such a table can only be "refreshed" by the
driver that originally installed it. This is because the
UninstallAcpiTable() interface takes a TableKey parameter that is only
available from the original InstallAcpiTable() call.

Summary: I'm okay with the ACPI installation event callback.

Thanks
Laszlo


> 
> Regards,
> Ray
> 
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Thursday, December 10, 2015 7:04 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star 
> <star.z...@intel.com>; Scott Duplichan <sc...@notabs.org>
> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> On 12/10/15 02:36, Ni, Ruiyu wrote:
>> Yes.
>>
>> Regards,
>> Ray
> 
> To summarize the cases in a table:
> 
>   CENTURY field in FADT  ACPI & RTC   Win7 accesses  Linux accesses
>  specs say
>   -  ---  -  --
>   0x00   RTC  offset 0x32nothing
>  centenary(Win7 bug)
>  feature
>  unsupported
> 
>   0x01..0x7F validthe offset the offset
>  offset   given  given
> 
>   

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Scott,
I debugged the issue further and had the below findings:
According to the ACPI spec 6.0 5.2.9 Fixed ACPI Description Table (FADT), the 
FADT.Century can be set to 0 indicating the RTC doesn't support to store 
century value. But the Win7 boot loader hal!HalpInitializeCmos() will firstly 
read and save the FADT.Century to a 4-byte location pointed by 
hal!_HalpCmosCenturyOffset. When the FADT.Century is 0, it will save 0x32 
(default value) to that location. Later hal!HalpReadCmosTime() skips to read 
the century value from CMOS when BIT 7 of the value poited by 
hal!_HalpCmosCenturyOffset is set.
So in order to tell Windows that RTC doesn't support to store century, we need 
to set the FADT.Century to 0x80 other than 0.
In summary, if FADT.Century is 0, Win7 boot loader reads century from 0x32; if 
FADT.Century & BIT7 != 0, it doesn't read century from CMOS; otherwise, it 
reads century from CMOS index FADT.Century.

But that caused another issue. Linux code strictly follows the ACPI spec which 
reads the century value from FADT.Century if it doesn't equal to zero. If we 
leave 0x80 in FADT.Century, Linux kernel will reads century from 0x80 location 
(actually from location 0 because CMOS address is 7-bit). Location 0 stores the 
seconds of the time which means Linux will read random century value.

So do you agree if a platform needs to support booting both Windows and Linux, 
it had better to set FADT.Century to 0x32 and save correct century value (0x20) 
to CMOS address 0x32?

Regards,
Ray


-Original Message-
From: Scott Duplichan [mailto:sc...@notabs.org] 
Sent: Friday, November 20, 2015 10:56 PM
To: Ni, Ruiyu <ruiyu...@intel.com>
Cc: 'Paolo Bonzini' <pbonz...@redhat.com>; 'edk2-devel@lists.01.org' 
<edk2-de...@ml01.01.org>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:

]Sent: Friday, November 20, 2015 01:37 AM
]To: Scott Duplichan <sc...@notabs.org>
]Cc: Paolo Bonzini <pbonz...@redhat.com>; edk2-devel@lists.01.org 
<edk2-de...@ml01.01.org>; Zeng, Star ]<star.z...@intel.com>
]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue
]
]Scott,
]The UEFI Windows 7 is fresh installed. Could you please tell me how to check
]whether a platform turns on the RTC alarm?
]
]Thanks,
]Ray

Hello Ray,

I think just about every system that can run UEFI supports RTC alarm, at
least in the x86 world. It is support for the century feature that could
possibly be disabled on some systems. For example, an internet search for
"RTC Century support" finds an AMD Bolton document that describes a 
CenturyEn bit: Enable RTC Century support. So apparently systems using
AMD Bolton could be configured for no RTC century support at all. I don't
know if anyone would actually do this. But if they did, it would make
sense for UEFI to not write a value into a CMOS century byte.

Tahnks,
Scott


> 在 2015年11月20日，10:49，Scott Duplichan <sc...@notabs.org> 写道：
> 
> Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:
> 
> ]Sent: Thursday, November 19, 2015 06:37 PM
> ]To: Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star <star.z...@intel.com>; 
> edk2-devel@lists.01.org <edk2-]de...@ml01.01.org>
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> ]
> ]If we strictly follow the spec, it's only needed to save the century value
> ]in CMOS when FADT.CenturyOffset is not zero. But the fact is even we set
> ]the FADT.CenturyOffset to 0 indicating the platform doesn't store century
> ]value in CMOS, UEFI Win7 still hang during booting until we updates the value
> ]in CMOS location. So we have to always save the century value in CMOS.
> ]
> ]Regards,
> ]Ray
> 
> It sounds like you are testing on a machine that requires programming of
> the RTC century value in order for the RTC alarm interrupt to work. If the
> century value is not programmed, the alarm interrupt will never occur and
> Windows 7 bootup will hang waiting for the interrupt. So a boot hang after
> setting FADT.CenturyOffset to zero with no CMOS x32 write is expected. It
> could be said that this machine requires FADT.CenturyOffset be set in order
> to boot Windows 7. I don't see how this justifies writing CMOS x32
> unconditionally.
> 
> What if a different system keeps some important value in CMOS x32 that is
> unrelated to the RTC alarm function? The UEFI developer will set
> FADT.CenturyOffset to zero and that will keep the OS from overwriting it.
> But with this patch, UEFI itself will overwrite the value at CMOS x32. The
> developer no longer has a configuration mechanism that prevents both the OS
> and UEFI itself from writing to CMOS.
> 
> Thanks,
> Scott
> 
> 
> ]-Original Message-
> ]Fro

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 12:16, Ni, Ruiyu wrote:
> Scott, I debugged the issue further and had the below findings: 
> According to the ACPI spec 6.0 5.2.9 Fixed ACPI Description Table
> (FADT), the FADT.Century can be set to 0 indicating the RTC doesn't
> support to store century value. But the Win7 boot loader
> hal!HalpInitializeCmos() will firstly read and save the FADT.Century
> to a 4-byte location pointed by hal!_HalpCmosCenturyOffset. When the
> FADT.Century is 0, it will save 0x32 (default value) to that
> location. Later hal!HalpReadCmosTime() skips to read the century
> value from CMOS when BIT 7 of the value poited by
> hal!_HalpCmosCenturyOffset is set. So in order to tell Windows that
> RTC doesn't support to store century, we need to set the FADT.Century
> to 0x80 other than 0. In summary, if FADT.Century is 0, Win7 boot
> loader reads century from 0x32; if FADT.Century & BIT7 != 0, it
> doesn't read century from CMOS; otherwise, it reads century from CMOS
> index FADT.Century.
> 
> But that caused another issue. Linux code strictly follows the ACPI
> spec which reads the century value from FADT.Century if it doesn't
> equal to zero. If we leave 0x80 in FADT.Century, Linux kernel will
> reads century from 0x80 location (actually from location 0 because
> CMOS address is 7-bit). Location 0 stores the seconds of the time
> which means Linux will read random century value.
> 
> So do you agree if a platform needs to support booting both Windows
> and Linux, it had better to set FADT.Century to 0x32 and save correct
> century value (0x20) to CMOS address 0x32?

That's fair enough, but you should not use RTC_ADDRESS_CENTURY
unconditionally in PcRtcSetTime.  Instead you should read the FADT
yourself and use the FADT.Century value if it is non-zero.  If it is
zero, I suppose writing the century to 0x32 is the only thing you can do.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Scott Duplichan]

Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:

]Sent: Wednesday, December 09, 2015 05:16 AM
]To: Scott Duplichan <sc...@notabs.org>
]Cc: 'Paolo Bonzini' <pbonz...@redhat.com>; 'edk2-devel@lists.01.org' 
<edk2-de...@ml01.01.org>; Zeng, Star ]<star.z...@intel.com>
]Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

]Scott,
]I debugged the issue further and had the below findings:
]According to the ACPI spec 6.0 5.2.9 Fixed ACPI Description Table (FADT),
]the FADT.Century can be set to 0 indicating the RTC doesn't support to
]store century value. But the Win7 boot loader hal!HalpInitializeCmos()
]will firstly read and save the FADT.Century to a 4-byte location pointed
]by hal!_HalpCmosCenturyOffset. When the FADT.Century is 0, it will save
]0x32 (default value) to that location. Later hal!HalpReadCmosTime() skips
]to read the century value from CMOS when BIT 7 of the value pointed by
]hal!_HalpCmosCenturyOffset is set. So in order to tell Windows that RTC
]doesn't support to store century, we need to set the FADT.Century to 0x80
]other than 0. In summary, if FADT.Century is 0, Win7 boot loader reads
]century from 0x32; if FADT.Century & BIT7 != 0, it doesn't read century
]from CMOS; otherwise, it reads century from CMOS index FADT.Century. But
]that caused another issue. Linux code strictly follows the ACPI spec which
]reads the century value from FADT.Century if it doesn't equal to zero. If
]we leave 0x80 in FADT.Century, Linux kernel will reads century from 0x80
]location (actually from location 0 because CMOS address is 7-bit). Location
]0 stores the seconds of the time which means Linux will read random century
]value.
]
]So do you agree if a platform needs to support booting both Windows and Linux,
]it had better to set FADT.Century to 0x32 and save correct century value (0x20)
]to CMOS address 0x32?
]
]Regards,
]Ray

Hello Ray,

That is great debug work to find Win7 has 0x32 hard-coded for RTC century offset
and uses that value is zero is found in ACPI. I think you are right and UEFI 
must
accommodate what sounds like a Win7 bug. Thanks for debugging the Win7 boot 
loader.

Thanks,
Scott


-Original Message-
From: Scott Duplichan [mailto:sc...@notabs.org] 
Sent: Friday, November 20, 2015 10:56 PM
To: Ni, Ruiyu <ruiyu...@intel.com>
Cc: 'Paolo Bonzini' <pbonz...@redhat.com>; 'edk2-devel@lists.01.org' 
<edk2-de...@ml01.01.org>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:

]Sent: Friday, November 20, 2015 01:37 AM
]To: Scott Duplichan <sc...@notabs.org>
]Cc: Paolo Bonzini <pbonz...@redhat.com>; edk2-devel@lists.01.org 
<edk2-de...@ml01.01.org>; Zeng, Star ]<star.z...@intel.com>
]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue
]
]Scott,
]The UEFI Windows 7 is fresh installed. Could you please tell me how to check
]whether a platform turns on the RTC alarm?
]
]Thanks,
]Ray

Hello Ray,

I think just about every system that can run UEFI supports RTC alarm, at
least in the x86 world. It is support for the century feature that could
possibly be disabled on some systems. For example, an internet search for
"RTC Century support" finds an AMD Bolton document that describes a 
CenturyEn bit: Enable RTC Century support. So apparently systems using
AMD Bolton could be configured for no RTC century support at all. I don't
know if anyone would actually do this. But if they did, it would make
sense for UEFI to not write a value into a CMOS century byte.

Tahnks,
Scott


> 在 2015年11月20日，10:49，Scott Duplichan <sc...@notabs.org> 写道：
> 
> Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:
> 
> ]Sent: Thursday, November 19, 2015 06:37 PM
> ]To: Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star <star.z...@intel.com>; 
> edk2-devel@lists.01.org <edk2-]de...@ml01.01.org>
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> ]
> ]If we strictly follow the spec, it's only needed to save the century value
> ]in CMOS when FADT.CenturyOffset is not zero. But the fact is even we set
> ]the FADT.CenturyOffset to 0 indicating the platform doesn't store century
> ]value in CMOS, UEFI Win7 still hang during booting until we updates the value
> ]in CMOS location. So we have to always save the century value in CMOS.
> ]
> ]Regards,
> ]Ray
> 
> It sounds like you are testing on a machine that requires programming of
> the RTC century value in order for the RTC alarm interrupt to work. If the
> century value is not programmed, the alarm interrupt will never occur and
> Windows 7 bootup will hang waiting for the interrupt. So a boot hang after
> setting FADT.CenturyOffset to zero with no CMOS x32 write is expected. It
> could be said that this machine

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 18:37, Laszlo Ersek wrote:
> - A DXE driver that runs before *both* the ACPI platform DXE driver, and
> this runtime DXE driver -- to be ordered by any means necessary --, *or*
> a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
> driver and this runtime DXE driver. I think this is the best approach.

Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good idea.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Kinney, Michael D]

I think we need to evaluate both methods discussed here:

1) RTC driver use info from FADT in SetTime() when FADT is available.  Add 
event notification to RTC driver to capture FADT info before ExitBootServices().
2) Use PCD in RTC driver and in ACPI FADT table.

Advantage of (1) is that it is compatible with existing ACPI Table 
implementations.

Mike


> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Wednesday, December 9, 2015 9:40 AM
> To: Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Scott
> Duplichan <sc...@notabs.org>
> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
> <star.z...@intel.com>
> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> 
> 
> On 09/12/2015 18:37, Laszlo Ersek wrote:
> > - A DXE driver that runs before *both* the ACPI platform DXE driver, and
> > this runtime DXE driver -- to be ordered by any means necessary --, *or*
> > a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
> > driver and this runtime DXE driver. I think this is the best approach.
> 
> Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good idea.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Laszlo Ersek]

On 12/09/15 18:55, Kinney, Michael D wrote:
> I think we need to evaluate both methods discussed here:
> 
> 1) RTC driver use info from FADT in SetTime() when FADT is available.  Add 
> event notification to RTC driver to capture FADT info before 
> ExitBootServices().
> 2) Use PCD in RTC driver and in ACPI FADT table.
> 
> Advantage of (1) is that it is compatible with existing ACPI Table 
> implementations.

I agree that (1) is compatible with existing *valid* ACPI table
implementations.

I can't resist mentioning though that I've seen platform firmware "in
the wild" that messed with ACPI tables in the exit-boot-services
callback. Obviously this is completely broken: installing new tables
involves memory allocation and therefore messes with the memory map.

Still, a technically valid driver that is nonetheless too clever for its
own good might poke data into existent ACPI tables (without memory
allocation or release) at exit-boot-services, derived from other data in
the system that *it* expects to become available asynchronously,
sometime before exit-boot-services. (Yes, I've seen this happen.)

Given that the order of callbacks is unspecified, I generally try to
steer clear from inter-module dependencies in any exit-boot-services
callback; I believe that any driver should use that callback only for
re-setting its internal (software or hardware) state. (Without touching
the memory allocation services, of course).

I think that the PCD is more robust, and in case a platform's ACPI
driver is *not* updated to consider it, then the situation is still no
worse than with Ray's current patch, in this thread. As I understand,
the PCD can be given a default value that would trigger the behavior
proposed in this patch.

Thanks
Laszlo

> 
> Mike
> 
> 
>> -Original Message-
>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
>> Sent: Wednesday, December 9, 2015 9:40 AM
>> To: Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D 
>> <michael.d.kin...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Scott
>> Duplichan <sc...@notabs.org>
>> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
>> <star.z...@intel.com>
>> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
>> issue
>>
>>
>>
>> On 09/12/2015 18:37, Laszlo Ersek wrote:
>>> - A DXE driver that runs before *both* the ACPI platform DXE driver, and
>>> this runtime DXE driver -- to be ordered by any means necessary --, *or*
>>> a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
>>> driver and this runtime DXE driver. I think this is the best approach.
>>
>> Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good idea.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Kinney, Michael D]

Paolo,

I agree SetTime() is not called in very many places.  But since the SetTime() 
service is added to Runtime Services Table when the RTC driver runs, the logic 
in SetTime() must be implemented to handle case where SetTime() is called 
before ACPI Tables are published.

Mike

> -Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> Sent: Wednesday, December 9, 2015 8:42 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Ni, Ruiyu 
> <ruiyu...@intel.com>; Scott Duplichan <sc...@notabs.org>
> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
> <star.z...@intel.com>
> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> 
> 
> On 09/12/2015 17:33, Kinney, Michael D wrote:
> > Paolo,
> >
> > The RTC driver runs and produces the runtime services before the ACPI 
> > tables are available so what you suggest is not always
> possible.
> 
> The only place where Ruiyu's patch actually uses RTC_ADDRESS_CENTURY is
> in SetTime, and that is not used at any place other than
> SetupBrowserDxe.  The ACPI tables should be available by then, shouldn't
> they?
> 
> Paolo
> 
> > Mike
> >
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> >> Paolo Bonzini
> >> Sent: Wednesday, December 9, 2015 3:50 AM
> >> To: Ni, Ruiyu <ruiyu...@intel.com>; Scott Duplichan <sc...@notabs.org>
> >> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
> >> <star.z...@intel.com>
> >> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> >> issue
> >>
> >>
> >>
> >> On 09/12/2015 12:16, Ni, Ruiyu wrote:
> >>> Scott, I debugged the issue further and had the below findings:
> >>> According to the ACPI spec 6.0 5.2.9 Fixed ACPI Description Table
> >>> (FADT), the FADT.Century can be set to 0 indicating the RTC doesn't
> >>> support to store century value. But the Win7 boot loader
> >>> hal!HalpInitializeCmos() will firstly read and save the FADT.Century
> >>> to a 4-byte location pointed by hal!_HalpCmosCenturyOffset. When the
> >>> FADT.Century is 0, it will save 0x32 (default value) to that
> >>> location. Later hal!HalpReadCmosTime() skips to read the century
> >>> value from CMOS when BIT 7 of the value poited by
> >>> hal!_HalpCmosCenturyOffset is set. So in order to tell Windows that
> >>> RTC doesn't support to store century, we need to set the FADT.Century
> >>> to 0x80 other than 0. In summary, if FADT.Century is 0, Win7 boot
> >>> loader reads century from 0x32; if FADT.Century & BIT7 != 0, it
> >>> doesn't read century from CMOS; otherwise, it reads century from CMOS
> >>> index FADT.Century.
> >>>
> >>> But that caused another issue. Linux code strictly follows the ACPI
> >>> spec which reads the century value from FADT.Century if it doesn't
> >>> equal to zero. If we leave 0x80 in FADT.Century, Linux kernel will
> >>> reads century from 0x80 location (actually from location 0 because
> >>> CMOS address is 7-bit). Location 0 stores the seconds of the time
> >>> which means Linux will read random century value.
> >>>
> >>> So do you agree if a platform needs to support booting both Windows
> >>> and Linux, it had better to set FADT.Century to 0x32 and save correct
> >>> century value (0x20) to CMOS address 0x32?
> >>
> >> That's fair enough, but you should not use RTC_ADDRESS_CENTURY
> >> unconditionally in PcRtcSetTime.  Instead you should read the FADT
> >> yourself and use the FADT.Century value if it is non-zero.  If it is
> >> zero, I suppose writing the century to 0x32 is the only thing you can do.
> >>
> >> Paolo
> >> ___
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Laszlo Ersek]

On 12/09/15 17:41, Paolo Bonzini wrote:
> 
> 
> On 09/12/2015 17:33, Kinney, Michael D wrote:
>> Paolo,
>>
>> The RTC driver runs and produces the runtime services before the
>> ACPI tables are available so what you suggest is not always
>> possible.
> 
> The only place where Ruiyu's patch actually uses RTC_ADDRESS_CENTURY is
> in SetTime, and that is not used at any place other than
> SetupBrowserDxe.  The ACPI tables should be available by then, shouldn't
> they?

The PcatRealTimeClockRuntimeDxe driver produces the "real time clock"
architectural protocol (see its INF file).

DXE drivers (which are part of the platform firmware, as opposed to UEFI
drivers, which can be third party) are expected to spell out all of
their startup protocol dependencies in their Depex sections.

(UEFI drivers have an implicit, constant depex that says "all of the
architectural protocols".)

This means that as soon as PcatRealTimeClockRuntimeDxe installs the real
time clock architectural protocol, two things can happen:

(1) Once the driver's entry point function exits successfully and the
DXE core / dispatcher gets back control, it can dispatch further drivers
whose dependencies that include "real time clock" arch protocol are now
satisfied. This can occur before ACPI tables are present.

(2) Another DXE driver may have been launched earlier, with no Depex on
the real time clock arch protocol. Such a driver may have registered a
protocol notify event / callback. Dependent on the TPL (task priority
level) of the callback function in this driver, and on the TPL that the
protocol provider driver is running *at* when it installs the real time
clock arch protocol, the callback can occur either immediately
(practically on the stack of the
gBS->InstallMultipleProtocolInterfaces() function call), or else when
the providing driver lowers the system TPL sufficiently for the callback
to be called. (In the latter case it is called on the stack of the
gBS->RestoreTPL() call of the providing driver.)

The upshot is that in this case, regardless of the TPLs, the reliant
driver can start using the real time clock arch protocol even before the
entry point function of the providing driver exits. In other words, even
earlier than in scenario (1).

At thes points in time, the ACPI tables may not be present *yet*.

Similarly, when the function is called at runtime (i.e., by the OS), the
FADT may not exist *any longer*, because the FADT is allowed to exist in
ACPI Reclaim type memory (which can be repurposed by the OS more or less
after the AML interpreter has completed the initial run).

I can see the following solutions:

- A DXE driver that runs before *both* the ACPI platform DXE driver, and
this runtime DXE driver -- to be ordered by any means necessary --, *or*
a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
driver and this runtime DXE driver. I think this is the best approach.

- PcatRealTimeClockRuntimeDxe could install a notification event /
callback for ... something, and look at the ACPI tables at that point,
and stash the result in a global variable (which would live in "runtime
service data" type memory).

The "something" is the tricky part. Should it be the "ready to boot"
event? Should it be the "exit boot services" event? Should it be the
installation of the RSDP by the generic ACPI table driver (the RSDP gets
linked into the system config table, and a notification is emitted about
such events)?

In the examples that I have seen thus far, trying to parse ACPI stuff in
(runtime) DXE driver D1, installed by a *sibling* DXE driver D2, has
only led to obscure bugs. It is also not nice to clutter
PcatRealTimeClockRuntimeDxe with such platform specifics.

For that reason I'd recommend the PCD. That PCD could be set (either at
build time, or dynamically) in accordance with the platform's hardware,
and then both this driver, and the platform ACPI driver (that installs
the FADT) could key off of the PCD.

--*--

In QEMU's / OVMF's case (of course :)), I would simply look up the QEMU
source code that is responsible for generating the FADT, and then
hardwire the (fixed) PCD accordingly :)

For the full-blown dynamic method that I recommend above, a new fw_cfg
file would be necessary, which would allow OVMF's PlatformPei to set the
PCD for PcatRealTimeClockRuntimeDxe so that it match the FADT generated
by QEMU.

Thanks
Laszlo

> 
> Paolo
> 
>> Mike
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Paolo Bonzini
>>> Sent: Wednesday, December 9, 2015 3:50 AM
>>> To: Ni, Ruiyu <ruiyu...@intel.com>; Scott Duplichan <sc...@notabs.org>
>>> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
>>> <star.z...@in

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Laszlo,
I agree with your concerns that the ACPI table can be modified at anytime 
before firmware hands off control to OS, after all it's a buffer reported by 
system configuration table. So in extreme cases, a platform may firstly set 
FADT.century to 0x80 but change it to 0x32 later. If the SetTime isn't called 
later, century won't be saved to CMOS. Even RTC driver hooks the 
ExitBootServices event to update CMOS, the FADT.century may be changed after 
RTC's callback.

So how about use a PCD named PcdCmosCenturyLocation type UINT8, and the PCD 
value should less then 0x80 otherwise assertion in RTC driver happens?
It's platform 's responsibility to convert 0 to 0x80 and save in ACPI table if 
it's a win-only platform. For a OS neutral platform, non-zero PCD should be 
always chosen.



Thanks,
Ray

> 在 2015年12月10日，02:07，Laszlo Ersek <ler...@redhat.com> 写道：
> 
>> On 12/09/15 18:55, Kinney, Michael D wrote:
>> I think we need to evaluate both methods discussed here:
>> 
>> 1) RTC driver use info from FADT in SetTime() when FADT is available.  Add 
>> event notification to RTC driver to capture FADT info before 
>> ExitBootServices().
>> 2) Use PCD in RTC driver and in ACPI FADT table.
>> 
>> Advantage of (1) is that it is compatible with existing ACPI Table 
>> implementations.
> 
> I agree that (1) is compatible with existing *valid* ACPI table
> implementations.
> 
> I can't resist mentioning though that I've seen platform firmware "in
> the wild" that messed with ACPI tables in the exit-boot-services
> callback. Obviously this is completely broken: installing new tables
> involves memory allocation and therefore messes with the memory map.
> 
> Still, a technically valid driver that is nonetheless too clever for its
> own good might poke data into existent ACPI tables (without memory
> allocation or release) at exit-boot-services, derived from other data in
> the system that *it* expects to become available asynchronously,
> sometime before exit-boot-services. (Yes, I've seen this happen.)
> 
> Given that the order of callbacks is unspecified, I generally try to
> steer clear from inter-module dependencies in any exit-boot-services
> callback; I believe that any driver should use that callback only for
> re-setting its internal (software or hardware) state. (Without touching
> the memory allocation services, of course).
> 
> I think that the PCD is more robust, and in case a platform's ACPI
> driver is *not* updated to consider it, then the situation is still no
> worse than with Ray's current patch, in this thread. As I understand,
> the PCD can be given a default value that would trigger the behavior
> proposed in this patch.
> 
> Thanks
> Laszlo
> 
>> 
>> Mike
>> 
>> 
>>> -Original Message-
>>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
>>> Sent: Wednesday, December 9, 2015 9:40 AM
>>> To: Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D 
>>> <michael.d.kin...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Scott
>>> Duplichan <sc...@notabs.org>
>>> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
>>> <star.z...@intel.com>
>>> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
>>> issue
>>> 
>>> 
>>> 
>>>> On 09/12/2015 18:37, Laszlo Ersek wrote:
>>>> - A DXE driver that runs before *both* the ACPI platform DXE driver, and
>>>> this runtime DXE driver -- to be ordered by any means necessary --, *or*
>>>> a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
>>>> driver and this runtime DXE driver. I think this is the best approach.
>>> 
>>> Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good idea.
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Yes.

Regards,
Ray


-Original Message-
From: Kinney, Michael D 
Sent: Thursday, December 10, 2015 9:33 AM
To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
Michael D <michael.d.kin...@intel.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
<star.z...@intel.com>
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Ray,

Do you prefer PCD solution?

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, December 9, 2015 4:45 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
> <ler...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>;
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Mike,
> The UEFI Spec doesn't require implementation to call 
> InstallConfigurationTable() every time the ACPI table is updated through
> EFI_ACPI_PROTOCOL.InstallAcpiTable().
> I checked the implementation:
> AcpiSupportDxe in IntelFrameworkModulePkg only calls 
> IntallConfigurationTable() in the first time InstallAcpiTable() is called.
> AcpiTableDxe in MdeModulePkg calls InstallConfigurationTable() every time 
> InstallAcpiTable() is called.
> 
> So we cannot depend on the event notification for EFI_ACPI_TABLE_GUID.
> 
> PI Spec defines a ACPI_SDT protocol (produced by AcpiTableDxe in 
> MdeModulePkg) which supports table installation notification. But
> it's an optional protocol and
> not every platform includes AcpiTableDxe to produce this protocol.
> Per my understanding, only EFI_ACPI_PROTOCOL is mandatory.
> 
> Regards,
> Ray
> 
> 
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, December 10, 2015 3:49 AM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
> Michael D <michael.d.kin...@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>;
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Ray,
> 
> The UEFI Specification 2.5 p. 136 - CreateEventEx() requires an event group 
> to be signaled every time a configuration table is installed
> or updated.
> 
> The RTC driver could create an event notification for the EFI_ACPI_TABLE_GUID 
> and cache FADT field in notification function.
> 
> Mike
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Wednesday, December 9, 2015 10:48 AM
> > To: Laszlo Ersek <ler...@redhat.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Paolo Bonzini 
> > <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>;
> > edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
> > <star.z...@intel.com>
> > Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> > issue
> >
> > Laszlo,
> > I agree with your concerns that the ACPI table can be modified at anytime 
> > before firmware hands off control to OS, after all it's a
> > buffer reported by system configuration table. So in extreme cases, a 
> > platform may firstly set FADT.century to 0x80 but change it to
> > 0x32 later. If the SetTime isn't called later, century won't be saved to 
> > CMOS. Even RTC driver hooks the ExitBootServices event to
> > update CMOS, the FADT.century may be changed after RTC's callback.
> >
> > So how about use a PCD named PcdCmosCenturyLocation type UINT8, and the PCD 
> > value should less then 0x80 otherwise assertion
> in
> > RTC driver happens?
> > It's platform 's responsibility to convert 0 to 0x80 and save in ACPI table 
> > if it's a win-only platform. For a OS neutral platform, non-
> zero
> > PCD should be always chosen.
> >
> >
> >
> > Thanks,
> > Ray
> >
> > > 在 2015年12月10日，02:07，Laszlo Ersek <ler...@redhat.com> 写道：
> > >
> > >> On 12/09/15 18:55, Kinney, Michael D wrote:
> > >> I think we need to evaluate both methods discussed here:
> > >>
> > >> 1) RTC driver use info from FADT in SetTime() when FADT is available.  
> > >> Add event notification to RTC driver to capture FADT info
> > before ExitBootServices().
> > >> 2) Use PCD in RTC driver and in ACPI FADT table.
&g

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Kinney, Michael D]

Ray,

Do you prefer PCD solution?

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, December 9, 2015 4:45 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
> <ler...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>;
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Mike,
> The UEFI Spec doesn't require implementation to call 
> InstallConfigurationTable() every time the ACPI table is updated through
> EFI_ACPI_PROTOCOL.InstallAcpiTable().
> I checked the implementation:
> AcpiSupportDxe in IntelFrameworkModulePkg only calls 
> IntallConfigurationTable() in the first time InstallAcpiTable() is called.
> AcpiTableDxe in MdeModulePkg calls InstallConfigurationTable() every time 
> InstallAcpiTable() is called.
> 
> So we cannot depend on the event notification for EFI_ACPI_TABLE_GUID.
> 
> PI Spec defines a ACPI_SDT protocol (produced by AcpiTableDxe in 
> MdeModulePkg) which supports table installation notification. But
> it's an optional protocol and
> not every platform includes AcpiTableDxe to produce this protocol.
> Per my understanding, only EFI_ACPI_PROTOCOL is mandatory.
> 
> Regards,
> Ray
> 
> 
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, December 10, 2015 3:49 AM
> To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
> Michael D <michael.d.kin...@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>;
> Zeng, Star <star.z...@intel.com>
> Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Ray,
> 
> The UEFI Specification 2.5 p. 136 - CreateEventEx() requires an event group 
> to be signaled every time a configuration table is installed
> or updated.
> 
> The RTC driver could create an event notification for the EFI_ACPI_TABLE_GUID 
> and cache FADT field in notification function.
> 
> Mike
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Wednesday, December 9, 2015 10:48 AM
> > To: Laszlo Ersek <ler...@redhat.com>
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Paolo Bonzini 
> > <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>;
> > edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
> > <star.z...@intel.com>
> > Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> > issue
> >
> > Laszlo,
> > I agree with your concerns that the ACPI table can be modified at anytime 
> > before firmware hands off control to OS, after all it's a
> > buffer reported by system configuration table. So in extreme cases, a 
> > platform may firstly set FADT.century to 0x80 but change it to
> > 0x32 later. If the SetTime isn't called later, century won't be saved to 
> > CMOS. Even RTC driver hooks the ExitBootServices event to
> > update CMOS, the FADT.century may be changed after RTC's callback.
> >
> > So how about use a PCD named PcdCmosCenturyLocation type UINT8, and the PCD 
> > value should less then 0x80 otherwise assertion
> in
> > RTC driver happens?
> > It's platform 's responsibility to convert 0 to 0x80 and save in ACPI table 
> > if it's a win-only platform. For a OS neutral platform, non-
> zero
> > PCD should be always chosen.
> >
> >
> >
> > Thanks,
> > Ray
> >
> > > 在 2015年12月10日，02:07，Laszlo Ersek <ler...@redhat.com> 写道：
> > >
> > >> On 12/09/15 18:55, Kinney, Michael D wrote:
> > >> I think we need to evaluate both methods discussed here:
> > >>
> > >> 1) RTC driver use info from FADT in SetTime() when FADT is available.  
> > >> Add event notification to RTC driver to capture FADT info
> > before ExitBootServices().
> > >> 2) Use PCD in RTC driver and in ACPI FADT table.
> > >>
> > >> Advantage of (1) is that it is compatible with existing ACPI Table 
> > >> implementations.
> > >
> > > I agree that (1) is compatible with existing *valid* ACPI table
> > > implementations.
> > >
> > > I can't resist mentioning though that I've seen platform firmware "in
> > > the wild" that messed with ACPI tables in the exit-boot-services
> > > callback. Obviously this is completely broke

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Mike,
The UEFI Spec doesn't require implementation to call 
InstallConfigurationTable() every time the ACPI table is updated through 
EFI_ACPI_PROTOCOL.InstallAcpiTable().
I checked the implementation:
AcpiSupportDxe in IntelFrameworkModulePkg only calls IntallConfigurationTable() 
in the first time InstallAcpiTable() is called.
AcpiTableDxe in MdeModulePkg calls InstallConfigurationTable() every time 
InstallAcpiTable() is called.

So we cannot depend on the event notification for EFI_ACPI_TABLE_GUID.

PI Spec defines a ACPI_SDT protocol (produced by AcpiTableDxe in MdeModulePkg) 
which supports table installation notification. But it's an optional protocol 
and
not every platform includes AcpiTableDxe to produce this protocol.
Per my understanding, only EFI_ACPI_PROTOCOL is mandatory.

Regards,
Ray


-Original Message-
From: Kinney, Michael D 
Sent: Thursday, December 10, 2015 3:49 AM
To: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Kinney, 
Michael D <michael.d.kin...@intel.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>; 
edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
<star.z...@intel.com>
Subject: RE: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

Ray,

The UEFI Specification 2.5 p. 136 - CreateEventEx() requires an event group to 
be signaled every time a configuration table is installed or updated.

The RTC driver could create an event notification for the EFI_ACPI_TABLE_GUID 
and cache FADT field in notification function.

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, December 9, 2015 10:48 AM
> To: Laszlo Ersek <ler...@redhat.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Paolo Bonzini 
> <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>;
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
> <star.z...@intel.com>
> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Laszlo,
> I agree with your concerns that the ACPI table can be modified at anytime 
> before firmware hands off control to OS, after all it's a
> buffer reported by system configuration table. So in extreme cases, a 
> platform may firstly set FADT.century to 0x80 but change it to
> 0x32 later. If the SetTime isn't called later, century won't be saved to 
> CMOS. Even RTC driver hooks the ExitBootServices event to
> update CMOS, the FADT.century may be changed after RTC's callback.
> 
> So how about use a PCD named PcdCmosCenturyLocation type UINT8, and the PCD 
> value should less then 0x80 otherwise assertion in
> RTC driver happens?
> It's platform 's responsibility to convert 0 to 0x80 and save in ACPI table 
> if it's a win-only platform. For a OS neutral platform, non-zero
> PCD should be always chosen.
> 
> 
> 
> Thanks,
> Ray
> 
> > 在 2015年12月10日，02:07，Laszlo Ersek <ler...@redhat.com> 写道：
> >
> >> On 12/09/15 18:55, Kinney, Michael D wrote:
> >> I think we need to evaluate both methods discussed here:
> >>
> >> 1) RTC driver use info from FADT in SetTime() when FADT is available.  Add 
> >> event notification to RTC driver to capture FADT info
> before ExitBootServices().
> >> 2) Use PCD in RTC driver and in ACPI FADT table.
> >>
> >> Advantage of (1) is that it is compatible with existing ACPI Table 
> >> implementations.
> >
> > I agree that (1) is compatible with existing *valid* ACPI table
> > implementations.
> >
> > I can't resist mentioning though that I've seen platform firmware "in
> > the wild" that messed with ACPI tables in the exit-boot-services
> > callback. Obviously this is completely broken: installing new tables
> > involves memory allocation and therefore messes with the memory map.
> >
> > Still, a technically valid driver that is nonetheless too clever for its
> > own good might poke data into existent ACPI tables (without memory
> > allocation or release) at exit-boot-services, derived from other data in
> > the system that *it* expects to become available asynchronously,
> > sometime before exit-boot-services. (Yes, I've seen this happen.)
> >
> > Given that the order of callbacks is unspecified, I generally try to
> > steer clear from inter-module dependencies in any exit-boot-services
> > callback; I believe that any driver should use that callback only for
> > re-setting its internal (software or hardware) state. (Without touching
> > the memory allocation services, of course).
> >
> > I think that the PCD is more robust, and in case a platform's ACPI
> > driver is *not* updated to consider it, 

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Kinney, Michael D]

Ray,

The UEFI Specification 2.5 p. 136 - CreateEventEx() requires an event group to 
be signaled every time a configuration table is installed or updated.

The RTC driver could create an event notification for the EFI_ACPI_TABLE_GUID 
and cache FADT field in notification function.

Mike

> -Original Message-
> From: Ni, Ruiyu
> Sent: Wednesday, December 9, 2015 10:48 AM
> To: Laszlo Ersek <ler...@redhat.com>
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Paolo Bonzini 
> <pbonz...@redhat.com>; Scott Duplichan <sc...@notabs.org>;
> edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Zeng, Star 
> <star.z...@intel.com>
> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> Laszlo,
> I agree with your concerns that the ACPI table can be modified at anytime 
> before firmware hands off control to OS, after all it's a
> buffer reported by system configuration table. So in extreme cases, a 
> platform may firstly set FADT.century to 0x80 but change it to
> 0x32 later. If the SetTime isn't called later, century won't be saved to 
> CMOS. Even RTC driver hooks the ExitBootServices event to
> update CMOS, the FADT.century may be changed after RTC's callback.
> 
> So how about use a PCD named PcdCmosCenturyLocation type UINT8, and the PCD 
> value should less then 0x80 otherwise assertion in
> RTC driver happens?
> It's platform 's responsibility to convert 0 to 0x80 and save in ACPI table 
> if it's a win-only platform. For a OS neutral platform, non-zero
> PCD should be always chosen.
> 
> 
> 
> Thanks,
> Ray
> 
> > 在 2015年12月10日，02:07，Laszlo Ersek <ler...@redhat.com> 写道：
> >
> >> On 12/09/15 18:55, Kinney, Michael D wrote:
> >> I think we need to evaluate both methods discussed here:
> >>
> >> 1) RTC driver use info from FADT in SetTime() when FADT is available.  Add 
> >> event notification to RTC driver to capture FADT info
> before ExitBootServices().
> >> 2) Use PCD in RTC driver and in ACPI FADT table.
> >>
> >> Advantage of (1) is that it is compatible with existing ACPI Table 
> >> implementations.
> >
> > I agree that (1) is compatible with existing *valid* ACPI table
> > implementations.
> >
> > I can't resist mentioning though that I've seen platform firmware "in
> > the wild" that messed with ACPI tables in the exit-boot-services
> > callback. Obviously this is completely broken: installing new tables
> > involves memory allocation and therefore messes with the memory map.
> >
> > Still, a technically valid driver that is nonetheless too clever for its
> > own good might poke data into existent ACPI tables (without memory
> > allocation or release) at exit-boot-services, derived from other data in
> > the system that *it* expects to become available asynchronously,
> > sometime before exit-boot-services. (Yes, I've seen this happen.)
> >
> > Given that the order of callbacks is unspecified, I generally try to
> > steer clear from inter-module dependencies in any exit-boot-services
> > callback; I believe that any driver should use that callback only for
> > re-setting its internal (software or hardware) state. (Without touching
> > the memory allocation services, of course).
> >
> > I think that the PCD is more robust, and in case a platform's ACPI
> > driver is *not* updated to consider it, then the situation is still no
> > worse than with Ray's current patch, in this thread. As I understand,
> > the PCD can be given a default value that would trigger the behavior
> > proposed in this patch.
> >
> > Thanks
> > Laszlo
> >
> >>
> >> Mike
> >>
> >>
> >>> -----Original Message-----
> >>> From: Paolo Bonzini [mailto:pbonz...@redhat.com]
> >>> Sent: Wednesday, December 9, 2015 9:40 AM
> >>> To: Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D 
> >>> <michael.d.kin...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; Scott
> >>> Duplichan <sc...@notabs.org>
> >>> Cc: 'edk2-devel@lists.01.org' <edk2-de...@ml01.01.org>; Zeng, Star 
> >>> <star.z...@intel.com>
> >>> Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> >>> issue
> >>>
> >>>
> >>>
> >>>> On 09/12/2015 18:37, Laszlo Ersek wrote:
> >>>> - A DXE driver that runs before *both* the ACPI platform DXE driver, and
> >>>> this runtime DXE driver -- to be ordered by any means necessary --, *or*
> >>>> a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
> >>>> driver and this runtime DXE driver. I think this is the best approach.
> >>>
> >>> Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good 
> >>> idea.
> >
> > ___
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 18:11, Kinney, Michael D wrote:
> Paolo,
> 
> I agree SetTime() is not called in very many places.  But since the
> SetTime() service is added to Runtime Services Table when the RTC
> driver runs, the logic in SetTime() must be implemented to handle
> case where SetTime() is called before ACPI Tables are published.

Right, but it can be implemented by simply skipping the century byte
(it's dead code anyway...).

Similarly, GetTime() could use the century byte as soon as ACPI tables
are published.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Scott Duplichan]

Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:

]Sent: Friday, November 20, 2015 01:37 AM
]To: Scott Duplichan <sc...@notabs.org>
]Cc: Paolo Bonzini <pbonz...@redhat.com>; edk2-devel@lists.01.org 
<edk2-de...@ml01.01.org>; Zeng, Star ]<star.z...@intel.com>
]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue
]
]Scott,
]The UEFI Windows 7 is fresh installed. Could you please tell me how to check
]whether a platform turns on the RTC alarm?
]
]Thanks,
]Ray

Hello Ray,

I think just about every system that can run UEFI supports RTC alarm, at
least in the x86 world. It is support for the century feature that could
possibly be disabled on some systems. For example, an internet search for
"RTC Century support" finds an AMD Bolton document that describes a 
CenturyEn bit: Enable RTC Century support. So apparently systems using
AMD Bolton could be configured for no RTC century support at all. I don't
know if anyone would actually do this. But if they did, it would make
sense for UEFI to not write a value into a CMOS century byte.

Tahnks,
Scott


> 在 2015年11月20日，10:49，Scott Duplichan <sc...@notabs.org> 写道：
> 
> Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:
> 
> ]Sent: Thursday, November 19, 2015 06:37 PM
> ]To: Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star <star.z...@intel.com>; 
> edk2-devel@lists.01.org <edk2-]de...@ml01.01.org>
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> ]
> ]If we strictly follow the spec, it's only needed to save the century value
> ]in CMOS when FADT.CenturyOffset is not zero. But the fact is even we set
> ]the FADT.CenturyOffset to 0 indicating the platform doesn't store century
> ]value in CMOS, UEFI Win7 still hang during booting until we updates the value
> ]in CMOS location. So we have to always save the century value in CMOS.
> ]
> ]Regards,
> ]Ray
> 
> It sounds like you are testing on a machine that requires programming of
> the RTC century value in order for the RTC alarm interrupt to work. If the
> century value is not programmed, the alarm interrupt will never occur and
> Windows 7 bootup will hang waiting for the interrupt. So a boot hang after
> setting FADT.CenturyOffset to zero with no CMOS x32 write is expected. It
> could be said that this machine requires FADT.CenturyOffset be set in order
> to boot Windows 7. I don't see how this justifies writing CMOS x32
> unconditionally.
> 
> What if a different system keeps some important value in CMOS x32 that is
> unrelated to the RTC alarm function? The UEFI developer will set
> FADT.CenturyOffset to zero and that will keep the OS from overwriting it.
> But with this patch, UEFI itself will overwrite the value at CMOS x32. The
> developer no longer has a configuration mechanism that prevents both the OS
> and UEFI itself from writing to CMOS.
> 
> Thanks,
> Scott
> 
> 
> ]-Original Message-
> ]From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo 
> Bonzini
> ]Sent: Friday, November 20, 2015 6:17 AM
> ]To: Zeng, Star <star.z...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; 
> edk2-devel@lists.01.org
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> 
> 
>> On 18/11/2015 06:08, Zeng, Star wrote:
>> 
>> @@ -508,6 +509,7 @@ PcRtcSetTime (
>>RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
>>RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
>>RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
>> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);
> 
> Should it be written only if the FADT CenturyOffset is zero?
> 
> Paolo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

If we strictly follow the spec, it's only needed to save the century value in 
CMOS when FADT.CenturyOffset is not zero.
But the fact is even we set the FADT.CenturyOffset to 0 indicating the platform 
doesn't store century value in CMOS, 
UEFI Win7 still hang during booting until we updates the value in CMOS location.

So we have to always save the century value in CMOS.

Regards,
Ray

-Original Message-
From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo Bonzini
Sent: Friday, November 20, 2015 6:17 AM
To: Zeng, Star <star.z...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; 
edk2-devel@lists.01.org
Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue



On 18/11/2015 06:08, Zeng, Star wrote:
> 
> @@ -508,6 +509,7 @@ PcRtcSetTime (
> RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
> RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
> RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);

Should it be written only if the FADT CenturyOffset is zero?

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 18/11/2015 06:08, Zeng, Star wrote:
> 
> @@ -508,6 +509,7 @@ PcRtcSetTime (
> RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
> RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
> RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);

Should it be written only if the FADT CenturyOffset is zero?

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Scott Duplichan]

Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:

]Sent: Thursday, November 19, 2015 06:37 PM
]To: Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star <star.z...@intel.com>; 
edk2-devel@lists.01.org <edk2-]de...@ml01.01.org>
]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue
]
]If we strictly follow the spec, it's only needed to save the century value
]in CMOS when FADT.CenturyOffset is not zero. But the fact is even we set
]the FADT.CenturyOffset to 0 indicating the platform doesn't store century
]value in CMOS, UEFI Win7 still hang during booting until we updates the value
]in CMOS location. So we have to always save the century value in CMOS.
]
]Regards,
]Ray

It sounds like you are testing on a machine that requires programming of
the RTC century value in order for the RTC alarm interrupt to work. If the
century value is not programmed, the alarm interrupt will never occur and
Windows 7 bootup will hang waiting for the interrupt. So a boot hang after
setting FADT.CenturyOffset to zero with no CMOS x32 write is expected. It
could be said that this machine requires FADT.CenturyOffset be set in order
to boot Windows 7. I don't see how this justifies writing CMOS x32
unconditionally.

What if a different system keeps some important value in CMOS x32 that is
unrelated to the RTC alarm function? The UEFI developer will set
FADT.CenturyOffset to zero and that will keep the OS from overwriting it.
But with this patch, UEFI itself will overwrite the value at CMOS x32. The
developer no longer has a configuration mechanism that prevents both the OS
and UEFI itself from writing to CMOS.

Thanks,
Scott


]-Original Message-
]From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo Bonzini
]Sent: Friday, November 20, 2015 6:17 AM
]To: Zeng, Star <star.z...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; 
edk2-devel@lists.01.org
]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue



On 18/11/2015 06:08, Zeng, Star wrote:
> 
> @@ -508,6 +509,7 @@ PcRtcSetTime (
> RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
> RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
> RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);

Should it be written only if the FADT CenturyOffset is zero?

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Ni, Ruiyu]

Scott,
The UEFI Windows 7 is fresh installed. Could you please tell me how to check 
whether a platform turns on the RTC alarm?

Thanks,
Ray

> 在 2015年11月20日，10:49，Scott Duplichan <sc...@notabs.org> 写道：
> 
> Ni, Ruiyu [mailto:ruiyu...@intel.com] wrote:
> 
> ]Sent: Thursday, November 19, 2015 06:37 PM
> ]To: Paolo Bonzini <pbonz...@redhat.com>; Zeng, Star <star.z...@intel.com>; 
> edk2-devel@lists.01.org <edk2-]de...@ml01.01.org>
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> ]
> ]If we strictly follow the spec, it's only needed to save the century value
> ]in CMOS when FADT.CenturyOffset is not zero. But the fact is even we set
> ]the FADT.CenturyOffset to 0 indicating the platform doesn't store century
> ]value in CMOS, UEFI Win7 still hang during booting until we updates the value
> ]in CMOS location. So we have to always save the century value in CMOS.
> ]
> ]Regards,
> ]Ray
> 
> It sounds like you are testing on a machine that requires programming of
> the RTC century value in order for the RTC alarm interrupt to work. If the
> century value is not programmed, the alarm interrupt will never occur and
> Windows 7 bootup will hang waiting for the interrupt. So a boot hang after
> setting FADT.CenturyOffset to zero with no CMOS x32 write is expected. It
> could be said that this machine requires FADT.CenturyOffset be set in order
> to boot Windows 7. I don't see how this justifies writing CMOS x32
> unconditionally.
> 
> What if a different system keeps some important value in CMOS x32 that is
> unrelated to the RTC alarm function? The UEFI developer will set
> FADT.CenturyOffset to zero and that will keep the OS from overwriting it.
> But with this patch, UEFI itself will overwrite the value at CMOS x32. The
> developer no longer has a configuration mechanism that prevents both the OS
> and UEFI itself from writing to CMOS.
> 
> Thanks,
> Scott
> 
> 
> ]-Original Message-
> ]From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo 
> Bonzini
> ]Sent: Friday, November 20, 2015 6:17 AM
> ]To: Zeng, Star <star.z...@intel.com>; Ni, Ruiyu <ruiyu...@intel.com>; 
> edk2-devel@lists.01.org
> ]Subject: Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang 
> issue
> 
> 
> 
>> On 18/11/2015 06:08, Zeng, Star wrote:
>> 
>> @@ -508,6 +509,7 @@ PcRtcSetTime (
>>RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
>>RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
>>RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
>> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);
> 
> Should it be written only if the FADT CenturyOffset is zero?
> 
> Paolo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Zeng, Star]

On 2015/11/17 16:48, Ruiyu Ni wrote:

The patch updates the Century value in CMOS location 50 (32h)
to avoid UEFI Win7 hang during booting. (Though Win8 is good.)
Per the ACPI spec the Century storage in CMOS is optional,
but the fact is even we set the FADT.CenturyOffset to 0 indicating
the platform doesn't store century value in CMOS, UEFI Win7 still
hang during booting until we updates the value in CMOS location.

The boot hang bug is a regression caused by check in@17624.
The patch 17624 completely doesn't use CMOS to store century value.
This patch only writes the century value to location 50.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Star Zeng 
---
  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 17 +
  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h |  6 +-
  2 files changed, 18 insertions(+), 5 deletions(-)


Reviewed-by: Star Zeng 



diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c 
b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 5abb71c..d293cbc 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -271,7 +271,7 @@ PcRtcInit (
  return EFI_DEVICE_ERROR;
}

-  ConvertEfiTimeToRtcTime (, RegisterB);
+  ConvertEfiTimeToRtcTime (, RegisterB, NULL);

//
// Set the Y/M/D info to variable as it has no corresponding hw registers.
@@ -442,6 +442,7 @@ PcRtcSetTime (
EFI_STATUS  Status;
EFI_TIMERtcTime;
RTC_REGISTER_B  RegisterB;
+  UINT8   Century;
UINT32  TimerVar;

if (Time == NULL) {
@@ -500,7 +501,7 @@ PcRtcSetTime (
RegisterB.Bits.Set  = 1;
RtcWrite (RTC_ADDRESS_REGISTER_B, RegisterB.Data);

-  ConvertEfiTimeToRtcTime (, RegisterB);
+  ConvertEfiTimeToRtcTime (, RegisterB, );

RtcWrite (RTC_ADDRESS_SECONDS, RtcTime.Second);
RtcWrite (RTC_ADDRESS_MINUTES, RtcTime.Minute);
@@ -508,6 +509,7 @@ PcRtcSetTime (
RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
+  RtcWrite (RTC_ADDRESS_CENTURY, Century);

//
// Allow updates of the RTC registers
@@ -725,7 +727,7 @@ PcRtcSetWakeupTime (
RegisterB.Data = RtcRead (RTC_ADDRESS_REGISTER_B);

if (Enable) {
-ConvertEfiTimeToRtcTime (, RegisterB);
+ConvertEfiTimeToRtcTime (, RegisterB, NULL);
} else {
  //
  // if the alarm is disable, record the current setting.
@@ -1048,11 +1050,14 @@ IsLeapYear (
@param   Time   On input, the time data read from UEFI to convert
On output, the time converted to RTC format
@param   RegisterB  Value of Register B of RTC, indicating data mode
+  @param   CenturyIt is set according to EFI_TIME Time.
+
  **/
  VOID
  ConvertEfiTimeToRtcTime (
IN OUT EFI_TIME*Time,
-  IN RTC_REGISTER_B  RegisterB
+  IN RTC_REGISTER_B  RegisterB,
+ OUT UINT8   *Century   OPTIONAL
)
  {
BOOLEAN IsPM;
@@ -1075,6 +1080,10 @@ ConvertEfiTimeToRtcTime (
//
// Set the Time/Date values.
//
+  if (Century != NULL) {
+*Century  = DecimalToBcd8 ((UINT8) (Time->Year / 100));
+  }
+
Time->Year  = (UINT16) (Time->Year % 100);

if (RegisterB.Bits.Dm == 0) {
diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h 
b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
index 026c108..516a8a9 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
@@ -61,6 +61,7 @@ typedef struct {
  #define RTC_ADDRESS_REGISTER_B11  // R/W
  #define RTC_ADDRESS_REGISTER_C12  // RO
  #define RTC_ADDRESS_REGISTER_D13  // RO
+#define RTC_ADDRESS_CENTURY   50  // Write Only
  //
  // Date and time initial values.
  // They are used if the RTC values are invalid during driver initialization
@@ -285,11 +286,14 @@ RtcTimeFieldsValid (
@param   Time   On input, the time data read from UEFI to convert
On output, the time converted to RTC format
@param   RegisterB  Value of Register B of RTC, indicating data mode
+  @param   CenturyIt is set according to EFI_TIME Time.
+
  **/
  VOID
  ConvertEfiTimeToRtcTime (
IN OUT EFI_TIME*Time,
-  IN RTC_REGISTER_B  RegisterB
+  IN RTC_REGISTER_B  RegisterB,
+ OUT UINT8   *Century   OPTIONAL
);





___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel