Re: [PATCHv13 9/9] x86/tdx: Add unaccepted memory support

[Tom Lendacky]

On 6/2/23 08:22, Tom Lendacky wrote:
> On 6/1/23 13:25, Kirill A. Shutemov wrote:
> > Hookup TDX-specific code to accept memory.
> >
> > Accepting the memory is done with ACCEPT_PAGE module call on every page
> > in the range. MAP_GPA hypercall is not required as the unaccepted memory
> > is considered private already.
> >
> > Extract the part of tdx_enc_status_changed() that does memory acceptance
> > in a new helper. Move the helper tdx-shared.c. It is going to be used by
> > both main kernel and decompressor.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/Kconfig                         |  2 +
> >   arch/x86/boot/compressed/Makefile        |  2 +-
> >   arch/x86/boot/compressed/error.c         | 19 +++++++
> >   arch/x86/boot/compressed/error.h         |  1 +
> >   arch/x86/boot/compressed/mem.c           | 35 +++++++++++-
> >   arch/x86/boot/compressed/tdx-shared.c    |  2 +
> >   arch/x86/coco/tdx/Makefile               |  2 +-
> >   arch/x86/coco/tdx/tdx-shared.c           | 71 ++++++++++++++++++++++++
> >   arch/x86/coco/tdx/tdx.c                  | 70 +----------------------
> >   arch/x86/include/asm/shared/tdx.h        |  2 +
> >   arch/x86/include/asm/unaccepted_memory.h | 24 ++++++++
> >   11 files changed, 160 insertions(+), 70 deletions(-)
> >   create mode 100644 arch/x86/boot/compressed/tdx-shared.c
> >   create mode 100644 arch/x86/coco/tdx/tdx-shared.c
> >   create mode 100644 arch/x86/include/asm/unaccepted_memory.h

> > diff --git a/arch/x86/boot/compressed/mem.c 
> > b/arch/x86/boot/compressed/mem.c
> > index 4ecf26576a77..d2b6948a7801 100644
> > --- a/arch/x86/boot/compressed/mem.c
> > +++ b/arch/x86/boot/compressed/mem.c
> > @@ -2,11 +2,44 @@
> >   #include "error.h"
> >   #include "misc.h"
> > +#include "tdx.h"
> > +#include <asm/shared/tdx.h>
> > +
> > +/*
> > + * accept_memory() and process_unaccepted_memory() called from EFI stub 
> > which
> > + * runs before decompresser and its early_tdx_detect().
> > + *
> > + * Enumerate TDX directly from the early users.
> > + */
> > +static bool early_is_tdx_guest(void)
> > +{
> > +    static bool once;
> > +    static bool is_tdx;
> > +
> > +    if (!IS_ENABLED(CONFIG_INTEL_TDX_GUEST))
> > +        return false;
> > +
> > +    if (!once) {
> > +        u32 eax, sig[3];
> > +
> > +        cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax,
> > +                &sig[0], &sig[2],  &sig[1]);
> > +        is_tdx = !memcmp(TDX_IDENT, sig, sizeof(sig));
> > +        once = true;
> > +    }
> > +
> > +    return is_tdx;
> > +}
> >   void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> >   {
> >       /* Platform-specific memory-acceptance call goes here */
> > -    error("Cannot accept memory");
> > +    if (early_is_tdx_guest()) {
> > +        if (tdx_accept_memory(start, end))
> > +            return;
> > +    }
> > +
> > +    error("Cannot accept memory: unknown platform\n");
>
> So this is a change in this version. If tdx_accept_memory() fails, you'll 
> report unknown platform. Wouldn't it be better to have an error message 
> that indicates a failure in the accept path?

Maybe you can keep it similar to the v12 version with just a new error 
message, something like:

	if (early_is_tdx_guest()) {
		if (!tdx_accept_memory(start, end))
			error("TDX error accepting memory\n");
	} else {
		error("Cannot accept memory: unknown platform\n");
	}

And similar in arch/x86/include/asm/unaccepted_memory.h.

Thanks,
Tom

> Thanks,
> Tom
>
> >   }
> >   void init_unaccepted_memory(void)