Re: [Qemu-devel] [PATCH arm-devs v2 5/8] arm/highbank: Fix CBAR intialisation

[Peter Maydell <peter.maydell@linaro.org>]

On 28 November 2013 03:29, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Fix the CBAR initialisation by using the newly defined static property.
> CBAR is now set before realization, so the intended value is now
> actually used.
>
> So I have kinda tested this. I booted an ARM kernel on Highbank with the
> stock Highbank DTB. It doesnt boot (and I will be doing something
> wrong), but before this patch I got this:
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at /workspaces/pcrost/public/linux2.git/arch/arm/mm/ioremap.c:301 __arm_ioremap_pfn_caller+0x180/0x198()
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W 3.13.0-rc1-next-20131126-dirty #2
> [<c0015164>] (unwind_backtrace) from [<c00118c0>] (show_stack+0x10/0x14)
> [<c00118c0>] (show_stack) from [<c02bd5fc>] (dump_stack+0x78/0x90)
> [<c02bd5fc>] (dump_stack) from [<c001f110>] (warn_slowpath_common+0x68/0x84)
> [<c001f110>] (warn_slowpath_common) from [<c001f1f4>] (warn_slowpath_null+0x1c/0x24)
> [<c001f1f4>] (warn_slowpath_null) from [<c0017c6c>] (__arm_ioremap_pfn_caller+0x180/0x198)
> [<c0017c6c>] (__arm_ioremap_pfn_caller) from [<c0017cd8>] (__arm_ioremap_caller+0x54/0x5c)
> [<c0017cd8>] (__arm_ioremap_caller) from [<c0017d10>] (__arm_ioremap+0x18/0x1c)
> [<c0017d10>] (__arm_ioremap) from [<c03913c0>] (highbank_init_irq+0x34/0x8c)
> [<c03913c0>] (highbank_init_irq) from [<c038c228>] (init_IRQ+0x28/0x2c)
> [<c038c228>] (init_IRQ) from [<c03899ec>] (start_kernel+0x234/0x398)
> [<c03899ec>] (start_kernel) from [<00008074>] (0x8074)
> ---[ end trace 3406ff24bd97382f ]---
>
> Which dissappears with this patch.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1:
> use error report rather than fprintf(stderr
>
>  hw/arm/highbank.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 1d19d8f..cb32325 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -236,14 +236,16 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
>
>          cpu = ARM_CPU(object_new(object_class_get_name(oc)));
>
> +        object_property_set_int(OBJECT(cpu), GIC_BASE_ADDR, "reset-cbar", &err);
> +        if (err) {
> +            error_report("%s", error_get_pretty(err));
> +            exit(1);
> +        }

It's kind of sad that we need five lines of code just to
say "set this property on this object which we should
know at compile time to exist" . (I was about to argue we
should just assert, but since we don't refuse to start with
wacky user-provided cpu-model strings we can't quite get
away with that.)

More significantly, this will break midway, because
cortex-a15 doesn't have this property. Fortunately, the
actual A15 does have a CBAR register so you can just
add a patch to set the feature bit for it.

Caution, this means our semantics for reset-cbar
are "actual reset value of register", which is not
the same as "base address of peripherals", because
for A15 the register has
 [31:15] bits 31:15 of base-addr
 [14:8] reserved, zero
 [7:0] bits 39:32 of base-addr

which makes a difference if you were nutty enough to
put the GIC above the 4GB boundary.

(As with the real hardware, setting this config property
doesn't actually change where the GIC & friends live
in the address space, incidentally.)

>          object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>          if (err) {
>              error_report("%s", error_get_pretty(err));
>              exit(1);
>          }
> -
> -        /* This will become a QOM property eventually */
> -        cpu->reset_cbar = GIC_BASE_ADDR;
>          cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ);
>      }

thanks
-- PMM