Re: Runtime Memory Validation in Intel-TDX and AMD-SNP

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Fri, Jul 23, 2021 at 06:23:39PM +0300, Mike Rapoport wrote:
> > @@ -1318,9 +1327,14 @@ void __init e820__memblock_setup(void)
> >  		if (entry->type == E820_TYPE_SOFT_RESERVED)
> >  			memblock_reserve(entry->addr, entry->size);
> >  
> > -		if (entry->type != E820_TYPE_RAM && entry->type != E820_TYPE_RESERVED_KERN)
> > +		if (entry->type != E820_TYPE_RAM &&
> > +		    entry->type != E820_TYPE_RESERVED_KERN &&
> > +		    entry->type != E820_TYPE_UNACCEPTED)
> >  			continue;
> 
> If I understand correctly, you assume that
> 
> * E820_TYPE_RAM and E820_TYPE_RESERVED_KERN regions are already accepted by
>   firmware/booloader
> * E820_TYPE_UNACCEPTED would have been E820_SYSTEM_RAM if we'd disabled
>   encryption
> 
> What happens with other types? Particularly E820_TYPE_ACPI and
> E820_TYPE_NVS that may reside in memory and might have been accepted by
> BIOS.

Any accessible memory that not marked as UNACCEPTED has to be accepted
before kernel gets control.

> >  
> > +		if (entry->type == E820_TYPE_UNACCEPTED)
> > +			mark_unaccepted(entry->addr, end);
> > +
> >  		memblock_add(entry->addr, entry->size);
> >  	}
> >  
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 72920af0b3c0..db9d1bcac9ed 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -944,6 +944,8 @@ void __init setup_arch(char **cmdline_p)
> >  	if (movable_node_is_enabled())
> >  		memblock_set_bottom_up(true);
> >  #endif
> > +	/* TODO: make conditional */
> > +	memblock_set_bottom_up(true);
>   
> If memory is accepted during memblock allocations this should not really
> matter.
> Bottom up would be preferable if we'd like to reuse as much of already
> accepted memory as possible before page allocator is up.

One of the main reason for this feature is to speed up boot time and
re-usinging preaccepted memory fits the goal.

> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -814,6 +814,7 @@ int __init_memblock memblock_reserve(phys_addr_t base, phys_addr_t size)
> >  	memblock_dbg("%s: [%pa-%pa] %pS\n", __func__,
> >  		     &base, &end, (void *)_RET_IP_);
> >  
> > +	accept_pages(base, base + size);
> 
> Hmm, I'm not sure memblock_reserve() is the right place to accept pages. It
> can be called to reserve memory owned by firmware which not necessarily
> would be encrypted. Besides, memblock_reserve() may be called for absent
> memory, could be it'll confuse TDX/SEV?

Such memory will not be marked as unaccepted and accept_pages() will do
nothing.

> Ideally, the call to accept_pages() should live in
> memblock_alloc_range_nid(), but unfortunately there still stale
> memblock_find_in_range() + memblock_reserve() pairs in x86 setup code.

memblock_reserve() is the root of memory allocation in the early boot and
it is natual place to do the trick. Unless we have a good reason to move
it somewhere I would keep it here.

-- 
 Kirill A. Shutemov