| Message ID | 1392301617-21844-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 13, 2014 at 8:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Newer versions of the Linux kernel (as of commit bc41b8724 in 3.12) > now assume that if the CPU is a Cortex-A9 and the reset value of the > PERIPHBASE/CBAR register is zero then the CPU is a specific buggy > single core A9 SoC, and will not try to start other cores. Since we > now have a CPU property for the reset value of the CBAR, we can > just fix the vexpress board model to correctly set CBAR so SMP > works again. To avoid duplicate boilerplate code in both the A9 > and A15 daughterboard init functions, we split out the CPU and > private memory region init to its own function. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Rob Herring <rob.herring@linaro.org> > --- > Thanks to Rob for tracking down this SMP boot issue and identifying > the offending kernel change (which personally I think is a terrible > hack, but it's in shipping kernels and our models ought to be > accurate for CBAR anyway). And i was working on the fix as well... > We should probably propagate this fix to all our A9-based models > before 2.0 release: I think the remaining ones to fix would be > realview-pbx-a9 and exynos4210. > > QOM syntax cribbed from the zynq board so I assume it is up to > current standards. > > hw/arm/vexpress.c | 116 +++++++++++++++++++++++++----------------------------- > 1 file changed, 54 insertions(+), 62 deletions(-) > > diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c > index ef1707a..e4ced8f 100644 > --- a/hw/arm/vexpress.c > +++ b/hw/arm/vexpress.c > @@ -32,6 +32,7 @@ > #include "sysemu/blockdev.h" > #include "hw/block/flash.h" > #include "sysemu/device_tree.h" > +#include "qemu/error-report.h" > #include <libfdt.h> > > #define VEXPRESS_BOARD_ID 0x8e0 > @@ -173,6 +174,57 @@ struct VEDBoardInfo { > DBoardInitFn *init; > }; > > +static void init_cpus(const char *cpu_model, const char *privdev, > + hwaddr periphbase, qemu_irq *pic) There is nothing really vexpress specific about this function other than number of irqs. This is really just expanding cpu_arm_init which is the route I was going down. Rob
On 13 February 2014 21:31, Rob Herring <robherring2@gmail.com> wrote: > On Thu, Feb 13, 2014 at 8:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> Newer versions of the Linux kernel (as of commit bc41b8724 in 3.12) >> now assume that if the CPU is a Cortex-A9 and the reset value of the >> PERIPHBASE/CBAR register is zero then the CPU is a specific buggy >> single core A9 SoC, and will not try to start other cores. Since we >> now have a CPU property for the reset value of the CBAR, we can >> just fix the vexpress board model to correctly set CBAR so SMP >> works again. To avoid duplicate boilerplate code in both the A9 >> and A15 daughterboard init functions, we split out the CPU and >> private memory region init to its own function. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Rob Herring <rob.herring@linaro.org> >> --- >> Thanks to Rob for tracking down this SMP boot issue and identifying >> the offending kernel change (which personally I think is a terrible >> hack, but it's in shipping kernels and our models ought to be >> accurate for CBAR anyway). > > And i was working on the fix as well... Ah, sorry. I checked your git repo to see if there was a patch in it, but didn't find anything so I guessed you'd just done a quick "get it working" patch. >> +static void init_cpus(const char *cpu_model, const char *privdev, >> + hwaddr periphbase, qemu_irq *pic) > > There is nothing really vexpress specific about this function other > than number of irqs. This is really just expanding cpu_arm_init which > is the route I was going down. However cpu_arm_init() is in target-arm and has no business instantiating devices. I think the correct long term approach is going to involve the A9 and A15 private peripheral devices instantiating the CPUs themselves. But I figured that two weeks before soft freeze was perhaps not the best time to open that can of worms, hence this patch which is localised to just the machine model code. thanks -- PMM
On Fri, Feb 14, 2014 at 7:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 February 2014 21:31, Rob Herring <robherring2@gmail.com> wrote: >> On Thu, Feb 13, 2014 at 8:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> Newer versions of the Linux kernel (as of commit bc41b8724 in 3.12) >>> now assume that if the CPU is a Cortex-A9 and the reset value of the >>> PERIPHBASE/CBAR register is zero then the CPU is a specific buggy >>> single core A9 SoC, and will not try to start other cores. Since we >>> now have a CPU property for the reset value of the CBAR, we can >>> just fix the vexpress board model to correctly set CBAR so SMP >>> works again. To avoid duplicate boilerplate code in both the A9 >>> and A15 daughterboard init functions, we split out the CPU and >>> private memory region init to its own function. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Rob Herring <rob.herring@linaro.org> >>> --- >>> Thanks to Rob for tracking down this SMP boot issue and identifying >>> the offending kernel change (which personally I think is a terrible >>> hack, but it's in shipping kernels and our models ought to be >>> accurate for CBAR anyway). >> >> And i was working on the fix as well... > > Ah, sorry. I checked your git repo to see if there was a patch > in it, but didn't find anything so I guessed you'd just done a > quick "get it working" patch. > >>> +static void init_cpus(const char *cpu_model, const char *privdev, >>> + hwaddr periphbase, qemu_irq *pic) >> >> There is nothing really vexpress specific about this function other >> than number of irqs. This is really just expanding cpu_arm_init which >> is the route I was going down. > > However cpu_arm_init() is in target-arm and has no > business instantiating devices. I think the correct long > term approach is going to involve the A9 and A15 > private peripheral devices instantiating the CPUs themselves. Agreed. Most of that common code should go away with the migration of CPU instantiation to MPCore, which should happen sooner or later. That will end up naturally share all the code with Zynq, Highbank and friends. Factoring this out to a global helper is backwards step to the old style of qdev-init helpers. Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> I've been thinking about the CPU-mpcore problem, and perhaps the most annoying part of it is propagating the user -cpu argument through to change to CPU model. On several occasions however we have declared this to be largely bogus for ARM. E.G. qemu-system-arm -M zynq -cpu "cortex-a8" doesn't really make any sense. So going into the release (which has a major revision bump last I knew), can we defeature the -cpu syntax at least for the MPCore boards, if not all ARM. If you want to BYO cpu, then it should be done with regular -device style mechanisms (some patches needed). Regards, Peter > But I figured that two weeks before soft freeze was perhaps > not the best time to open that can of worms, hence this patch > which is localised to just the machine model code. > > thanks > -- PMM >
On 13 February 2014 23:39, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > I've been thinking about the CPU-mpcore problem, and perhaps the most > annoying part of it is propagating the user -cpu argument through to > change to CPU model. On several occasions however we have declared > this to be largely bogus for ARM. E.G. > > qemu-system-arm -M zynq -cpu "cortex-a8" > > doesn't really make any sense. So going into the release (which has a > major revision bump last I knew), can we defeature the -cpu syntax at > least for the MPCore boards, if not all ARM. If you want to BYO cpu, > then it should be done with regular -device style mechanisms (some > patches needed). Yes, I think that makes sense: anything with a set of private peripherals really expects to see the right set, not some random other set, and any real board with a real SoC expects to see that SoC's CPU, not some random other thing. We should probably provide a helpful error message rather than totally ignoring -cpu. If you want to write patches to do that I'll accept them. (The 'virt' board is the odd one out here given that it picks the private peripheral set to match the specified CPU. But 'virt' is odd in lots of ways.) thanks -- PMM
On 13 February 2014 14:26, Peter Maydell <peter.maydell@linaro.org> wrote: > +static void init_cpus(const char *cpu_model, const char *privdev, > + hwaddr periphbase, qemu_irq *pic) > +{ > + ObjectClass *cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); > + DeviceState *dev; > + SysBusDevice *busdev; > + int n; > + > + if (!cpu_oc) { > + fprintf(stderr, "Unable to find CPU definition\n"); > + exit(1); > + } > + > + /* Create the private peripheral devices (including the GIC) */ > + dev = qdev_create(NULL, privdev); > + qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); > + qdev_init_nofail(dev); > + busdev = SYS_BUS_DEVICE(dev); > + sysbus_mmio_map(busdev, 0, periphbase); This makes vexpress-a15 segfault on startup, because it creates the a15mpcore_priv device first and the CPUs afterwards. That's a problem because a15mpcore_priv's realize function iterates through all the CPUs to connect their generic timer output GPIO lines to the appropriate input lines on the GIC. So the CPU has to be created first. I'll do a respin that moves the "create and realize CPUs" bit of the loop to the top of this function. thanks -- PMM
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index ef1707a..e4ced8f 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -32,6 +32,7 @@ #include "sysemu/blockdev.h" #include "hw/block/flash.h" #include "sysemu/device_tree.h" +#include "qemu/error-report.h" #include <libfdt.h> #define VEXPRESS_BOARD_ID 0x8e0 @@ -173,6 +174,57 @@ struct VEDBoardInfo { DBoardInitFn *init; }; +static void init_cpus(const char *cpu_model, const char *privdev, + hwaddr periphbase, qemu_irq *pic) +{ + ObjectClass *cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); + DeviceState *dev; + SysBusDevice *busdev; + int n; + + if (!cpu_oc) { + fprintf(stderr, "Unable to find CPU definition\n"); + exit(1); + } + + /* Create the private peripheral devices (including the GIC) */ + dev = qdev_create(NULL, privdev); + qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); + qdev_init_nofail(dev); + busdev = SYS_BUS_DEVICE(dev); + sysbus_mmio_map(busdev, 0, periphbase); + + /* Interrupts [42:0] are from the motherboard; + * [47:43] are reserved; [63:48] are daughterboard + * peripherals. Note that some documentation numbers + * external interrupts starting from 32 (because there + * are internal interrupts 0..31). + */ + for (n = 0; n < 64; n++) { + pic[n] = qdev_get_gpio_in(dev, n); + } + + /* Create the actual CPUs and connect them to the GIC */ + for (n = 0; n < smp_cpus; n++) { + Object *cpuobj = object_new(object_class_get_name(cpu_oc)); + Error *err = NULL; + + object_property_set_int(cpuobj, periphbase, "reset-cbar", &err); + if (err) { + error_report("%s", error_get_pretty(err)); + exit(1); + } + object_property_set_bool(cpuobj, true, "realized", &err); + if (err) { + error_report("%s", error_get_pretty(err)); + exit(1); + } + + sysbus_connect_irq(busdev, n, + qdev_get_gpio_in(DEVICE(cpuobj), ARM_CPU_IRQ)); + } +} + static void a9_daughterboard_init(const VEDBoardInfo *daughterboard, ram_addr_t ram_size, const char *cpu_model, @@ -181,25 +233,12 @@ static void a9_daughterboard_init(const VEDBoardInfo *daughterboard, MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); MemoryRegion *lowram = g_new(MemoryRegion, 1); - DeviceState *dev; - SysBusDevice *busdev; - int n; - qemu_irq cpu_irq[4]; ram_addr_t low_ram_size; if (!cpu_model) { cpu_model = "cortex-a9"; } - for (n = 0; n < smp_cpus; n++) { - ARMCPU *cpu = cpu_arm_init(cpu_model); - if (!cpu) { - fprintf(stderr, "Unable to find CPU definition\n"); - exit(1); - } - cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ); - } - if (ram_size > 0x40000000) { /* 1GB is the maximum the address space permits */ fprintf(stderr, "vexpress-a9: cannot model more than 1GB RAM\n"); @@ -221,23 +260,7 @@ static void a9_daughterboard_init(const VEDBoardInfo *daughterboard, memory_region_add_subregion(sysmem, 0x60000000, ram); /* 0x1e000000 A9MPCore (SCU) private memory region */ - dev = qdev_create(NULL, "a9mpcore_priv"); - qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); - qdev_init_nofail(dev); - busdev = SYS_BUS_DEVICE(dev); - sysbus_mmio_map(busdev, 0, 0x1e000000); - for (n = 0; n < smp_cpus; n++) { - sysbus_connect_irq(busdev, n, cpu_irq[n]); - } - /* Interrupts [42:0] are from the motherboard; - * [47:43] are reserved; [63:48] are daughterboard - * peripherals. Note that some documentation numbers - * external interrupts starting from 32 (because the - * A9MP has internal interrupts 0..31). - */ - for (n = 0; n < 64; n++) { - pic[n] = qdev_get_gpio_in(dev, n); - } + init_cpus(cpu_model, "a9mpcore_priv", 0x1e000000, pic); /* Daughterboard peripherals : 0x10020000 .. 0x20000000 */ @@ -296,29 +319,14 @@ static void a15_daughterboard_init(const VEDBoardInfo *daughterboard, const char *cpu_model, qemu_irq *pic) { - int n; MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); MemoryRegion *sram = g_new(MemoryRegion, 1); - qemu_irq cpu_irq[4]; - DeviceState *dev; - SysBusDevice *busdev; if (!cpu_model) { cpu_model = "cortex-a15"; } - for (n = 0; n < smp_cpus; n++) { - ARMCPU *cpu; - - cpu = cpu_arm_init(cpu_model); - if (!cpu) { - fprintf(stderr, "Unable to find CPU definition\n"); - exit(1); - } - cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ); - } - { /* We have to use a separate 64 bit variable here to avoid the gcc * "comparison is always false due to limited range of data type" @@ -337,23 +345,7 @@ static void a15_daughterboard_init(const VEDBoardInfo *daughterboard, memory_region_add_subregion(sysmem, 0x80000000, ram); /* 0x2c000000 A15MPCore private memory region (GIC) */ - dev = qdev_create(NULL, "a15mpcore_priv"); - qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); - qdev_init_nofail(dev); - busdev = SYS_BUS_DEVICE(dev); - sysbus_mmio_map(busdev, 0, 0x2c000000); - for (n = 0; n < smp_cpus; n++) { - sysbus_connect_irq(busdev, n, cpu_irq[n]); - } - /* Interrupts [42:0] are from the motherboard; - * [47:43] are reserved; [63:48] are daughterboard - * peripherals. Note that some documentation numbers - * external interrupts starting from 32 (because there - * are internal interrupts 0..31). - */ - for (n = 0; n < 64; n++) { - pic[n] = qdev_get_gpio_in(dev, n); - } + init_cpus(cpu_model, "a15mpcore_priv", 0x2c000000, pic); /* A15 daughterboard peripherals: */
Newer versions of the Linux kernel (as of commit bc41b8724 in 3.12) now assume that if the CPU is a Cortex-A9 and the reset value of the PERIPHBASE/CBAR register is zero then the CPU is a specific buggy single core A9 SoC, and will not try to start other cores. Since we now have a CPU property for the reset value of the CBAR, we can just fix the vexpress board model to correctly set CBAR so SMP works again. To avoid duplicate boilerplate code in both the A9 and A15 daughterboard init functions, we split out the CPU and private memory region init to its own function. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reported-by: Rob Herring <rob.herring@linaro.org> --- Thanks to Rob for tracking down this SMP boot issue and identifying the offending kernel change (which personally I think is a terrible hack, but it's in shipping kernels and our models ought to be accurate for CBAR anyway). We should probably propagate this fix to all our A9-based models before 2.0 release: I think the remaining ones to fix would be realview-pbx-a9 and exynos4210. QOM syntax cribbed from the zynq board so I assume it is up to current standards. hw/arm/vexpress.c | 116 +++++++++++++++++++++++++----------------------------- 1 file changed, 54 insertions(+), 62 deletions(-)