Re: [PATCH 6/7] target/i386: add VMX features

[Liran Alon]

> On 17 Sep 2019, at 13:34, Paolo Bonzini <address@hidden> wrote:
> 
> Add code to convert the VMX feature words back into MSR values,
> allowing the user to enable/disable VMX features as they wish.  The same
> infrastructure enables support for limiting VMX features in named
> CPU models.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> +static uint64_t make_vmx_msr_value(uint32_t index, uint32_t features)
> +{
> +    uint32_t default1, can_be_one, can_be_zero;
> +    uint32_t must_be_one;
> +
> +    switch (index) {
> +    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
> +        default1 = 0x00000016;
> +        break;
> +    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
> +        default1 = 0x0401e172;
> +        break;
> +    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> +        default1 = 0x000011ff;
> +        break;
> +    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> +        default1 = 0x00036dff;
> +        break;
> +    case MSR_IA32_VMX_PROCBASED_CTLS2:
> +        default1 = 0;
> +        break;
> +    default:
> +        abort();
> +    }
> +

See below.

> +    /*
> +     * Bits 0-30, 32-44 and 50-53 come from the host.  KVM should
> +     * not change them for backwards compatibility.
> +     */
> +    uint64_t fixed_vmx_basic = kvm_vmx_basic & 0x003c1fff7fffffffULL;
> +
> +    /*
> +     * Same for bits 0-4 and 25-27.  Bits 16-24 (CR3 target count) can
> +     * change in the future but are always zero for now, clear them to be
> +     * future proof.  Bits 32-63 in theory could change, though KVM does
> +     * not support dual-monitor treatment and probably never will; mask
> +     * them out as well.
> +     */
> +    uint64_t fixed_vmx_misc = kvm_vmx_misc & 0x0e00001f;

I haven’t yet read deeply entire patch-series but I’m definitely against having
these hard-coded values in code instead of explicitly building proper bitmap
with well-defined bit names. This is error-prone and less readable.
(E.g. Am I suppose as a reader to convert 0x0401e172 to which processor-based 
controls it represents?)

-Liran