On Fri, Apr 08, 2022 at 10:26:14AM -0700, Dave Hansen wrote:
> On 4/5/22 16:43, Kirill A. Shutemov wrote:
> > +void mark_unaccepted(struct boot_params *params, u64 start, u64 end)
> > +{
> > + /*
> > + * The accepted memory bitmap only works at PMD_SIZE granularity.
> > + * If a request comes in to mark memory as unaccepted which is not
> > + * PMD_SIZE-aligned, simply accept the memory now since it can not be
> > + * *marked* as unaccepted.
> > + */
> > +
> > + /*
> > + * Accept small regions that might not be able to be represented
> > + * in the bitmap:
> > + */
> > + if (end - start < 2 * PMD_SIZE) {
> > + __accept_memory(start, end);
> > + return;
> > + }
>
> This is not my first time looking at this code and I still had to think
> about this a bit. That's not good. That pathological case here is
> actually something like this:
>
> | 4k | 2044k + 2044k | 4k |
> ^ 0x0 ^ 2MB ^ 4MB
>
> Where we have a 2MB-aligned 4k accepted area, a 4088k unaccepted area,
> then another 4k accepted area. That will not result in any bits being
> set in the accepted memory bitmap because no 2MB region is fully accepted.
>
> The one oddball case is this:
>
> | 4k | 2044k | 2048k |
> ^ 0x0 ^ 2MB ^ 4MB
>
> Which would fall into the if() above, but *can* have part of its range
> marked in the bitmap.
>
> Maybe we need something more like this:
>
> /*
> * Accept small regions that might not be able to be represented
> * in the bitmap. This is a bit imprecise and may accept some
> * areas that could have been represented in the bitmap instead.
> * But, the imprecision makes the code simpler by ensuring that
> * at least one bit will be set int the bitmap below.
> */
Okay, will change.
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 2c3dac5ecb36..b17ceec757d0 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -243,6 +243,21 @@ config EFI_DISABLE_PCI_DMA
> > options "efi=disable_early_pci_dma" or "efi=no_disable_early_pci_dma"
> > may be used to override this option.
> >
> > +config UNACCEPTED_MEMORY
> > + bool
> > + depends on EFI_STUB
> > + depends on !KEXEC_CORE
>
> The changelog should probably say something about how the kexec()
> incompatibility is going to be rectified in the future.
Okay.
> > + help
> > + Some Virtual Machine platforms, such as Intel TDX, require
> > + some memory to be "accepted" by the guest before it can be used.
> > + This mechanism helps prevent malicious hosts from making changes
> > + to guest memory.
> > +
> > + UEFI specification v2.9 introduced EFI_UNACCEPTED_MEMORY memory type.
> > +
> > + This option adds support for unaccepted memory and makes such memory
> > + usable by kernel.
> > +
> > endmenu
>
> BTW, what happens if this is compiled out? Do TDX guests just lose all
> the unaccepted memory?
No. It will not have access to unaccepted memory and will only use memory
accepted by BIOS.
> Should TDX be selecting this or something?
Yes, it should and we do this.
> > @@ -504,6 +506,13 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s
> > e820_type = E820_TYPE_PMEM;
> > break;
> >
> > + case EFI_UNACCEPTED_MEMORY:
> > + if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY))
> > + continue;
>
> This seems worthy of a pr_info(). We're effectively throwing away
> memory with this "continue", right?
Yes. In this case we threat unaccepted as reserved and inaccessible to
kernel.
Maybe pr_warn() is more appropriate.
--
Kirill A. Shutemov
