| Message ID | 1369827271-20921-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Am 29.05.2013 13:34, schrieb Stefano Stabellini: > This fixes a regression introduced by: > > commit 62fc403f11523169eb4264de31279745f48e3ecc > Author: Igor Mammedov <imammedo@redhat.com> > Date: Mon Apr 29 18:54:13 2013 +0200 > > target-i386: Attach ICC bus to CPU on its creation > > X86CPU should have parent bus so it could provide bus for child APIC. > > The commit makes it mandatory to pass a valid ICC bus to cpu_x86_create, > but cpu_x86_init just passes NULL to it. This is not entirely accurate: The ICC bus is only needed for softmmu, the envisioned remaining use case of cpu_x86_init() was bsd/linux-user. And sorry for not catching this use case, I was in a hurry for the Hard Freeze. > xen_machine_pv uses cpu_x86_init, therefore it has been broken. > > This patch fixes the problem by removing the dummy CPU creation > altogether from xen_init_pv, relying on the fact that QEMU can now cope > with a machine without an emulated CPU. > > This fix should be backported to QEMU 1.5. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > CC: imammedo@redhat.com > CC: qemu-stable@nongnu.org Change looks okay, Reviewed-by: Andreas Färber <afaerber@suse.de> But for the future please avoid "this patch" in the commit message (because later on it's a "commit", not a patch) and also please put additional notices such as for backporting below the --- line. Cheers, Andreas > --- > hw/i386/xen_machine_pv.c | 16 ---------------- > 1 files changed, 0 insertions(+), 16 deletions(-) > > diff --git a/hw/i386/xen_machine_pv.c b/hw/i386/xen_machine_pv.c > index f829a52..9f2e291 100644 > --- a/hw/i386/xen_machine_pv.c > +++ b/hw/i386/xen_machine_pv.c > @@ -23,7 +23,6 @@ > */ > > #include "hw/hw.h" > -#include "hw/i386/pc.h" > #include "hw/boards.h" > #include "hw/xen/xen_backend.h" > #include "xen_domainbuild.h" > @@ -31,27 +30,12 @@ > > static void xen_init_pv(QEMUMachineInitArgs *args) > { > - const char *cpu_model = args->cpu_model; > const char *kernel_filename = args->kernel_filename; > const char *kernel_cmdline = args->kernel_cmdline; > const char *initrd_filename = args->initrd_filename; > - X86CPU *cpu; > - CPUState *cs; > DriveInfo *dinfo; > int i; > > - /* Initialize a dummy CPU */ > - if (cpu_model == NULL) { > -#ifdef TARGET_X86_64 > - cpu_model = "qemu64"; > -#else > - cpu_model = "qemu32"; > -#endif > - } > - cpu = cpu_x86_init(cpu_model); > - cs = CPU(cpu); > - cs->halted = 1; > - > /* Initialize backend core & drivers */ > if (xen_be_init() != 0) { > fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__); >
Il 29/05/2013 14:11, Andreas Färber ha scritto: >> xen_machine_pv uses cpu_x86_init, therefore it has been broken. >> >> This patch fixes the problem by removing the dummy CPU creation >> altogether from xen_init_pv, relying on the fact that QEMU can now cope >> with a machine without an emulated CPU. >> >> This fix should be backported to QEMU 1.5. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> CC: imammedo@redhat.com >> CC: qemu-stable@nongnu.org > > Change looks okay, > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > But for the future please avoid "this patch" in the commit message > (because later on it's a "commit", not a patch) and also please put > additional notices such as for backporting below the --- line. I read it as a warning to backporters that the patch, even though it may apply, was not tested and may even be wrong in 1.4. Paolo
On Wed, 29 May 2013, Paolo Bonzini wrote: > Il 29/05/2013 14:11, Andreas Färber ha scritto: > >> xen_machine_pv uses cpu_x86_init, therefore it has been broken. > >> > >> This patch fixes the problem by removing the dummy CPU creation > >> altogether from xen_init_pv, relying on the fact that QEMU can now cope > >> with a machine without an emulated CPU. > >> > >> This fix should be backported to QEMU 1.5. > >> > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> CC: imammedo@redhat.com > >> CC: qemu-stable@nongnu.org > > > > Change looks okay, > > > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > > > But for the future please avoid "this patch" in the commit message > > (because later on it's a "commit", not a patch) and also please put > > additional notices such as for backporting below the --- line. > > I read it as a warning to backporters that the patch, even though it may > apply, was not tested and may even be wrong in 1.4. Yes, that's right. In any case Andreas has a point about moving notes about backports or patch revision changes under the --- line, I just have to find a way to make it work with guilt (that would also cut those notes from the commit messages therefore when I generate the patches they would go missing).
On Wed, 29 May 2013 14:29:51 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 29/05/2013 14:11, Andreas Färber ha scritto: > >> xen_machine_pv uses cpu_x86_init, therefore it has been broken. > >> > >> This patch fixes the problem by removing the dummy CPU creation > >> altogether from xen_init_pv, relying on the fact that QEMU can now cope > >> with a machine without an emulated CPU. > >> > >> This fix should be backported to QEMU 1.5. > >> > >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> CC: imammedo@redhat.com > >> CC: qemu-stable@nongnu.org > > > > Change looks okay, > > > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > > > But for the future please avoid "this patch" in the commit message > > (because later on it's a "commit", not a patch) and also please put > > additional notices such as for backporting below the --- line. > > I read it as a warning to backporters that the patch, even though it may > apply, was not tested and may even be wrong in 1.4. it only needs to be backported to 1.5, 1.4 is not affected. > > Paolo >
diff --git a/hw/i386/xen_machine_pv.c b/hw/i386/xen_machine_pv.c index f829a52..9f2e291 100644 --- a/hw/i386/xen_machine_pv.c +++ b/hw/i386/xen_machine_pv.c @@ -23,7 +23,6 @@ */ #include "hw/hw.h" -#include "hw/i386/pc.h" #include "hw/boards.h" #include "hw/xen/xen_backend.h" #include "xen_domainbuild.h" @@ -31,27 +30,12 @@ static void xen_init_pv(QEMUMachineInitArgs *args) { - const char *cpu_model = args->cpu_model; const char *kernel_filename = args->kernel_filename; const char *kernel_cmdline = args->kernel_cmdline; const char *initrd_filename = args->initrd_filename; - X86CPU *cpu; - CPUState *cs; DriveInfo *dinfo; int i; - /* Initialize a dummy CPU */ - if (cpu_model == NULL) { -#ifdef TARGET_X86_64 - cpu_model = "qemu64"; -#else - cpu_model = "qemu32"; -#endif - } - cpu = cpu_x86_init(cpu_model); - cs = CPU(cpu); - cs->halted = 1; - /* Initialize backend core & drivers */ if (xen_be_init() != 0) { fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
This fixes a regression introduced by: commit 62fc403f11523169eb4264de31279745f48e3ecc Author: Igor Mammedov <imammedo@redhat.com> Date: Mon Apr 29 18:54:13 2013 +0200 target-i386: Attach ICC bus to CPU on its creation X86CPU should have parent bus so it could provide bus for child APIC. The commit makes it mandatory to pass a valid ICC bus to cpu_x86_create, but cpu_x86_init just passes NULL to it. xen_machine_pv uses cpu_x86_init, therefore it has been broken. This patch fixes the problem by removing the dummy CPU creation altogether from xen_init_pv, relying on the fact that QEMU can now cope with a machine without an emulated CPU. This fix should be backported to QEMU 1.5. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: imammedo@redhat.com CC: qemu-stable@nongnu.org --- hw/i386/xen_machine_pv.c | 16 ---------------- 1 files changed, 0 insertions(+), 16 deletions(-)