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
