[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add suppor
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL |
Date: |
Wed, 6 Jul 2016 00:19:58 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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 <address@hidden>
> Reviewed-by: Paolo Bonzini <address@hidden>
> ---
> 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.
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?)
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
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Gerd Hoffmann, 2016/07/01
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL,
Laszlo Ersek <=
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Haozhong Zhang, 2016/07/05
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Paolo Bonzini, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Haozhong Zhang, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Laszlo Ersek, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Haozhong Zhang, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Laszlo Ersek, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Haozhong Zhang, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Laszlo Ersek, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Laszlo Ersek, 2016/07/06
- Re: [Qemu-devel] [SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL, Haozhong Zhang, 2016/07/06