[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6902141659442_20bb411003c@iweiny-mobl.notmuch>
Date: Wed, 29 Oct 2025 08:18:14 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: Sagi Shahar <sagis@...gle.com>, <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>, Binbin Wu <binbin.wu@...ux.intel.com>, "Oliver
 Upton" <oliver.upton@...ux.dev>, "Pratik R. Sampat"
	<pratikrajesh.sampat@....com>, Reinette Chatre <reinette.chatre@...el.com>,
	Ira Weiny <ira.weiny@...el.com>, Chao Gao <chao.gao@...el.com>, Chenyi Qiang
	<chenyi.qiang@...el.com>
CC: <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
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.
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?
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
 
