[SeaBIOS] [PATCH v3] fw/msr_feature_control: add support to set MSR_IA32_FEATURE_CONTROL

Haozhong Zhang haozhong.zhang at intel.com
Wed Jul 6 03:26:19 CEST 2016


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 at intel.com>
> > Reviewed-by: Paolo Bonzini <pbonzini at 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



More information about the SeaBIOS mailing list