Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
On 8/5/2021 10:29 PM, Dave Hansen wrote: On 8/5/21 7:23 AM, Peter Zijlstra wrote: This is assuming any of this is actually performance critical, based off of this using static_call() to begin with. This code is not performance critical. I think I sent folks off on a wild goose chase when I asked that we make an effort to optimize code that does: if (some_hyperv_check()) foo(); if (some_amd_feature_check()) bar(); with checks that will actually compile away when Hyper-V or some_amd_feature() is disabled. That's less about performance and just about good hygiene. I *wanted* to see cpu_feature_enabled(X86_FEATURE...) checks. Someone suggested using static calls, and off we went... Could we please just use cpu_feature_enabled()? Yes, cpu_feature_enabled() works. The target is just to run platform code after platform check. I will update this in the next version. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
On 8/5/21 7:23 AM, Peter Zijlstra wrote: > This is assuming any of this is actually performance critical, based off > of this using static_call() to begin with. This code is not performance critical. I think I sent folks off on a wild goose chase when I asked that we make an effort to optimize code that does: if (some_hyperv_check()) foo(); if (some_amd_feature_check()) bar(); with checks that will actually compile away when Hyper-V or some_amd_feature() is disabled. That's less about performance and just about good hygiene. I *wanted* to see cpu_feature_enabled(X86_FEATURE...) checks. Someone suggested using static calls, and off we went... Could we please just use cpu_feature_enabled()? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
On Thu, Aug 05, 2021 at 10:05:17PM +0800, Tianyu Lan wrote:
> static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> {
> + return static_call(x86_set_memory_enc)(addr, numpages, enc);
> }
Hurpmh... So with a bit of 'luck' you get code-gen like:
__set_memory_enc_dec:
jmp __SCT_x86_set_memory_enc;
set_memory_encrypted:
mov $1, %rdx
jmp __set_memory_enc_dec
set_memory_decrypted:
mov $0, %rdx
jmp __set_memory_enc_dec
Which, to me, seems exceedingly daft. Best to make all 3 of those
inlines and use EXPORT_STATIC_CALL_TRAMP_GPL(x86_set_memory_enc) or
something.
This is assuming any of this is actually performance critical, based off
of this using static_call() to begin with.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
On Thu, Aug 05, 2021 at 10:05:17PM +0800, Tianyu Lan wrote:
> +static int default_set_memory_enc(unsigned long addr, int numpages, bool
> enc)
> +{
> + return 0;
> +}
> +
> +DEFINE_STATIC_CALL(x86_set_memory_enc, default_set_memory_enc);
That's spelled:
DEFINE_STATIC_CALL_RET0(x86_set_memory_enc, __set_memory_enc_dec);
And then you can remove the default_set_memory_enc() thing.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
Hi Dave:
Thanks for review.
On 8/5/2021 3:27 AM, Dave Hansen wrote:
On 8/4/21 11:44 AM, Tianyu Lan wrote:
+static int default_set_memory_enc(unsigned long addr, int numpages, bool enc);
+DEFINE_STATIC_CALL(x86_set_memory_enc, default_set_memory_enc);
+
#define CPA_FLUSHTLB 1
#define CPA_ARRAY 2
#define CPA_PAGES_ARRAY 4
@@ -1981,6 +1985,11 @@ int set_memory_global(unsigned long addr, int numpages)
}
static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
+{
+ return static_call(x86_set_memory_enc)(addr, numpages, enc);
+}
+
+static int default_set_memory_enc(unsigned long addr, int numpages, bool enc)
{
struct cpa_data cpa;
int ret;
It doesn't make a lot of difference to add this infrastructure and then
ignore it for the existing in-tree user:
static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
{
struct cpa_data cpa;
int ret;
/* Nothing to do if memory encryption is not active */
if (!mem_encrypt_active())
return 0;
Shouldn't the default be to just "return 0"? Then on
mem_encrypt_active() systems, do the bulk of what is in
__set_memory_enc_dec() today.
OK. I try moving code in __set_memory_enc_dec() to sev file
mem_encrypt.c and this requires to expose cpa functions and structure.
Please have a look.
Tom, Joerg and Brijesh, Could you review at sev code change?
Thanks.
diff --git a/arch/x86/include/asm/set_memory.h
b/arch/x86/include/asm/set_memory.h
index 43fa081a1adb..991366612deb 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -4,6 +4,25 @@
#include
#include
+#include
+
+/*
+ * The current flushing context - we pass it instead of 5 arguments:
+ */
+struct cpa_data {
+ unsigned long *vaddr;
+ pgd_t *pgd;
+ pgprot_tmask_set;
+ pgprot_tmask_clr;
+ unsigned long numpages;
+ unsigned long curpage;
+ unsigned long pfn;
+ unsigned intflags;
+ unsigned intforce_split : 1,
+ force_static_prot : 1,
+ force_flush_all : 1;
+ struct page **pages;
+};
/*
* The set_memory_* API can be used to change various attributes of a
virtual
@@ -83,6 +102,11 @@ int set_pages_rw(struct page *page, int numpages);
int set_direct_map_invalid_noflush(struct page *page);
int set_direct_map_default_noflush(struct page *page);
bool kernel_page_present(struct page *page);
+int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias);
+void cpa_flush(struct cpa_data *data, int cache);
+
+int dummy_set_memory_enc(unsigned long addr, int numpages, bool enc);
+DECLARE_STATIC_CALL(x86_set_memory_enc, dummy_set_memory_enc);
extern int kernel_set_to_readonly;
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index ff08dc463634..49e957c4191f 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -20,6 +20,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -178,6 +180,45 @@ void __init sme_map_bootdata(char *real_mode_data)
__sme_early_map_unmap_mem(__va(cmdline_paddr), COMMAND_LINE_SIZE, true);
}
+static int sev_set_memory_enc(unsigned long addr, int numpages, bool enc)
+{
+ struct cpa_data cpa;
+ int ret;
+
+ /* Should not be working on unaligned addresses */
+ if (WARN_ONCE(addr & ~PAGE_MASK, "misaligned address: %#lx\n", addr))
+ addr &= PAGE_MASK;
+
+ memset(, 0, sizeof(cpa));
+ cpa.vaddr =
+ cpa.numpages = numpages;
+ cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
+ cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
+ cpa.pgd = init_mm.pgd;
+
+ /* Must avoid aliasing mappings in the highmem code */
+ kmap_flush_unused();
+ vm_unmap_aliases();
+
+ /*
+* Before changing the encryption attribute, we need to flush caches.
+*/
+ cpa_flush(, !this_cpu_has(X86_FEATURE_SME_COHERENT));
+
+ ret = __change_page_attr_set_clr(, 1);
+
+ /*
+* After changing the encryption attribute, we need to flush TLBs again
+* in case any speculative TLB caching occurred (but no need to flush
+* caches again). We could just use cpa_flush_all(), but in case TLB
+* flushing gets optimized in the cpa_flush() path use the same logic
+* as above.
+*/
+ cpa_flush(, 0);
+
+ return ret;
+}
+
void __init sme_early_init(void)
{
unsigned int i;
@@ -185,6 +226,8 @@ void __init sme_early_init(void)
if (!sme_me_mask)
return;
+ static_call_update(x86_set_memory_enc, sev_set_memory_enc);
+
early_pmd_flags = __sme_set(early_pmd_flags);
__supported_pte_mask = __sme_set(__supported_pte_mask);
diff --git a/arch/x86/mm/pat/set_memory.c
Re: [PATCH V2 03/14] x86/set_memory: Add x86_set_memory_enc static call support
On 8/4/21 11:44 AM, Tianyu Lan wrote:
> +static int default_set_memory_enc(unsigned long addr, int numpages, bool
> enc);
> +DEFINE_STATIC_CALL(x86_set_memory_enc, default_set_memory_enc);
> +
> #define CPA_FLUSHTLB 1
> #define CPA_ARRAY 2
> #define CPA_PAGES_ARRAY 4
> @@ -1981,6 +1985,11 @@ int set_memory_global(unsigned long addr, int numpages)
> }
>
> static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> +{
> + return static_call(x86_set_memory_enc)(addr, numpages, enc);
> +}
> +
> +static int default_set_memory_enc(unsigned long addr, int numpages, bool enc)
> {
> struct cpa_data cpa;
> int ret;
It doesn't make a lot of difference to add this infrastructure and then
ignore it for the existing in-tree user:
> static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> {
> struct cpa_data cpa;
> int ret;
>
> /* Nothing to do if memory encryption is not active */
> if (!mem_encrypt_active())
> return 0;
Shouldn't the default be to just "return 0"? Then on
mem_encrypt_active() systems, do the bulk of what is in
__set_memory_enc_dec() today.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
