RE: [RFC PATCH 1/1] x86/mm: Mark CoCo VM pages invalid while moving between private and shared

[Michael Kelley (LINUX)]

From: Tom Lendacky <thomas.lendacky@xxxxxxx> Sent: Wednesday, August 2, 2023 2:58 PM
> 

[snip]

> >
> > diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> > index 28be6df..2859ec3 100644
> > --- a/arch/x86/hyperv/ivm.c
> > +++ b/arch/x86/hyperv/ivm.c
> > @@ -308,7 +308,8 @@ static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bo
> >   		return false;
> >
> >   	for (i = 0, pfn = 0; i < pagecount; i++) {
> > -		pfn_array[pfn] = virt_to_hvpfn((void *)kbuffer + i * HV_HYP_PAGE_SIZE);
> > +		pfn_array[pfn] = slow_virt_to_phys((void *)kbuffer +
> > +					i * HV_HYP_PAGE_SIZE) >> HV_HYP_PAGE_SHIFT;
> 
> Definitely needs a comment here (and below) that slow_virt_to_phys() is
> being used because of making the page not present.

Agreed.

> 
> >   		pfn++;
> >
> >   		if (pfn == HV_MAX_MODIFY_GPA_REP_COUNT || i == pagecount - 1) {
> > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> > index 1ee7bed..59db55e 100644
> > --- a/arch/x86/kernel/sev.c
> > +++ b/arch/x86/kernel/sev.c
> > @@ -784,7 +784,7 @@ static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long
> >   		hdr->end_entry = i;
> >
> >   		if (is_vmalloc_addr((void *)vaddr)) {
> > -			pfn = vmalloc_to_pfn((void *)vaddr);
> > +			pfn = slow_virt_to_phys((void *)vaddr) >> PAGE_SHIFT;
> >   			use_large_entry = false;
> >   		} else {
> >   			pfn = __pa(vaddr) >> PAGE_SHIFT;
> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index bda9f12..8a194c7 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -2136,6 +2136,11 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> >   	if (WARN_ONCE(addr & ~PAGE_MASK, "misaligned address: %#lx\n", addr))
> >   		addr &= PAGE_MASK;
> >
> > +	/* set_memory_np() removes aliasing mappings and flushes the TLB */
> 
> Is there any case where the TLB wouldn't be flushed when it should? Since,
> for SEV at least, the TLB flush being removed below was always performed.

The TLB is flushed as long as set_memory_np() actually changes some
PTEs.  It doesn’t make any sense to be doing a private<->shared transition
on pages that aren't marked PRESENT, so clearing the PRESENT bit will
always change the PTEs and cause the TLB to be flushed.  The decision is
made several levels down in __change_page_attr() where the CPA_FLUSHTLB
flag is set.  The flush is done in change_page_attr_set_clr() based on that
flag.  The data cache is *not* flushed.

Also, even if the memory *was* already not PRESENT, then the PTEs
would not be cached in the TLB anyway, and the TLB would not need
to be flushed.

Michael

> 
> > +	ret = set_memory_np(addr, numpages);
> > +	if (ret)
> > +		return ret;
> > +
> >   	memset(&cpa, 0, sizeof(cpa));
> >   	cpa.vaddr = &addr;
> >   	cpa.numpages = numpages;
> > @@ -2143,36 +2148,23 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> >   	cpa.mask_clr = enc ? pgprot_decrypted(empty) : pgprot_encrypted(empty);
> >   	cpa.pgd = init_mm.pgd;
> >
> > -	/* Must avoid aliasing mappings in the highmem code */
> > -	kmap_flush_unused();
> > -	vm_unmap_aliases();
> > -
> >   	/* Flush the caches as needed before changing the encryption attribute. */
> > -	if (x86_platform.guest.enc_tlb_flush_required(enc))
> > -		cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required());
> > +	if (x86_platform.guest.enc_cache_flush_required())
> > +		cpa_flush(&cpa, 1);
> >
> >   	/* Notify hypervisor that we are about to set/clr encryption attribute. */
> >   	if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc))
> >   		return -EIO;
> >
> >   	ret = __change_page_attr_set_clr(&cpa, 1);
> > -
> > -	/*
> > -	 * After changing the encryption attribute, we need to flush TLBs again
> > -	 * in case any speculative TLB caching occurred (but no need to flush
> > -	 * caches again).  We could just use cpa_flush_all(), but in case TLB
> > -	 * flushing gets optimized in the cpa_flush() path use the same logic
> > -	 * as above.
> > -	 */
> > -	cpa_flush(&cpa, 0);
> > +	if (ret)
> > +		return ret;
> >
> >   	/* Notify hypervisor that we have successfully set/clr encryption attribute. */
> > -	if (!ret) {
> > -		if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> > -			ret = -EIO;
> > -	}
> > +	if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
> > +		return -EIO;
> >
> > -	return ret;
> > +	return set_memory_p(&addr, numpages);
> >   }
> >
> >   static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)