[v6,4/5] x86/kvm: use __decrypted attribute in shared variables

Commit Message

Brijesh Singh Sept. 7, 2018, 5:57 p.m. UTC

Commit: 368a540e0232 (x86/kvmclock: Remove memblock dependency)
caused SEV guest regression. When SEV is active, we map the shared
variables (wall_clock and hv_clock_boot) with C=0 to ensure that both
the guest and the hypervisor are able to access the data. To map the
variables we use kernel_physical_mapping_init() to split the large pages,
but splitting large pages requires allocating a new PMD, which fails now
that kvmclock initialization is called early during boot.

Recently we added a special .data..decrypted section to hold the shared
variables. This section is mapped with C=0 early during boot. Use
__decrypted attribute to put the wall_clock and hv_clock_boot in
.data..decrypted section so that they are mapped with C=0.

Signed-off-by: Brijesh Singh <[e-mail]>
Reviewed-by: Tom Lendacky <[e-mail]>
Fixes: 368a540e0232 ("x86/kvmclock: Remove memblock dependency")
Cc: Tom Lendacky <[e-mail]>
Cc: [e-mail]
Cc: Thomas Gleixner <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: "H. Peter Anvin" <[e-mail]>
Cc: [e-mail]
Cc: Paolo Bonzini <[e-mail]>
Cc: Sean Christopherson <[e-mail]>
Cc: [e-mail]
Cc: "Radim Krčmář" <[e-mail]>
---
 arch/x86/kernel/kvmclock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Borislav Petkov Sept. 10, 2018, 12:04 p.m. UTC | #1

On Fri, Sep 07, 2018 at 12:57:29PM -0500, Brijesh Singh wrote:
> Commit: 368a540e0232 (x86/kvmclock: Remove memblock dependency)
> caused SEV guest regression.

When mentioning a commit in the commit message, put it on a separate
line, like this:

"Commit

  368a540e0232 (x86/kvmclock: Remove memblock dependency)

caused a SEV guest regression."

> When SEV is active, we map the shared

Use passive tone in your commit message: no "we", etc...

> variables (wall_clock and hv_clock_boot) with C=0 to ensure that both
> the guest and the hypervisor are able to access the data. To map the
> variables we use kernel_physical_mapping_init() to split the large pages,

"... to potentially split large pages used for that mapping... "

> but splitting large pages requires allocating a new PMD, which fails now
> that kvmclock initialization is called early during boot.

"... before the memblock allocator is initialized."

> Recently we added a special .data..decrypted section to hold the shared
> variables.

You don't really need that sentence.

> This section is mapped with C=0 early during boot. Use
> __decrypted attribute to put the wall_clock and hv_clock_boot in
> .data..decrypted section so that they are mapped with C=0.

"... so that they're mapped decrypted." Readers don't care about C=0
- they simply wanna know what C=0 represents, i.e., memory is not
encrypted.

With that:

Reviewed-by: Borislav Petkov <[e-mail]>

Paolo Bonzini Sept. 10, 2018, 12:29 p.m. UTC | #2

On 07/09/2018 19:57, Brijesh Singh wrote:
> Commit: 368a540e0232 (x86/kvmclock: Remove memblock dependency)
> caused SEV guest regression. When SEV is active, we map the shared
> variables (wall_clock and hv_clock_boot) with C=0 to ensure that both
> the guest and the hypervisor are able to access the data. To map the
> variables we use kernel_physical_mapping_init() to split the large pages,
> but splitting large pages requires allocating a new PMD, which fails now
> that kvmclock initialization is called early during boot.
> 
> Recently we added a special .data..decrypted section to hold the shared
> variables. This section is mapped with C=0 early during boot. Use
> __decrypted attribute to put the wall_clock and hv_clock_boot in
> .data..decrypted section so that they are mapped with C=0.
> 
> Signed-off-by: Brijesh Singh <[e-mail]>
> Reviewed-by: Tom Lendacky <[e-mail]>
> Fixes: 368a540e0232 ("x86/kvmclock: Remove memblock dependency")
> Cc: Tom Lendacky <[e-mail]>
> Cc: [e-mail]
> Cc: Thomas Gleixner <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: "H. Peter Anvin" <[e-mail]>
> Cc: [e-mail]
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Sean Christopherson <[e-mail]>
> Cc: [e-mail]
> Cc: "Radim Krčmář" <[e-mail]>
> ---
>  arch/x86/kernel/kvmclock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 1e67646..376fd3a 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -61,8 +61,8 @@ early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
>  	(PAGE_SIZE / sizeof(struct pvclock_vsyscall_time_info))
>  
>  static struct pvclock_vsyscall_time_info
> -			hv_clock_boot[HVC_BOOT_ARRAY_SIZE] __aligned(PAGE_SIZE);
> -static struct pvclock_wall_clock wall_clock;
> +			hv_clock_boot[HVC_BOOT_ARRAY_SIZE] __decrypted __aligned(PAGE_SIZE);
> +static struct pvclock_wall_clock wall_clock __decrypted;
>  static DEFINE_PER_CPU(struct pvclock_vsyscall_time_info *, hv_clock_per_cpu);
>  
>  static inline struct pvclock_vcpu_time_info *this_cpu_pvti(void)
> 

Acked-by: Paolo Bonzini <[e-mail]>

(Though perhaps __noencrypt or __unencrypted would be a bit more
accurate; likewise for the freeing function added in patch 5).

Paolo

Borislav Petkov Sept. 10, 2018, 12:33 p.m. UTC | #3

On Mon, Sep 10, 2018 at 02:29:07PM +0200, Paolo Bonzini wrote:
> (Though perhaps __noencrypt or __unencrypted would be a bit more
> accurate; likewise for the freeing function added in patch 5).

It has been raised already:

https://lkml.kernel.org/r/[e-mail]

Paolo Bonzini Sept. 10, 2018, 12:46 p.m. UTC | #4

On 10/09/2018 14:33, Borislav Petkov wrote:
> On Mon, Sep 10, 2018 at 02:29:07PM +0200, Paolo Bonzini wrote:
>> (Though perhaps __noencrypt or __unencrypted would be a bit more
>> accurate; likewise for the freeing function added in patch 5).
> 
> It has been raised already:
> 
> https://lkml.kernel.org/r/[e-mail]

Fair enough (I am just back from vacation and still in delete-happy
mode, so I missed that).

Paolo

Sean Christopherson Sept. 10, 2018, 1:15 p.m. UTC | #5

On Mon, 2018-09-10 at 14:04 +0200, Borislav Petkov wrote:
> On Fri, Sep 07, 2018 at 12:57:29PM -0500, Brijesh Singh wrote:
> > 
> > Commit: 368a540e0232 (x86/kvmclock: Remove memblock dependency)
> > caused SEV guest regression.
> When mentioning a commit in the commit message, put it on a separate
> line, like this:
> 
> "Commit
> 
>   368a540e0232 (x86/kvmclock: Remove memblock dependency)
> 
> caused a SEV guest regression."

Heh, that was the original formatting until I asked him to change it:
https://lkml.org/lkml/2018/8/29/809.  Checkpatch throws an error if
there's a newline after 'Commit'.  Though looking at this again, there
shouldn't be a colon after 'Commit' and there should be quotes inside
the parentheses, e.g. this satisfies checkpatch:

Commit 368a540e0232 ("x86/kvmclock: Remove memblock dependency")

Thomas Gleixner Sept. 10, 2018, 1:29 p.m. UTC | #6

On Mon, 10 Sep 2018, Sean Christopherson wrote:

> On Mon, 2018-09-10 at 14:04 +0200, Borislav Petkov wrote:
> > On Fri, Sep 07, 2018 at 12:57:29PM -0500, Brijesh Singh wrote:
> > > 
> > > Commit: 368a540e0232 (x86/kvmclock: Remove memblock dependency)
> > > caused SEV guest regression.
> > When mentioning a commit in the commit message, put it on a separate
> > line, like this:
> > 
> > "Commit
> > 
> >   368a540e0232 (x86/kvmclock: Remove memblock dependency)
> > 
> > caused a SEV guest regression."
> 
> Heh, that was the original formatting until I asked him to change it:
> https://lkml.org/lkml/2018/8/29/809.  Checkpatch throws an error if
> there's a newline after 'Commit'.  Though looking at this again, there
> shouldn't be a colon after 'Commit' and there should be quotes inside
> the parentheses, e.g. this satisfies checkpatch:
> 
> Commit 368a540e0232 ("x86/kvmclock: Remove memblock dependency")

The whole 5 lines of 'commit bla broke' are bogus to me. They convey not
really useful information and the commit line will repeat itself in the
Fixes: tag. So something like:

  The recent removal of the memblock dependency caused a SEV guest
  regression because bla....


  Fixes: 368a540e0232 ("x86/kvmclock: Remove memblock dependency")

Is much more useful than the above independent of its formatting.

Thanks,

	tglx

Borislav Petkov Sept. 10, 2018, 3:34 p.m. UTC | #7

On Mon, Sep 10, 2018 at 06:15:10AM -0700, Sean Christopherson wrote:
> Heh, that was the original formatting until I asked him to change it:
> https://lkml.org/lkml/2018/8/29/809.  Checkpatch throws an error if
> there's a newline after 'Commit'.  Though looking at this again, there
> shouldn't be a colon after 'Commit' and there should be quotes inside
> the parentheses, e.g. this satisfies checkpatch:

Yeah, we don't take checkpatch too seriously - only when it makes sense
to a human brain. Roughly speaking. In this case, the logic should be,
the commit which is the culprit should be visible immediately.

Although after the Fixes: tag thing got introduced, that argument is not
as important anymore, I'd say.

As to the formatting, I think adding this to your git config helps in
that regard:

[alias]
	...
        one = show -s --pretty='format:%h (\"%s\")'

so that you can do:

$ git one 368a540e0232
368a540e0232 ("x86/kvmclock: Remove memblock dependency")

Patch

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index 1e67646..376fd3a 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -61,8 +61,8 @@  early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
 	(PAGE_SIZE / sizeof(struct pvclock_vsyscall_time_info))
 
 static struct pvclock_vsyscall_time_info
-			hv_clock_boot[HVC_BOOT_ARRAY_SIZE] __aligned(PAGE_SIZE);
-static struct pvclock_wall_clock wall_clock;
+			hv_clock_boot[HVC_BOOT_ARRAY_SIZE] __decrypted __aligned(PAGE_SIZE);
+static struct pvclock_wall_clock wall_clock __decrypted;
 static DEFINE_PER_CPU(struct pvclock_vsyscall_time_info *, hv_clock_per_cpu);
 
 static inline struct pvclock_vcpu_time_info *this_cpu_pvti(void)