[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ccb1dcbc-7fc3-46a4-b7d2-571afea9e39d@linux.intel.com>
Date: Thu, 30 Oct 2025 14:01:05 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Ira Weiny <ira.weiny@...el.com>, Sagi Shahar <sagis@...gle.com>
Cc: linux-kselftest@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
 Shuah Khan <shuah@...nel.org>, Sean Christopherson <seanjc@...gle.com>,
 Ackerley Tng <ackerleytng@...gle.com>, Ryan Afranji <afranji@...gle.com>,
 Andrew Jones <ajones@...tanamicro.com>,
 Isaku Yamahata <isaku.yamahata@...el.com>,
 Erdem Aktas <erdemaktas@...gle.com>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>,
 Roger Wang <runanwang@...gle.com>, Oliver Upton <oliver.upton@...ux.dev>,
 "Pratik R. Sampat" <pratikrajesh.sampat@....com>,
 Reinette Chatre <reinette.chatre@...el.com>, Chao Gao <chao.gao@...el.com>,
 Chenyi Qiang <chenyi.qiang@...el.com>, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org
Subject: Re: [PATCH v12 16/23] KVM: selftests: Setup memory regions for TDX on
 vm creation
On 10/29/2025 9:18 PM, Ira Weiny wrote:
> Sagi Shahar wrote:
>> Guest registers are inaccessible to kvm for TDX VMs. In order to set
>> register values for TDX we use a special boot code which loads the
> NIT: who is 'we'?
>
>> register values from memory and write them into the appropriate
>> registers.
>>
>> This patch sets up the memory regions used for the boot code and the
>> boot parameters for TDX.
> NIT: This is not needed.  Use imperative mood.
>
>> Signed-off-by: Sagi Shahar <sagis@...gle.com>
>> ---
>>   tools/testing/selftests/kvm/lib/kvm_util.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>> index 0e6a487ca7a4..086e8a2a4d99 100644
>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>> @@ -4,6 +4,7 @@
>>    *
>>    * Copyright (C) 2018, Google LLC.
>>    */
>> +#include "tdx/tdx_util.h"
>>   #include "test_util.h"
>>   #include "kvm_util.h"
>>   #include "processor.h"
>> @@ -435,7 +436,7 @@ void kvm_set_files_rlimit(uint32_t nr_vcpus)
>>   static bool is_guest_memfd_required(struct vm_shape shape)
>>   {
>>   #ifdef __x86_64__
>> -	return shape.type == KVM_X86_SNP_VM;
>> +	return (shape.type == KVM_X86_SNP_VM || shape.type == KVM_X86_TDX_VM);
> This caused me to dig a bit to understand why this hunk was needed given
> the commit message only discusses guest registers.  I did not recall any
> use of is_guest_memfd_required() in the vm_tdx_setup_*() calls so I was a
> bit confused.
>
> With this hunk considered the changelog should read something like:
>
> <commit message>
>
> Guest memfd is required for the primary memory region of TDX VMs.
>
> Furthermore, guest registers are inaccessible to kvm for TDX VMs.  TDX
> must use use special boot code which loads the register values from memory
> and writes them into the appropriate registers.
>
> Use guest_memfd for the primary memory regions and call the TDX boot code
> functions for TDX VMs.
>
> </commit message>
>
> This clearly explains why the change to is_guest_memfd_required() is
> needed.
+1
>
> In addition, the structure of this series is a bit odd to me.  I assume
> this patch exists after the setup calls were added to ensure
> bisect-ability?
I think it's better to switch the order of this patch and patch 15.
Patch 15 relies on the memory regions added by this patch for boot code and
parameters.
>
> Ira
>
>>   #else
>>   	return false;
>>   #endif
>> @@ -469,6 +470,12 @@ struct kvm_vm *__vm_create(struct vm_shape shape, uint32_t nr_runnable_vcpus,
>>   	for (i = 0; i < NR_MEM_REGIONS; i++)
>>   		vm->memslots[i] = 0;
>>   
>> +	if (is_tdx_vm(vm)) {
>> +		/* Setup additional mem regions for TDX. */
>> +		vm_tdx_setup_boot_code_region(vm);
>> +		vm_tdx_setup_boot_parameters_region(vm, nr_runnable_vcpus);
>> +	}
>> +
>>   	kvm_vm_elf_load(vm, program_invocation_name);
>>   
>>   	/*
>> -- 
>> 2.51.1.851.g4ebd6896fd-goog
>>
>
Powered by blists - more mailing lists
 
