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

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

On Mon, Apr 18, 2022 at 11:01:12PM +0200, Borislav Petkov wrote:
> On Mon, Apr 18, 2022 at 11:24:31PM +0300, Kirill A. Shutemov wrote:
> > <linux/bitmap> doesn't include <linux/kernel.h> or similar things.
> > Is it okay for now?
> 
> No, it is not ok because those linux/ includes are moving targets. They
> keep changing and then that indirectly influences the decompressor.
> 
> How much functionality from linux/bitmap.h do you actually need?

Below is the bare minimum required to compile bitmap.c in decompresser.
I only made it work on my config/compiler and did not care about all
#ifdef branches.

I find it strange that you go after <linux/bitmap.h> which has limited
exposure while <linux/acpi.h> and <linux/efi.h> are there already.
Starting small will backfire if once we find out that monstrous headers
depend on what we try to replace. Bit fish has to be addressed first.

What do you want me to do here?

// <linux/bitmap.h>
//
#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))

// <uapi/linux/swab.>

/**
 * __swab64 - return a byteswapped 64-bit value
 * @x: value to byteswap
 */
#ifdef __HAVE_BUILTIN_BSWAP64__
#define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
#else
#define __swab64(x)				\
	(__builtin_constant_p((__u64)(x)) ?	\
	___constant_swab64(x) :			\
	__fswab64(x))
#endif

static __always_inline unsigned long __swab(const unsigned long y)
{
#if __BITS_PER_LONG == 64
	return __swab64(y);
#else /* __BITS_PER_LONG == 32 */
	return __swab32(y);
#endif
}

// <linux/swab.h>

# define swab __swab

// <linux/bits.h>

#define BIT_WORD(nr)		((nr) / BITS_PER_LONG)

// <asm/bitops.h>
//
/**
 * __ffs - find first set bit in word
 * @word: The word to search
 *
 * Undefined if no bit exists, so code should check against 0 first.
 */
static __always_inline unsigned long __ffs(unsigned long word)
{
	asm("rep; bsf %1,%0"
		: "=r" (word)
		: "rm" (word));
	return word;
}


> > But the goal of the function is not to accept the memory, but mark it
> > as unaccepted in the bitmap.
> 
> Really?
> 
> +	 * Accept small regions that might not be able to be represented
> +	 * in the bitmap:
> +	 */
> +	if (end - start < 2 * PMD_SIZE) {
> +		__accept_memory(start, end);
> 
> That looks like it is accepting to me.

Yes, really.

As 1 bit represents 2M, not all chunks can be represented in the bitmap
and they have to be accepted. But the *goal* is to record unaccepted
memory into bitmap. Some accepting is a side effect.

The early_accept_memory() name is just wrong.

> > Conceptionally, it is just memory that requires additional action before
> > it can be accessed. Yes, at the moment TDX and SEV are the only users.
> > It is implementation detail that TDX and SEV use memory encryption.
> 
> So there *might* be some potential future use. Nothing concrete at the
> moment.
> 
> > Because I don't think it is a good fit. Frankly, even <asm/coco.h> fits
> > better, although I'm no a fan either.
> > 
> > Do we have file shortage? I would rather keep it separate.
> 
> So I have not read a single argument for why the unaccepted memory gunk
> should be separate.

> We have perfectly fine mem_encrypt.[ch] files everywhere which already
> contain code which deals with the kernel running as encrypted guest.

And some stuff for encrypted host (SME).

> The unaccepted memory stuff is part of that - not something separate. If
> it gets to get used for something different, sure, then it can be carved
> out because it might need to be built separately, without the rest of
> the encryption code. But as it is now, it doesn't have to. So please put
> it in those files.

Okay, I will do as you want, but I really hate it.

With one hand you try to unwind header mess in decompresser code and with
another propose to create a kitchen-sink header because topics somewhat
related. Looks contradictory.

-- 
 Kirill A. Shutemov