Received-SPF: pass (google.com: domain of linux-kernel+bounces-80169-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 25 Feb 2024 17:54:36 +0200
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, 
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"Rafael J. Wysocki" <rafael@kernel.org>, Peter Zijlstra <peterz@infradead.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, 
	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>, Elena Reshetova <elena.reshetova@intel.com>, 
	Jun Nakajima <jun.nakajima@intel.com>, Rick Edgecombe <rick.p.edgecombe@intel.com>, 
	Tom Lendacky <thomas.lendacky@amd.com>, "Kalra, Ashish" <ashish.kalra@amd.com>, 
	Sean Christopherson <seanjc@google.com>, "Huang, Kai" <kai.huang@intel.com>, Baoquan He <bhe@redhat.com>, 
	kexec@lists.infradead.org, linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv7 08/16] x86/tdx: Account shared memory
Message-ID: <z6dsqhxzj4beyv4vj5uq6khpt6ti4mvs3uxunvli5cvi2hgfmv@rqbyr4wlzb6f>
References: <20240212104448.2589568-1-kirill.shutemov@linux.intel.com>
 <20240212104448.2589568-9-kirill.shutemov@linux.intel.com>
 <a69a3d67-7352-4bf4-8766-a55ce97e46bd@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a69a3d67-7352-4bf4-8766-a55ce97e46bd@intel.com>

On Fri, Feb 23, 2024 at 11:08:18AM -0800, Dave Hansen wrote:
> On 2/12/24 02:44, Kirill A. Shutemov wrote:
> > The kernel will convert all shared memory back to private during kexec.
> > The direct mapping page tables will provide information on which memory
> > is shared.
> > 
> > It is extremely important to convert all shared memory. If a page is
> > missed, it will cause the second kernel to crash when it accesses it.
> > 
> > Keep track of the number of shared pages. This will allow for
> > cross-checking against the shared information in the direct mapping and
> > reporting if the shared bit is lost.
> > 
> > Include a debugfs interface that allows for the check to be performed at
> > any point.
> 
> When I read this, I thought you were going to do some automatic
> checking.  Could you make it more clear here that it's 100% up to the
> user to figure out if the numbers in debugfs match and whether there's a
> problem?  This would also be a great place to mention that the whole
> thing is racy.

What about this:

  Include a debugfs interface to dump the number of shared pages in the
  direct mapping and the expected number. There is no serialization
  against memory conversion. The numbers might not match if access to the
  debugfs interface races with the conversion.

> > +static atomic_long_t nr_shared;
> > +
> > +static inline bool pte_decrypted(pte_t pte)
> > +{
> > +	return cc_mkdec(pte_val(pte)) == pte_val(pte);
> > +}
> 
> Name this pte_is_decrypted(), please.

But why? pte_decrypted() is consistent with other pte helpers pte_none(),
pte_present, pte_dirty(), ...

> >  /* Called from __tdx_hypercall() for unrecoverable failure */
> >  noinstr void __noreturn __tdx_hypercall_failed(void)
> >  {
> > @@ -821,6 +829,11 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> >  	if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> >  		return -EIO;
> >  
> > +	if (enc)
> > +		atomic_long_sub(numpages, &nr_shared);
> > +	else
> > +		atomic_long_add(numpages, &nr_shared);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -896,3 +909,59 @@ void __init tdx_early_init(void)
> >  
> >  	pr_info("Guest detected\n");
> >  }
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +static int tdx_shared_memory_show(struct seq_file *m, void *p)
> > +{
> > +	unsigned long addr, end;
> > +	unsigned long found = 0;
> > +
> > +	addr = PAGE_OFFSET;
> > +	end  = PAGE_OFFSET + get_max_mapped();
> > +
> > +	while (addr < end) {
> > +		unsigned long size;
> > +		unsigned int level;
> > +		pte_t *pte;
> > +
> > +		pte = lookup_address(addr, &level);
> > +		size = page_level_size(level);
> > +
> > +		if (pte && pte_decrypted(*pte))
> > +			found += size / PAGE_SIZE;
> > +
> > +		addr += size;
> > +
> > +		cond_resched();
> > +	}
> 
> This is totally racy, right?  Nothing prevents the PTE from
> flip-flopping all over the place.

Yes.

> > +	seq_printf(m, "Number of shared pages in kernel page tables:  %16lu\n",
> > +		   found);
> > +	seq_printf(m, "Number of pages accounted as shared:           %16ld\n",
> > +		   atomic_long_read(&nr_shared));
> > +	return 0;
> > +}
> 
> Ditto with 'nr_shared'.  There's nothing to say that the page table walk
> has anything to do with 'nr_shared' by the time we get down here.
> 
> That's not _fatal_ for a debug interface, but the pitfalls need to at
> least be discussed.  Better yet would be to make sure this and the cpa
> code don't stomp on each other.

Serializing is cumbersome here. I can also just drop the interface.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov