[PATCH 0/2] xen: fix illegal rtc device usage of pv guests

A recent change in mc146818_get_time() resulted in WARN splats when
booting a Xen PV guest.

The main reason is that there is a code path resulting in accessing a
RTC device which is not present, which has been made obvious by a
call of WARN() in this case.

This small series is fixing this issue by:

- avoiding the RTC device access from drivers/base/power/trace.c in
  cast there is no legacy RTC device available
- resetting the availability flag of a legacy RTC device for Xen PV
  guests

Juergen Gross (2):
  PM: base: power: don't try to use non-existing RTC for storing data
  xen: reset legacy rtc flag for PV domU

 arch/x86/xen/enlighten_pv.c |  7 +++++++
 drivers/base/power/trace.c  | 10 ++++++++++
 2 files changed, 17 insertions(+)

-- 
2.26.2

In there is no legacy RTC device, don't try to use it for storing trace
data across suspend/resume.

Cc: <redacted>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/base/power/trace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index a97f33d0c59f..b7c80849455c 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c

@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/rtc.h>
 #include <linux/suspend.h>
+#include <linux/init.h>
 
 #include <linux/mc146818rtc.h>
 

@@ -165,6 +166,9 @@ void generate_pm_trace(const void *tracedata, unsigned int user)
 	const char *file = *(const char **)(tracedata + 2);
 	unsigned int user_hash_value, file_hash_value;
 
+	if (!x86_platform.legacy.rtc)
+		return 0;
+
 	user_hash_value = user % USERHASH;
 	file_hash_value = hash_string(lineno, file, FILEHASH);
 	set_magic_time(user_hash_value, file_hash_value, dev_hash_value);

@@ -267,6 +271,9 @@ static struct notifier_block pm_trace_nb = {
 
 static int __init early_resume_init(void)
 {
+	if (!x86_platform.legacy.rtc)
+		return 0;
+
 	hash_value_early_read = read_magic_time();
 	register_pm_notifier(&pm_trace_nb);
 	return 0;

@@ -277,6 +284,9 @@ static int __init late_resume_init(void)
 	unsigned int val = hash_value_early_read;
 	unsigned int user, file, dev;
 
+	if (!x86_platform.legacy.rtc)
+		return 0;
+
 	user = val % USERHASH;
 	val = val / USERHASH;
 	file = val % FILEHASH;

-- 
2.26.2

On Fri, Sep 03, 2021 at 10:49:36AM +0200, Juergen Gross wrote:

Why does the driver core code here care about a platform/arch-specific
thing at all?  Did you just break all other arches?

thanks,

greg k-h

On 03.09.21 10:56, Greg Kroah-Hartman wrote:

On Fri, Sep 03, 2021 at 10:49:36AM +0200, Juergen Gross wrote:

quoted

In there is no legacy RTC device, don't try to use it for storing trace
data across suspend/resume.

Cc: <redacted>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index a97f33d0c59f..b7c80849455c 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c

@@ -13,6 +13,7 @@
  #include <linux/export.h>
  #include <linux/rtc.h>
  #include <linux/suspend.h>
+#include <linux/init.h>
  
  #include <linux/mc146818rtc.h>
  

@@ -165,6 +166,9 @@ void generate_pm_trace(const void *tracedata, unsigned int user)
  	const char *file = *(const char **)(tracedata + 2);
  	unsigned int user_hash_value, file_hash_value;
  
+	if (!x86_platform.legacy.rtc)
+		return 0;

Why does the driver core code here care about a platform/arch-specific
thing at all?  Did you just break all other arches?

This file is only compiled for x86. It depends on CONFIG_PM_TRACE_RTC,
which has a "depends on X86" attribute.


Juergen

On Fri, Sep 03, 2021 at 11:01:58AM +0200, Juergen Gross wrote:

On 03.09.21 10:56, Greg Kroah-Hartman wrote:

quoted

On Fri, Sep 03, 2021 at 10:49:36AM +0200, Juergen Gross wrote:

quoted

In there is no legacy RTC device, don't try to use it for storing trace
data across suspend/resume.

Cc: <redacted>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index a97f33d0c59f..b7c80849455c 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c

@@ -13,6 +13,7 @@
  #include <linux/export.h>
  #include <linux/rtc.h>
  #include <linux/suspend.h>
+#include <linux/init.h>
  #include <linux/mc146818rtc.h>

@@ -165,6 +166,9 @@ void generate_pm_trace(const void *tracedata, unsigned int user)
  	const char *file = *(const char **)(tracedata + 2);
  	unsigned int user_hash_value, file_hash_value;
+	if (!x86_platform.legacy.rtc)
+		return 0;

Why does the driver core code here care about a platform/arch-specific
thing at all?  Did you just break all other arches?

This file is only compiled for x86. It depends on CONFIG_PM_TRACE_RTC,
which has a "depends on X86" attribute.

Odd, and not obvious at all :(

Ok, I'll let Rafael review this...

On Fri, Sep 3, 2021 at 11:02 AM Juergen Gross [off-list ref] wrote:

On 03.09.21 10:56, Greg Kroah-Hartman wrote:

quoted

On Fri, Sep 03, 2021 at 10:49:36AM +0200, Juergen Gross wrote:

quoted

In there is no legacy RTC device, don't try to use it for storing trace
data across suspend/resume.

Cc: <redacted>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index a97f33d0c59f..b7c80849455c 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c

@@ -13,6 +13,7 @@
  #include <linux/export.h>
  #include <linux/rtc.h>
  #include <linux/suspend.h>
+#include <linux/init.h>

  #include <linux/mc146818rtc.h>

@@ -165,6 +166,9 @@ void generate_pm_trace(const void *tracedata, unsigned int user)
     const char *file = *(const char **)(tracedata + 2);
     unsigned int user_hash_value, file_hash_value;

+    if (!x86_platform.legacy.rtc)
+            return 0;

Why does the driver core code here care about a platform/arch-specific
thing at all?  Did you just break all other arches?

This file is only compiled for x86. It depends on CONFIG_PM_TRACE_RTC,
which has a "depends on X86" attribute.

This feature uses the CMOS RTC memory to store data, so if that memory
is not present, it's better to avoid using it.

Please feel free to add

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

to this patch or let me know if you want me to take it.

On 06.09.21 19:07, Rafael J. Wysocki wrote:

On Fri, Sep 3, 2021 at 11:02 AM Juergen Gross [off-list ref] wrote:

quoted

On 03.09.21 10:56, Greg Kroah-Hartman wrote:

quoted

On Fri, Sep 03, 2021 at 10:49:36AM +0200, Juergen Gross wrote:

quoted

In there is no legacy RTC device, don't try to use it for storing trace
data across suspend/resume.

Cc: <redacted>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
   drivers/base/power/trace.c | 10 ++++++++++
   1 file changed, 10 insertions(+)

diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index a97f33d0c59f..b7c80849455c 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c

@@ -13,6 +13,7 @@
   #include <linux/export.h>
   #include <linux/rtc.h>
   #include <linux/suspend.h>
+#include <linux/init.h>

   #include <linux/mc146818rtc.h>

@@ -165,6 +166,9 @@ void generate_pm_trace(const void *tracedata, unsigned int user)
      const char *file = *(const char **)(tracedata + 2);
      unsigned int user_hash_value, file_hash_value;

+    if (!x86_platform.legacy.rtc)
+            return 0;

Why does the driver core code here care about a platform/arch-specific
thing at all?  Did you just break all other arches?

This file is only compiled for x86. It depends on CONFIG_PM_TRACE_RTC,
which has a "depends on X86" attribute.

This feature uses the CMOS RTC memory to store data, so if that memory
is not present, it's better to avoid using it.

Please feel free to add

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

Thanks!

to this patch or let me know if you want me to take it.

No, I can take it with the other patch of this small series, thanks.


Juergen