Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-22 Thread Luis R. Rodriguez 
On Mon, Feb 22, 2016 at 09:34:26AM -0500, Boris Ostrovsky wrote:
> On 02/22/2016 05:27 AM, Borislav Petkov wrote:
> >On Mon, Feb 22, 2016 at 07:07:56AM +0100, Luis R. Rodriguez wrote:
> >>diff --git a/arch/x86/include/asm/x86_init.h 
> >>b/arch/x86/include/asm/x86_init.h
> >>index 1ae89a2721d6..fe0d579b63e3 100644
> >>--- a/arch/x86/include/asm/x86_init.h
> >>+++ b/arch/x86/include/asm/x86_init.h
> >>@@ -84,11 +84,14 @@ struct x86_init_paging {
> >>   *boot cpu
> >>   * @timer_init:   initialize the platform timer (default 
> >> PIT/HPET)
> >>   * @wallclock_init:   init the wallclock device
> >>+ * @no_cmos_rtc:   set when platform has no CMOS real-time clock
> >>+ * present
> >>   */
> >>  struct x86_init_timers {
> >>void (*setup_percpu_clockev)(void);
> >>void (*timer_init)(void);
> >>void (*wallclock_init)(void);
> >>+   bool no_cmos_rtc;
> >I'd add
> >
> > u64 flags;
> >
> >to x86_init_ops and then set X86_PLATFORM_NO_RTC or so in there. The
> >reason being, others could use that flags field too, for other stuff and
> >define more bits.
> 
> Maybe timer_flags or platform_flags (or something else) to be a
> little more cscope-friendly?

Sure, I'll go with platform_flags on x86_init_ops. Will repost a new
series after 0-day testing.

  Luis

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-22 Thread Boris Ostrovsky 

On 02/22/2016 05:27 AM, Borislav Petkov wrote:

On Mon, Feb 22, 2016 at 07:07:56AM +0100, Luis R. Rodriguez wrote:

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 1ae89a2721d6..fe0d579b63e3 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -84,11 +84,14 @@ struct x86_init_paging {
   *boot cpu
   * @timer_init:   initialize the platform timer (default 
PIT/HPET)
   * @wallclock_init:   init the wallclock device
+ * @no_cmos_rtc:   set when platform has no CMOS real-time clock
+ * present
   */
  struct x86_init_timers {
void (*setup_percpu_clockev)(void);
void (*timer_init)(void);
void (*wallclock_init)(void);
+   bool no_cmos_rtc;

I'd add

u64 flags;

to x86_init_ops and then set X86_PLATFORM_NO_RTC or so in there. The
reason being, others could use that flags field too, for other stuff and
define more bits.


Maybe timer_flags or platform_flags (or something else) to be a little 
more cscope-friendly?


-boris

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-22 Thread Borislav Petkov 
On Mon, Feb 22, 2016 at 07:07:56AM +0100, Luis R. Rodriguez wrote:
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 1ae89a2721d6..fe0d579b63e3 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -84,11 +84,14 @@ struct x86_init_paging {
>   *   boot cpu
>   * @timer_init:  initialize the platform timer (default 
> PIT/HPET)
>   * @wallclock_init:  init the wallclock device
> + * @no_cmos_rtc: set when platform has no CMOS real-time clock
> + *   present
>   */
>  struct x86_init_timers {
>   void (*setup_percpu_clockev)(void);
>   void (*timer_init)(void);
>   void (*wallclock_init)(void);
> + bool no_cmos_rtc;

I'd add

u64 flags;

to x86_init_ops and then set X86_PLATFORM_NO_RTC or so in there. The
reason being, others could use that flags field too, for other stuff and
define more bits.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-21 Thread Luis R. Rodriguez 
On Fri, Feb 19, 2016 at 03:48:41PM +0100, Luis R. Rodriguez wrote:
> On Fri, Feb 19, 2016 at 02:22:12PM +0100, Juergen Gross wrote:
> > On 19/02/16 14:08, Luis R. Rodriguez wrote:
> > > The current check is a super long winded way of asking if this
> > > is on lguest. The flags is used for legacy features, this is
> > 
> > What about Xen pv-domU? I wouldn't expect those to have PV_SUPPORTED_RTC
> > set.
> 
> Hrm, I see -- how do we check for that in a standard more clean way?

OK we have 4 types of x86 platforms that disable RTC:

  * Intel MID
  * Lguest
  * Xen dom-U
  * x86 on legacy systems annotated with an ACPI legacy flag

So it would seem its best to just generalize the disabling of
the RTC for any x86 platform, we could also fold the ACPI check
into the FADT parse table routine, which is called during
setup_arch().

So how about this:

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index f6192502149e..c261402340e3 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -19,12 +19,6 @@ static inline int paravirt_enabled(void)
return pv_info.paravirt_enabled;
 }
 
-static inline int paravirt_has_feature(unsigned int feature)
-{
-   WARN_ON_ONCE(!pv_info.paravirt_enabled);
-   return (pv_info.features & feature);
-}
-
 static inline void load_sp0(struct tss_struct *tss,
 struct thread_struct *thread)
 {
diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index 77db5616a473..2489d6a08e89 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -70,14 +70,9 @@ struct pv_info {
 #endif
 
int paravirt_enabled;
-   unsigned int features;/* valid only if paravirt_enabled is set */
const char *name;
 };
 
-#define paravirt_has(x) paravirt_has_feature(PV_SUPPORTED_##x)
-/* Supported features */
-#define PV_SUPPORTED_RTC(1<<0)
-
 struct pv_init_ops {
/*
 * Patch may replace one of the defined code sequences with
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 20c11d1aa4cc..10f3614265c1 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -472,7 +472,6 @@ static inline unsigned long current_top_of_stack(void)
 #else
 #define __cpuidnative_cpuid
 #define paravirt_enabled() 0
-#define paravirt_has(x)0
 
 static inline void load_sp0(struct tss_struct *tss,
struct thread_struct *thread)
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 1ae89a2721d6..fe0d579b63e3 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -84,11 +84,14 @@ struct x86_init_paging {
  * boot cpu
  * @timer_init:initialize the platform timer (default 
PIT/HPET)
  * @wallclock_init:init the wallclock device
+ * @no_cmos_rtc:   set when platform has no CMOS real-time clock
+ * present
  */
 struct x86_init_timers {
void (*setup_percpu_clockev)(void);
void (*timer_init)(void);
void (*wallclock_init)(void);
+   bool no_cmos_rtc;
 };
 
 /**
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e75907601a41..6b2cac0f276b 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -913,6 +913,10 @@ late_initcall(hpet_insert_resource);
 
 static int __init acpi_parse_fadt(struct acpi_table_header *table)
 {
+   if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) {
+   pr_debug("ACPI: not registering RTC platform device\n");
+   x86_init.timers.no_cmos_rtc = true;
+   }
 
 #ifdef CONFIG_X86_PM_TIMER
/* detect the location of the ACPI PM Timer */
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 4af8d063fb36..ef92aa84c2e3 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef CONFIG_X86_32
 /*
@@ -188,19 +189,7 @@ static __init int add_rtc_cmos(void)
if (of_have_populated_dt())
return 0;
 
-   /* Intel MID platforms don't have ioport rtc */
-   if (intel_mid_identify_cpu())
-   return -ENODEV;
-
-#ifdef CONFIG_ACPI
-   if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) {
-   /* This warning can likely go away again in a year or two. */
-   pr_info("ACPI: not registering RTC platform device\n");
-   return -ENODEV;
-   }
-#endif
-
-   if (paravirt_enabled() && !paravirt_has(RTC))
+   if (x86_init.timers.no_cmos_rtc)
return -ENODEV;
 
platform_device_register(&rtc_device);
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 4ba229ac3f4f..62535a869e06 100644
--- a/arch/x86/lguest/boo

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-19 Thread Luis R. Rodriguez 
On Fri, Feb 19, 2016 at 02:22:12PM +0100, Juergen Gross wrote:
> On 19/02/16 14:08, Luis R. Rodriguez wrote:
> > The current check is a super long winded way of asking if this
> > is on lguest. The flags is used for legacy features, this is
> 
> What about Xen pv-domU? I wouldn't expect those to have PV_SUPPORTED_RTC
> set.

Hrm, I see -- how do we check for that in a standard more clean way?

  Luis

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-19 Thread David Vrabel 
On 19/02/16 13:08, Luis R. Rodriguez wrote:
> The current check is a super long winded way of asking if this
> is on lguest. The flags is used for legacy features, this is
> likely inspired by the ACPI IA-PC boot architecture flags, where
> as for RTC it annotates No CMOS real-time clock present. I don't
> expect we will be implementing more legacy features, its simply
> pointless so just remove this legacy flag feature thing as well.

No.  This check is needed for Xen PV domU guests and without it PV
guests don't get a console (as I think the history of the change that
introduced this makes clear).

I think this trend towards a coarse-grained subarch instead of a set of
feature bits is the wrong direction.

David

Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check

2016-02-19 Thread Juergen Gross 
On 19/02/16 14:08, Luis R. Rodriguez wrote:
> The current check is a super long winded way of asking if this
> is on lguest. The flags is used for legacy features, this is

What about Xen pv-domU? I wouldn't expect those to have PV_SUPPORTED_RTC
set.


Juergen

> likely inspired by the ACPI IA-PC boot architecture flags, where
> as for RTC it annotates No CMOS real-time clock present. I don't
> expect we will be implementing more legacy features, its simply
> pointless so just remove this legacy flag feature thing as well.
> 
> Signed-off-by: Luis R. Rodriguez 
> ---
>  arch/x86/include/asm/paravirt.h   | 6 --
>  arch/x86/include/asm/paravirt_types.h | 5 -
>  arch/x86/include/asm/processor.h  | 1 -
>  arch/x86/kernel/rtc.c | 3 ++-
>  arch/x86/xen/enlighten.c  | 3 ---
>  5 files changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index f6192502149e..c261402340e3 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -19,12 +19,6 @@ static inline int paravirt_enabled(void)
>   return pv_info.paravirt_enabled;
>  }
>  
> -static inline int paravirt_has_feature(unsigned int feature)
> -{
> - WARN_ON_ONCE(!pv_info.paravirt_enabled);
> - return (pv_info.features & feature);
> -}
> -
>  static inline void load_sp0(struct tss_struct *tss,
>struct thread_struct *thread)
>  {
> diff --git a/arch/x86/include/asm/paravirt_types.h 
> b/arch/x86/include/asm/paravirt_types.h
> index 77db5616a473..2489d6a08e89 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -70,14 +70,9 @@ struct pv_info {
>  #endif
>  
>   int paravirt_enabled;
> - unsigned int features;/* valid only if paravirt_enabled is set */
>   const char *name;
>  };
>  
> -#define paravirt_has(x) paravirt_has_feature(PV_SUPPORTED_##x)
> -/* Supported features */
> -#define PV_SUPPORTED_RTC(1<<0)
> -
>  struct pv_init_ops {
>   /*
>* Patch may replace one of the defined code sequences with
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index 20c11d1aa4cc..10f3614265c1 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -472,7 +472,6 @@ static inline unsigned long current_top_of_stack(void)
>  #else
>  #define __cpuid  native_cpuid
>  #define paravirt_enabled()   0
> -#define paravirt_has(x)  0
>  
>  static inline void load_sp0(struct tss_struct *tss,
>   struct thread_struct *thread)
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 4af8d063fb36..8471523ee127 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #ifdef CONFIG_X86_32
>  /*
> @@ -200,7 +201,7 @@ static __init int add_rtc_cmos(void)
>   }
>  #endif
>  
> - if (paravirt_enabled() && !paravirt_has(RTC))
> + if (boot_params.hdr.hardware_subarch == X86_SUBARCH_LGUEST)
>   return -ENODEV;
>  
>   platform_device_register(&rtc_device);
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 5b3f1c763806..5c06169bce27 100644
> --- 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",
>  };
>  
> @@ -1526,8 +1525,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;
>