Re: [Qemu-arm] [PATCH v3 7/7] raspi: add raspberry pi 2 machine

[Peter Crosthwaite]

On Tue, Jan 12, 2016 at 3:53 PM, Andrew Baumann
<address@hidden> wrote:
>> From: Peter Crosthwaite [mailto:address@hidden
>> Sent: Monday, 11 January 2016 19:58
> [...]
>> > +static void write_board_setup(ARMCPU *cpu, const struct arm_boot_info
>> *info)
>>
>> This is almost identical to Highbank, I'm guessing you are stubbing monitor
>> firmware where you get away with nopping all the SMCs? Maybe we should
>> split
>> Highbanks version off to common code, and parameterise the few
>> differences.
>>
>> write_board_setup_dummy_monitior(ARMCPU *cpu ..., uint32_t scr_flags);
>>
>> or something like it. Makes sense to be in arm/boot.c .
>
> Actually, I added this only to make Linux happy (and yes, it was derived from 
> highbank). Without it, I was seeing complaints about:
> Ignoring attempt to switch CPSR_A flag from non-secure world with SCR.AW bit 
> clear
> Ignoring attempt to switch CPSR_F flag from non-secure world with SCR.FW bit 
> clear
>
> I don't believe anything actually uses the SMC handler after boot. I think 
> it's just an architectural requirement to flush the change to non-secure mode.
>
> I would prefer not to touch highbank, because I don't know how to test it. Is 
> it better to submit this patch without the board setup?
>

I can test Highbank for you, or you can use this project:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg00755.html

The build takes a while and costs about 50GB of disk space, but the
amount of setup needed should be pretty low.

FWIW, that was the test system that found the need for this FW in HB.

>>
>> > +{
>> > +    static const uint32_t board_setup[] = {
>> > +        /* MVBAR_ADDR: secure monitor vectors */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xE1B0F00E, /* movs pc, lr ;SMC exception return */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        0xEAFFFFFE, /* (spin) */
>> > +        /* BOARDSETUP_ADDR */
>> > +        0xE3A00B01, /* mov     r0, #0x400             ;MVBAR_ADDR */
>> > +        0xEE0C0F30, /* mcr     p15, 0, r0, c12, c0, 1 ;set MVBAR */
>> > +        0xE3A00031, /* mov     r0, #0x31              ;enable AW, FW, NS 
>> > */
>> > +        0xEE010F11, /* mcr     p15, 0, r0, c1, c1, 0  ;write SCR */
>>
>> If combining with HB, could you do this as read-modify-write?
>>
>> > +        0xE1A0100E, /* mov     r1, lr                 ;save LR across SMC 
>> > */
>> > +        0xE1600070, /* smc     #0                     ;monitor call */
>> > +        0xE1A0F001, /* mov     pc, r1                 ;return */
>>
>> I'm looking at the Highbank version which doesn't save lr across the SMC and
>> wondering why it doesn't. Is this banked by CPU mode and do you get from
>> already-in-monitor-mode? Or simply, that Highbank code may have a bug?
>
> I think it's needed because I call the board setup blob on each core (from 
> the smpboot blob), but highbank doesn't. I found that I needed to do this to 
> avoid the above warnings on an SMP boot; I don't know why highbank doesn't.
>
> [...]
>> > +    /* Allocate and map RAM */
>> > +    memory_region_allocate_system_memory(&s->ram,
>> OBJECT(machine), "ram",
>> > +                                         machine->ram_size);
>> > +    memory_region_add_subregion_overlap(get_system_memory(), 0,
>> &s->ram, 0);
>>
>> I thought the SoC handled this now? Why do you need to add to
>> system_memory?
>
> If I don't map it here, how do RAM accesses from the CPUs work?
>

Do the CPUs not have their AS'es connected to your custom ASes by the SoC layer?

> Or are you saying that I should still do this, but do it in the SoC not the 
> board level?
>

I was hoping we could get away with 0 use of system_memory.

Regards,
Peter

> [...]
>> > +static void raspi2_machine_init(MachineClass *mc)
>> > +{
>> > +    mc->desc = "Raspberry Pi 2";
>> > +    mc->init = raspi2_init;
>> > +    mc->block_default_type = IF_SD;
>>
>> > +    mc->no_parallel = 1;
>> > +    mc->no_floppy = 1;
>> > +    mc->no_cdrom = 1;
>>
>> Curious, what do these do from a user-visible point of view? Maybe we
>> should
>> add them to more ARM boards as they certainly make sense.
>
> I think they turn off some redundant stuff in the UI (e.g., there's no 
> View->Parallel menu option). I'm guessing they also disable the -cdrom and  
> -fd* options, but didn't test that.
>
> Cheers,
> Andrew