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
