| Date | Mon, 3 Jan 2022 17:28:19 -0600 | From | Venu Busireddy <> | Subject | Re: [PATCH v8 12/40] x86/sev: Add helper for validating pages in early enc attribute changes |
| |
On 2021-12-10 09:43:04 -0600, Brijesh Singh wrote: > The early_set_memory_{encrypt,decrypt}() are used for changing the
s/encrypt,decrypt/encrypted,decrypted/
> page from decrypted (shared) to encrypted (private) and vice versa. > When SEV-SNP is active, the page state transition needs to go through > additional steps. > > If the page is transitioned from shared to private, then perform the > following after the encryption attribute is set in the page table: > > 1. Issue the page state change VMGEXIT to add the page as a private > in the RMP table. > 2. Validate the page after its successfully added in the RMP table. > > To maintain the security guarantees, if the page is transitioned from > private to shared, then perform the following before clearing the > encryption attribute from the page table. > > 1. Invalidate the page. > 2. Issue the page state change VMGEXIT to make the page shared in the > RMP table. > > The early_set_memory_{encrypt,decrypt} can be called before the GHCB
s/encrypt,decrypt/encrypted,decrypted/
> is setup, use the SNP page state MSR protocol VMGEXIT defined in the GHCB > specification to request the page state change in the RMP table. > > While at it, add a helper snp_prep_memory() that can be used outside > the sev specific files to change the page state for a specified memory > range. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
And with a few other nits below:
Reviewed-by: Venu Busireddy <venu.busireddy@oracle.com>
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > + > + /* > + * Ask the hypervisor to mark the memory pages as private in the RMP > + * table. > + */
Indentation is off. While at it, you may want to collapse it into a one line comment.
> + 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 (!cc_platform_has(CC_ATTR_SEV_SNP)) > + return; > + > + /* > + * Invalidate the memory pages before they are marked shared in the > + * RMP table. > + */
Collapse into one line?
> + pvalidate_pages(vaddr, npages, 0); > + > + /* Ask hypervisor to mark the memory pages shared in the RMP table. */
Indentation is off.
> + /* > + * ON SNP, the page state in the RMP table must happen > + * before the page table updates. > + */ > + early_snp_set_memory_shared((unsigned long)__va(pa), pa, 1);
I know "1" implies "true", but to emphasize that the argument is actually a boolean, could you please change the "1" to "true?"
> + } > + > /* Change the page encryption mask. */ > new_pte = pfn_pte(pfn, new_prot); > set_pte_atomic(kpte, new_pte); > + > + /* > + * If page is set encrypted in the page table, then update the RMP table to > + * add this page as private. > + */ > + if (enc) > + early_snp_set_memory_private((unsigned long)__va(pa), pa, 1);
Here too, could you please change the "1" to "true?"
Venu
|