Re: [PATCH v5 2/5] x86/kexec: do unconditional WBINVD for bare-metal in relocate_kernel()

Huang, Kai
2024-08-15T23:45:25Z

>
> #define ARCH_HAS_KIMAGE_ARCH
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 9c9ac606893e..07ca9d3361a3 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -392,7 +392,7 @@ void machine_kexec(struct kimage *image)
> (unsigned long)page_list,
> image->start,
> image->preserve_context,
> - host_mem_enc_active);
> + !boot_cpu_has(X86_FEATURE_HYPERVISOR));
>
>

LKP reported below warning:

All warnings (new ones prefixed by >>):

arch/x86/kernel/machine_kexec_64.c: In function 'machine_kexec':
>> arch/x86/kernel/machine_kexec_64.c:325:22: warning: variable
'host_mem_enc_active' set but not used [-Wunused-but-set-variable]
325 | unsigned int host_mem_enc_active;
| ^~~~~~~~~~~~~~~~~~~

This is due to while rebasing I didn't pay enough attention to the recent code
from commit

93c1800b3799f ("x86/kexec: Fix bug with call depth tracking")

which introduced the host_mem_enc_active variable in order to avoid
cc_platform_has() function call after load_segments() to resolve a problem
when call depth tracking is on.

A 100% safe way is to replace

host_mem_enc_active = cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT);

... with

bare_metal = !boot_cpu_has(X86_FEATURE_HYPERVISOR);

but I think we can just remove that variable and directly use

!boot_cpu_has(X86_FEATURE_HYPERVISOR)

as the last argument of calling the relocate_kernel(), because AFAICT the
above X86_FEATURE_HYPERVISOR bit test will always generate inline code thus
there will be no additional CALL/RET.

The incremental diff will be:

--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -331,16 +331,9 @@ static void kexec_save_processor_start(struct kimage
*image)
void machine_kexec(struct kimage *image)
{
unsigned long page_list[PAGES_NR];
- unsigned int host_mem_enc_active;
int save_ftrace_enabled;
void *control_page;

- /*
- * This must be done before load_segments() since if call depth
tracking
- * is used then GS must be valid to make any function calls.
- */
- host_mem_enc_active = cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT);
-

Am I missing anything?

I'll send out a new version with the above and put some explanation to the
changelog if I don't see any other feedback on the rest TDX patches in the
coming days. Thanks!