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

[Laszlo Ersek]

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.)

> 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.

With the typo fix in the Cc tag:

Reviewed-by: Laszlo Ersek <address@hidden>

Thank you
Laszlo