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

[Venu Busireddy]

On 2022-01-11 15:22:01 -0600, Brijesh Singh wrote:
> 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.

Isn't the line length limit 100 now? Also, there are quite a few lines
that are longer than 80 characters in this file, and elsewhere.

But you can ignore my 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?
> > 
> 
> same as above.

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.

Sorry. While reading the code, I was looking at the invocations
of pvalidate_pages(), where 0 and 1 are passed instead of "false"
and "true" for the third argument. But while replying to the thread,
I marked my comment at the wrong place. I meant to suggest to change
the third argument to pvalidate_pages().


> > > +	}
> > > +
> > >   	/* 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