Patchwork [Xen-devel] target-i386: Introduce ICC bus/device/bridge

login
register
mail settings
Submitter Stefano Stabellini
Date May 28, 2013, 1:44 p.m.
Message ID <alpine.DEB.2.02.1305281358150.4799@kaball.uk.xensource.com>
Download mbox | patch
Permalink /patch/246865/
State New
Headers show

Comments

Stefano Stabellini - May 28, 2013, 1:44 p.m.
On Tue, 28 May 2013, Andreas Färber wrote:
> Am 28.05.2013 13:49, schrieb Stefano Stabellini:
> > On Mon, 27 May 2013, Igor Mammedov wrote:
> >> On Fri, 24 May 2013 08:56:14 -0600
> >> jacek burghardt <jaceksburghardt@gmail.com> wrote:
> >>
> >>> I wonder if anyone has patch that allows to tlak to icc bus introduced in
> >>> qemu upstream ?
> >>> https://github.com/qemu/qemu/commit/f0513d2c0156799e0c75a108ab9a049eea4f9607
> >>>
> >>>  icc-bridge will serve as a parent for icc-bus and provide
> >>>   mmio mapping services to child icc-devices.
> >>> * icc-device will replace SysBusDevice as a parent of APIC
> >>>   and IOAPIC devices.
> >>
> >> looking at xen_init_pv() it creates dummy CPU which appears
> >> not to be used by PV guest. What was the purpose in creating it?
> >>
> > 
> > I think that the purpose used to be keeping the rest of QEMU happy,
> > because it used to assume that a CPU was being emulated. As a matter of
> > fact the Xen PV machine does not actually emulate anything, especially
> > it doesn't emulate the CPU.
> > Nowadays QEMU is capable of handling machines without cpus, so I suggest
> > removing this code from xen_init_pv altogether.
> > 
> > jacek, can you please confirm that the patch below solves your problem?
> 
> That's based on top of your other patches though, right? Accessing
> first_cpu with no CPU created is doomed to fail.

That is done for HVM guests (pc_xen_hvm_init and xen-all.c), while this
code is for PV guests (xen_machine_pv.c).


> qtest does create CPUs, it just doesn't execute them.
> 
> Not arguing against this, just cautioning that additional NULL checks
> may be needed elsewhere.

That's a good point.

I actually tested it and seems to work fine but I wouldn't want to paint
me into a corner by setting up a new use case that nobody else is
testing.
On the other hand I dislike having to create a "useless" x86 cpu, that
at the moment doesn't even work because cpu_x86_init seems to be broken
by the lack of the ICC bridge.

Overall if the QEMU machine model supports it (and it looks like it
does), I would rather choose the "no cpu" option.

See below the uglier alternative:

---
Paolo Bonzini - May 28, 2013, 2:12 p.m.
Il 28/05/2013 15:44, Stefano Stabellini ha scritto:
> 
> I actually tested it and seems to work fine but I wouldn't want to paint
> me into a corner by setting up a new use case that nobody else is
> testing.
> On the other hand I dislike having to create a "useless" x86 cpu, that
> at the moment doesn't even work because cpu_x86_init seems to be broken
> by the lack of the ICC bridge.
> 
> Overall if the QEMU machine model supports it (and it looks like it
> does), I would rather choose the "no cpu" option.

I also prefer this.  I wonder if this change would make xen_machine_pv.c
work for any architecture rather than just i386, at least for xen_mode
== XEN_ATTACH.

Paolo
Stefano Stabellini - May 28, 2013, 3:14 p.m.
On Tue, 28 May 2013, Paolo Bonzini wrote:
> Il 28/05/2013 15:44, Stefano Stabellini ha scritto:
> > 
> > I actually tested it and seems to work fine but I wouldn't want to paint
> > me into a corner by setting up a new use case that nobody else is
> > testing.
> > On the other hand I dislike having to create a "useless" x86 cpu, that
> > at the moment doesn't even work because cpu_x86_init seems to be broken
> > by the lack of the ICC bridge.
> > 
> > Overall if the QEMU machine model supports it (and it looks like it
> > does), I would rather choose the "no cpu" option.
> 
> I also prefer this.  I wonder if this change would make xen_machine_pv.c
> work for any architecture rather than just i386, at least for xen_mode
> == XEN_ATTACH.

Right, that is might goal. I want to have xen_machine_pv on ARM.

Patch

diff --git a/hw/i386/xen_machine_pv.c b/hw/i386/xen_machine_pv.c
index f829a52..260b211 100644
--- a/hw/i386/xen_machine_pv.c
+++ b/hw/i386/xen_machine_pv.c
@@ -22,6 +22,7 @@ 
  * THE SOFTWARE.
  */
 
+#include "hw/cpu/icc_bus.h"
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
 #include "hw/boards.h"
@@ -35,10 +36,9 @@  static void xen_init_pv(QEMUMachineInitArgs *args)
     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;
+    DeviceState *icc_bridge;
 
     /* Initialize a dummy CPU */
     if (cpu_model == NULL) {
@@ -48,9 +48,10 @@  static void xen_init_pv(QEMUMachineInitArgs *args)
         cpu_model = "qemu32";
 #endif
     }
-    cpu = cpu_x86_init(cpu_model);
-    cs = CPU(cpu);
-    cs->halted = 1;
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+    pc_cpus_init(cpu_model, icc_bridge);
 
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {