From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Lendacky Subject: Re: [RFC PATCH v2 12/20] x86: Add support for changing memory encryption attribute Date: Mon, 12 Sep 2016 10:41:29 -0500 Message-ID: <4e423d15-7fe2-450a-05dd-1674bd281124@amd.com> References: <20160822223529.29880.50884.stgit@tlendack-t1.amdoffice.net> <20160822223749.29880.10183.stgit@tlendack-t1.amdoffice.net> <20160909172314.ifcteua7nr52mzgs@pd.tnic> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20160909172314.ifcteua7nr52mzgs-fF5Pk5pvG8Y@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Borislav Petkov Cc: linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kasan-dev-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Arnd Bergmann , Jonathan Corbet , Matt Fleming , Joerg Roedel , Konrad Rzeszutek Wilk , Andrey Ryabinin , Ingo Molnar , Andy Lutomirski , "H. Peter Anvin" , Paolo Bonzini , Alexander Potapenko , Thomas Gleixner , Dm List-Id: linux-efi@vger.kernel.org On 09/09/2016 12:23 PM, Borislav Petkov wrote: > On Mon, Aug 22, 2016 at 05:37:49PM -0500, Tom Lendacky wrote: >> This patch adds support to be change the memory encryption attribute for >> one or more memory pages. >> >> Signed-off-by: Tom Lendacky >> --- >> arch/x86/include/asm/cacheflush.h | 3 + >> arch/x86/include/asm/mem_encrypt.h | 13 ++++++ >> arch/x86/mm/mem_encrypt.c | 43 +++++++++++++++++++++ >> arch/x86/mm/pageattr.c | 75 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 134 insertions(+) > > ... > >> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c >> index 72c292d..0ba9382 100644 >> --- a/arch/x86/mm/pageattr.c >> +++ b/arch/x86/mm/pageattr.c >> @@ -1728,6 +1728,81 @@ int set_memory_4k(unsigned long addr, int numpages) >> __pgprot(0), 1, 0, NULL); >> } >> >> +static int __set_memory_enc_dec(struct cpa_data *cpa) >> +{ >> + unsigned long addr; >> + int numpages; >> + int ret; >> + >> + if (*cpa->vaddr & ~PAGE_MASK) { >> + *cpa->vaddr &= PAGE_MASK; >> + >> + /* People should not be passing in unaligned addresses */ >> + WARN_ON_ONCE(1); > > Let's make this more user-friendly: > > if (WARN_ONCE(*cpa->vaddr & ~PAGE_MASK, "Misaligned address: 0x%lx\n", *cpa->vaddr)) > *cpa->vaddr &= PAGE_MASK; Will do. > >> + } >> + >> + addr = *cpa->vaddr; >> + numpages = cpa->numpages; >> + >> + /* Must avoid aliasing mappings in the highmem code */ >> + kmap_flush_unused(); >> + vm_unmap_aliases(); >> + >> + ret = __change_page_attr_set_clr(cpa, 1); >> + >> + /* Check whether we really changed something */ >> + if (!(cpa->flags & CPA_FLUSHTLB)) >> + goto out; >> + >> + /* >> + * On success we use CLFLUSH, when the CPU supports it to >> + * avoid the WBINVD. >> + */ >> + if (!ret && static_cpu_has(X86_FEATURE_CLFLUSH)) >> + cpa_flush_range(addr, numpages, 1); >> + else >> + cpa_flush_all(1); > > So if we fail (ret != 0) we do WBINVD unconditionally even if we don't > have to? Looking at __change_page_attr_set_clr() isn't it possible for some of the pages to be changed before an error is encountered since it is looping? If so, we may still need to flush. The CPA_FLUSHTLB flag should take care of a failing case where no attributes have actually been changed. Thanks, Tom > > Don't you want this instead: > > ret = __change_page_attr_set_clr(cpa, 1); > if (ret) > goto out; > > /* Check whether we really changed something */ > if (!(cpa->flags & CPA_FLUSHTLB)) > goto out; > > /* > * On success we use CLFLUSH, when the CPU supports it to > * avoid the WBINVD. > */ > if (static_cpu_has(X86_FEATURE_CLFLUSH)) > cpa_flush_range(addr, numpages, 1); > else > cpa_flush_all(1); > > out: > return ret; > } > > ? >