Re: [PATCH v8 12/40] x86/sev: Add helper for validating pages in early enc attribute changes

Brijesh Singh
2022-01-11T21:22:08Z

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