On Tue, Oct 27, 2015 at 5:26 AM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On 25 October 2015 at 23:13, Peter Crosthwaite
> <crosthwaitepe...@gmail.com> 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 <crosthwaite.pe...@gmail.com>
> > ---
> > 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?
>
No. 0x1000 was just picked as a nice round number.
>
> > +
> > #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
>
What could we to that is noticeable to the guest?
> 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?
>
>
It is handling both the initial setup and nopping at the same time. It
could be done split but this allowed a solutions without a jump table,
which is hard to maintain in machine code.
> > + /* 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.
>
>
Linux can manage this so I went this was for the simplicity of an
all-in-one table. Otherwise I need to have two tables. One to trap the
first SMC to 0x8 which then sets up MVBAR, then another table for the
runtime nop.
> > + 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"...
>
Yes.
So to fix I will have two tables (in the same blob), the 0 table will
unconditionally smc from reset, then set MVBAR to the MVBAR table. Then the
0 table needs to corrupt itself to some default before returning to to boot
blob. Open to suggestions on what the contents of that 0 table should be
post monitor setup. The MVBAR table then erets from every vector.
Regards,
Peter
>
> > + 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
>
