Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

[Moger, Babu]

On 11/1/19 1:35 PM, Jim Mattson wrote:
> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@xxxxxxx> wrote:
>>
>> The UMIP (User-Mode Instruction Prevention) feature bit should be
>> set if the emulation (kvm_x86_ops->umip_emulated) is supported
>> which is done already.
>>
>> Remove the unconditional setting of this bit.
>>
>> Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP")
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>>  arch/x86/kvm/cpuid.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index f68c0c753c38..5b81ba5ad428 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
>>         /* cpuid 7.0.ecx*/
>>         const u32 kvm_cpuid_7_0_ecx_x86_features =
>>                 F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) |
>> -               F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
>> +               F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) |
>>                 F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
>>                 F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
>>
> 
> This isn't unconditional. This is masked by the features on the boot
> CPU. Since UMIP can be virtualized (without emulation) on CPUs that
> support UMIP, you should leave this alone.
> 

There is some inconstancy here.

static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32

unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
...

case 7: {
             ..
            entry->ecx |= f_umip;
            ..
        }

and
static bool svm_umip_emulated(void)
{
        return false;
}

static inline bool vmx_umip_emulated(void)
{
        return vmcs_config.cpu_based_2nd_exec_ctrl &
                SECONDARY_EXEC_DESC;
}


It appears that intention was to enable the UMIP if SVM or VMX support
umip emulation. But that is not how it is working now. Now it is enabled
if boot CPU supports UMIP.