Patchwork target-i386: n270 can MOVBE

login
register
mail settings
Submitter Borislav Petkov
Date Feb. 8, 2013, 9:30 a.m.
Message ID <1360315802-959-1-git-send-email-bp@alien8.de>
Download mbox | patch
Permalink /patch/219074/
State New
Headers show

Comments

Borislav Petkov - Feb. 8, 2013, 9:30 a.m.
From: Borislav Petkov <bp@suse.de>

The Atom core (cpu name "n270" in QEMU speak) supports MOVBE. This is
needed when booting 3.8 and later linux kernels built with the MATOM
target because we require MOVBE in order to boot properly now.

Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Borislav Petkov <bp@suse.de>
---

Right, so I was playing with booting MATOM kernels in QEMU.
As it turns out, QEMU's n270 model doesn't advertize MOVBE
although the real hardware supports it. Quick search pointed me to
http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg04317.html
which adds that support, among other things. I've merged Richard's
patchset with qemu's current master and after applying this patch below,
I can report success booting an MATOM kernel with QEMU. The same kernel
boots on the real n270 hardware, btw.

 target-i386/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Andreas Färber - Feb. 8, 2013, 11:38 a.m.
Hi,

Am 08.02.2013 10:30, schrieb Borislav Petkov:
> From: Borislav Petkov <bp@suse.de>
> 
> The Atom core (cpu name "n270" in QEMU speak) supports MOVBE. This is
> needed when booting 3.8 and later linux kernels built with the MATOM
> target because we require MOVBE in order to boot properly now.
> 
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>

Please CC me on cpu.c changes (cf. MAINTAINERS).

> ---
> 
> Right, so I was playing with booting MATOM kernels in QEMU.
> As it turns out, QEMU's n270 model doesn't advertize MOVBE
> although the real hardware supports it. Quick search pointed me to
> http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg04317.html
> which adds that support, among other things. I've merged Richard's
> patchset with qemu's current master and after applying this patch below,
> I can report success booting an MATOM kernel with QEMU. The same kernel
> boots on the real n270 hardware, btw.
> 
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9f38e4435e53..83816edd8410 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -610,7 +610,8 @@ static x86_def_t builtin_x86_defs[] = {
>              CPUID_ACPI | CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
>              /* Some CPUs got no CPUID_SEP */
>          .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> -            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
> +            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR |
> +	    CPUID_EXT_MOVBE,

Tab. Please use scripts/checkpatch.pl to verify before sending.

>          .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
>              CPUID_EXT2_NX,
>          .ext3_features = CPUID_EXT3_LAHF_LM,

Otherwise if someone can ack (or if you can point me to a manual), this
looks like a good bugfix for v1.4. CC'ing some more CPU'ish people.

Regards,
Andreas
Eduardo Habkost - Feb. 8, 2013, 3:19 p.m.
On Fri, Feb 08, 2013 at 10:30:02AM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> The Atom core (cpu name "n270" in QEMU speak) supports MOVBE. This is
> needed when booting 3.8 and later linux kernels built with the MATOM
> target because we require MOVBE in order to boot properly now.
> 
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> 
> Right, so I was playing with booting MATOM kernels in QEMU.
> As it turns out, QEMU's n270 model doesn't advertize MOVBE
> although the real hardware supports it. Quick search pointed me to
> http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg04317.html
> which adds that support, among other things. I've merged Richard's
> patchset with qemu's current master and after applying this patch below,
> I can report success booting an MATOM kernel with QEMU. The same kernel
> boots on the real n270 hardware, btw.
> 
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9f38e4435e53..83816edd8410 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -610,7 +610,8 @@ static x86_def_t builtin_x86_defs[] = {
>              CPUID_ACPI | CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
>              /* Some CPUs got no CPUID_SEP */
>          .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
> -            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
> +            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR |
> +	    CPUID_EXT_MOVBE,
>          .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |


"-machine pc-1.3 -cpu n270" (and older machine-types) needs to keep
MOVBE disabled, or you will break live migration.

Personally I wouldn't mind declaring n270 as a non-migratable CPU model.
But libvirt supports n270, so it already expects n270 to expose a stable
ABI on each machine-type.


>              CPUID_EXT2_NX,
>          .ext3_features = CPUID_EXT3_LAHF_LM,
> -- 
> 1.8.1.2.422.g08c0e7f
> 
>
Borislav Petkov - Feb. 8, 2013, 3:34 p.m.
On Fri, Feb 08, 2013 at 01:19:02PM -0200, Eduardo Habkost wrote:
> "-machine pc-1.3 -cpu n270" (and older machine-types) needs to keep
> MOVBE disabled, or you will break live migration.
>
> Personally I wouldn't mind declaring n270 as a non-migratable CPU
> model. But libvirt supports n270, so it already expects n270 to expose
> a stable ABI on each machine-type.

From looking at the code - and I've never looked at qemu code before
last night - we could filter migration features in x86_cpu_realize()
when called down the pc_init1() path.

We simply need to look the QEMUMachine.is_default flag and leave the
feature on only if pc-1.4 which sets the is_default thing.

Or is there a more preferred way to do that?

Thanks.
Eduardo Habkost - Feb. 8, 2013, 3:58 p.m.
On Fri, Feb 08, 2013 at 04:34:58PM +0100, Borislav Petkov wrote:
> On Fri, Feb 08, 2013 at 01:19:02PM -0200, Eduardo Habkost wrote:
> > "-machine pc-1.3 -cpu n270" (and older machine-types) needs to keep
> > MOVBE disabled, or you will break live migration.
> >
> > Personally I wouldn't mind declaring n270 as a non-migratable CPU
> > model. But libvirt supports n270, so it already expects n270 to expose
> > a stable ABI on each machine-type.
> 
> From looking at the code - and I've never looked at qemu code before
> last night - we could filter migration features in x86_cpu_realize()
> when called down the pc_init1() path.

x86_cpu_realize() is too late, as it would make "-cpu n270,-movbe" fail
to disable the feature. The features need to be be initialized properly
before cpu_x86_parse_featurestr(), on cpu_x86_register().

> 
> We simply need to look the QEMUMachine.is_default flag and leave the
> feature on only if pc-1.4 which sets the is_default thing.

is_default has nothing to do with it. If we enable MOVBE on pc-1.4,
pc-1.4 will need to keep MOVBE enabled forever, it doesn't matter if it
is the default machine-type or not.

> 
> Or is there a more preferred way to do that?

pc_init_pci_1_3() needs to ask the CPU code to disable MOVBE on n270.

As we don't have a decent method to do that today, we are using static
variables and compatibility-setup functions called from the machine init
function. See disable_kvm_pv_eoi() for example.

One day we will be able to do that using properties on the machine-type
compat_props tables, but we can't do that yet.

People can easily work around the lack of the feature today, using
"-cpu n270,+movbe", so I believe it is worth waiting a little bit and
fixing this on QEMU 1.5, when we will (hopefully) have a decent
compatibility system based on compat_props.
Richard Henderson - Feb. 8, 2013, 4:49 p.m.
On 2013-02-08 03:38, Andreas Färber wrote:
> Otherwise if someone can ack (or if you can point me to a manual), this
> looks like a good bugfix for v1.4. CC'ing some more CPU'ish people.
>
It wouldn't be a bug fix without extracting the patch out of
my x86-next tree that actually implements movbe.  I don't think
this can go into 1.4.


r~
H. Peter Anvin - Feb. 8, 2013, 4:50 p.m.
This is problematic since the n270 qemu model won't boot a real kernel compiled for that box.

Eduardo Habkost <ehabkost@redhat.com> wrote:

>On Fri, Feb 08, 2013 at 10:30:02AM +0100, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@suse.de>
>> 
>> The Atom core (cpu name "n270" in QEMU speak) supports MOVBE. This is
>> needed when booting 3.8 and later linux kernels built with the MATOM
>> target because we require MOVBE in order to boot properly now.
>> 
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Borislav Petkov <bp@suse.de>
>> ---
>> 
>> Right, so I was playing with booting MATOM kernels in QEMU.
>> As it turns out, QEMU's n270 model doesn't advertize MOVBE
>> although the real hardware supports it. Quick search pointed me to
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg04317.html
>> which adds that support, among other things. I've merged Richard's
>> patchset with qemu's current master and after applying this patch
>below,
>> I can report success booting an MATOM kernel with QEMU. The same
>kernel
>> boots on the real n270 hardware, btw.
>> 
>>  target-i386/cpu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 9f38e4435e53..83816edd8410 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -610,7 +610,8 @@ static x86_def_t builtin_x86_defs[] = {
>>              CPUID_ACPI | CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
>>              /* Some CPUs got no CPUID_SEP */
>>          .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR |
>CPUID_EXT_SSSE3 |
>> -            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 |
>CPUID_EXT_XTPR,
>> +            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 |
>CPUID_EXT_XTPR |
>> +	    CPUID_EXT_MOVBE,
>>          .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
>
>
>"-machine pc-1.3 -cpu n270" (and older machine-types) needs to keep
>MOVBE disabled, or you will break live migration.
>
>Personally I wouldn't mind declaring n270 as a non-migratable CPU
>model.
>But libvirt supports n270, so it already expects n270 to expose a
>stable
>ABI on each machine-type.
>
>
>>              CPUID_EXT2_NX,
>>          .ext3_features = CPUID_EXT3_LAHF_LM,
>> -- 
>> 1.8.1.2.422.g08c0e7f
>> 
>>
Borislav Petkov - Feb. 8, 2013, 5:35 p.m.
On Fri, Feb 08, 2013 at 01:58:58PM -0200, Eduardo Habkost wrote:

[ … ]

> As we don't have a decent method to do that today, we are using static
> variables and compatibility-setup functions called from the machine init
> function. See disable_kvm_pv_eoi() for example.
> 
> One day we will be able to do that using properties on the machine-type
> compat_props tables, but we can't do that yet.

Ok, understood.

> People can easily work around the lack of the feature today, using
> "-cpu n270,+movbe",

Are you sure?

$ qemu-system-i386 -snapshot ... -cpu n270,+movbe ...

from latest qemu master doesn't seem to work here. We still don't see
bit 22 in ECX of CPUID.EAX(1) advertized to the guest.

But that's not the big problem - we still need the actual implementation
of MOVBE in qemu otherwise the guest kernel #GPs when trying to execute
that instruction.

Thanks.
Eduardo Habkost - Feb. 8, 2013, 6:23 p.m.
On Fri, Feb 08, 2013 at 06:35:56PM +0100, Borislav Petkov wrote:
> On Fri, Feb 08, 2013 at 01:58:58PM -0200, Eduardo Habkost wrote:
> 
> [ … ]
> 
> > As we don't have a decent method to do that today, we are using static
> > variables and compatibility-setup functions called from the machine init
> > function. See disable_kvm_pv_eoi() for example.
> > 
> > One day we will be able to do that using properties on the machine-type
> > compat_props tables, but we can't do that yet.
> 
> Ok, understood.
> 
> > People can easily work around the lack of the feature today, using
> > "-cpu n270,+movbe",
> 
> Are you sure?
> 
> $ qemu-system-i386 -snapshot ... -cpu n270,+movbe ...

Using TCG, right?

> 
> from latest qemu master doesn't seem to work here. We still don't see
> bit 22 in ECX of CPUID.EAX(1) advertized to the guest.

Replace "the lack of the feature" with "the lack of this specific patch"
on my message above. For either your patch or the "+movbe" workaround to
work, you first need the feature to be working.


> 
> But that's not the big problem - we still need the actual implementation
> of MOVBE in qemu otherwise the guest kernel #GPs when trying to execute
> that instruction.

If you don't have the implementation of MOVBE working (and included on
TCG_EXT_FEATURES), neither your patch or "+movbe" can help you.
Borislav Petkov - Feb. 8, 2013, 10 p.m.
On Fri, Feb 08, 2013 at 04:23:04PM -0200, Eduardo Habkost wrote:
> If you don't have the implementation of MOVBE working (and included on
> TCG_EXT_FEATURES), neither your patch or "+movbe" can help you.

Yes, running -cpu n270,+movbe with Richard's patchset merged works.

Thanks.

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9f38e4435e53..83816edd8410 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -610,7 +610,8 @@  static x86_def_t builtin_x86_defs[] = {
             CPUID_ACPI | CPUID_SS | CPUID_HT | CPUID_TM | CPUID_PBE,
             /* Some CPUs got no CPUID_SEP */
         .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 |
-            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR,
+            CPUID_EXT_DSCPL | CPUID_EXT_EST | CPUID_EXT_TM2 | CPUID_EXT_XTPR |
+	    CPUID_EXT_MOVBE,
         .ext2_features = (PPRO_FEATURES & CPUID_EXT2_AMD_ALIASES) |
             CPUID_EXT2_NX,
         .ext3_features = CPUID_EXT3_LAHF_LM,