| Message ID | 20210804184513.512888-4-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/Hyper-V: Add Hyper-V Isolation VM support | expand |
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.
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 <asm/page.h> #include <asm-generic/set_memory.h> +#include <linux/static_call.h> + +/* + * The current flushing context - we pass it instead of 5 arguments: + */ +struct cpa_data { + unsigned long *vaddr; + pgd_t *pgd; + pgprot_t mask_set; + pgprot_t mask_clr; + unsigned long numpages; + unsigned long curpage; + unsigned long pfn; + unsigned int flags; + unsigned int force_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 <linux/bitops.h> #include <linux/dma-mapping.h> #include <linux/virtio_config.h> +#include <linux/highmem.h> +#include <linux/static_call.h> #include <asm/tlbflush.h> #include <asm/fixmap.h> @@ -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(&cpa, 0, sizeof(cpa)); + cpa.vaddr = &addr; + 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(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT)); + + ret = __change_page_attr_set_clr(&cpa, 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(&cpa, 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 b/arch/x86/mm/pat/set_memory.c index ad8a5c586a35..4f15f7c89dbc 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -18,6 +18,7 @@ #include <linux/libnvdimm.h> #include <linux/vmstat.h> #include <linux/kernel.h> +#include <linux/static_call.h> #include <asm/e820/api.h> #include <asm/processor.h> @@ -32,24 +33,6 @@ #include "../mm_internal.h" -/* - * The current flushing context - we pass it instead of 5 arguments: - */ -struct cpa_data { - unsigned long *vaddr; - pgd_t *pgd; - pgprot_t mask_set; - pgprot_t mask_clr; - unsigned long numpages; - unsigned long curpage; - unsigned long pfn; - unsigned int flags; - unsigned int force_split : 1, - force_static_prot : 1, - force_flush_all : 1; - struct page **pages; -}; - enum cpa_warn { CPA_CONFLICT, CPA_PROTECT, @@ -66,6 +49,13 @@ static const int cpa_warn_level = CPA_PROTECT; */ static DEFINE_SPINLOCK(cpa_lock); +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); + #define CPA_FLUSHTLB 1 #define CPA_ARRAY 2 #define CPA_PAGES_ARRAY 4 @@ -357,7 +347,7 @@ static void __cpa_flush_tlb(void *data) flush_tlb_one_kernel(fix_addr(__cpa_addr(cpa, i))); } -static void cpa_flush(struct cpa_data *data, int cache) +void cpa_flush(struct cpa_data *data, int cache) { struct cpa_data *cpa = data; unsigned int i; @@ -1587,8 +1577,6 @@ static int __change_page_attr(struct cpa_data *cpa, int primary) return err; } -static int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias); - static int cpa_process_alias(struct cpa_data *cpa) { struct cpa_data alias_cpa; @@ -1646,7 +1634,7 @@ static int cpa_process_alias(struct cpa_data *cpa) return 0; } -static int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias) +int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias) { unsigned long numpages = cpa->numpages; unsigned long rempages = numpages; @@ -1982,45 +1970,7 @@ int set_memory_global(unsigned long addr, int numpages) 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; - - /* Should not be working on unaligned addresses */ - if (WARN_ONCE(addr & ~PAGE_MASK, "misaligned address: %#lx\n", addr)) - addr &= PAGE_MASK; - - memset(&cpa, 0, sizeof(cpa)); - cpa.vaddr = &addr; - 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(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT)); - - ret = __change_page_attr_set_clr(&cpa, 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(&cpa, 0); - - return ret; + return static_call(x86_set_memory_enc)(addr, numpages, enc); } int set_memory_encrypted(unsigned long addr, int numpages)
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.
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.
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()?
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.
diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h index 43fa081a1adb..490f2cfc00fa 100644 --- a/arch/x86/include/asm/set_memory.h +++ b/arch/x86/include/asm/set_memory.h @@ -4,6 +4,7 @@ #include <asm/page.h> #include <asm-generic/set_memory.h> +#include <linux/static_call.h> /* * The set_memory_* API can be used to change various attributes of a virtual @@ -84,6 +85,9 @@ 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 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; #ifdef CONFIG_X86_64 diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index ad8a5c586a35..68e9ab522cea 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -18,6 +18,7 @@ #include <linux/libnvdimm.h> #include <linux/vmstat.h> #include <linux/kernel.h> +#include <linux/static_call.h> #include <asm/e820/api.h> #include <asm/processor.h> @@ -66,6 +67,9 @@ static const int cpa_warn_level = CPA_PROTECT; */ static DEFINE_SPINLOCK(cpa_lock); +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;