linux-kernel - Re: [PATCH v4 04/14] x86/rtc: replace paravirt rtc check with platform legacy quirk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAB=NE6UfgbUiF0v4X2O+FzGCqU3eevEXejrRO4bGWtmYOr0mxg@mail.gmail.com>
Date:	Thu, 7 Apr 2016 23:29:32 -0700
From:	"Luis R. Rodriguez" <mcgrof@...nel.org>
To:	Juergen Gross <jgross@...e.com>
Cc:	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Rusty Russell <rusty@...tcorp.com.au>, X86 ML <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...capital.net>,
	David Vrabel <david.vrabel@...rix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	lguest@...ts.ozlabs.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Joey Lee <jlee@...e.com>, Gary Lin <glin@...e.com>,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <lenb@...nel.org>,
	"Moore, Robert" <robert.moore@...el.com>,
	Lv Zheng <lv.zheng@...el.com>, Toshi Kani <toshi.kani@...com>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	kozerkov@...allels.com, Josh Triplett <josh@...htriplett.org>,
	Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH v4 04/14] x86/rtc: replace paravirt rtc check with
 platform legacy quirk

On Thu, Apr 7, 2016 at 10:18 PM, Juergen Gross <jgross@...e.com> wrote:
> On 08/04/16 02:32, Luis R. Rodriguez wrote:
>> On Thu, Apr 07, 2016 at 08:55:54AM -0400, Boris Ostrovsky wrote:
>>> On 04/06/2016 08:06 PM, Luis R. Rodriguez wrote:
>>>> We have 4 types of x86 platforms that disable RTC:
>>>>
>>>>   * Intel MID
>>>>   * Lguest - uses paravirt
>>>>   * Xen dom-U - uses paravirt
>>>>   * x86 on legacy systems annotated with an ACPI legacy flag
>>>>
>>>> We can consolidate all of these into a platform specific legacy
>>>> quirk set early in boot through i386_start_kernel() and through
>>>> x86_64_start_reservations(). This deals with the RTC quirks which
>>>> we can rely on through the hardware subarch, the ACPI check can
>>>> be dealt with separately.
>>>>
>>>> v2: split the subarch check from the ACPI check, clarify
>>>>     on the ACPI change commit log why ordering works
>>>>
>>>> Suggested-by: Ingo Molnar <mingo@...nel.org>
>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
>>
>> <-- snip -->
>>
>>>> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
>>>> new file mode 100644
>>>> index 000000000000..1b114ac5996f
>>>> --- /dev/null
>>>> +++ b/arch/x86/kernel/platform-quirks.c
>>>> @@ -0,0 +1,18 @@
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/init.h>
>>>> +
>>>> +#include <asm/setup.h>
>>>> +#include <asm/bios_ebda.h>
>>>> +
>>>> +void __init x86_early_init_platform_quirks(void)
>>>> +{
>>>> +   x86_platform.legacy.rtc = 1;
>>>> +
>>>> +   switch (boot_params.hdr.hardware_subarch) {
>>>> +   case X86_SUBARCH_XEN:
>>>> +   case X86_SUBARCH_LGUEST:
>>>> +   case X86_SUBARCH_INTEL_MID:
>>>> +           x86_platform.legacy.rtc = 0;
>>>> +           break;
>>>> +   }
>>>> +}
>>>
>>> What about Xen dom0 (aka initial domain)?
>>
>> Indeed, thanks for catching this, the hunk below removes the re-enablement of
>> the the RTC for dom0:
>>
>>>> --- a/arch/x86/xen/enlighten.c
>>>> +++ b/arch/x86/xen/enlighten.c
>>>> @@ -1192,7 +1192,6 @@ static const struct pv_info xen_info __initconst = {
>>>>  #ifdef CONFIG_X86_64
>>>>     .extra_user_64bit_cs = FLAT_USER_CS64,
>>>>  #endif
>>>> -   .features = 0,
>>>>     .name = "Xen",
>>>>  };
>>>> @@ -1525,8 +1524,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
>>>>     /* Install Xen paravirt ops */
>>>>     pv_info = xen_info;
>>>> -   if (xen_initial_domain())
>>>> -           pv_info.features |= PV_SUPPORTED_RTC;
>>>>     pv_init_ops = xen_init_ops;
>>>>     if (!xen_pvh_domain()) {
>>>>             pv_cpu_ops = xen_cpu_ops;
>>
>> This should then break dom0 unless of course you have the respective next
>> patch applied and that disabled the RTC due to an ACPI setting on your
>> platform. Juergen, can you check to see if that was the case for your
>> testing platform on dom0 ?
>
> Are you sure it would break?

No, suspected that it should though.

> Wouldn't it just fall back to another
> clock source, e.g. hpet?

I suppose so.

> I looked into my test system: seems as if add_rtc_cmos() is returning
> before the .legacy.rtc test.

OK thanks...

>> This highlights a semantic gap issue. From a quick cursory review, I think
>> we can address this temporarily by just using a check:
>>
>> void __init x86_early_init_platform_quirks(void)
>> {
>>       x86_platform.legacy.rtc = 1;
>>
>>       switch (boot_params.hdr.hardware_subarch) {
>>       case X86_SUBARCH_XEN:
>>       case X86_SUBARCH_LGUEST:
>>       case X86_SUBARCH_INTEL_MID:
>> -             x86_platform.legacy.rtc = 0;
>> +             if (x86_init.mpparse.get_smp_config != x86_init_uint_noop)
>> +                     x86_platform.legacy.rtc = 0;
>
> No! Why don't you just use the explicit test xen_initial_domain() ?

Because we don't want to sprinkle Xen specific code outside of Xen
code. What do you think about the second possibility I listed?
Otherwise, any other ideas?

  Luis