Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 07/06/16 13:04, Laszlo Ersek wrote:
> On 07/06/16 08:42, Laszlo Ersek wrote:
> > On 07/06/16 08:28, Haozhong Zhang wrote:
> >> Hi Ashok,
> >>
> >> On 07/06/16 02:18, Paolo Bonzini wrote:
> >>>> I forgot to restore MSR_IA32_FEATURE_CONTROL in the resume path, and
> >>>> MSR_IA32_FEATURE_CONTROL is zero after S3 resume.
> >>>
> >>> This is a bug.  Sorry Laszlo. :)
> >>>
> >>>> Not restore MSR_IA32_FEATURE_CONTROL during S3 resume does not affect
> >>>> at least Linux guest (tested 4.5). Current QEMU may advise the guest
> >>>> firmware to set bit 20 (for LMCE), bit 2 (for VMX) and bit 0 (lock
> >>>> bit).
> >>>> - For LMCE, Linux only checks bit 20 and bit 0 at boot time and then
> >>>>   keeps using the result even after resume.
> >>>
> >>> On real hardware, LMCE would not be enabled after resume.  I'm not
> >>> sure what would happen, but it wouldn't be good.
> >>
> >> Could you help to check if the LMCE bit in MSR_IA32_FEATURE_CONTROL is
> >> set after S3 resume on the real hardware?
> > 
> > The SDM says that IA32_FEATURE_CONTROL is zeroed on logical processor reset.
> > 
> >   23.7 ENABLING AND ENTERING VMX OPERATION
> > 
> >   [...] VMXON is also controlled by the IA32_FEATURE_CONTROL MSR (MSR
> >   address 3AH). This MSR is cleared to zero when a logical processor is
> >   reset. [...]
> 
> Actually, I think there is a bug in KVM at the moment. I ran the
> following test:
> 
> - modified OVMF to set the MSR to value 0x5 on just the BSP
> - booted an i440fx and a Q35 (SMM-enabled) OVMF guest
> - checked "rdmsr -a 0x3a" in both
> - ran "pm-suspend" in both guests, woke them
> - repeated the rdmsr command
> 
> The result is that the BSP had the 0x5 MSR value both after cold boot
> and after S3 resume. So, KVM does not seem to implement clearing of the MSR.
>

Interesting result, is setting MSR on BSP also called after S3 resume?
I went through your test steps with OVMF replaced by a modified
SeaBIOS which only sets MSR_IA32_FEATURE_CONTROL on BSP at boot time,
the result before S3 resume is the value on BSP is 5 and others are 0,
and the result after S3 resume is values on all CPUs are 0.

> I checked kvm/next (currently at
> 196f20ca52e8c7281932663c348fa54b82d03914), and vmx_vcpu_reset() does not
> seem to zero vmx->msr_ia32_feature_control.
>

The function reset cpu state in QEMU after S3 resume is
x86_cpu_reset(CPUState *s) in target-i386/cpu.c which is called for
all vcpus and does

    memset(env, 0, offsetof(CPUX86State, cpuid_level));

CPUX86State.msr_ia32_feature_control is before .cpuid_level, so guest
MSR_IA32_FEATURE_CONTROL on all vcpus should be zero after S3 resume.

Thanks,
Haozhong

> Now, what I absolutely can't tell you is whether this zeroing should
> happen regardless of "init_event", or just for a specific value of
> "init_event". Whenever I look at "init_event", I have to track it down
> to the commit that added it, then locate all the commits that fixed it,
> then guess whether the SDM language "logical processor reset" implies a
> specific "init_event" value or not. So, I have no idea about "init_event".
> 
> Either way, why it is important to me: the edk2 modules that are built
> into OVMF use loads of INIT-SIPI-SIPI sequences, for implementing
> multiprocessing in the firmware (regardless of the feature at hand). For
> example, EFI_MP_SERVICES_PROTOCOL.StartupAllAps() boots all APs with a
> programmer-specified routine, after which the APs are HLTed. (Until the
> next such call.) INIT-SIPI-SIPI is used by the implementation for
> booting the APs.
> 
> So I need to know whether those INIT-SIPI-SIPI sequences are supposed to
> clear the MSR -- in other words, whether I have to write patches that
> explicitly sustain the MSR across these IPIs.
> 
> If the answer is "yes", then I would like to ask for a KVM patch that
> zeroes vmx->msr_ia32_feature_control "under the right circumstances".
> 
> I can only attempt to re-program the MSR in all the necessary places in
> edk2 if I can trust that KVM will clear the MSR (visibly to the "rdmsr"
> command in the guest) as long as I miss at least one location in edk2.
> 
> Thanks!
> Laszlo
>