From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Luis R. Rodriguez" Subject: Re: [PATCH v4 04/14] x86/rtc: replace paravirt rtc check with platform legacy quirk Date: Thu, 7 Apr 2016 23:29:32 -0700 Message-ID: References: <1459987594-5434-1-git-send-email-mcgrof@kernel.org> <1459987594-5434-5-git-send-email-mcgrof@kernel.org> <570658DA.7060509@oracle.com> <20160408003207.GN1990@wotan.suse.de> <57073F0F.400@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <57073F0F.400@suse.com> Sender: linux-acpi-owner@vger.kernel.org To: Juergen Gross Cc: Boris Ostrovsky , Borislav Petkov , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , Rusty Russell , X86 ML , "linux-kernel@vger.kernel.org" , Andy Lutomirski , David Vrabel , Konrad Rzeszutek Wilk , "xen-devel@lists.xensource.com" , lguest@lists.ozlabs.org, Andy Shevchenko , Joey Lee , Gary Lin , Matt Fleming , Andrew Cooper , "Rafael J. Wysocki" , Len Brown , "Moore, Robert" , Lv Zheng , Toshi Kani List-Id: xen-devel@lists.xenproject.org On Thu, Apr 7, 2016 at 10:18 PM, Juergen Gross 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 >>>> Signed-off-by: Luis R. Rodriguez >> >> <-- 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 >>>> +#include >>>> + >>>> +#include >>>> +#include >>>> + >>>> +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