Re: [PATCH v12 16/23] KVM: selftests: Setup memory regions for TDX on vm creation

[Ira Weiny <ira.weiny@intel.com>]

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@google.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
>