FAILED: patch "[PATCH] x86/sev: Unroll string mmio with" failed to apply to 5.16-stable tree

The patch below does not apply to the 5.16-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 4009a4ac82dd95b8cd2b62bd30019476983f0aff Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Mon, 21 Mar 2022 10:33:51 +0100
Subject: [PATCH] x86/sev: Unroll string mmio with
CC_ATTR_GUEST_UNROLL_STRING_IO

The io-specific memcpy/memset functions use string mmio accesses to do their work. Under SEV, the hypervisor can't emulate these instructions because they read/write directly from/to encrypted memory.

KVM will inject a page fault exception into the guest when it is asked to emulate string mmio instructions for an SEV guest:

  BUG: unable to handle page fault for address: ffffc90000065068
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 8000100000067 P4D 8000100000067 PUD 80001000fb067 PMD 80001000fc067 PTE 80000000fed40173
  Oops: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.17.0-rc7 #3

As string mmio for an SEV guest can not be supported by the hypervisor, unroll the instructions for CC_ATTR_GUEST_UNROLL_STRING_IO enabled kernels.

This issue appears when kernels are launched in recent libvirt-managed SEV virtual machines, because virt-install started to add a tpm-crb device to the guest by default and proactively because, raisins:

https://github.com/virt-manager/virt-manager/commit/eb58c09f488b0633ed1eea012cd311e48864401e

and as that commit says, the default adding of a TPM can be disabled with "virt-install ... --tpm none".

The kernel driver for tpm-crb uses memcpy_to/from_io() functions to access MMIO memory, resulting in a page-fault injected by KVM and crashing the kernel at boot.

[ bp: Massage and extend commit message. ]

index df50451d94ef..3e2f33fc33de 100644
@@ -22,7 +22,7 @@ static __always_inline void rep_movs(void *to, const void *from, size_t n)
 		     : "memory");
 }
 
-void memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
+static void string_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	if (unlikely(!n))
 		return;

@@ -38,9 +38,8 @@ void memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 	}
 	rep_movs(to, (const void *)from, n);
 }
-EXPORT_SYMBOL(memcpy_fromio);
 
-void memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
+static void string_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	if (unlikely(!n))
 		return;

@@ -56,14 +55,64 @@ void memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 	}
 	rep_movs((void *)to, (const void *) from, n);
 }
+
+static void unrolled_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
+{
+	const volatile char __iomem *in = from;
+	char *out = to;
+	int i;
+
+	for (i = 0; i < n; ++i)
+		out[i] = readb(&in[i]);
+}
+
+static void unrolled_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
+{
+	volatile char __iomem *out = to;
+	const char *in = from;
+	int i;
+
+	for (i = 0; i < n; ++i)
+		writeb(in[i], &out[i]);
+}
+
+static void unrolled_memset_io(volatile void __iomem *a, int b, size_t c)
+{
+	volatile char __iomem *mem = a;
+	int i;
+
+	for (i = 0; i < c; ++i)
+		writeb(b, &mem[i]);
+}
+
+void memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
+{
+	if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO))
+		unrolled_memcpy_fromio(to, from, n);
+	else
+		string_memcpy_fromio(to, from, n);
+}
+EXPORT_SYMBOL(memcpy_fromio);
+
+void memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
+{
+	if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO))
+		unrolled_memcpy_toio(to, from, n);
+	else
+		string_memcpy_toio(to, from, n);
+}
 EXPORT_SYMBOL(memcpy_toio);
 
 void memset_io(volatile void __iomem *a, int b, size_t c)
 {
-	/*
-	 * TODO: memset can mangle the IO patterns quite a bit.
-	 * perhaps it would be better to use a dumb one:
-	 */
-	memset((void *)a, b, c);
+	if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO)) {
+		unrolled_memset_io(a, b, c);
+	} else {
+		/*
+		 * TODO: memset can mangle the IO patterns quite a bit.
+		 * perhaps it would be better to use a dumb one:
+		 */
+		memset((void *)a, b, c);
+	}
 }
 EXPORT_SYMBOL(memset_io);

Really?

$ git checkout -b 5.16.y stable/linux-5.16.y
Updating files: 100% (19623/19623), done.
branch '5.16.y' set up to track 'stable/linux-5.16.y'.
$ git cherry-pick 4009a4ac82dd95b8cd2b62bd30019476983f0aff
[5.16.y 045eac1dbd58] x86/sev: Unroll string mmio with CC_ATTR_GUEST_UNROLL_STRING_IO
 Author: Joerg Roedel <[email protected]>
 Date: Mon Mar 21 10:33:51 2022 +0100
 1 file changed, 57 insertions(+), 8 deletions(-)
$ git status
On branch 5.16.y
Your branch is ahead of 'stable/linux-5.16.y' by 1 commit.
  (use "git push" to publish your local commits)

It works here...

Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg

And then when you build the tree:

arch/x86/lib/iomem.c: In function ‘memcpy_fromio’:
arch/x86/lib/iomem.c:90:29: error: ‘CC_ATTR_GUEST_UNROLL_STRING_IO’ undeclared (first use in this function)
90 | if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO))

      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

arch/x86/lib/iomem.c:90:29: note: each undeclared identifier is reported only once for each function it appears in arch/x86/lib/iomem.c: In function ‘memcpy_toio’:
arch/x86/lib/iomem.c:99:29: error: ‘CC_ATTR_GUEST_UNROLL_STRING_IO’ undeclared (first use in this function)
99 | if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO))

      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/lib/iomem.c: In function ‘memset_io’:
arch/x86/lib/iomem.c:108:29: error: ‘CC_ATTR_GUEST_UNROLL_STRING_IO’ undeclared (first use in this function)
  108 |         if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO)) {
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

make[1]: *** [scripts/Makefile.build:287: arch/x86/lib/iomem.o] Error 1

I only have one "FAILED" email template, do I need another one for when the patch applies yet breaks the build?

thanks,

greg k-h

Bah, building is overrated.

I guess you need

8260b9820f70 ("x86/sev: Use CC_ATTR attribute to generalize string I/O unroll")

before that.

I guess you could change that first sentence to

"The patch below does not apply (or build) to the X.XX-stable tree."

It seems I took it literally to mean it only doesn't apply.

:-)

Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg

Fair enough :)

So, 5.16 is now dead and end-of-life, but the original commit here still looks to be needed in older kernels going back quite a ways. And 8260b9820f70 ("x86/sev: Use CC_ATTR attribute to generalize string I/O unroll") does not apply cleanly to 5.15 or older and given that it's touching memory encryption code, I'm not comfortable guessing about the backport.

So can you, or anyone else that cares, provide a set of backported patches for this issue that we can apply to the stable trees?

thanks,

greg k-h

Yes, I will provide backports for older stable kernels. It is on my todo list for next week :)

Regards,

Jörg Rödel
[email protected]

SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
 
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev