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

[Boris Ostrovsky]

On 04/08/2016 02:29 AM, Luis R. Rodriguez wrote:
> On Thu, Apr 7, 2016 at 10:18 PM, Juergen Gross <jgross@xxxxxxxx> 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@xxxxxxxxxx>
> > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > > <-- 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...

It works because the clock must have been discovered by ACPI prior to 
add_rtc_cmos() call. It's PNP0b00 object, I believe. The rest of the 
routine is to handle the case when RTC is not found in ACPI tables for 
whatever reasons (I think).

That's why we added paravirt_has(RTC) --- dom0 should be able to handle 
such cases, just like bare metal.

-boris