Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fe62b3a6-2c28-a069-d880-772fe6c13125@amd.com>
Date:   Wed, 17 May 2023 09:32:27 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCHv11 0/9] mm, x86/cc, efi: Implement support for unaccepted
 memory
Content-Language: en-US
To:     "Kirill A. Shutemov" <kirill@shutemov.name>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Sean Christopherson <seanjc@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Joerg Roedel <jroedel@suse.de>,
        Ard Biesheuvel <ardb@kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Ingo Molnar <mingo@redhat.com>,
        Dario Faggioli <dfaggioli@suse.com>,
        Mike Rapoport <rppt@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        marcelo.cerri@canonical.com, tim.gardner@canonical.com,
        khalid.elmously@canonical.com, philip.cox@canonical.com,
        aarcange@redhat.com, peterx@redhat.com, x86@kernel.org,
        linux-mm@kvack.org, linux-coco@lists.linux.dev,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230513220418.19357-1-kirill.shutemov@linux.intel.com>
 <aa35a445-f410-b11f-bf21-3e482647faae@amd.com>
 <20230516232204.3k53vh6pdnimdrhi@box.shutemov.name>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20230516232204.3k53vh6pdnimdrhi@box.shutemov.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?d0p0emxvb1ZCRDg0cWh0V1NhRDRFQWt0QlptbmVSaldzNnBHNEZEbDRjNnkw?=
 =?utf-8?B?ejVabmZPQndWWEJGSDUwYlZmamo0aGJRRjdKZEpJekg3VXZJbWVzbmJhazB2?=
 =?utf-8?B?dUZJdVppdjJEN1ZwSFlhWFhGOWlRY2VKSmxhM1VhYURnNTg3dklBekJEdWJH?=
 =?utf-8?B?UEZhbFZKYWkwZ0pBdnFnQWNISjJ3RiszNUNzTTQvdEs3R2hlcjhMQXhzTGZh?=
 =?utf-8?B?Rm1hWnkvNDI0LzVhdWFkRitRUHlINzBmMFVvS2U3YStvYzI3OUZBb3Urekxw?=
 =?utf-8?B?bSt3cGdpNUVvdTYyRDVUdUtaNDhkMHJ1RXhxYVFBK0NjZ0RIS21aUGFCaHJn?=
 =?utf-8?B?YlF2TXEwZi9DeEZhUUppeVpwYnJ1OHN1Q2VXak1ZNjhMOWFGV2t4TXdJNkZU?=
 =?utf-8?B?YVFncjQyelZIZFFGSEJjc0liTDNWOVg0UGtOQ3FUd0p6bER2Z2llR2t1WmhC?=
 =?utf-8?B?eHo0VEJvamEralZrbVFnRmEvMkhDa2l3S3NxbkZDdTd1aFZFMjBHNHpiOE8y?=
 =?utf-8?B?MXAwczIrRDBqWGFMWnB2NUp5MVZ2RTNoU3ZBM2piM1dBVFh5dEZJNzNQSnh3?=
 =?utf-8?B?M1k3ZTI1OGJnK1V1NUdVUEVDbkVHMjl6amJ4aFJhTWFkNDFTMDdKbGpXSEdC?=
 =?utf-8?B?emttNzFoTXlpd0diakIxL2FXK2xlOFRFTjF1M0o2aFZlbWUwUzMxN2ZYbGxu?=
 =?utf-8?B?OEFMcmNwZHZJWmd6eGRtRXpyZStucEpjNS8zYWxJb3pFY2Uzd0VwV1lPbnFn?=
 =?utf-8?B?UGRyZW1MaUhRd0w3dkhzMkRWcWE3NXVOOEVscmU1NGdidW5FUkVVa0FNYlJS?=
 =?utf-8?B?c2FCaUllbjhrLzREbGtNcHp0bzN3QUhTNGFmeUZNcWovREhhSG8yRWJqQWNj?=
 =?utf-8?B?U0UwUCtFVEpTbjU0djRuR2ZTaGZlU1hnT0VtbldPVFhUanBtWmh6V0hZU0sz?=
 =?utf-8?B?dnJOUTJDR0JkRzZKU3NLQkdvT2V2U0phOFZEWDF2ckJPU2J4K2dvbzhZT0gx?=
 =?utf-8?B?b2IzM3duNmVva2FoNTZ5SVpkN3RpWmI5N3NvNC8wVWdqUHR3ek8yb2x2OVN1?=
 =?utf-8?B?TzZwR3dvQkg4UUdKTzh6SktNRTlHczVQUFI4dzA4NDArRU5qUlZZaERxTUF2?=
 =?utf-8?B?bTU1QS9leVRpR2dmY1QrVGU1SnJsbGp1dGt4NkJRa0VWY2d3SERobDhwcHlx?=
 =?utf-8?B?NUxTNmFqdWJlcDY0ZWN2b0NMbVZIVGJVajlxOUhwaEFJcG42MU5SbDFtbnNW?=
 =?utf-8?B?S0VVSENGVVFxRDdaTW81UXRwcHV4SzJ1SFdYTUs1ZGI5Ny9YRTBEUXU1bG93?=
 =?utf-8?B?QmNSeWRWbm1xaVhVa2hTVFFsWjZkNzgrbjIyNHJJa2JTZHhraWZwSU51SXVC?=
 =?utf-8?B?VlVzRFFhYW9IcDBXTW1HTEFrTUd6L0UzbXN4akFuUUIybGhjVllkYTBYaTh6?=
 =?utf-8?B?cVVJYWdIRmV2LzdtNHRWdlJSeEc5MW43aVY2ODBsU2wvdHFVL3ZUNnpJUU1w?=
 =?utf-8?B?VGRYbUZTQkFFZjVjbnhCZHVOQ2FMV0lRenFXNEFhM0xpVTNoRElNRFhna2JF?=
 =?utf-8?B?empvSGFrR0pzd25DcENlTWtXK2w1VnVadGk5SS9GNFZSMFhaWElEQkVRbHFa?=
 =?utf-8?B?a09IK1M4c3hpbFAvMkkycWkvNFdtbmNUTlZtUnlNSXZpVlROZW5IZ3l5S1d2?=
 =?utf-8?B?UW5Jd1NrRzN0OTd4RzJNL0xjQWp1V3lRWHVwS1dlem5MbEU1ZmgzRkcyVWR5?=
 =?utf-8?B?d0FnaDVkSVNidVk5RnUzVTZ0M3hnSmdUSnFleGdGaGhySlkxRUhHRWFDaHNn?=
 =?utf-8?B?cThhZWIraWx2TktpZzlvOWFhYitNTk50SDFaNGY4SFVOVklpUFY5bmU3UGtM?=
 =?utf-8?B?ZSs0b1FYaG04NUpYdmtZZkUvNStWL3JzSG9MREpRTUMyWUR4TlhnVWZ4aGkv?=
 =?utf-8?B?UUVBWnA0aHIrSWU2dUo5QmFPVU0wcXQ1ZlRSWkFpTnZjQWpQTzNld3h4eDIw?=
 =?utf-8?B?cjRkK3hLa014dkhodkhiNE45Y0gyRHZjVHVSN0xOY2gyb2tIQklzZ29aQlJr?=
 =?utf-8?B?Z1pKOHpjb0g1aHVmS0xtMjNLNUdlVEkyZkxkaEw5eTlxc3dKc0dkdlg5Y0xr?=
 =?utf-8?Q?krMM3S8p09f82rzmULnWCVaVq?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 567dd25e-6994-4bbf-f912-08db56e38c3b
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2023 14:32:31.5360
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ECSwRAcLCwmGUng8Dl0sc5L4Esww1BbZCfO2d4gieQlLiPrBuXI6ipu8GwZZbay4F6e4tExPh1iJm7e5Zr4HHw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB7473
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/16/23 18:22, Kirill A. Shutemov wrote:
> On Tue, May 16, 2023 at 05:41:55PM -0500, Tom Lendacky wrote:
>> On 5/13/23 17:04, Kirill A. Shutemov wrote:
>>> UEFI Specification version 2.9 introduces the concept of memory
>>> acceptance: some Virtual Machine platforms, such as Intel TDX or AMD
>>> SEV-SNP, requiring memory to be accepted before it can be used by the
>>> guest. Accepting happens via a protocol specific for the Virtual
>>> Machine platform.
>>>
>>> Accepting memory is costly and it makes VMM allocate memory for the
>>> accepted guest physical address range. It's better to postpone memory
>>> acceptance until memory is needed. It lowers boot time and reduces
>>> memory overhead.
>>>
>>> The kernel needs to know what memory has been accepted. Firmware
>>> communicates this information via memory map: a new memory type --
>>> EFI_UNACCEPTED_MEMORY -- indicates such memory.
>>>
>>> Range-based tracking works fine for firmware, but it gets bulky for
>>> the kernel: e820 has to be modified on every page acceptance. It leads
>>> to table fragmentation, but there's a limited number of entries in the
>>> e820 table
>>>
>>> Another option is to mark such memory as usable in e820 and track if the
>>> range has been accepted in a bitmap. One bit in the bitmap represents
>>> 2MiB in the address space: one 4k page is enough to track 64GiB or
>>> physical address space.
>>>
>>> In the worst-case scenario -- a huge hole in the middle of the
>>> address space -- It needs 256MiB to handle 4PiB of the address
>>> space.
>>>
>>> Any unaccepted memory that is not aligned to 2M gets accepted upfront.
>>>
>>> The approach lowers boot time substantially. Boot to shell is ~2.5x
>>> faster for 4G TDX VM and ~4x faster for 64G.
>>>
>>> TDX-specific code isolated from the core of unaccepted memory support. It
>>> supposed to help to plug-in different implementation of unaccepted memory
>>> such as SEV-SNP.
>>>
>>> -- Fragmentation study --
>>>
>>> Vlastimil and Mel were concern about effect of unaccepted memory on
>>> fragmentation prevention measures in page allocator. I tried to evaluate
>>> it, but it is tricky. As suggested I tried to run multiple parallel kernel
>>> builds and follow how often kmem:mm_page_alloc_extfrag gets hit.
>>>
>>> See results in the v9 of the patchset[1][2]
>>>
>>> [1] https://lore.kernel.org/all/20230330114956.20342-1-kirill.shutemov@linux.intel.com
>>> [2] https://lore.kernel.org/all/20230416191940.ex7ao43pmrjhru2p@box.shutemov.name
>>>
>>> --
>>>
>>> The tree can be found here:
>>>
>>> https://github.com/intel/tdx.git guest-unaccepted-memory
>>
>> I get some failures when building without TDX support selected in my
>> kernel config after adding unaccepted memory support for SNP:
>>
>>    In file included from arch/x86/boot/compressed/../../coco/tdx/tdx-shared.c:1,
>>                     from arch/x86/boot/compressed/tdx-shared.c:2:
>>    ./arch/x86/include/asm/tdx.h: In function ?tdx_kvm_hypercall?:
>>    ./arch/x86/include/asm/tdx.h:72:17: error: ?ENODEV? undeclared (first use in this function)
>>       72 |         return -ENODEV;
>>          |                 ^~~~~~
>>    ./arch/x86/include/asm/tdx.h:72:17: note: each undeclared identifier is reported only once for each function it appears in
>>
>> Adding an include for linux/errno.h gets past that error, but then
>> I get the following:
>>
>>    ld: arch/x86/boot/compressed/tdx-shared.o: in function `tdx_enc_status_changed_phys':
>>    tdx-shared.c:(.text+0x42): undefined reference to `__tdx_hypercall'
>>    ld: tdx-shared.c:(.text+0x7f): undefined reference to `__tdx_module_call'
>>    ld: tdx-shared.c:(.text+0xce): undefined reference to `__tdx_module_call'
>>    ld: tdx-shared.c:(.text+0x13b): undefined reference to `__tdx_module_call'
>>    ld: tdx-shared.c:(.text+0x153): undefined reference to `cc_mkdec'
>>    ld: tdx-shared.c:(.text+0x15d): undefined reference to `cc_mkdec'
>>    ld: tdx-shared.c:(.text+0x18e): undefined reference to `__tdx_hypercall'
>>    ld: arch/x86/boot/compressed/vmlinux: hidden symbol `__tdx_hypercall' isn't defined
>>    ld: final link failed: bad value
>>
>> So it looks like arch/x86/boot/compressed/tdx-shared.c is being
>> built, while arch/x86/boot/compressed/tdx.c isn't.
> 
> Right. I think this should help:
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 78f67e0a2666..b13a58021086 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -106,8 +106,8 @@ ifdef CONFIG_X86_64
>   endif
> 
>   vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> -vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o
> -vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o $(obj)/tdx-shared.o
> +vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o $(obj)/tdx-shared.o
> +vmlinux-objs-$(CONFIG_UNACCEPTED_MEMORY) += $(obj)/mem.o
> 
>   vmlinux-objs-$(CONFIG_EFI) += $(obj)/efi.o
>   vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_mixed.o
> 
>> After setting TDX in the kernel config, I can build successfully, but
>> I'm running into an error when trying to accept memory during
>> decompression.
>>
>> In drivers/firmware/efi/libstub/unaccepted_memory.c, I can see that the
>> unaccepted_table is allocated, but when accept_memory() is invoked the
>> table address is now zero. I thought maybe it had to do with bss, but even
>> putting it in the .data section didn't help. I'll keep digging, but if you
>> have any ideas, that would be great.
> 
> Not right away. But maybe seeing your side of enabling would help.

Let me get something pushed up where you can access it and I'll also send
you my kernel config.

In the mean time I added the following and everything worked. But I'm not
sure how acceptable it is to always be checking for the table when the
value is zero is.


diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index f4642c4f25dd..8c5632ab1208 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -183,8 +183,13 @@ void accept_memory(phys_addr_t start, phys_addr_t end)
  	unsigned long bitmap_size;
  	u64 unit_size;
  
-	if (!unaccepted_table)
-		return;
+	if (!unaccepted_table) {
+		efi_guid_t unaccepted_table_guid = LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID;
+
+		unaccepted_table = get_efi_config_table(unaccepted_table_guid);
+		if (!unaccepted_table)
+			return;
+	}
  
  	unit_size = unaccepted_table->unit_size;
  

Thanks,
Tom

>