Re: [Qemu-devel] [PATCH v4 3/3] i386: publish advised value of MSR_IA32_FEATURE_CONTROL via fw_cfg

[Laszlo Ersek]

Hi Haozhong,

On 06/16/16 13:19, Haozhong Zhang wrote:
> On 06/16/16 11:52, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 08:06, Haozhong Zhang wrote:
>>> It's a prerequisite that certain bits of MSR_IA32_FEATURE_CONTROL should
>>> be set before some features (e.g. VMX and LMCE) can be used, which is
>>> usually done by the firmware. This patch adds a fw_cfg file
>>> "etc/msr_feature_control" which contains the advised value of
>>> MSR_IA32_FEATURE_CONTROL and can be used by guest firmware (e.g. SeaBIOS).
>>>
>>> Suggested-by: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Haozhong Zhang <address@hidden>
>>> ---
>>>  hw/i386/pc.c      | 28 ++++++++++++++++++++++++++++
>>>  target-i386/cpu.h |  4 ++++
>>>  2 files changed, 32 insertions(+)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 7198ed5..d8178a5 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1147,6 +1147,33 @@ void pc_cpus_init(PCMachineState *pcms)
>>>      smbios_set_cpuid(cpu->env.cpuid_version, 
>>> cpu->env.features[FEAT_1_EDX]);
>>>  }
>>>  
>>> +static void pc_build_feature_control_file(PCMachineState *pcms)
>>> +{
>>> +    X86CPU *cpu = X86_CPU(pcms->possible_cpus->cpus[0].cpu);
>>> +    CPUX86State *env = &cpu->env;
>>> +    uint32_t unused, ecx, edx, feature_control_bits = 0;
>>> +    uint32_t *val;
>>> +
>>> +    cpu_x86_cpuid(env, 1, 0, &unused, &unused, &ecx, &edx);
>>> +    if (ecx & CPUID_EXT_VMX) {
>>> +        feature_control_bits |= FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>>> +    }
>>> +
>>> +    if ((edx & (CPUID_EXT2_MCE | CPUID_EXT2_MCA)) ==
>>> +        (CPUID_EXT2_MCE | CPUID_EXT2_MCA) &&
>>> +        (env->mcg_cap & MCG_LMCE_P)) {
>>> +        feature_control_bits |= FEATURE_CONTROL_LMCE;
>>> +    }
>>> +
>>> +    if (!feature_control_bits) {
>>> +        return;
>>> +    }
>>> +
>>> +    val = g_malloc(sizeof(*val));
>>> +    *val = feature_control_bits | FEATURE_CONTROL_LOCKED;
>>> +    fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, 
>>> sizeof(*val));
>>> +}
>>> +
>>>  static
>>>  void pc_machine_done(Notifier *notifier, void *data)
>>>  {
>>> @@ -1174,6 +1201,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>>>      acpi_setup();
>>>      if (pcms->fw_cfg) {
>>>          pc_build_smbios(pcms->fw_cfg);
>>> +        pc_build_feature_control_file(pcms);
>>>      }
>>>  }
>>>  
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index f0cb04f..5e07c7a 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -332,6 +332,10 @@
>>>  #define MSR_TSC_ADJUST                  0x0000003b
>>>  #define MSR_IA32_TSCDEADLINE            0x6e0
>>>  
>>> +#define FEATURE_CONTROL_LOCKED                    (1<<0)
>>> +#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX (1<<2)
>>> +#define FEATURE_CONTROL_LMCE                      (1<<20)
>>> +
>>>  #define MSR_P6_PERFCTR0                 0xc1
>>>  
>>>  #define MSR_IA32_SMBASE                 0x9e
>>>
>>
>> Reviewed-by: Paolo Bonzini <address@hidden>
>>
>> Have you prepared a patch for SeaBIOS already?
> 
> Yes, I'll send it after I fix the type error (uint32_t => uint64_t) in
> next version.

This should be supported by OVMF as well (thanks Paolo for the heads-up).

I'm glad to code that up, but I'd like to ask you to file an RFE
(Request For Enhancement) in the upstream edk2 tracker for OVMF:

https://github.com/tianocore/edk2/issues/new

In the RFE,
- the subject line should include "OvmfPkg",
- please describe what the feature is good for (going into the details
  of a specific use case is welcome),
- please specify the pathname and the internal format of the fw_cfg
  file,
- please clarify if this MSR is considered part of the "chipset state"
  that the firmware is responsible for at *every* boot. In particular
  whether you expect that the firmware program this MSR at S3 resume as
  well.

Thanks!
Laszlo