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

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

[Paolo Bonzini]

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.

Reported-by: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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);
     }
 #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) {
-- 
2.5.0

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 <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@intel.com>

This is a typo, please fix it up as

Cc: Laszlo Ersek <lersek@redhat.com>

> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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 <lersek@redhat.com>

Thank you
Laszlo

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 <michael.d.kinney@intel.com>
> > Cc: Laszlo Ersek <lersek@intel.com>
> 
> This is a typo, please fix it up as
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  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 <lersek@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Paolo Bonzini]

On 13/10/2015 21:30, Eduardo Habkost wrote:
> 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 <lersek@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Eduardo, it's okay if you take it through your x86 branch; it's not
super urgent.

Paolo

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

[Eduardo Habkost]

On Tue, Oct 13, 2015 at 09:42:50PM +0200, Paolo Bonzini wrote:
> 
> 
> On 13/10/2015 21:30, Eduardo Habkost wrote:
> > 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 <lersek@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Eduardo, it's okay if you take it through your x86 branch; it's not
> super urgent.

Applied. Thanks!

-- 
Eduardo

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

[Laszlo Ersek]

Eduardo,

On 10/13/15 21:45, Eduardo Habkost wrote:
> On Tue, Oct 13, 2015 at 09:42:50PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 13/10/2015 21:30, Eduardo Habkost wrote:
>>> 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 <lersek@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Eduardo, it's okay if you take it through your x86 branch; it's not
>> super urgent.
> 
> Applied. Thanks!
> 

can you please send a PULL with this?

We're in soft freeze for 2.5, according to
<http://wiki.qemu.org/Planning/2.5>, but this qualifies I think.

Thanks!
Laszlo

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

[Eduardo Habkost]

On Fri, Oct 23, 2015 at 04:40:01PM +0200, Laszlo Ersek wrote:
> Eduardo,
> 
> On 10/13/15 21:45, Eduardo Habkost wrote:
> > On Tue, Oct 13, 2015 at 09:42:50PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 13/10/2015 21:30, Eduardo Habkost wrote:
> >>> 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 <lersek@redhat.com>
> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >>
> >> Eduardo, it's okay if you take it through your x86 branch; it's not
> >> super urgent.
> > 
> > Applied. Thanks!
> > 
> 
> can you please send a PULL with this?
> 
> We're in soft freeze for 2.5, according to
> <http://wiki.qemu.org/Planning/2.5>, but this qualifies I think.

Done (and pulled by Peter). Sorry for delaying the pull request until
after soft freeze.

-- 
Eduardo

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

[Laszlo Ersek]

On 10/23/15 20:19, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 04:40:01PM +0200, Laszlo Ersek wrote:
>> Eduardo,
>>
>> On 10/13/15 21:45, Eduardo Habkost wrote:
>>> On Tue, Oct 13, 2015 at 09:42:50PM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 13/10/2015 21:30, Eduardo Habkost wrote:
>>>>> 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 <lersek@redhat.com>
>>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>
>>>> Eduardo, it's okay if you take it through your x86 branch; it's not
>>>> super urgent.
>>>
>>> Applied. Thanks!
>>>
>>
>> can you please send a PULL with this?
>>
>> We're in soft freeze for 2.5, according to
>> <http://wiki.qemu.org/Planning/2.5>, but this qualifies I think.
> 
> Done (and pulled by Peter). Sorry for delaying the pull request until
> after soft freeze.

Not a problem, and thanks a lot to you both :) I wasn't concerned about
the soft freeze (mainly), I just got bored rebasing the patch again and
again :)

Thanks!
Laszlo