Re: [PATCH v7 10/20] x86/virt/tdx: Use all system memory when initializing TDX module as TDX memory

["Huang, Kai" <kai.huang@intel.com>]

On Fri, 2022-11-25 at 10:28 +0100, David Hildenbrand wrote:
> On 24.11.22 10:06, Huang, Kai wrote:
> > On Wed, 2022-11-23 at 17:50 -0800, Dan Williams wrote:
> > > >    
> > > > @@ -968,6 +969,15 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > > >    	unsigned long start_pfn = start >> PAGE_SHIFT;
> > > >    	unsigned long nr_pages = size >> PAGE_SHIFT;
> > > >    
> > > > +	/*
> > > > +	 * For now if TDX is enabled, all pages in the page allocator
> > > > +	 * must be TDX memory, which is a fixed set of memory regions
> > > > +	 * that are passed to the TDX module.  Reject the new region
> > > > +	 * if it is not TDX memory to guarantee above is true.
> > > > +	 */
> > > > +	if (!tdx_cc_memory_compatible(start_pfn, start_pfn + nr_pages))
> > > > +		return -EINVAL;
> > > 
> > > arch_add_memory() does not add memory to the page allocator.  For
> > > example, memremap_pages() uses arch_add_memory() and explicitly does not
> > > release the memory to the page allocator.
> > 
> > Indeed.  Sorry I missed this.
> > 
> > > This check belongs in
> > > add_memory_resource() to prevent new memory that violates TDX from being
> > > onlined.
> > 
> > This would require adding another 'arch_cc_memory_compatible()' to the common
> > add_memory_resource() (I actually long time ago had such patch to work with the
> > memremap_pages() you mentioned above).
> > 
> > How about adding a memory_notifier to the TDX code, and reject online of TDX
> > incompatible memory (something like below)?  The benefit is this is TDX code
> > self contained and won't pollute the common mm code:
> > 
> > +static int tdx_memory_notifier(struct notifier_block *nb,
> > +                              unsigned long action, void *v)
> > +{
> > +       struct memory_notify *mn = v;
> > +
> > +       if (action != MEM_GOING_ONLINE)
> > +               return NOTIFY_OK;
> > +
> > +       /*
> > +        * Not all memory is compatible with TDX.  Reject
> > +        * online of any incompatible memory.
> > +        */
> > +       return tdx_cc_memory_compatible(mn->start_pfn,
> > +                       mn->start_pfn + mn->nr_pages) ? NOTIFY_OK : NOTIFY_BAD;
> > +}
> > +
> > +static struct notifier_block tdx_memory_nb = {
> > +       .notifier_call = tdx_memory_notifier,
> > +};
> 
> With mhp_memmap_on_memory() some memory might already be touched during 
> add_memory() (because part of the hotplug memory is used for holding the 
> memmap), not when actually onlining memory. So in that case, this would 
> be too late.

Hi David,

Thanks for the review!

Right. The memmap pages are added to the zone before online_pages(), but IIUC
those memmap pages will never be free pages thus won't be allocated by the page
allocator, correct?  Therefore in practice there won't be problem even they are
not TDX compatible memory.

> 
> add_memory_resource() sounds better, even though I disgust such TDX 
> special handling in common code.
> 

Given above, do you still prefer changing add_memory_resource()?  If so I'll
change to modify add_memory_resource().