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(&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/se
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