Re: [PATCHv4 3/8] efi/x86: Implement support for unaccepted memory

[Borislav Petkov <bp@alien8.de>]

On Mon, Apr 18, 2022 at 06:55:45PM +0300, Kirill A. Shutemov wrote:
> I'm confused here. What is wrong with linux/ include namespace?

The problem is that you need all kinds of workarounds so that the
decompressor builds. Just look at the beginning of

arch/x86/boot/compressed/misc.h

Even you had to do them:

/* cpu_feature_enabled() cannot be used this early */
#define USE_EARLY_PGTABLE_L5

That thing sprinkled everywhere is not a clean solution.

> Yes, we had story with <asm/io.h> that actually caused issue in
> decompression code, but linux/ has a lot of perfectly portable
> library-like stuff.

Yes, those are fine except that not everything that leaks into the
decompressor code through includes is perfectly portable.

> Could you explain what rules are?

Library-like stuff like types.h, linkage.h, etc we could include for now
but including linux/kernel.h which pulls in everything but the kitchen
sink is bad.

So I'd like for the decompressor to be completely separate from kernel
proper because it is a whole different thing and I want for us to be
able to include headers in it without ugly workarounds just so that
kernel proper include changes do not influence the decompressor.

> Hm. accept_or_mark_unaccepted()?

What's wrong with early_accept_memory()?

> > Immediately? As opposed to delayed?
> 
> Yes. Otherwise accept is delayed until the first allocation of the memory.

Yes, put that in the comment pls.

> Memory encryption can be a reason to have unaccepted memory, but it is not
> 1:1 match. Unaccepted memory can be present without memory ecnryption if
> data secruty and integrity guaranteed by other means.

Really?

Please elaborate. I thought memory acceptance is a feature solely for
TDX and SNP guests to use.

> <asm/mem_encrypt.h> is very AMD SME/SEV centric.

So?

> I'm not sure it need to exist in the way it is now.

I'm not sure what your argument actually is for having yet another
separate header vs putting it in a header which already deals with that
stuff.

> Okay, I will move it into a separate function, but it has to be called
> from allocate_e820() because it allocates and free the map.

You mean, you want for allocate_e820() to call this new function because
both allocate and free?

Might have to explain what you mean here exactly.

> > And you're saying that that efi_allocate_pages() below can really give a
> > 256M contiguous chunk?
> 
> Yes, that's assumption. Is it too high ask to deal with 4PiB of PA?

From my experience, asking firmware to do stuff for ya is always a risky
thing. I guess such a huge allocation, when it fails, will be caught
early in platform verification so whatever...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette