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

[Laszlo Ersek <lersek@redhat.com>]

On 07/07/16 14:12, Paolo Bonzini wrote:
> 
>> I've worried that if I only *call* these interfaces to set the MSR, then
>> the next (independent) use of the same interfaces would clear the MSR
>> through the INIT-SIPI-SIPI. That would have forced me to modify the
>> protocol / PPI implementations so that any use of them would reprogram
>> the MSR every time, after the INIT-SIPI-SIPI.
>>
>> This way however (hopefully) it should suffice to call the PPI only --
>> the results should survive from PEI to DXE to the runtime OS on the
>> normal boot path, and from PEI to the runtime OS on the S3 resume path.
> 
> This is correct (for what I understand).  Out of curiosity, why is
> it not enough to just add the MSR to the ACPI_CPU_DATA?  Or is it
> what you're doing, but OVMF was not restoring MTRRs at S3 resume
> time either?

At the time of writing the above emails, I had not done anything in
particular yet. I was just focusing on an idea that would hopefully
allow me to keep things simple (and under OvmfPkg).

The idea is that I set the feature control MSR once in PlatformPei (on
all the CPUs), using EFI_PEI_MP_SERVICES_PPI. This would happen
regardless of the boot path (S3 vs. normal boot path). Then the MSR set
thus would survive into the OS (through DXE on the nromal boot path, and
directly on the S3 boot path).

Until you confirmed that INIT IPIs would not clear the MSR, the above
idea was not feasible, for the following reason:

- on the S3 resume path, any other PEIM clients of
  EFI_PEI_MP_SERVICES_PPI might invoke EFI_PEI_MP_SERVICES_PPI, raising
  an INIT IPI (for utilizing the APs), thereby clearing the MSR;

- on the normal boot path, the same, *plus*: in DXE, any client of
  EFI_MP_SERVICES_PROTOCOL could cause the same. (In fact just the
  startup of the EFI_MP_SERVICES_PROTOCOL implementation in CpuDxe
  involves INIT-SIPI-SIPI.)

Ultimately, the idea I'm working on is pretty simple, same as any other
chipset register configuration we do in PlatformPei. The difference is
"only" that I need to pull in the EFI_PEI_MP_SERVICES_PPI implementation
(CpuMpPei), and then call that from PlatformPei to write the MSR on all
APs as well.

The risk was that any other INIT IPIs (involved in further use of
EFI_PEI_MP_SERVICES_PPI through PEI, and EFI_MP_SERVICES_PROTOCOL
through DXE) would undo that work. But, you excluded this risk, so the
idea should be fine. (I'm about to test it soon...)

The ACPI_CPU_DATA field would be related to PiSmmCpuDxeSmm and
CpuS3DataDxe. With the above idea, I don't expect to have to touch those
at all (I could be proved wrong, of course -- this is why it is
important for QEMU to clear the MSR whenever it is architecturally
required). PlatformPei should program the MSR independently of the SMM
driver stack.

So this is something I have to test in four situations: { cold boot, S3
resume } x { SMM driver stack absent, SMM driver stack present }.

Regarding MTRRs... that's a bit messy. PlatformPei only progams the
MTRRs only on the BSP. For the normal boot path, this is no problem,
because when EFI_MP_SERVICES_PROTOCOL starts up (in CpuDxe), the MTRR
settings are broad-cast to all APs. It is also not a problem for the S3
resume path, when the SMM driver stack is used, because CpuS3DataDxe
saves the MTRRs at End-of-DXE, and at S3 resume, PiSmmCpuDxeSmm restores
them.

There is one path where the firmware does not restore MTRRs on APs: S3
resume without the SMM driver stack. In practice this doesn't seem to
cause problems. Maybe Linux restores those MTRRs anyway, when the APs
are onlined after resume -- even at cold boot, Linux checks the MTRR
config, and if it's inconsistent between BSP and APs, the APs are adapted.

(If I understand correctly, on S3 resume, SeaBIOS doesn't reprogram the
MTRRs even on the BSP, and historically this has caused no problems. So
in that sense OVMF is "no worse". :))

Thanks
Laszlo