Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 25 October 2015 at 23:13, 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.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> Changed since RFC:
> Use bootloader callback to load blob.
> Change "firmware" to "board-setup" for consistency.
> Tweak commit message.
>
>  hw/arm/highbank.c | 57 
> ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index be04b27..98daba0 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -28,6 +28,9 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/error-report.h"
>
> +#define BOARD_SETUP_ADDR        0x0
> +#define LOAD_ADDR               0x1000

Are these obliged to be on different (4K) pages?

> +
>  #define SMP_BOOT_ADDR           0x100
>  #define SMP_BOOT_REG            0x40
>  #define MPCORE_PERIPHBASE       0xfff10000
> @@ -36,6 +39,38 @@
>
>  /* Board init.  */
>
> +static void hb_write_board_setup(ARMCPU *cpu,
> +                                 const struct arm_boot_info *info)
> +{
> +    int n;
> +    uint32_t board_setup_blob[] = {
> +        /* Reset */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        /* smc */

It would probably be a good idea to actually have a full vector
table here, even if the remaining entries just do "jump off
to an infinite-loop error location", so that if the guest is
buggy and causes an exception in early bootup before it's got
its own vector table set up then we do something noticeable
rather than just leaping back into the start of the kernel
boot sequence.

> +        0xe10f0000, /* mrs r0, CPSR */
> +        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
> +        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
> +        /* if (!monitor_mode) { */
> +            0x11600070, /* smcne - go to monitor mode */
> +            0x112fff1e, /* bxne lr - return to caller */
> +        /* } */

This seems a bit weird. At the reset entry point we know we're
not in monitor mode (will be secure SVC). At the SMC entry point
(and indeed every other entry point for MVBAR) we know we are
in monitor mode. So why have a runtime check?

> +        /* do setup from monitor mode */
> +        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR */

This is an unfortunate choice of location for the monitor
vector table, because it turns out to be zero, which will
also be the initial default for the non-monitor vector table.

> +        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
> +        0xe58fe008, /* save lr */
> +        0xe8dfc000, /* exception return */

...doesn't this mean we set the MVBAR for any SMC the guest
does? That seems like an odd way to implement "nop the
cache operations"...

> +        0,
> +        0,
> +        0, /* exception return link will end up here */
> +    };
> +    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), BOARD_SETUP_ADDR);
> +}
> +

thanks
-- PMM