Re: [Qemu-devel] [PATCH] hw/i386: fill in the CENTURY field of the FADT (FACP) ACPI table

[Laszlo Ersek <lersek@redhat.com>]

On 01/07/16 12:24, Paolo Bonzini wrote:
> 
> 
> On 07/01/2016 12:22, Michael S. Tsirkin wrote:
>> On Thu, Jan 07, 2016 at 11:07:46AM +0100, Paolo Bonzini wrote:
>>>
>>>
>>> On 04/01/2016 18:19, Laszlo Ersek wrote:
>>>> On 12/10/15 19:53, Igor Mammedov wrote:
>>>>> On Thu, 10 Dec 2015 18:25:34 +0100
>>>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>
>>>>>> The ACPI specification (minimally versions 1.0b through 6.0) define
>>>>>> the FADT.CENTURY field as:
>>>>>>
>>>>>>   The RTC CMOS RAM index to the century of data value (hundred and
>>>>>>   thousand year decimals). If this field contains a zero, then the RTC
>>>>>>   centenary feature is not supported. If this field has a non-zero
>>>>>> value, then this field contains an index into RTC RAM space that OSPM
>>>>>> can use to program the centenary field.
>>>>>>
>>>>>> The x86 targets generate ACPI payload, emulate an RTC
>>>>>> (CONFIG_MC146818RTC), and that RTC supports the "centenary
>>>>>> feature" (see occurrences of RTC_CENTURY in cmos_ioport_write() and
>>>>>> cmos_ioport_read() in "hw/timer/mc146818rtc.c".)
>>>>>>
>>>>>> However, FADT.CENTURY is left at zero currently:
>>>>>>
>>>>>>   [06Ch 0108   1]            RTC Century Index : 00
>>>>>>
>>>>>> which -- according to analysis done by Ruiyu Ni at Intel -- should
>>>>>> cause Linux and Windows 8+ to think the RTC centenary feature is
>>>>>> unavailable, and cause Windows 7 to (incorrectly) assume that the
>>>>>> offset to use is constant 0x32. (0x32 happens to be the right value
>>>>>> on QEMU, but Windows 7 is wrong to assume anything at all).
>>>>>>
>>>>>> Exposing the right nonzero offset in FADT.CENTURY informs Linux and
>>>>>> Windows 8+ about the right capabilities of the hardware, plus it
>>>>>> retrofits our FADT to Windows 7's behavior.
>>>>>>
>>>>>> Regression tested with the following guests (all UEFI installs):
>>>>>> - i386 Q35: Fedora 21 ("Fedlet" edition)
>>>>>> - x86_64:
>>>>>>   - i440fx:
>>>>>>     - Fedora 21
>>>>>>     - RHEL 6 and 7
>>>>>>     - Windows 7 and 10
>>>>>>     - Windows Server 2008 R2 and 2012 R2
>>>>>>   - Q35:
>>>>>>     - Fedora 22
>>>>>>     - Windows 8.1
>>>>>>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> (supporter:ACPI/SMBIOS)
>>>>>> Cc: Igor Mammedov <imammedo@redhat.com> (supporter:ACPI/SMBIOS)
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
>>>>>> Cc: Richard Henderson <rth@twiddle.net> (maintainer:X86)
>>>>>> Cc: Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>> ---
>>>>>>  hw/i386/acpi-build.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> 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;
>>>>>>  }
>>>>>>  
>>>>>>  
>>>>>
>>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>>>>
>>>>
>>>> Thanks.
>>>>
>>>> Can someone please pick up this patch?
>>>
>>> It should probably go in through Michael's tree, but I've queued it too
>>> so that it isn't forgotten.
>>>
>>> Paolo
>>
>> Yes - thanks!
>> I picked this up - should I add your Reviewed-by tag?
> 
> Sure!
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo
> 

Thanks guys.
Laszlo