2022-04-02 16:36:16

by Laurent Dufour

[permalink] [raw]
Subject: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits
mode (MSR[SF] unset).

The change in MSR is done in enter_rtas() in a relatively complex way,
since the MSR value could be hardcoded.

Furthermore, a panic has been reported when hitting the watchdog interrupt
while running in RTAS, this leads to the following stack trace:

[69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP
[69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat TB:997504470175378 (15980ms ago)
[69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
[69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
[69244.027587][ C24] Supported: No, Unreleased kernel
[69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
[69244.027609][ C24] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
[69244.027612][ C24] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
[69244.027615][ C24] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
[69244.027625][ C24] CFAR: 000000000000011c IRQMASK: 1
[69244.027625][ C24] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
[69244.027625][ C24] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
[69244.027625][ C24] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
[69244.027625][ C24] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
[69244.027625][ C24] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
[69244.027625][ C24] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
[69244.027625][ C24] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
[69244.027625][ C24] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
[69244.027663][ C24] NIP [000000001fb41050] 0x1fb41050
[69244.027696][ C24] LR [000000001fb4104c] 0x1fb4104c
[69244.027699][ C24] Call Trace:
[69244.027701][ C24] Instruction dump:
[69244.027723][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[69244.027728][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1]
[69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
[69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
[69244.028171][T87504] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
[69244.028307][T87504] Supported: No, Unreleased kernel
[69244.028385][T87504] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
[69244.028408][T87504] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
[69244.028418][T87504] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
[69244.028429][T87504] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
[69244.028444][T87504] CFAR: 000000000000011c IRQMASK: 1
[69244.028444][T87504] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
[69244.028444][T87504] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
[69244.028444][T87504] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
[69244.028444][T87504] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
[69244.028444][T87504] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
[69244.028444][T87504] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
[69244.028444][T87504] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
[69244.028444][T87504] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
[69244.028534][T87504] NIP [000000001fb41050] 0x1fb41050
[69244.028543][T87504] LR [000000001fb4104c] 0x1fb4104c
[69244.028549][T87504] Call Trace:
[69244.028554][T87504] Instruction dump:
[69244.028561][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[69244.028575][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[69244.028607][T87504] ---[ end trace 3ddec07f638c34a2 ]---

This happens because MSR[RI] is unset when entering RTAS but there is no
valid reason to not set it here.

RTAS is expected to be called with MSR[RI] as specified in PAPR+ section
"7.2.1 Machine State":

R1–7.2.1–9. If called with MSR[RI] equal to 1, then RTAS must protect its
own critical regions from recursion by setting the MSRRI bit to 0 when in
the critical regions.

Fixing this by reviewing the way MSR is compute before calling RTAS. Now a
hardcoded value meaning real mode, 32 bits and Recoverable Interrupt is
loaded.

In addition a check is added in do_enter_rtas() to detect calls made with
MSR[RI] unset, as we are forcing it on later.

This patch has been tested on the following machines:
Power KVM Guest
P8 S822L (host Ubuntu kernel 5.11.0-49-generic)
PowerVM LPAR
P8 9119-MME (FW860.A1)
p9 9008-22L (FW950.00)
P10 9080-HEX (FW1010.00)

Changes in V2:
- Change comment in code to indicate NMI (Nick's comment)
- Add reference to PAPR+ in the change log (Michael's comment)

Cc: [email protected]
Suggested-by: Nicholas Piggin <[email protected]>
Signed-off-by: Laurent Dufour <[email protected]>
---
arch/powerpc/kernel/entry_64.S | 20 ++++++++------------
arch/powerpc/kernel/rtas.c | 5 +++++
2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 9581906b5ee9..65cb14b56f8d 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
clrldi r4,r4,2 /* convert to realmode address */
mtlr r4

- li r0,0
- ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
- andc r0,r6,r0
-
- li r9,1
- rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
- ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
- andc r6,r0,r9
-
__enter_rtas:
- sync /* disable interrupts so SRR0/1 */
- mtmsrd r0 /* don't get trashed */
-
LOAD_REG_ADDR(r4, rtas)
ld r5,RTASENTRY(r4) /* get the rtas->entry value */
ld r4,RTASBASE(r4) /* get the rtas->base value */
+
+ /* RTAS runs in 32bits real mode but let MSR[]RI on as we may hit
+ * NMI (SRESET or MCE). RTAS should disable RI in its critical
+ * regions (as specified in PAPR+ section 7.2.1). */
+ LOAD_REG_IMMEDIATE(r6, MSR_ME|MSR_RI)
+
+ li r0,0
+ mtmsrd r0,1 /* disable RI before using SRR0/1 */

mtspr SPRN_SRR0,r5
mtspr SPRN_SRR1,r6
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 1f42aabbbab3..d7775b8c8853 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -49,6 +49,11 @@ void enter_rtas(unsigned long);

static inline void do_enter_rtas(unsigned long args)
{
+ unsigned long msr;
+
+ msr = mfmsr();
+ BUG_ON(!(msr & MSR_RI));
+
enter_rtas(args);

srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
--
2.35.1


2022-04-13 08:33:48

by Nicholas Piggin

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Excerpts from Laurent Dufour's message of April 2, 2022 12:06 am:
> RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits
> mode (MSR[SF] unset).
>
> The change in MSR is done in enter_rtas() in a relatively complex way,
> since the MSR value could be hardcoded.
>
> Furthermore, a panic has been reported when hitting the watchdog interrupt
> while running in RTAS, this leads to the following stack trace:
>
> [69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP
> [69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat TB:997504470175378 (15980ms ago)
> [69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
> [69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
> [69244.027587][ C24] Supported: No, Unreleased kernel
> [69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
> [69244.027609][ C24] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
> [69244.027612][ C24] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
> [69244.027615][ C24] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
> [69244.027625][ C24] CFAR: 000000000000011c IRQMASK: 1
> [69244.027625][ C24] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
> [69244.027625][ C24] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
> [69244.027625][ C24] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
> [69244.027625][ C24] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
> [69244.027625][ C24] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
> [69244.027625][ C24] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
> [69244.027625][ C24] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
> [69244.027625][ C24] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
> [69244.027663][ C24] NIP [000000001fb41050] 0x1fb41050
> [69244.027696][ C24] LR [000000001fb4104c] 0x1fb4104c
> [69244.027699][ C24] Call Trace:
> [69244.027701][ C24] Instruction dump:
> [69244.027723][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.027728][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1]
> [69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> [69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
> [69244.028171][T87504] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
> [69244.028307][T87504] Supported: No, Unreleased kernel
> [69244.028385][T87504] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
> [69244.028408][T87504] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
> [69244.028418][T87504] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
> [69244.028429][T87504] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
> [69244.028444][T87504] CFAR: 000000000000011c IRQMASK: 1
> [69244.028444][T87504] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
> [69244.028444][T87504] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
> [69244.028444][T87504] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
> [69244.028444][T87504] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
> [69244.028444][T87504] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
> [69244.028444][T87504] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
> [69244.028444][T87504] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
> [69244.028444][T87504] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
> [69244.028534][T87504] NIP [000000001fb41050] 0x1fb41050
> [69244.028543][T87504] LR [000000001fb4104c] 0x1fb4104c
> [69244.028549][T87504] Call Trace:
> [69244.028554][T87504] Instruction dump:
> [69244.028561][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.028575][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.028607][T87504] ---[ end trace 3ddec07f638c34a2 ]---
>
> This happens because MSR[RI] is unset when entering RTAS but there is no
> valid reason to not set it here.
>
> RTAS is expected to be called with MSR[RI] as specified in PAPR+ section
> "7.2.1 Machine State":
>
> R1–7.2.1–9. If called with MSR[RI] equal to 1, then RTAS must protect its
> own critical regions from recursion by setting the MSRRI bit to 0 when in
> the critical regions.
>
> Fixing this by reviewing the way MSR is compute before calling RTAS. Now a
> hardcoded value meaning real mode, 32 bits and Recoverable Interrupt is
> loaded.
>
> In addition a check is added in do_enter_rtas() to detect calls made with
> MSR[RI] unset, as we are forcing it on later.
>
> This patch has been tested on the following machines:
> Power KVM Guest
> P8 S822L (host Ubuntu kernel 5.11.0-49-generic)
> PowerVM LPAR
> P8 9119-MME (FW860.A1)
> p9 9008-22L (FW950.00)
> P10 9080-HEX (FW1010.00)
>
> Changes in V2:
> - Change comment in code to indicate NMI (Nick's comment)
> - Add reference to PAPR+ in the change log (Michael's comment)
>
> Cc: [email protected]
> Suggested-by: Nicholas Piggin <[email protected]>
> Signed-off-by: Laurent Dufour <[email protected]>

Still looks good,

Reviewed-by: Nicholas Piggin <[email protected]>

> ---
> arch/powerpc/kernel/entry_64.S | 20 ++++++++------------
> arch/powerpc/kernel/rtas.c | 5 +++++
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 9581906b5ee9..65cb14b56f8d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
> clrldi r4,r4,2 /* convert to realmode address */
> mtlr r4
>
> - li r0,0
> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
> - andc r0,r6,r0
> -
> - li r9,1
> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
> - andc r6,r0,r9
> -
> __enter_rtas:
> - sync /* disable interrupts so SRR0/1 */
> - mtmsrd r0 /* don't get trashed */
> -
> LOAD_REG_ADDR(r4, rtas)
> ld r5,RTASENTRY(r4) /* get the rtas->entry value */
> ld r4,RTASBASE(r4) /* get the rtas->base value */
> +
> + /* RTAS runs in 32bits real mode but let MSR[]RI on as we may hit
> + * NMI (SRESET or MCE). RTAS should disable RI in its critical
> + * regions (as specified in PAPR+ section 7.2.1). */
> + LOAD_REG_IMMEDIATE(r6, MSR_ME|MSR_RI)
> +
> + li r0,0
> + mtmsrd r0,1 /* disable RI before using SRR0/1 */
>
> mtspr SPRN_SRR0,r5
> mtspr SPRN_SRR1,r6
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 1f42aabbbab3..d7775b8c8853 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>
> static inline void do_enter_rtas(unsigned long args)
> {
> + unsigned long msr;
> +
> + msr = mfmsr();
> + BUG_ON(!(msr & MSR_RI));
> +
> enter_rtas(args);
>
> srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
> --
> 2.35.1
>
>

2022-04-22 02:38:11

by Laurent Dufour

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

On 13/04/2022, 07:58:42, Nicholas Piggin wrote:
> Excerpts from Laurent Dufour's message of April 2, 2022 12:06 am:
>> RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits
>> mode (MSR[SF] unset).
>>
>> The change in MSR is done in enter_rtas() in a relatively complex way,
>> since the MSR value could be hardcoded.
>>
>> Furthermore, a panic has been reported when hitting the watchdog interrupt
>> while running in RTAS, this leads to the following stack trace:
>>
>> [69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP
>> [69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat TB:997504470175378 (15980ms ago)
>> [69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
>> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
>> [69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
>> [69244.027587][ C24] Supported: No, Unreleased kernel
>> [69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
>> [69244.027609][ C24] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
>> [69244.027612][ C24] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
>> [69244.027615][ C24] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
>> [69244.027625][ C24] CFAR: 000000000000011c IRQMASK: 1
>> [69244.027625][ C24] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
>> [69244.027625][ C24] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
>> [69244.027625][ C24] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
>> [69244.027625][ C24] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
>> [69244.027625][ C24] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
>> [69244.027625][ C24] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
>> [69244.027625][ C24] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
>> [69244.027625][ C24] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
>> [69244.027663][ C24] NIP [000000001fb41050] 0x1fb41050
>> [69244.027696][ C24] LR [000000001fb4104c] 0x1fb4104c
>> [69244.027699][ C24] Call Trace:
>> [69244.027701][ C24] Instruction dump:
>> [69244.027723][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.027728][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1]
>> [69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> [69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
>> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
>> [69244.028171][T87504] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
>> [69244.028307][T87504] Supported: No, Unreleased kernel
>> [69244.028385][T87504] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
>> [69244.028408][T87504] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
>> [69244.028418][T87504] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
>> [69244.028429][T87504] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
>> [69244.028444][T87504] CFAR: 000000000000011c IRQMASK: 1
>> [69244.028444][T87504] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
>> [69244.028444][T87504] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
>> [69244.028444][T87504] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
>> [69244.028444][T87504] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
>> [69244.028444][T87504] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
>> [69244.028444][T87504] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
>> [69244.028444][T87504] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
>> [69244.028444][T87504] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
>> [69244.028534][T87504] NIP [000000001fb41050] 0x1fb41050
>> [69244.028543][T87504] LR [000000001fb4104c] 0x1fb4104c
>> [69244.028549][T87504] Call Trace:
>> [69244.028554][T87504] Instruction dump:
>> [69244.028561][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.028575][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.028607][T87504] ---[ end trace 3ddec07f638c34a2 ]---
>>
>> This happens because MSR[RI] is unset when entering RTAS but there is no
>> valid reason to not set it here.
>>
>> RTAS is expected to be called with MSR[RI] as specified in PAPR+ section
>> "7.2.1 Machine State":
>>
>> R1–7.2.1–9. If called with MSR[RI] equal to 1, then RTAS must protect its
>> own critical regions from recursion by setting the MSRRI bit to 0 when in
>> the critical regions.
>>
>> Fixing this by reviewing the way MSR is compute before calling RTAS. Now a
>> hardcoded value meaning real mode, 32 bits and Recoverable Interrupt is
>> loaded.
>>
>> In addition a check is added in do_enter_rtas() to detect calls made with
>> MSR[RI] unset, as we are forcing it on later.
>>
>> This patch has been tested on the following machines:
>> Power KVM Guest
>> P8 S822L (host Ubuntu kernel 5.11.0-49-generic)
>> PowerVM LPAR
>> P8 9119-MME (FW860.A1)
>> p9 9008-22L (FW950.00)
>> P10 9080-HEX (FW1010.00)
>>
>> Changes in V2:
>> - Change comment in code to indicate NMI (Nick's comment)
>> - Add reference to PAPR+ in the change log (Michael's comment)
>>
>> Cc: [email protected]
>> Suggested-by: Nicholas Piggin <[email protected]>
>> Signed-off-by: Laurent Dufour <[email protected]>
>
> Still looks good,
>
> Reviewed-by: Nicholas Piggin <[email protected]>

Hi Michael,

Do you consider peeking that patch ?

Thanks,
Laurent.

>
>> ---
>> arch/powerpc/kernel/entry_64.S | 20 ++++++++------------
>> arch/powerpc/kernel/rtas.c | 5 +++++
>> 2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 9581906b5ee9..65cb14b56f8d 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>> clrldi r4,r4,2 /* convert to realmode address */
>> mtlr r4
>>
>> - li r0,0
>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>> - andc r0,r6,r0
>> -
>> - li r9,1
>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>> - andc r6,r0,r9
>> -
>> __enter_rtas:
>> - sync /* disable interrupts so SRR0/1 */
>> - mtmsrd r0 /* don't get trashed */
>> -
>> LOAD_REG_ADDR(r4, rtas)
>> ld r5,RTASENTRY(r4) /* get the rtas->entry value */
>> ld r4,RTASBASE(r4) /* get the rtas->base value */
>> +
>> + /* RTAS runs in 32bits real mode but let MSR[]RI on as we may hit
>> + * NMI (SRESET or MCE). RTAS should disable RI in its critical
>> + * regions (as specified in PAPR+ section 7.2.1). */
>> + LOAD_REG_IMMEDIATE(r6, MSR_ME|MSR_RI)
>> +
>> + li r0,0
>> + mtmsrd r0,1 /* disable RI before using SRR0/1 */
>>
>> mtspr SPRN_SRR0,r5
>> mtspr SPRN_SRR1,r6
>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index 1f42aabbbab3..d7775b8c8853 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>>
>> static inline void do_enter_rtas(unsigned long args)
>> {
>> + unsigned long msr;
>> +
>> + msr = mfmsr();
>> + BUG_ON(!(msr & MSR_RI));
>> +
>> enter_rtas(args);
>>
>> srr_regs_clobbered(); /* rtas uses SRRs, invalidate */
>> --
>> 2.35.1
>>
>>

2022-05-03 16:23:32

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Laurent Dufour <[email protected]> writes:
> RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits
> mode (MSR[SF] unset).

Probably also worth mentioning that it runs in big endian mode :)

It is specified in PAPR (R1-7.2.1-6).

> The change in MSR is done in enter_rtas() in a relatively complex way,
> since the MSR value could be hardcoded.
>
> Furthermore, a panic has been reported when hitting the watchdog interrupt
> while running in RTAS, this leads to the following stack trace:
>
> [69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP
> [69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat TB:997504470175378 (15980ms ago)
> [69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
> [69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
> [69244.027587][ C24] Supported: No, Unreleased kernel
> [69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
> [69244.027609][ C24] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
> [69244.027612][ C24] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
> [69244.027615][ C24] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
> [69244.027625][ C24] CFAR: 000000000000011c IRQMASK: 1
> [69244.027625][ C24] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
> [69244.027625][ C24] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
> [69244.027625][ C24] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
> [69244.027625][ C24] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
> [69244.027625][ C24] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
> [69244.027625][ C24] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
> [69244.027625][ C24] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
> [69244.027625][ C24] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
> [69244.027663][ C24] NIP [000000001fb41050] 0x1fb41050
> [69244.027696][ C24] LR [000000001fb4104c] 0x1fb4104c
> [69244.027699][ C24] Call Trace:
> [69244.027701][ C24] Instruction dump:
> [69244.027723][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.027728][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1]
> [69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> [69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
> [69244.028171][T87504] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
> [69244.028307][T87504] Supported: No, Unreleased kernel
> [69244.028385][T87504] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
> [69244.028408][T87504] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
> [69244.028418][T87504] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
> [69244.028429][T87504] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
> [69244.028444][T87504] CFAR: 000000000000011c IRQMASK: 1
> [69244.028444][T87504] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
> [69244.028444][T87504] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
> [69244.028444][T87504] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
> [69244.028444][T87504] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
> [69244.028444][T87504] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
> [69244.028444][T87504] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
> [69244.028444][T87504] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
> [69244.028444][T87504] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
> [69244.028534][T87504] NIP [000000001fb41050] 0x1fb41050
> [69244.028543][T87504] LR [000000001fb4104c] 0x1fb4104c
> [69244.028549][T87504] Call Trace:
> [69244.028554][T87504] Instruction dump:
> [69244.028561][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.028575][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> [69244.028607][T87504] ---[ end trace 3ddec07f638c34a2 ]---
>
> This happens because MSR[RI] is unset when entering RTAS but there is no
> valid reason to not set it here.
>
> RTAS is expected to be called with MSR[RI] as specified in PAPR+ section
> "7.2.1 Machine State":
>
> R1–7.2.1–9. If called with MSR[RI] equal to 1, then RTAS must protect its
> own critical regions from recursion by setting the MSRRI bit to 0 when in
> the critical regions.
>
> Fixing this by reviewing the way MSR is compute before calling RTAS. Now a
> hardcoded value meaning real mode, 32 bits and Recoverable Interrupt is
> loaded.
>
> In addition a check is added in do_enter_rtas() to detect calls made with
> MSR[RI] unset, as we are forcing it on later.
>
> This patch has been tested on the following machines:
> Power KVM Guest
> P8 S822L (host Ubuntu kernel 5.11.0-49-generic)
> PowerVM LPAR
> P8 9119-MME (FW860.A1)
> p9 9008-22L (FW950.00)
> P10 9080-HEX (FW1010.00)
>
> Changes in V2:
> - Change comment in code to indicate NMI (Nick's comment)
> - Add reference to PAPR+ in the change log (Michael's comment)
>
> Cc: [email protected]
> Suggested-by: Nicholas Piggin <[email protected]>
> Signed-off-by: Laurent Dufour <[email protected]>
> ---
> arch/powerpc/kernel/entry_64.S | 20 ++++++++------------
> arch/powerpc/kernel/rtas.c | 5 +++++
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 9581906b5ee9..65cb14b56f8d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
> clrldi r4,r4,2 /* convert to realmode address */
> mtlr r4
>
> - li r0,0
> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
> - andc r0,r6,r0
> -
> - li r9,1
> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
> - andc r6,r0,r9

One advantage of the old method is it can adapt to new MSR bits being
set by the kernel.

For example we used to use RTAS on powernv, and this code didn't need
updating to cater to MSR_HV being set. We will probably never use RTAS
on bare-metal again, so that's OK.

But your change might break secure virtual machines, because it clears
MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
don't know whether it matters if it's called with MSR_S set or not?

Not sure if anyone will remember, or has a working setup they can test.
Maybe for now we just copy MSR_S from the kernel MSR the way the
current code does.

> __enter_rtas:
> - sync /* disable interrupts so SRR0/1 */
> - mtmsrd r0 /* don't get trashed */
> -
> LOAD_REG_ADDR(r4, rtas)
> ld r5,RTASENTRY(r4) /* get the rtas->entry value */
> ld r4,RTASBASE(r4) /* get the rtas->base value */
> +
> + /* RTAS runs in 32bits real mode but let MSR[]RI on as we may hit

"32-bit big endian real mode"

> + * NMI (SRESET or MCE). RTAS should disable RI in its critical
> + * regions (as specified in PAPR+ section 7.2.1). */
> + LOAD_REG_IMMEDIATE(r6, MSR_ME|MSR_RI)
> +
> + li r0,0
> + mtmsrd r0,1 /* disable RI before using SRR0/1 */
>
> mtspr SPRN_SRR0,r5
> mtspr SPRN_SRR1,r6
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 1f42aabbbab3..d7775b8c8853 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>
> static inline void do_enter_rtas(unsigned long args)
> {
> + unsigned long msr;
> +
> + msr = mfmsr();
> + BUG_ON(!(msr & MSR_RI));

I'm not sure about this.

We call RTAS in some low-level places, so if we ever hit this BUG_ON
then it might cause us to crash badly, or recursively BUG.

A WARN_ON_ONCE() might be safer?

cheers

2022-05-03 19:02:02

by Laurent Dufour

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

On 03/05/2022, 18:16:29, Fabiano Rosas wrote:
> Michael Ellerman <[email protected]> writes:
>
>>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>>> index 9581906b5ee9..65cb14b56f8d 100644
>>> --- a/arch/powerpc/kernel/entry_64.S
>>> +++ b/arch/powerpc/kernel/entry_64.S
>>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>>> clrldi r4,r4,2 /* convert to realmode address */
>>> mtlr r4
>>>
>>> - li r0,0
>>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>>> - andc r0,r6,r0
>>> -
>>> - li r9,1
>>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>>> - andc r6,r0,r9
>>
>> One advantage of the old method is it can adapt to new MSR bits being
>> set by the kernel.
>>
>> For example we used to use RTAS on powernv, and this code didn't need
>> updating to cater to MSR_HV being set. We will probably never use RTAS
>> on bare-metal again, so that's OK.
>>
>> But your change might break secure virtual machines, because it clears
>> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
>> don't know whether it matters if it's called with MSR_S set or not?
>>
>> Not sure if anyone will remember, or has a working setup they can test.
>> Maybe for now we just copy MSR_S from the kernel MSR the way the
>> current code does.
>
> Would the kernel even be able to change the bit? I think only urfid can
> clear MSR_S.

That's a good point, thanks Fabiano!

The POWER ISA programming note about MSR[S] is explicit:
"MSR[S] can be set to 1 only by the System Call instruction and some
interrupts. It can be set to 0 only by urfid."

Since RTAS is entered using rfid, MSR[S] should remain whatever is the
value in SRR1.

And that's what POWER ISA is saying about the rfid instruction, in the
synopsis of the instruction the bit 41 of the resulting MSR (aka MSR[S]) is
not impacted.

So there is no need to take care of this MSR bit in our code, but for sure,
this should be well commented.

Michael, do you agree?

Cheers,
Laurent.

2022-05-03 22:43:30

by Fabiano Rosas

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Michael Ellerman <[email protected]> writes:

>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 9581906b5ee9..65cb14b56f8d 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>> clrldi r4,r4,2 /* convert to realmode address */
>> mtlr r4
>>
>> - li r0,0
>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>> - andc r0,r6,r0
>> -
>> - li r9,1
>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>> - andc r6,r0,r9
>
> One advantage of the old method is it can adapt to new MSR bits being
> set by the kernel.
>
> For example we used to use RTAS on powernv, and this code didn't need
> updating to cater to MSR_HV being set. We will probably never use RTAS
> on bare-metal again, so that's OK.
>
> But your change might break secure virtual machines, because it clears
> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
> don't know whether it matters if it's called with MSR_S set or not?
>
> Not sure if anyone will remember, or has a working setup they can test.
> Maybe for now we just copy MSR_S from the kernel MSR the way the
> current code does.

Would the kernel even be able to change the bit? I think only urfid can
clear MSR_S.

2022-05-04 02:15:03

by Laurent Dufour

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Thanks Michael for reviewing this.

On 03/05/2022, 17:06:41, Michael Ellerman wrote:
> Laurent Dufour <[email protected]> writes:
>> RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits
>> mode (MSR[SF] unset).
>
> Probably also worth mentioning that it runs in big endian mode :)
>
> It is specified in PAPR (R1-7.2.1-6).

Sure!

>
>> The change in MSR is done in enter_rtas() in a relatively complex way,
>> since the MSR value could be hardcoded.
>>
>> Furthermore, a panic has been reported when hitting the watchdog interrupt
>> while running in RTAS, this leads to the following stack trace:
>>
>> [69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP
>> [69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat TB:997504470175378 (15980ms ago)
>> [69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
>> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
>> [69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
>> [69244.027587][ C24] Supported: No, Unreleased kernel
>> [69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
>> [69244.027609][ C24] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
>> [69244.027612][ C24] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
>> [69244.027615][ C24] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
>> [69244.027625][ C24] CFAR: 000000000000011c IRQMASK: 1
>> [69244.027625][ C24] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
>> [69244.027625][ C24] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
>> [69244.027625][ C24] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
>> [69244.027625][ C24] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
>> [69244.027625][ C24] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
>> [69244.027625][ C24] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
>> [69244.027625][ C24] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
>> [69244.027625][ C24] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
>> [69244.027663][ C24] NIP [000000001fb41050] 0x1fb41050
>> [69244.027696][ C24] LR [000000001fb4104c] 0x1fb4104c
>> [69244.027699][ C24] Call Trace:
>> [69244.027701][ C24] Instruction dump:
>> [69244.027723][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.027728][ C24] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1]
>> [69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> [69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) rpcsec_gss_krb5(E) auth_rpcgss(E)
>> nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) sd_mod(E) t10_pi(E)
>> [69244.028171][T87504] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E)
>> [69244.028307][T87504] Supported: No, Unreleased kernel
>> [69244.028385][T87504] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: G E X 5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c
>> [69244.028408][T87504] NIP: 000000001fb41050 LR: 000000001fb4104c CTR: 0000000000000000
>> [69244.028418][T87504] REGS: c00000000fc33d60 TRAP: 0100 Tainted: G E X (5.14.21-150400.71.1.bz196362_2-default)
>> [69244.028429][T87504] MSR: 8000000002981000 <SF,VEC,VSX,ME> CR: 48800002 XER: 20040020
>> [69244.028444][T87504] CFAR: 000000000000011c IRQMASK: 1
>> [69244.028444][T87504] GPR00: 0000000000000003 ffffffffffffffff 0000000000000001 00000000000050dc
>> [69244.028444][T87504] GPR04: 000000001ffb6100 0000000000000020 0000000000000001 000000001fb09010
>> [69244.028444][T87504] GPR08: 0000000020000000 0000000000000000 0000000000000000 0000000000000000
>> [69244.028444][T87504] GPR12: 80040000072a40a8 c00000000ff8b680 0000000000000007 0000000000000034
>> [69244.028444][T87504] GPR16: 000000001fbf6e94 000000001fbf6d84 000000001fbd1db0 000000001fb3f008
>> [69244.028444][T87504] GPR20: 000000001fb41018 ffffffffffffffff 000000000000017f fffffffffffff68f
>> [69244.028444][T87504] GPR24: 000000001fb18fe8 000000001fb3e000 000000001fb1adc0 000000001fb1cf40
>> [69244.028444][T87504] GPR28: 000000001fb26000 000000001fb460f0 000000001fb17f18 000000001fb17000
>> [69244.028534][T87504] NIP [000000001fb41050] 0x1fb41050
>> [69244.028543][T87504] LR [000000001fb4104c] 0x1fb4104c
>> [69244.028549][T87504] Call Trace:
>> [69244.028554][T87504] Instruction dump:
>> [69244.028561][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.028575][T87504] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
>> [69244.028607][T87504] ---[ end trace 3ddec07f638c34a2 ]---
>>
>> This happens because MSR[RI] is unset when entering RTAS but there is no
>> valid reason to not set it here.
>>
>> RTAS is expected to be called with MSR[RI] as specified in PAPR+ section
>> "7.2.1 Machine State":
>>
>> R1–7.2.1–9. If called with MSR[RI] equal to 1, then RTAS must protect its
>> own critical regions from recursion by setting the MSRRI bit to 0 when in
>> the critical regions.
>>
>> Fixing this by reviewing the way MSR is compute before calling RTAS. Now a
>> hardcoded value meaning real mode, 32 bits and Recoverable Interrupt is
>> loaded.
>>
>> In addition a check is added in do_enter_rtas() to detect calls made with
>> MSR[RI] unset, as we are forcing it on later.
>>
>> This patch has been tested on the following machines:
>> Power KVM Guest
>> P8 S822L (host Ubuntu kernel 5.11.0-49-generic)
>> PowerVM LPAR
>> P8 9119-MME (FW860.A1)
>> p9 9008-22L (FW950.00)
>> P10 9080-HEX (FW1010.00)
>>
>> Changes in V2:
>> - Change comment in code to indicate NMI (Nick's comment)
>> - Add reference to PAPR+ in the change log (Michael's comment)
>>
>> Cc: [email protected]
>> Suggested-by: Nicholas Piggin <[email protected]>
>> Signed-off-by: Laurent Dufour <[email protected]>
>> ---
>> arch/powerpc/kernel/entry_64.S | 20 ++++++++------------
>> arch/powerpc/kernel/rtas.c | 5 +++++
>> 2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 9581906b5ee9..65cb14b56f8d 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>> clrldi r4,r4,2 /* convert to realmode address */
>> mtlr r4
>>
>> - li r0,0
>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>> - andc r0,r6,r0
>> -
>> - li r9,1
>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>> - andc r6,r0,r9
>
> One advantage of the old method is it can adapt to new MSR bits being
> set by the kernel.
>
> For example we used to use RTAS on powernv, and this code didn't need
> updating to cater to MSR_HV being set. We will probably never use RTAS
> on bare-metal again, so that's OK.
>
> But your change might break secure virtual machines, because it clears
> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
> don't know whether it matters if it's called with MSR_S set or not?
>
> Not sure if anyone will remember, or has a working setup they can test.
> Maybe for now we just copy MSR_S from the kernel MSR the way the
> current code does.

I could update the code to deal with MSR[S], but I can't see how I would
test that :/

>
>> __enter_rtas:
>> - sync /* disable interrupts so SRR0/1 */
>> - mtmsrd r0 /* don't get trashed */
>> -
>> LOAD_REG_ADDR(r4, rtas)
>> ld r5,RTASENTRY(r4) /* get the rtas->entry value */
>> ld r4,RTASBASE(r4) /* get the rtas->base value */
>> +
>> + /* RTAS runs in 32bits real mode but let MSR[]RI on as we may hit
>
> "32-bit big endian real mode"

Yep!

>
>> + * NMI (SRESET or MCE). RTAS should disable RI in its critical
>> + * regions (as specified in PAPR+ section 7.2.1). */
>> + LOAD_REG_IMMEDIATE(r6, MSR_ME|MSR_RI)
>> +
>> + li r0,0
>> + mtmsrd r0,1 /* disable RI before using SRR0/1 */
>>
>> mtspr SPRN_SRR0,r5
>> mtspr SPRN_SRR1,r6
>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>> index 1f42aabbbab3..d7775b8c8853 100644
>> --- a/arch/powerpc/kernel/rtas.c
>> +++ b/arch/powerpc/kernel/rtas.c
>> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>>
>> static inline void do_enter_rtas(unsigned long args)
>> {
>> + unsigned long msr;
>> +
>> + msr = mfmsr();
>> + BUG_ON(!(msr & MSR_RI));
>
> I'm not sure about this.
>
> We call RTAS in some low-level places, so if we ever hit this BUG_ON
> then it might cause us to crash badly, or recursively BUG.
>
> A WARN_ON_ONCE() might be safer?

I'm afraid a BUG_ON is required here. Since MSR[RI] is set on RTAS exit so
if it was not set when calling RTAS, that's a real issue and should
generate unexpected behaviour.

Do you have places in mind where RTAS could be called with !MSR[RI]?

Cheers,
Laurent.

2022-05-04 12:51:36

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Laurent Dufour <[email protected]> writes:
> On 03/05/2022, 17:06:41, Michael Ellerman wrote:
>> Laurent Dufour <[email protected]> writes:
...
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index 1f42aabbbab3..d7775b8c8853 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>>>
>>> static inline void do_enter_rtas(unsigned long args)
>>> {
>>> + unsigned long msr;
>>> +
>>> + msr = mfmsr();
>>> + BUG_ON(!(msr & MSR_RI));
>>
>> I'm not sure about this.
>>
>> We call RTAS in some low-level places, so if we ever hit this BUG_ON
>> then it might cause us to crash badly, or recursively BUG.
>>
>> A WARN_ON_ONCE() might be safer?
>
> I'm afraid a BUG_ON is required here. Since MSR[RI] is set on RTAS exit so
> if it was not set when calling RTAS, that's a real issue and should
> generate unexpected behaviour.
>
> Do you have places in mind where RTAS could be called with !MSR[RI]?

The main one I can think of is if someone is using
CONFIG_UDBG_RTAS_CONSOLE, then udbg_rtascon_putc() is wired up as
udbg_putc() and that might be called from anywhere, including xmon.

There's also RTAS calls in low-level xics interrupt code, that might get
called during panic/crash.

I don't expect any of those places to be called with MSR[RI] unset, but
I'm worried that if we're already crashing and for some reason MSR[RI]
is unset, then that BUG_ON will just make things worse.

eg. imagine taking a BUG_ON() for every character we try to print as
part of an oops.

Admittedly CONFIG_UDBG_RTAS_CONSOLE is old and probably not used much
anymore, but I'm still a bit paranoid :)

cheers

2022-05-04 13:04:19

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Fabiano Rosas <[email protected]> writes:
> Michael Ellerman <[email protected]> writes:
>>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>>> index 9581906b5ee9..65cb14b56f8d 100644
>>> --- a/arch/powerpc/kernel/entry_64.S
>>> +++ b/arch/powerpc/kernel/entry_64.S
>>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>>> clrldi r4,r4,2 /* convert to realmode address */
>>> mtlr r4
>>>
>>> - li r0,0
>>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>>> - andc r0,r6,r0
>>> -
>>> - li r9,1
>>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>>> - andc r6,r0,r9
>>
>> One advantage of the old method is it can adapt to new MSR bits being
>> set by the kernel.
>>
>> For example we used to use RTAS on powernv, and this code didn't need
>> updating to cater to MSR_HV being set. We will probably never use RTAS
>> on bare-metal again, so that's OK.
>>
>> But your change might break secure virtual machines, because it clears
>> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
>> don't know whether it matters if it's called with MSR_S set or not?
>>
>> Not sure if anyone will remember, or has a working setup they can test.
>> Maybe for now we just copy MSR_S from the kernel MSR the way the
>> current code does.
>
> Would the kernel even be able to change the bit? I think only urfid can
> clear MSR_S.

Good point :)

cheers

2022-05-04 15:55:07

by Laurent Dufour

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

On 04/05/2022, 07:59:29, Michael Ellerman wrote:
> Laurent Dufour <[email protected]> writes:
>> On 03/05/2022, 17:06:41, Michael Ellerman wrote:
>>> Laurent Dufour <[email protected]> writes:
> ...
>>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>>> index 1f42aabbbab3..d7775b8c8853 100644
>>>> --- a/arch/powerpc/kernel/rtas.c
>>>> +++ b/arch/powerpc/kernel/rtas.c
>>>> @@ -49,6 +49,11 @@ void enter_rtas(unsigned long);
>>>>
>>>> static inline void do_enter_rtas(unsigned long args)
>>>> {
>>>> + unsigned long msr;
>>>> +
>>>> + msr = mfmsr();
>>>> + BUG_ON(!(msr & MSR_RI));
>>>
>>> I'm not sure about this.
>>>
>>> We call RTAS in some low-level places, so if we ever hit this BUG_ON
>>> then it might cause us to crash badly, or recursively BUG.
>>>
>>> A WARN_ON_ONCE() might be safer?
>>
>> I'm afraid a BUG_ON is required here. Since MSR[RI] is set on RTAS exit so
>> if it was not set when calling RTAS, that's a real issue and should
>> generate unexpected behaviour.
>>
>> Do you have places in mind where RTAS could be called with !MSR[RI]?
>
> The main one I can think of is if someone is using
> CONFIG_UDBG_RTAS_CONSOLE, then udbg_rtascon_putc() is wired up as
> udbg_putc() and that might be called from anywhere, including xmon.
>
> There's also RTAS calls in low-level xics interrupt code, that might get
> called during panic/crash.
>
> I don't expect any of those places to be called with MSR[RI] unset, but
> I'm worried that if we're already crashing and for some reason MSR[RI]
> is unset, then that BUG_ON will just make things worse.
>
> eg. imagine taking a BUG_ON() for every character we try to print as
> part of an oops.
>
> Admittedly CONFIG_UDBG_RTAS_CONSOLE is old and probably not used much
> anymore, but I'm still a bit paranoid :)

I think you're right to be paranoid :)

This part of code can be really sensitive.
I boot a kernel built with CONFIG_UDBG_RTAS_CONSOLE, xmon is working fine,
but I cannot pretend this is covering all the RTAS call cases.

My hope with BUG_ON() is to raise the issue, as soon as possible, so it can
be addressed during the test phase.

2022-05-04 20:41:22

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS

Laurent Dufour <[email protected]> writes:
> On 03/05/2022, 18:16:29, Fabiano Rosas wrote:
>> Michael Ellerman <[email protected]> writes:
>>
>>>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>>>> index 9581906b5ee9..65cb14b56f8d 100644
>>>> --- a/arch/powerpc/kernel/entry_64.S
>>>> +++ b/arch/powerpc/kernel/entry_64.S
>>>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas)
>>>> clrldi r4,r4,2 /* convert to realmode address */
>>>> mtlr r4
>>>>
>>>> - li r0,0
>>>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI
>>>> - andc r0,r6,r0
>>>> -
>>>> - li r9,1
>>>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG)
>>>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE
>>>> - andc r6,r0,r9
>>>
>>> One advantage of the old method is it can adapt to new MSR bits being
>>> set by the kernel.
>>>
>>> For example we used to use RTAS on powernv, and this code didn't need
>>> updating to cater to MSR_HV being set. We will probably never use RTAS
>>> on bare-metal again, so that's OK.
>>>
>>> But your change might break secure virtual machines, because it clears
>>> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I
>>> don't know whether it matters if it's called with MSR_S set or not?
>>>
>>> Not sure if anyone will remember, or has a working setup they can test.
>>> Maybe for now we just copy MSR_S from the kernel MSR the way the
>>> current code does.
>>
>> Would the kernel even be able to change the bit? I think only urfid can
>> clear MSR_S.
>
> That's a good point, thanks Fabiano!
>
> The POWER ISA programming note about MSR[S] is explicit:
> "MSR[S] can be set to 1 only by the System Call instruction and some
> interrupts. It can be set to 0 only by urfid."
>
> Since RTAS is entered using rfid, MSR[S] should remain whatever is the
> value in SRR1.
>
> And that's what POWER ISA is saying about the rfid instruction, in the
> synopsis of the instruction the bit 41 of the resulting MSR (aka MSR[S]) is
> not impacted.
>
> So there is no need to take care of this MSR bit in our code, but for sure,
> this should be well commented.
>
> Michael, do you agree?

Yep.

Can you send a v3 with updated change log and comment mentioning MSR_S
and MSR_LE, thanks.

cheers