Re: [Qemu-devel] [PATCH] target-i386: allow any alignment for SMBASE

[Eduardo Habkost]

On Mon, Oct 12, 2015 at 07:32:17PM +0200, Laszlo Ersek wrote:
> On 10/12/15 18:25, Paolo Bonzini wrote:
> > Processors up to the Pentium (says Bochs---I do not have old enough
> > manuals) require a 32KiB alignment for the SMBASE, but newer processors
> > do not need that, and Tiano Core will use non-aligned SMBASE values.
> 
> The Quark package has the following comment & code:
> 
>   //
>   // Allocate buffer for all of the tiles.
>   // For 486/Pentium, the SMBASE (and hence the buffer) must be aligned on a 
> 32KB boundary
>   //
>   Buffer = AllocatePages (EFI_SIZE_TO_PAGES (SIZE_64KB + TileSize * 
> (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - 1)));
>   Buffer = (VOID *)(((UINTN)Buffer & 0xFFFF8000) + SIZE_32KB);
> 
> whereas Mike's patch
> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/2707/focus=2714>
> has:
> 
>   // Allocate buffer for all of the tiles.
>   //
>   // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
>   // Volume 3C, Section 34.11 SMBASE Relocation
>   // For 486(FamilyId:4)/Pentium(FamilyId:5), the SMBASE (and hence the 
> buffer)
>   // must be aligned on a 32KB boundary
>   //
>   if ((FamilyId == 4) || (FamilyId == 5)) {
>     Buffer = AllocateAlignedPages (EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * 
> (mMaxNumberOfCpus - 1)), SIZE_32KB);
>   } else {
>     Buffer = AllocatePages (EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * 
> (mMaxNumberOfCpus - 1)));
>   }
> 
> The most recent Intel SDM I have on my disk (the unified mammoth
> edition) is "325462-054US, April 2015"; the section that Mike marked
> above states the following (parentheses theirs):
> 
>   (For Pentium and Intel486 processors, the SMBASE values must be
>   aligned on a 32-KByte boundary or the processor will enter shutdown
>   state during the execution of a RSM instruction.)

An old Pentium manual I have here says the same:

"The Pentium processor (510\60, 567\66) provides a control register,
SMBASE. The address space used as SMRAM can be modified by changing the
SMBASE register before exiting an SMI handler routine. SMBASE can be
changed to any 32K aligned value (values that are not 32K aligned will
cause the CPU to enter the shutdown state when executing the RSM
instruction). SMBASE is set to the default value of 30000H on RESET, but
is not changed on INIT. If the SMBASE register is changed during an SMM
handler, all following SMI# requests will initiate a state save at the
new SMBASE."

> 
> > Reported-by: Michael D Kinney <address@hidden>
> > Cc: Laszlo Ersek <address@hidden>
> 
> This is a typo, please fix it up as
> 
> Cc: Laszlo Ersek <address@hidden>
> 
> > Cc: Jordan Justen <address@hidden>
> > Cc: Eduardo Habkost <address@hidden>
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > ---
> >  target-i386/smm_helper.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c
> > index 02e24b9..c272a98 100644
> > --- a/target-i386/smm_helper.c
> > +++ b/target-i386/smm_helper.c
> > @@ -266,7 +266,7 @@ void helper_rsm(CPUX86State *env)
> >
> >      val = x86_ldl_phys(cs, sm_state + 0x7efc); /* revision ID */
> >      if (val & 0x20000) {
> > -        env->smbase = x86_ldl_phys(cs, sm_state + 0x7f00) & ~0x7fff;
> > +        env->smbase = x86_ldl_phys(cs, sm_state + 0x7f00);
> >      }
> 
> This is under #ifdef TARGET_X86_64, so I guess it's right to remove the
> masking unconditionally -- I think that because I believe the
> intersection of TARGET_X86_64 and (FamilyId == 4 || FamilyId == 5)
> should be empty. (In particular, the save state area in this branch is
> laid out according to the AMD definition, not the Intel one, so the
> Intel SDM is not normative here anyway.)
> 
> >  #else
> >      cpu_x86_update_cr0(env, x86_ldl_phys(cs, sm_state + 0x7ffc));
> > @@ -319,7 +319,7 @@ void helper_rsm(CPUX86State *env)
> >
> >      val = x86_ldl_phys(cs, sm_state + 0x7efc); /* revision ID */
> >      if (val & 0x20000) {
> > -        env->smbase = x86_ldl_phys(cs, sm_state + 0x7ef8) & ~0x7fff;
> > +        env->smbase = x86_ldl_phys(cs, sm_state + 0x7ef8);
> >      }
> >  #endif
> >      if ((env->hflags2 & HF2_SMM_INSIDE_NMI_MASK) == 0) {
> >
> 
> I first thought it would be better to keep the mask for (FamilyId == 4
> || FamilyId == 5). But, since we don't "enter shutdown state" even if
> the alignment is incorrect (i.e., we don't emulate the letter of the SDM
> for incorrect guest code anyway), and the patch retains behavior for
> correct guest code, I think this branch should be fine as well.

Yeah, the shutdown behavior was never implemented before, so now we are
just deviating from the documented Pentium & 486 behavior in a different
(and less surprising?) way.

> 
> With the typo fix in the Cc tag:
> 
> Reviewed-by: Laszlo Ersek <address@hidden>

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo