Message ID | 1536343050-18532-5-git-send-email-brijesh.singh@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: Fix SEV guest regression | expand |
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 <bp@suse.de>
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 <brijesh.singh@amd.com> > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > Fixes: 368a540e0232 ("x86/kvmclock: Remove memblock dependency") > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: kvm@vger.kernel.org > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Borislav Petkov <bp@suse.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: linux-kernel@vger.kernel.org > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Sean Christopherson <sean.j.christopherson@intel.com> > Cc: kvm@vger.kernel.org > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > --- > 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 <pbonzini@redhat.com> (Though perhaps __noencrypt or __unencrypted would be a bit more accurate; likewise for the freeing function added in patch 5). Paolo
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/20180830092606.GC18459@nazgul.tnic
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/20180830092606.GC18459@nazgul.tnic Fair enough (I am just back from vacation and still in delete-happy mode, so I missed that). Paolo
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")
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
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")
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)