| Message ID | 1360315802-959-1-git-send-email-bp@alien8.de |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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.
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.
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~
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 >> >>
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.
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.
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.
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,