Re: [PATCH 4/5] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Cédric Le Goater]

On 1/7/20 9:34 AM, Philippe Mathieu-Daudé wrote:
> On 1/7/20 8:34 AM, Cédric Le Goater wrote:
>> The overhead for the OpenBMC firmware images using the a custom U-Boot
>> is around 2 seconds, which is fine, but with a U-Boot from mainline,
>> it takes an extra 50 seconds or so to reach Linux. A quick survey on
>> the number of reads performed on the flash memory region gives the
>> following figures :
>>
>>    OpenBMC U-Boot      922478 (~ 3.5 MBytes)
>>    Mainline U-Boot   20569977 (~ 80  MBytes)
>>
>> QEMU must be trashing the TCG TBs and reloading text very often. Some
>> addresses are read more than 250.000 times. Until we find a solution
>> to improve boot time, execution from MMIO is not activated by default.
>>
>> Setting this option also breaks migration compatibility.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> Reviewed-by: Peter Maydell <address@hidden>
>> ---
>>   include/hw/arm/aspeed.h |  2 ++
>>   hw/arm/aspeed.c         | 44 ++++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index 4423cd0cda71..18521484b90e 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -19,6 +19,8 @@ typedef struct AspeedBoardState AspeedBoardState;
>>     typedef struct AspeedMachine {
>>       MachineState parent_obj;
>> +
>> +    bool mmio_exec;
>>   } AspeedMachine;
>>     #define ASPEED_MACHINE_CLASS(klass) \
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 0a7dfd29868b..bf23579fa53d 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -260,11 +260,18 @@ static void aspeed_machine_init(MachineState *machine)
>>            * SoC and 128MB for the AST2500 SoC, which is twice as big as
>>            * needed by the flash modules of the Aspeed machines.
>>            */
>> -        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> -                               fl->size, &error_abort);
>> -        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> -                                    boot_rom);
>> -        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>> +        if (ASPEED_MACHINE(machine)->mmio_exec) {
>> +            memory_region_init_alias(boot_rom, OBJECT(bmc), 
>> "aspeed.boot_rom",
>> +                                     &fl->mmio, 0, fl->size);
>> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> +                                        boot_rom);
>> +        } else {
>> +            memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> +                                   fl->size, &error_abort);
>> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> +                                        boot_rom);
>> +            write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>> +        }
> 
> Nitpick: I'd use rom_add_file_mr() in write_boot_rom(), and keep the 
> memory_region_add_subregion() call after the if/else statement.
> 
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

I would rather address changes in follow-up  patches.

Thanks for the hint. 

C. 

> 
>>       }
>>         aspeed_board_binfo.ram_size = ram_size;
>> @@ -398,6 +405,30 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
>> *bmc)
>>       /* Bus 11: TODO ucd90160@64 */
>>   }
>>   +static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
>> +{
>> +    return ASPEED_MACHINE(obj)->mmio_exec;
>> +}
>> +
>> +static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
>> +{
>> +    ASPEED_MACHINE(obj)->mmio_exec = value;
>> +}
>> +
>> +static void aspeed_machine_instance_init(Object *obj)
>> +{
>> +    ASPEED_MACHINE(obj)->mmio_exec = false;
>> +}
>> +
>> +static void aspeed_machine_class_props_init(ObjectClass *oc)
>> +{
>> +    object_class_property_add_bool(oc, "execute-in-place",
>> +                                   aspeed_get_mmio_exec,
>> +                                   aspeed_set_mmio_exec, &error_abort);
>> +    object_class_property_set_description(oc, "execute-in-place",
>> +                           "boot directly from CE0 flash device", 
>> &error_abort);
>> +}
>> +
>>   static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -407,6 +438,8 @@ static void aspeed_machine_class_init(ObjectClass *oc, 
>> void *data)
>>       mc->no_floppy = 1;
>>       mc->no_cdrom = 1;
>>       mc->no_parallel = 1;
>> +
>> +    aspeed_machine_class_props_init(oc);
>>   }
>>     static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void 
>> *data)
>> @@ -549,6 +582,7 @@ static const TypeInfo aspeed_machine_types[] = {
>>           .name          = TYPE_ASPEED_MACHINE,
>>           .parent        = TYPE_MACHINE,
>>           .instance_size = sizeof(AspeedMachine),
>> +        .instance_init = aspeed_machine_instance_init,
>>           .class_size    = sizeof(AspeedMachineClass),
>>           .class_init    = aspeed_machine_class_init,
>>           .abstract      = true,
>>
>