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

[Laszlo Ersek]

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