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

[Brijesh Singh]

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