Messages in this thread |  | | Subject | Re: [PATCH v8 12/40] x86/sev: Add helper for validating pages in early enc attribute changes | From | Brijesh Singh <> | Date | Tue, 11 Jan 2022 15:22:01 -0600 |
| |
Hi Venu,
On 1/3/22 5:28 PM, Venu Busireddy wrote: ...
>> + >> + /* >> + * 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. >
Based on previous review feedback I tried to keep the comment to 80 character limit.
>> + 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? >
same as above.
...
>> + /* >> + * 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?" >
I assume you mean the last argument to the early_snp_set_memory_{private,shared}. Please note that its a number of pages (unsigned int). The 'true' does not make sense to me.
>> + } >> + >> /* 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?" >
same as above.
thanks
|  |