Re: [Qemu-devel] [PATCH v2 5/5] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 30 October 2015 at 05:35, Peter Crosthwaite
<address@hidden> wrote:
> Firstly, enable monitor mode and PSCI, both are which are features of
> this board.
>
> In addition to PSCI, this board also uses SMC for cache maintainence
> ops. This means we need a secure monitor to catch these and nop them.
> Use the ARM boot board-setup feature to implement this. All traps to
> monitor mode implement the nop.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> Changed since v1:
> fallthrough all of trap table to nop implementation
> use movw for table address
> leave loader at 0
> Move MVBAR (and blob to non-zero)
> Split nop implementation from MVBAR setup
> set secure boot for board
> implement NS switch in blob
> Changed since RFC:
> Use bootloader callback to load blob.
> Change "firmware" to "board-setup" for consistency.
> Tweak commit message.
>
>  hw/arm/highbank.c | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index be04b27..f3578a3 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -32,10 +32,55 @@
>  #define SMP_BOOT_REG            0x40
>  #define MPCORE_PERIPHBASE       0xfff10000
>
> +#define MVBAR_ADDR              0x200
> +
>  #define NIRQ_GIC                160
>
>  /* Board init.  */
>
> +/* MVBAR_ADDR is limited by precision of movw */
> +
> +QEMU_BUILD_BUG_ON(MVBAR_ADDR >= (1 << 16));
> +
> +#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
> +                        extract32((x), 12,  4) << 16)
> +
> +static void hb_write_board_setup(ARMCPU *cpu,
> +                                 const struct arm_boot_info *info)
> +{
> +    int n;
> +    uint32_t board_setup_blob[] = {
> +        /* MVBAR_ADDR */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +#define ERET_ADDR (MVBAR_ADDR + 8 * sizeof(uint32_t))
> +        0xe58fe008, /* save lr */
> +        0xe8dfc000, /* exception return */
> +        0,
> +        0,
> +        0, /* exception return link will end up here */
> +#define BOARD_SETUP_ADDR (ERET_ADDR + 5 * sizeof(uint32_t))
> +        0xe3000000 + ARMV7_IMM16(MVBAR_ADDR), /* movw r0, MVBAR_ADDR */
> +        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
> +        0xee110f11, /* mrc p15, 0, r0, c1 , c1, 0 - get scr */
> +        0xe3810001, /* orr r0, #1 - set NS */
> +        0xee010f11, /* mcr p15, 0, r0, c1 , c1, 0 - set scr */
> +        0xe1600070, /* smc - go to monitor mode to flush NS change */
> +        0xe12fff1e, /* bx lr - return to caller */
> +    };

This still confuses me. What I was expecting to see was something like:

    /* Monitor mode vector table; entry points which will only be reached
     * if the guest kernel is buggy are tight loops to avoid executing
     * random code by accident. The SMC vector entrypoint just returns,
     * making all SMC calls operate as NOPs.
     */
    0xwhatever, /* notused1: b notused1 */
    0xwhatever, /* notused2: b notused2 */
    0xwhatever, /* smc: movs pc, lr */
    0xwhatever, /* prefetch_abort: b prefetch_abort */
    0xwhatever, /* data_abort: b data_abort */
    0xwhatever, /* notused3: b notused3 */
    0xwhatever, /* irq: b irq */
    0xwhatever, /* fiq: b fiq */
    /* Entry point for reset (set via board_setup_addr) */
    [code here to set mvbar, etc]

Note that MOVS PC, LR is much simpler for exception return than RFE,
because you don't need to mess about saving the LR and SPSR to
memory in order to just read them back again. I'd have used ERET,
which has a more explanatory mnemonic, but that only exists in
CPUs with the virtualization extensions.

> +    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
> +        board_setup_blob[n] = tswap32(board_setup_blob[n]);
> +    }
> +    rom_add_blob_fixed("board-setup", board_setup_blob,
> +                       sizeof(board_setup_blob), MVBAR_ADDR);
> +}
> +
>  static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
>  {
>      int n;
> @@ -248,16 +293,13 @@ static void calxeda_init(MachineState *machine, enum 
> cxmachines machine_id)
>          cpuobj = object_new(object_class_get_name(oc));
>          cpu = ARM_CPU(cpuobj);
>
> -        /* By default A9 and A15 CPUs have EL3 enabled.  This board does not
> -         * currently support EL3 so the CPU EL3 property is disabled before
> -         * realization.
> -         */
> -        if (object_property_find(cpuobj, "has_el3", NULL)) {
> -            object_property_set_bool(cpuobj, false, "has_el3", &err);
> -            if (err) {
> -                error_report_err(err);
> -                exit(1);
> -            }
> +        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_SMC,
> +                                "psci-conduit", &error_abort);
> +
> +        if (n) {
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            object_property_set_bool(cpuobj, true,
> +                                     "start-powered-off", &error_abort);
>          }
>
>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
> @@ -378,6 +420,10 @@ static void calxeda_init(MachineState *machine, enum 
> cxmachines machine_id)
>      highbank_binfo.loader_start = 0;
>      highbank_binfo.write_secondary_boot = hb_write_secondary;
>      highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
> +    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
> +    highbank_binfo.write_board_setup = hb_write_board_setup;
> +    highbank_binfo.secure_board_setup = true;
> +
>      arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);

If the user hands us "-cpu something-without-el3" I think we'll
now go blithely ahead with the secure_board_setup with hilarious
consquences, where previously we'd probably at least manage to
try to boot the kernel. (You can argue that the user shouldn't
be handing us silly arguments if you like.)

thanks
-- PMM