Re: [RFC PATCH v2] target/i386: add support for VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 8/8/2023 10:40 AM, Ake Koomsin wrote:
> Current QEMU can expose waitpkg to guests when it is available. However,
> VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE is still not recognized and
> masked by QEMU. This can lead to an unexpected situation when a L1
> hypervisor wants to expose waitpkg to a L2 guest. The L1 hypervisor can
> assume that VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE exists as waitpkg is
> available. The L1 hypervisor then can accidentally expose waitpkg to the
> L2 guest. This will cause invalid opcode exception in the L2 guest when
> it executes waitpkg related instructions.
> 
> This patch adds VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE support, and
> sets up dependency between the bit and CPUID_7_0_ECX_WAITPKG. QEMU should
> not expose waitpkg feature if VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE is
> not available to avoid unexpected invalid opcode exception in L2 guests.
> 
> Signed-off-by: Ake Koomsin <ake@igel.co.jp>
> ---
> 
> v2:
> - Fix typo in the patch header (targer -> target)
> 
> v1:
> https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg01048.html
> 
>   target/i386/cpu.c | 6 +++++-
>   target/i386/cpu.h | 1 +
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 97ad229d8b..00f913b638 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1228,7 +1228,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>               "vmx-invpcid-exit", "vmx-vmfunc", "vmx-shadow-vmcs", "vmx-encls-exit",
>               "vmx-rdseed-exit", "vmx-pml", NULL, NULL,
>               "vmx-xsaves", NULL, NULL, NULL,
> -            NULL, "vmx-tsc-scaling", NULL, NULL,
> +            NULL, "vmx-tsc-scaling", "vmx-enable-user-wait-pause", NULL,
>               NULL, NULL, NULL, NULL,
>           },
>           .msr = {
> @@ -1545,6 +1545,10 @@ static FeatureDep feature_dependencies[] = {
>           .from = { FEAT_8000_0001_ECX,       CPUID_EXT3_SVM },
>           .to = { FEAT_SVM,                   ~0ull },
>       },
> +    {
> +        .from = { FEAT_VMX_SECONDARY_CTLS,  VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE },
> +        .to = { FEAT_7_0_ECX,               CPUID_7_0_ECX_WAITPKG },
> +    },

Is this what we really want?

It prevents us from exposing WAITPKG to the guest when we don't want to 
expose VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE.

An easy example is that, we want to launch a VM without virtualization 
capability. And this VM cannot have WAITPKG?

>   };
>   
>   typedef struct X86RegisterInfo32 {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index e0771a1043..a6000e93bd 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1111,6 +1111,7 @@ uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
>   #define VMX_SECONDARY_EXEC_ENABLE_PML               0x00020000
>   #define VMX_SECONDARY_EXEC_XSAVES                   0x00100000
>   #define VMX_SECONDARY_EXEC_TSC_SCALING              0x02000000
> +#define VMX_SECONDARY_EXEC_ENABLE_USER_WAIT_PAUSE   0x04000000
>   
>   #define VMX_PIN_BASED_EXT_INTR_MASK                 0x00000001
>   #define VMX_PIN_BASED_NMI_EXITING                   0x00000008