On 07/06/16 00:19, Laszlo Ersek wrote:
On 06/22/16 08:53, Haozhong Zhang wrote:
OS usually expects BIOS to set certain bits in MSR_IA32_FEATURE_CONTROL for some features (e.g. VMX and LMCE). QEMU provides a fw_cfg file "etc/msr_feature_control" to advise bits that should be set in MSR_IA32_FEATURE_CONTROL. If this file exists, SeaBIOS will set the advised bits in that MSR.
Signed-off-by: Haozhong Zhang haozhong.zhang@intel.com Reviewed-by: Paolo Bonzini pbonzini@redhat.com
Changes in v3:
- Move msr_feature_control_setup() to paravirt.c.
Changes in v2:
- Call msr_feature_control_setup() before smp_setup().
- Use wrmsr_smp() instead of wrmsr() on BSP.
- Rename smp_mtrr and smp_mtrr_count to smp_msr and smp_msr_count as they are not only used for MTRR now.
src/fw/paravirt.c | 12 +++++++++++- src/fw/smp.c | 20 ++++++++++---------- 2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c index 8ed4380..73a08f0 100644 --- a/src/fw/paravirt.c +++ b/src/fw/paravirt.c @@ -131,6 +131,15 @@ qemu_preinit(void) dprintf(1, "RamSize: 0x%08x [cmos]\n", RamSize); }
+#define MSR_IA32_FEATURE_CONTROL 0x0000003a
+static void msr_feature_control_setup(void) +{
- u64 feature_control_bits = romfile_loadint("etc/msr_feature_control", 0);
- if (feature_control_bits)
wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);+}
void qemu_platform_setup(void) { @@ -149,8 +158,9 @@ qemu_platform_setup(void) smm_device_setup(); smm_setup();
- // Initialize mtrr and smp
// Initialize mtrr, msr_feature_control and smp mtrr_setup();
msr_feature_control_setup(); smp_setup();
// Create bios tables
diff --git a/src/fw/smp.c b/src/fw/smp.c index 579acdb..6e706e4 100644 --- a/src/fw/smp.c +++ b/src/fw/smp.c @@ -10,7 +10,7 @@ #include "output.h" // dprintf #include "romfile.h" // romfile_loadint #include "stacks.h" // yield -#include "util.h" // smp_setup +#include "util.h" // smp_setup, msr_feature_control_setup #include "x86.h" // wrmsr
#define APIC_ICR_LOW ((u8*)BUILD_APIC_ADDR + 0x300) @@ -20,20 +20,20 @@
#define APIC_ENABLED 0x0100
-static struct { u32 index; u64 val; } smp_mtrr[32]; -static u32 smp_mtrr_count; +static struct { u32 index; u64 val; } smp_msr[32]; +static u32 smp_msr_count;
void wrmsr_smp(u32 index, u64 val) { wrmsr(index, val);
- if (smp_mtrr_count >= ARRAY_SIZE(smp_mtrr)) {
- if (smp_msr_count >= ARRAY_SIZE(smp_msr)) { warn_noalloc(); return; }
- smp_mtrr[smp_mtrr_count].index = index;
- smp_mtrr[smp_mtrr_count].val = val;
- smp_mtrr_count++;
- smp_msr[smp_msr_count].index = index;
- smp_msr[smp_msr_count].val = val;
- smp_msr_count++;
}
u32 MaxCountCPUs; @@ -58,10 +58,10 @@ handle_smp(void) u8 apic_id = ebx>>24; dprintf(DEBUG_HDL_smp, "handle_smp: apic_id=%d\n", apic_id);
- // MTRR setup
- // MTRR and MSR_IA32_FEATURE_CONTROL setup int i;
- for (i=0; i<smp_mtrr_count; i++)
wrmsr(smp_mtrr[i].index, smp_mtrr[i].val);
for (i=0; i<smp_msr_count; i++)
wrmsr(smp_msr[i].index, smp_msr[i].val);// Set bit on FoundAPICIDs FoundAPICIDs[apic_id/32] |= (1 << (apic_id % 32));
For https://github.com/tianocore/edk2/issues/97 (i.e., doing the same in OVMF), I've been trying to come up with designs.
The combined requirement that the MSR be written on all CPUs *and* at S3 resume too is a big complication for OVMF. If we can relax either of those (that is, write the MSR on all CPUs but only on cold boot, or else, write the MSR at cold boot and resume too, but only on the BSP), that would be a relief for OVMF.
So I recalled that this patch programmed the MSR on all APs, but I wanted to see if it really did the same on S3 resume. And, I think it doesn't. I tried to construct a "maximal" call tree from the bottom up, and this is all I can come up with:
reset_vector() [src/romlayout.S] entry_post() [src/romlayout.S] handle_post() [src/post.c] dopost() [src/post.c] reloc_preinit() [src/post.c] maininit() [src/post.c] platform_hardware_setup() [src/post.c] qemu_platform_setup() [src/fw/paravirt.c] msr_feature_control_setup() [src/fw/paravirt.c] wrmsr_smp() [src/fw/smp.c] wrmsr() [src/x86.h] -- on the BSP // + stash in array smp_setup() [src/fw/smp.c] entry_smp() [src/romlayout.S] -- on all APs handle_smp() [src/fw/smp.c] wrmsr() from array [src/x86.h]
smp_setup() is not reached on the S3 resume path:
reset_vector() [src/romlayout.S] entry_post() [src/romlayout.S] entry_resume() [src/romlayout.S] <--- on a different branch here handle_resume() [src/resume.c] handle_resume32() [src/resume.c] s3_resume() [src/resume.c] ... farcall16big()
I also checked "docs/Execution_and_code_flow.md", and my findings seem to be consistent with it.
I forgot to restore MSR_IA32_FEATURE_CONTROL in the resume path, and MSR_IA32_FEATURE_CONTROL is zero after S3 resume.
Now, MSR_IA32_FEATURE_CONTROL not being set on S3 is perfectly fine with me. I just want to avoid implementing something for OVMF that is several times harder than necessary.
So, is the fact (*) that SeaBIOS does not program the MSR on S3 a bug (or missing feature) in SeaBIOS, or is it okay? (For example because the resume code in Linux copies the MSR's value from the BSP to the APs anyway?)
Not sure if it can be treated as a bug or not.
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. - For VMX, if KVM in Linux (guest) finds bit 20 and bit 0 are cleared in MSR_IA32_FEATURE_CONTROL while cpuid.1 says VMX is supported, it will set those bits. Therefore, KVM just works after resume even though MSR_IA32_FEATURE_CONTROL is not properly set by firmware.
I'm not sure whether other operating systems or virtual machine monitors can work properly in this case. If all of them are fine with this, I think we can remove the S3 requirement.
Thanks, Haozhong
If it is okay, then we should remove the S3 requirement from https://github.com/tianocore/edk2/issues/97.
(*) "fact" unless I missed something, of course
Thanks! Laszlo