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

[Peter Crosthwaite]

On Fri, Nov 6, 2015 at 6:47 AM, Peter Maydell <address@hidden> wrote:
> On 3 November 2015 at 04:30, Peter Crosthwaite
> <address@hidden> wrote:
>> Firstly, enable monitor mode and PSCI, both are which are features of
>
> "both of which"
>

Fixed.

>> 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.
>>

Clarified this a little better as since v3 the non-smc traps are
spinning rather than nopping.

>> As a KVM CPU cannot run in secure mode, do not do the board-setup if
>> not running TCG. Report a warning explaining the limitation is this
>> case.
>
> "in this case".

Fixed.

>
>> @@ -371,6 +410,16 @@ 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;
>> +    if (tcg_enabled()) {
>
> This also needs to be !kvm_enabled(), so 'make check' works.
>

Fixed.

>> +        highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
>> +        highbank_binfo.write_board_setup = hb_write_board_setup;
>> +        highbank_binfo.secure_board_setup = true;
>> +    } else {
>> +        error_report("WARNING: TCG unavailable - "
>> +                     "unable to load built-in Monitor support.\n"
>> +                     "Some guests (such as Linux) may not boot\n");
>
> You can't have newlines in an error_report() string. I suggest
>
>         error_report("WARNING: cannot load built-in Monitor support if KVM "
>                      "is enabled. Some guests (such as Linux) may not boot.");
>

Taken verbatim, although I line wrapped earlier due to new guideline
to stay a little back from 80 chars if possible.

> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
>

Thanks.

Regards,
Peter

> thanks
> -- PMM