Hello Boris,
On Wed, May 12, 2021 at 03:15:37PM +0200, Borislav Petkov wrote:
> On Fri, Apr 23, 2021 at 03:58:43PM +0000, Ashish Kalra wrote:
> > +static inline void notify_page_enc_status_changed(unsigned long pfn,
> > + int npages, bool enc)
> > +{
> > + PVOP_VCALL3(mmu.notify_page_enc_status_changed, pfn, npages, enc);
> > +}
>
> Now the question is whether something like that is needed for TDX, and,
> if so, could it be shared by both.
>
> Sean?
>
> > +void notify_addr_enc_status_changed(unsigned long vaddr, int npages,
> > + bool enc)
>
> Let that line stick out.
>
> > +{
> > +#ifdef CONFIG_PARAVIRT
> > + unsigned long sz = npages << PAGE_SHIFT;
> > + unsigned long vaddr_end = vaddr + sz;
> > +
> > + while (vaddr < vaddr_end) {
> > + int psize, pmask, level;
> > + unsigned long pfn;
> > + pte_t *kpte;
> > +
> > + kpte = lookup_address(vaddr, &level);
> > + if (!kpte || pte_none(*kpte))
> > + return;
>
> What does this mean exactly? On the first failure to lookup the address,
> you return? Why not continue so that you can notify about the remaining
> pages in [vaddr - vaddr_end)?
What's the use of notification of a partial page list, even a single
incorrect guest page encryption status can crash the guest/migrated
guest.
> Also, what does it mean for the current range if the lookup fails?
> Innocuous situation or do you need to signal it with a WARN or so?
>
Yes, it makes sense to signal it with a WARN or so.
> > +
> > + pfn = pg_level_to_pfn(level, kpte, NULL);
> > + if (!pfn)
> > + continue;
>
> Same here: if it hits the default case, wouldn't it make sense to
> WARN_ONCE or so to catch potential misuse? Or better yet, the WARN_ONCE
> should be in pg_level_to_pfn().
Yes, it makes sense to add a WARN_ONCE() in pg_level_to_pfn().
>
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index 16f878c26667..45e65517405a 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -2012,6 +2012,13 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
> > */
> > cpa_flush(&cpa, 0);
> >
> > + /*
> > + * Notify hypervisor that a given memory range is mapped encrypted
> > + * or decrypted. The hypervisor will use this information during the
> > + * VM migration.
> > + */
> > + notify_addr_enc_status_changed(addr, numpages, enc);
>
> If you notify about a range then that function should be called
>
> notify_range_enc_status_changed
>
Ok.
Thanks,
Ashish
> or so.
>
> --
> Regards/Gruss,
> Boris.
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7CAshish.Kalra%40amd.com%7Cb880e2dae4d24f208c8b08d915480b4a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637564221487050648%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=q%2FOAt%2FQqv0t%2BXDhjvPQAEYj67XQIUWbis0MXGMu4EZY%3D&reserved=0
