xen: fix illegal rtc device usage of pv guests

PATCH In review Juergen Gross 4 participants 10 messages Sep 3, 2021 – Sep 10, 2021

Subsystems (7)

Stable branch x86 Driver core Suspend to RAM +3 more subsystems

lore

Share & tools

lore

Juergen Gross

author maintainer

2021-09-03 08:49 UTC #0 lore raw

cover 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(+)

Signature · 1 line

2.26.2

Juergen Gross

author maintainer

2021-09-03 08:49 UTC #1 lore raw

PATCH 1/2 PM: base: power: don't try to use non-existing RTC for storing data

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

Cc <[email protected]>

Signed-off-by Juergen Gross <[email protected]>

---
 drivers/base/power/trace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/base/power/trace.c +10 -0

index a97f33d0c59f..b7c80849455c 100644
@@ -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;

Signature · 1 line

2.26.2

Greg Kroah-Hartman

maintainer

2021-09-03 08:56 UTC #2 lore raw

Re: [PATCH 1/2] PM: base: power: don't try to use non-existing RTC for storing data

Quoted 22 lines · Juergen Gross

In there is no legacy RTC device, don't try to use it for storing trace data across suspend/resume.
Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>
---
 drivers/base/power/trace.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/base/power/trace.c +1 -0
index a97f33d0c59f..b7c80849455c 100644
@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/rtc.h>
 #include <linux/suspend.h>
+#include <linux/init.h>
 
 #include <linux/mc146818rtc.h>
 

drivers/base/power/trace.c void generate_pm_trace(const void *tracedata, unsigned int user) · line 170

Quoted 5 lines

drivers/base/power/trace.c +2 -0

@@ -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?

thanks,

greg k-h

Juergen Gross

author maintainer

2021-09-03 09:02 UTC #3 lore raw

Re: [PATCH 1/2] PM: base: power: don't try to use non-existing RTC for storing data

Quoted 26 lines · Greg Kroah-Hartman

Quoted 25 lines · Juergen Gross

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

Cc <[email protected]>

Signed-off-by Juergen Gross <[email protected]>

---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

drivers/base/power/trace.c +1 -0

index a97f33d0c59f..b7c80849455c 100644
@@ -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;

Quoted 5 lines

drivers/base/power/trace.c void generate_pm_trace(const void *tracedata, unsigned int user) · line 170
Quoted 2 lines
drivers/base/power/trace.c +2 -0
+	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

Greg Kroah-Hartman

maintainer

2021-09-03 09:09 UTC #4 lore raw

Re: [PATCH 1/2] PM: base: power: don't try to use non-existing RTC for storing data

Quoted 30 lines · Juergen Gross

Quoted 20 lines · Greg Kroah-Hartman
Quoted 19 lines · Juergen Gross
In there is no legacy RTC device, don't try to use it for storing trace data across suspend/resume.
Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>
---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)
drivers/base/power/trace.c +1 -0
index a97f33d0c59f..b7c80849455c 100644
@@ -13,6 +13,7 @@
  #include <linux/export.h>
  #include <linux/rtc.h>
  #include <linux/suspend.h>
+#include <linux/init.h>
  #include <linux/mc146818rtc.h>
Quoted 8 lines
drivers/base/power/trace.c void generate_pm_trace(const void *tracedata, unsigned int user) · line 169
Quoted 5 lines
drivers/base/power/trace.c +2 -0
@@ -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?

Quoted 2 lines

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...

Rafael J. Wysocki

maintainer Reviewed-by

2021-09-06 17:07 UTC #5 lore raw

Re: [PATCH 1/2] PM: base: power: don't try to use non-existing RTC for storing data

In reply to Juergen Gross

Quoted 28 lines · Juergen Gross

Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Juergen Gross
In there is no legacy RTC device, don't try to use it for storing trace data across suspend/resume.
Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>
---
  drivers/base/power/trace.c | 10 ++++++++++
  1 file changed, 10 insertions(+)
drivers/base/power/trace.c +1 -0
index a97f33d0c59f..b7c80849455c 100644
@@ -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;
 

Quoted 8 lines

Quoted 5 lines
drivers/base/power/trace.c void generate_pm_trace(const void *tracedata, unsigned int user) · line 170
Quoted 2 lines
drivers/base/power/trace.c +2 -0
+    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 <[email protected]>

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

Juergen Gross

author maintainer

2021-09-07 04:18 UTC #6 lore raw

Re: [PATCH 1/2] PM: base: power: don't try to use non-existing RTC for storing data

Quoted 38 lines · Rafael J. Wysocki

Quoted 28 lines · Juergen Gross
Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Juergen Gross
In there is no legacy RTC device, don't try to use it for storing trace data across suspend/resume.
Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>
---
   drivers/base/power/trace.c | 10 ++++++++++
   1 file changed, 10 insertions(+)
drivers/base/power/trace.c +1 -0
index a97f33d0c59f..b7c80849455c 100644
@@ -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;
 
Quoted 8 lines
Quoted 5 lines
drivers/base/power/trace.c void generate_pm_trace(const void *tracedata, unsigned int user) · line 170
Quoted 2 lines
drivers/base/power/trace.c +2 -0
+    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.

Quoted 6 lines

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 <[email protected]>

Thanks!

Quoted 3 lines

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

Juergen Gross

author maintainer

2021-09-03 08:49 UTC #7 lore raw

PATCH 2/2 xen: reset legacy rtc flag for PV domU

In reply to Juergen Gross

A Xen PV guest doesn't have a legacy RTC device, so reset the legacy RTC flag. Otherwise the following WARN splat will occur at boot:

[    1.333404] WARNING: CPU: 1 PID: 1 at /home/gross/linux/head/drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1be/0x210
[    1.333404] Modules linked in:
[    1.333404] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W         5.14.0-rc7-default+ #282
[    1.333404] RIP: e030:mc146818_get_time+0x1be/0x210
[    1.333404] Code: c0 64 01 c5 83 fd 45 89 6b 14 7f 06 83 c5 64 89 6b 14 41 83 ec 01 b8 02 00 00 00 44 89 63 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 <0f> 0b 48 c7 c7 30 0e ef 82 4c 89 e6 e8 71 2a 24 00 48 c7 c0 ff ff
[    1.333404] RSP: e02b:ffffc90040093df8 EFLAGS: 00010002
[    1.333404] RAX: 00000000000000ff RBX: ffffc90040093e34 RCX: 0000000000000000
[    1.333404] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000d
[    1.333404] RBP: ffffffff82ef0e30 R08: ffff888005013e60 R09: 0000000000000000
[    1.333404] R10: ffffffff82373e9b R11: 0000000000033080 R12: 0000000000000200
[    1.333404] R13: 0000000000000000 R14: 0000000000000002 R15: ffffffff82cdc6d4
[    1.333404] FS:  0000000000000000(0000) GS:ffff88807d440000(0000) knlGS:0000000000000000
[    1.333404] CS:  10000e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.333404] CR2: 0000000000000000 CR3: 000000000260a000 CR4: 0000000000050660
[    1.333404] Call Trace:
[    1.333404]  ? wakeup_sources_sysfs_init+0x30/0x30
[    1.333404]  ? rdinit_setup+0x2b/0x2b
[    1.333404]  early_resume_init+0x23/0xa4
[    1.333404]  ? cn_proc_init+0x36/0x36
[    1.333404]  do_one_initcall+0x3e/0x200
[    1.333404]  kernel_init_freeable+0x232/0x28e
[    1.333404]  ? rest_init+0xd0/0xd0
[    1.333404]  kernel_init+0x16/0x120
[    1.333404]  ret_from_fork+0x1f/0x30

Cc <[email protected]>

Signed-off-by Juergen Gross <[email protected]>

---
 arch/x86/xen/enlighten_pv.c | 7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/xen/enlighten_pv.c +7 -0

index 753f63734c13..349f780a1567 100644
@@ -1214,6 +1214,11 @@ static void __init xen_dom0_set_legacy_features(void)
 	x86_platform.legacy.rtc = 1;
 }
 
+static void __init xen_domu_set_legacy_features(void)
+{
+	x86_platform.legacy.rtc = 0;
+}
+
 /* First C function to be called on Xen boot */
 asmlinkage __visible void __init xen_start_kernel(void)
 {

@@ -1359,6 +1364,8 @@ asmlinkage __visible void __init xen_start_kernel(void)
 		add_preferred_console("xenboot", 0, NULL);
 		if (pci_xen)
 			x86_init.pci.arch_init = pci_xen_init;
+		x86_platform.set_legacy_features =
+				xen_domu_set_legacy_features;
 	} else {
 		const struct dom0_vga_console_info *info =
 			(void *)((char *)xen_start_info +

Signature · 1 line

2.26.2

Boris Ostrovsky

reviewer Reviewed-by on v1 2/2

2021-09-08 14:54 UTC #8 lore raw

Re: [PATCH 2/2] xen: reset legacy rtc flag for PV domU

Quoted 28 lines · Juergen Gross

A Xen PV guest doesn't have a legacy RTC device, so reset the legacy RTC flag. Otherwise the following WARN splat will occur at boot:

[    1.333404] WARNING: CPU: 1 PID: 1 at /home/gross/linux/head/drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1be/0x210
[    1.333404] Modules linked in:
[    1.333404] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W         5.14.0-rc7-default+ #282
[    1.333404] RIP: e030:mc146818_get_time+0x1be/0x210
[    1.333404] Code: c0 64 01 c5 83 fd 45 89 6b 14 7f 06 83 c5 64 89 6b 14 41 83 ec 01 b8 02 00 00 00 44 89 63 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 <0f> 0b 48 c7 c7 30 0e ef 82 4c 89 e6 e8 71 2a 24 00 48 c7 c0 ff ff
[    1.333404] RSP: e02b:ffffc90040093df8 EFLAGS: 00010002
[    1.333404] RAX: 00000000000000ff RBX: ffffc90040093e34 RCX: 0000000000000000
[    1.333404] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000d
[    1.333404] RBP: ffffffff82ef0e30 R08: ffff888005013e60 R09: 0000000000000000
[    1.333404] R10: ffffffff82373e9b R11: 0000000000033080 R12: 0000000000000200
[    1.333404] R13: 0000000000000000 R14: 0000000000000002 R15: ffffffff82cdc6d4
[    1.333404] FS:  0000000000000000(0000) GS:ffff88807d440000(0000) knlGS:0000000000000000
[    1.333404] CS:  10000e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.333404] CR2: 0000000000000000 CR3: 000000000260a000 CR4: 0000000000050660
[    1.333404] Call Trace:
[    1.333404]  ? wakeup_sources_sysfs_init+0x30/0x30
[    1.333404]  ? rdinit_setup+0x2b/0x2b
[    1.333404]  early_resume_init+0x23/0xa4
[    1.333404]  ? cn_proc_init+0x36/0x36
[    1.333404]  do_one_initcall+0x3e/0x200
[    1.333404]  kernel_init_freeable+0x232/0x28e
[    1.333404]  ? rest_init+0xd0/0xd0
[    1.333404]  kernel_init+0x16/0x120
[    1.333404]  ret_from_fork+0x1f/0x30

Quoted 2 lines

Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>

Fixes 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") ?

Reviewed-by Boris Ostrovsky <[email protected]>

Juergen Gross

author maintainer

2021-09-10 07:28 UTC #9 lore raw

Re: [PATCH 2/2] xen: reset legacy rtc flag for PV domU

Quoted 30 lines · Boris Ostrovsky

Quoted 28 lines · Juergen Gross

A Xen PV guest doesn't have a legacy RTC device, so reset the legacy RTC flag. Otherwise the following WARN splat will occur at boot:

[    1.333404] WARNING: CPU: 1 PID: 1 at /home/gross/linux/head/drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1be/0x210
[    1.333404] Modules linked in:
[    1.333404] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W         5.14.0-rc7-default+ #282
[    1.333404] RIP: e030:mc146818_get_time+0x1be/0x210
[    1.333404] Code: c0 64 01 c5 83 fd 45 89 6b 14 7f 06 83 c5 64 89 6b 14 41 83 ec 01 b8 02 00 00 00 44 89 63 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 <0f> 0b 48 c7 c7 30 0e ef 82 4c 89 e6 e8 71 2a 24 00 48 c7 c0 ff ff
[    1.333404] RSP: e02b:ffffc90040093df8 EFLAGS: 00010002
[    1.333404] RAX: 00000000000000ff RBX: ffffc90040093e34 RCX: 0000000000000000
[    1.333404] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 000000000000000d
[    1.333404] RBP: ffffffff82ef0e30 R08: ffff888005013e60 R09: 0000000000000000
[    1.333404] R10: ffffffff82373e9b R11: 0000000000033080 R12: 0000000000000200
[    1.333404] R13: 0000000000000000 R14: 0000000000000002 R15: ffffffff82cdc6d4
[    1.333404] FS:  0000000000000000(0000) GS:ffff88807d440000(0000) knlGS:0000000000000000
[    1.333404] CS:  10000e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.333404] CR2: 0000000000000000 CR3: 000000000260a000 CR4: 0000000000050660
[    1.333404] Call Trace:
[    1.333404]  ? wakeup_sources_sysfs_init+0x30/0x30
[    1.333404]  ? rdinit_setup+0x2b/0x2b
[    1.333404]  early_resume_init+0x23/0xa4
[    1.333404]  ? cn_proc_init+0x36/0x36
[    1.333404]  do_one_initcall+0x3e/0x200
[    1.333404]  kernel_init_freeable+0x232/0x28e
[    1.333404]  ? rest_init+0xd0/0xd0
[    1.333404]  kernel_init+0x16/0x120
[    1.333404]  ret_from_fork+0x1f/0x30

Quoted 5 lines

Quoted 2 lines
Cc <[email protected]>
Signed-off-by Juergen Gross <[email protected]>
Fixes 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs") ?

I don't think so. Its rather:

Fixes 8d152e7a5c7537 ("x86/rtc: Replace paravirt rtc check with platform legacy quirk")

as this was the commit leading to domUs having the rtc capability being set.

Quoted 1 line

Reviewed-by Boris Ostrovsky <[email protected]>

Thanks,

Juergen