On Wed, Jun 02, 2021 at 09:04:05AM -0500, Brijesh Singh wrote:
> @@ -65,6 +65,12 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
> /* RMP page size */
> #define RMP_PG_SIZE_4K 0
>
> +/* Memory opertion for snp_prep_memory() */
> +enum snp_mem_op {
> + MEMORY_PRIVATE,
> + MEMORY_SHARED
See below.
> +};
> +
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> extern struct static_key_false sev_es_enable_key;
> extern void __sev_es_ist_enter(struct pt_regs *regs);
> @@ -103,6 +109,11 @@ static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
>
> return rc;
> }
> +void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
> + unsigned int npages);
> +void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
> + unsigned int npages);
Align arguments on the opening brace.
> +void __init snp_prep_memory(unsigned long paddr, unsigned int sz, int op);
> #else
> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> static inline void sev_es_ist_exit(void) { }
> @@ -110,6 +121,15 @@ static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { ret
> static inline void sev_es_nmi_complete(void) { }
> static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> +static inline void __init
> +early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned int npages)
Put those { } at the end of the line:
early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned int npages) { }
no need for separate lines. Ditto below.
> +{
> +}
> +static inline void __init
> +early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, unsigned int npages)
> +{
> +}
> +static inline void __init snp_prep_memory(unsigned long paddr, unsigned int sz, int op) { }
> #endif
>
> #endif
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 455c09a9b2c2..6e9b45bb38ab 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -532,6 +532,111 @@ static u64 get_jump_table_addr(void)
> return ret;
> }
>
> +static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool validate)
> +{
> + unsigned long vaddr_end;
> + int rc;
> +
> + vaddr = vaddr & PAGE_MASK;
> + vaddr_end = vaddr + (npages << PAGE_SHIFT);
> +
> + while (vaddr < vaddr_end) {
> + rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
> + if (WARN(rc, "Failed to validate address 0x%lx ret %d", vaddr, rc))
> + sev_es_terminate(1, GHCB_TERM_PVALIDATE);
^^
I guess that 1 should be a define too, if we have to be correct:
sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PVALIDATE);
or so. Ditto for all other calls of this.
> +
> + vaddr = vaddr + PAGE_SIZE;
> + }
> +}
> +
> +static void __init early_set_page_state(unsigned long paddr, unsigned int npages, int op)
> +{
> + unsigned long paddr_end;
> + u64 val;
> +
> + paddr = paddr & PAGE_MASK;
> + paddr_end = paddr + (npages << PAGE_SHIFT);
> +
> + while (paddr < paddr_end) {
> + /*
> + * Use the MSR protocol because this function can be called before the GHCB
> + * is established.
> + */
> + sev_es_wr_ghcb_msr(GHCB_MSR_PSC_REQ_GFN(paddr >> PAGE_SHIFT, op));
> + VMGEXIT();
> +
> + val = sev_es_rd_ghcb_msr();
> +
> + if (GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP)
>From a previous review:
Does that one need a warning too or am I being too paranoid?
> + goto e_term;
> +
> + if (WARN(GHCB_MSR_PSC_RESP_VAL(val),
> + "Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n",
> + op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared",
> + paddr, GHCB_MSR_PSC_RESP_VAL(val)))
> + goto e_term;
> +
> + paddr = paddr + PAGE_SIZE;
> + }
> +
> + return;
> +
> +e_term:
> + sev_es_terminate(1, GHCB_TERM_PSC);
> +}
> +
> +void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
> + unsigned int npages)
> +{
> + if (!sev_feature_enabled(SEV_SNP))
> + return;
> +
> + /* Ask hypervisor to add the memory pages in RMP table as a 'private'. */
Ask the hypervisor to mark the memory pages as private in the RMP table.
> + early_set_page_state(paddr, npages, SNP_PAGE_STATE_PRIVATE);
> +
> + /* Validate the memory pages after they've been added in the RMP table. */
> + pvalidate_pages(vaddr, npages, 1);
> +}
> +
> +void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
> + unsigned int npages)
> +{
> + if (!sev_feature_enabled(SEV_SNP))
> + return;
> +
> + /*
> + * Invalidate the memory pages before they are marked shared in the
> + * RMP table.
> + */
> + pvalidate_pages(vaddr, npages, 0);
> +
> + /* Ask hypervisor to make the memory pages shared in the RMP table. */
mark
> + early_set_page_state(paddr, npages, SNP_PAGE_STATE_SHARED);
> +}
> +
> +void __init snp_prep_memory(unsigned long paddr, unsigned int sz, int op)
> +{
> + unsigned long vaddr, npages;
> +
> + vaddr = (unsigned long)__va(paddr);
> + npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +
> + switch (op) {
> + case MEMORY_PRIVATE: {
> + early_snp_set_memory_private(vaddr, paddr, npages);
> + return;
> + }
> + case MEMORY_SHARED: {
> + early_snp_set_memory_shared(vaddr, paddr, npages);
> + return;
> + }
> + default:
> + break;
> + }
> +
> + WARN(1, "invalid memory op %d\n", op);
A lot easier, diff ontop of your patch:
---
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 7c2cb5300e43..2ad4b5ab3f6c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -65,12 +65,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
/* RMP page size */
#define RMP_PG_SIZE_4K 0
-/* Memory opertion for snp_prep_memory() */
-enum snp_mem_op {
- MEMORY_PRIVATE,
- MEMORY_SHARED
-};
-
#ifdef CONFIG_AMD_MEM_ENCRYPT
extern struct static_key_false sev_es_enable_key;
extern void __sev_es_ist_enter(struct pt_regs *regs);
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 2a5dce42af35..991d7964cee9 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -662,20 +662,13 @@ void __init snp_prep_memory(unsigned long paddr, unsigned int sz, int op)
vaddr = (unsigned long)__va(paddr);
npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
- switch (op) {
- case MEMORY_PRIVATE: {
+ if (op == SNP_PAGE_STATE_PRIVATE)
early_snp_set_memory_private(vaddr, paddr, npages);
- return;
- }
- case MEMORY_SHARED: {
+ else if (op == SNP_PAGE_STATE_SHARED)
early_snp_set_memory_shared(vaddr, paddr, npages);
- return;
+ else {
+ WARN(1, "invalid memory page op %d\n", op);
}
- default:
- break;
- }
-
- WARN(1, "invalid memory op %d\n", op);
}
int sev_es_setup_ap_jump_table(struct real_mode_header *rmh)
---
> static char sme_early_buffer[PAGE_SIZE] __initdata __aligned(PAGE_SIZE);
>
> +/*
> + * When SNP is active, changes the page state from private to shared before
s/changes/change/
> + * copying the data from the source to destination and restore after the copy.
> + * This is required because the source address is mapped as decrypted by the
> + * caller of the routine.
> + */
> +static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
> + unsigned long paddr, bool decrypt)
> +{
> + unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +
> + if (!sev_feature_enabled(SEV_SNP) || !decrypt) {
> + memcpy(dst, src, sz);
> + return;
> + }
> +
> + /*
> + * If the paddr needs to be accessed decrypted, mark the page
What do you mean "If" - this is the SNP version of memcpy. Just say:
/*
* With SNP, the page address needs to be ...
*/
> + * shared in the RMP table before copying it.
> + */
> + early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages);
> +
> + memcpy(dst, src, sz);
> +
> + /* Restore the page state after the memcpy. */
> + early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages);
> +}
> +
> /*
> * This routine does not change the underlying encryption setting of the
> * page(s) that map this memory. It assumes that eventually the memory is
> @@ -96,8 +125,8 @@ static void __init __sme_early_enc_dec(resource_size_t paddr,
> * Use a temporary buffer, of cache-line multiple size, to
> * avoid data corruption as documented in the APM.
> */
> - memcpy(sme_early_buffer, src, len);
> - memcpy(dst, sme_early_buffer, len);
> + snp_memcpy(sme_early_buffer, src, len, paddr, enc);
> + snp_memcpy(dst, sme_early_buffer, len, paddr, !enc);
>
> early_memunmap(dst, len);
> early_memunmap(src, len);
> @@ -277,9 +306,23 @@ static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
> else
> sme_early_decrypt(pa, size);
>
> + /*
> + * If page is getting mapped decrypted in the page table, then the page state
> + * change in the RMP table must happen before the page table updates.
> + */
> + if (!enc)
> + early_snp_set_memory_shared((unsigned long)__va(pa), pa, 1);
Merge the two branches:
/* Encrypt/decrypt the contents in-place */
if (enc) {
sme_early_encrypt(pa, size);
} else {
sme_early_decrypt(pa, size);
/*
* On SNP, the page state change in the RMP table must happen
* before the page table updates.
*/
early_snp_set_memory_shared((unsigned long)__va(pa), pa, 1);
}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
