Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Peter Maydell]

On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Peter Maydell]

On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
> The FMC controller on the Aspeed SoCs support DMA to access the flash
> modules. It can operate in a normal mode, to copy to or from the flash
> module mapping window, or in a checksum calculation mode, to evaluate
> the best clock settings for reads.
>
> The model introduces a custom address space for DMAs populated with
> the required regions : an alias region on the AHB window for the flash
> devices and another alias on the SDRAM.
>
> Our primary need is to support the checksum calculation mode and the
> model only implements synchronous DMA accesses. Something to improve
> in the future.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>


> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
> +{
> +    MemTxResult result;
> +    uint32_t data;
> +
> +    while (s->regs[R_DMA_LEN]) {
> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> +            result = address_space_read(&s->dma_as, s->regs[R_DMA_DRAM_ADDR],
> +                                        MEMTXATTRS_UNSPECIFIED,
> +                                        (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
> +                return;
> +            }

Does the device really not report DMA read/write failures via
a status register bit or similar ?


> +
> +/*
> + * Populate our custom address space for DMAs with only the regions we
> + * need : the AHB window for the flash devices and the SDRAM.
> + */
> +static void aspeed_smc_dma_setup(AspeedSMCState *s)
> +{
> +    char name[32];
> +    MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
> +    MemoryRegion *sdram_alias = g_new(MemoryRegion, 1);
> +
> +    snprintf(name, sizeof(name), "%s-dma", s->ctrl->name);

I would suggest using g_strdup_printf()/g_free(), since it's not
immediately obvious here that s->ctrl->name is guaranteed
to fit into the fixed-size array.

> +    memory_region_init(&s->dma_mr, OBJECT(s), name,
> +                       s->sdram_base + s->max_ram_size);
> +    address_space_init(&s->dma_as, &s->dma_mr, name);
> +
> +    snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
> +    memory_region_init_alias(flash_alias, OBJECT(s), name, &s->mmio_flash,
> +                             0, s->ctrl->flash_window_size);
> +    memory_region_add_subregion(&s->dma_mr, s->ctrl->flash_window_base,
> +                                flash_alias);
> +
> +    memory_region_init_alias(sdram_alias, OBJECT(s), "ram", sysmem,
> +                             s->sdram_base, s->max_ram_size);
> +    memory_region_add_subregion(&s->dma_mr, s->sdram_base, sdram_alias);

Rather than having the DMA device directly grab the system_memory
MR like this, it's better to have the device have a MemoryRegion
property, which the SoC sets with whatever the DMA device should
be able to see.

Otherwise, patch looks good, though I don't know enough about
the device/SoC to review those details.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)

[Peter Maydell]

On 25 September 2018 at 15:10, Cédric Le Goater <clg@kaod.org> wrote:
> On 9/25/18 2:20 PM, Peter Maydell wrote:
>> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>>> Hello,
>>>
>>> This series adds a couple of cleanups and two main features to the
>>> SMC controller of the Aspeed machines :
>>>
>>>  - a 'execute-in-place' property to boot directly from a memory region
>>>    alias of the FMC flash module using MMIO execution. This is not
>>>    activated by default because boot time needs to be improved on
>>>    recent firmwares. It also breaks migration compatibility.
>>>
>>>  - support for DMA to access the flash modules. Our primary need is
>>>    the checksum calculation which is used to evaluate the best clock
>>>    settings for reads.
>>
>> I've picked out the easy parts of this series and added
>> them to target-arm.next:
>>
>>>   aspeed/timer: fix compile breakage with clang 3.4.2
>>>   hw/arm/aspeed: change the FMC flash model of the AST2500 evb
>>>   hw/arm/aspeed: Add an Aspeed machine class
>>>   aspeed/smc: fix some alignment issues
>>
>> and left the rest for the moment (I've still got this series
>> on my to-review list).
>
> I will wait for your comments on the DMA part before resending.

I've now commented on that patch. I think I'm done with review on
this version, unless there's something specific you want more
input on ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Cédric Le Goater]

On 10/2/18 12:56 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>> The FMC controller on the Aspeed SoCs support DMA to access the flash
>> modules. It can operate in a normal mode, to copy to or from the flash
>> module mapping window, or in a checksum calculation mode, to evaluate
>> the best clock settings for reads.
>>
>> The model introduces a custom address space for DMAs populated with
>> the required regions : an alias region on the AHB window for the flash
>> devices and another alias on the SDRAM.
>>
>> Our primary need is to support the checksum calculation mode and the
>> model only implements synchronous DMA accesses. Something to improve
>> in the future.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> 
>> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
>> +{
>> +    MemTxResult result;
>> +    uint32_t data;
>> +
>> +    while (s->regs[R_DMA_LEN]) {
>> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>> +            result = address_space_read(&s->dma_as, s->regs[R_DMA_DRAM_ADDR],
>> +                                        MEMTXATTRS_UNSPECIFIED,
>> +                                        (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +                return;
>> +            }
> 
> Does the device really not report DMA read/write failures via
> a status register bit or similar ?

The Interrupt Control and Status Register (FM0C8) has a DMA Status
BIT(11) to indicate that a DMA transfer is in progress or not. Nothing
for errors.

There are a SPI command abort status BIT(10) and a SPI Write Address
Protected status BIT(11) but these are for the command and address
filters.
 
>> +
>> +/*
>> + * Populate our custom address space for DMAs with only the regions we
>> + * need : the AHB window for the flash devices and the SDRAM.
>> + */
>> +static void aspeed_smc_dma_setup(AspeedSMCState *s)
>> +{
>> +    char name[32];
>> +    MemoryRegion *sysmem = get_system_memory();
>> +    MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>> +    MemoryRegion *sdram_alias = g_new(MemoryRegion, 1);
>> +
>> +    snprintf(name, sizeof(name), "%s-dma", s->ctrl->name);
> 
> I would suggest using g_strdup_printf()/g_free(), since it's not
> immediately obvious here that s->ctrl->name is guaranteed
> to fit into the fixed-size array.

yes. sure.

>> +    memory_region_init(&s->dma_mr, OBJECT(s), name,
>> +                       s->sdram_base + s->max_ram_size);
>> +    address_space_init(&s->dma_as, &s->dma_mr, name);
>> +
>> +    snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
>> +    memory_region_init_alias(flash_alias, OBJECT(s), name, &s->mmio_flash,
>> +                             0, s->ctrl->flash_window_size);
>> +    memory_region_add_subregion(&s->dma_mr, s->ctrl->flash_window_base,
>> +                                flash_alias);
>> +
>> +    memory_region_init_alias(sdram_alias, OBJECT(s), "ram", sysmem,
>> +                             s->sdram_base, s->max_ram_size);
>> +    memory_region_add_subregion(&s->dma_mr, s->sdram_base, sdram_alias);
> 
> Rather than having the DMA device directly grab the system_memory
> MR like this, it's better to have the device have a MemoryRegion
> property, which the SoC sets with whatever the DMA device should
> be able to see.

ok. I see, but it seems I have a chicken & egg problem. 

The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c 
but it is initialized after the SoC is. I don't know how to lazy bind 
this region in the Aspeed SMC model using the Aspeed SoC model as a 
proxy to pass the region property through a link or/and alias.

If I could find a way, the model would be much cleaner.  

> Otherwise, patch looks good, though I don't know enough about
> the device/SoC to review those details.

For the moment the only use of these registers is in the Aspeed custom 
u-boot of the SDK : 

or in the rewrite I proposed in mainline :


Thanks,

C.

Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)

[Cédric Le Goater]

On 10/2/18 12:57 PM, Peter Maydell wrote:
> On 25 September 2018 at 15:10, Cédric Le Goater <clg@kaod.org> wrote:
>> On 9/25/18 2:20 PM, Peter Maydell wrote:
>>> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>>>> Hello,
>>>>
>>>> This series adds a couple of cleanups and two main features to the
>>>> SMC controller of the Aspeed machines :
>>>>
>>>>  - a 'execute-in-place' property to boot directly from a memory region
>>>>    alias of the FMC flash module using MMIO execution. This is not
>>>>    activated by default because boot time needs to be improved on
>>>>    recent firmwares. It also breaks migration compatibility.
>>>>
>>>>  - support for DMA to access the flash modules. Our primary need is
>>>>    the checksum calculation which is used to evaluate the best clock
>>>>    settings for reads.
>>>
>>> I've picked out the easy parts of this series and added
>>> them to target-arm.next:
>>>
>>>>   aspeed/timer: fix compile breakage with clang 3.4.2
>>>>   hw/arm/aspeed: change the FMC flash model of the AST2500 evb
>>>>   hw/arm/aspeed: Add an Aspeed machine class
>>>>   aspeed/smc: fix some alignment issues
>>>
>>> and left the rest for the moment (I've still got this series
>>> on my to-review list).
>>
>> I will wait for your comments on the DMA part before resending.
> 
> I've now commented on that patch. I think I'm done with review on
> this version, unless there's something specific you want more
> input on ?

No, a part from the chicken & egg problem on the RAM memory region
may be if you have some idea.  I will dig in before re-sending anyhow.

Thanks,

C. 

Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Cédric Le Goater]

On 10/2/18 12:49 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


The DRAM training in mainline u-boot is also significantly slower on 
real HW. I haven't spotted the exact area but the code is jumping back
and forth.  This patch should help tracking the culprit.
   
Thanks,

C.

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Cédric Le Goater]

With the links,

[ ... ]

>> Otherwise, patch looks good, though I don't know enough about
>> the device/SoC to review those details.
> 
> For the moment the only use of these registers is in the Aspeed custom 
> u-boot of the SDK : 

https://github.com/openbmc/u-boot/blob/v2016.07-aspeed-openbmc/arch/arm/mach-aspeed/platform_g5.S#L2314
 
> or in the rewrite I proposed in mainline :

http://patchwork.ozlabs.org/patch/972868/


C.

Re: [Qemu-devel] [PATCH v2 00/11] aspeed: misc fixes and enhancements (SMC)

[Cédric Le Goater]

On 9/25/18 2:30 PM, Peter Maydell wrote:
> On 25 September 2018 at 13:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>>> Hello,
>>>
>>> This series adds a couple of cleanups and two main features to the
>>> SMC controller of the Aspeed machines :
>>>
>>>  - a 'execute-in-place' property to boot directly from a memory region
>>>    alias of the FMC flash module using MMIO execution. This is not
>>>    activated by default because boot time needs to be improved on
>>>    recent firmwares. It also breaks migration compatibility.
>>>
>>>  - support for DMA to access the flash modules. Our primary need is
>>>    the checksum calculation which is used to evaluate the best clock
>>>    settings for reads.
>>
>> I've picked out the easy parts of this series and added
>> them to target-arm.next:
>>
>>>   aspeed/timer: fix compile breakage with clang 3.4.2
>>>   hw/arm/aspeed: change the FMC flash model of the AST2500 evb
>>>   hw/arm/aspeed: Add an Aspeed machine class
>>>   aspeed/smc: fix some alignment issues
>>
>> and left the rest for the moment (I've still got this series
>> on my to-review list).
> 
> PS: review from people familiar with the SoC would also be helpful.

Joel has reviewed the u-boot patches. May be, he can chime in ? 

Thanks,

C. 

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Peter Maydell]

On 2 October 2018 at 16:48, Cédric Le Goater <clg@kaod.org> wrote:
> On 10/2/18 12:56 PM, Peter Maydell wrote:
>> Rather than having the DMA device directly grab the system_memory
>> MR like this, it's better to have the device have a MemoryRegion
>> property, which the SoC sets with whatever the DMA device should
>> be able to see.
>
> ok. I see, but it seems I have a chicken & egg problem.
>
> The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c
> but it is initialized after the SoC is. I don't know how to lazy bind
> this region in the Aspeed SMC model using the Aspeed SoC model as a
> proxy to pass the region property through a link or/and alias.
>
> If I could find a way, the model would be much cleaner.

Usually what you want to pass is not the RAM region directly but
some container object (which contains the RAM region among
other things). This is often a container that you're passing
to the SoC anyway so it can put its devices in it.

(The direct answer to your chicken-and-egg problem is that you
can create a container before creating the SoC, pass that to
the SoC, then create the RAM afterwards and put it in the
container after. But that feels a bit like a workaround for
a less than ideal structure, somehow.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Cédric Le Goater]

On 10/2/18 6:12 PM, Peter Maydell wrote:
> On 2 October 2018 at 16:48, Cédric Le Goater <clg@kaod.org> wrote:
>> On 10/2/18 12:56 PM, Peter Maydell wrote:
>>> Rather than having the DMA device directly grab the system_memory
>>> MR like this, it's better to have the device have a MemoryRegion
>>> property, which the SoC sets with whatever the DMA device should
>>> be able to see.
>>
>> ok. I see, but it seems I have a chicken & egg problem.
>>
>> The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c
>> but it is initialized after the SoC is. I don't know how to lazy bind
>> this region in the Aspeed SMC model using the Aspeed SoC model as a
>> proxy to pass the region property through a link or/and alias.
>>
>> If I could find a way, the model would be much cleaner.
> 
> Usually what you want to pass is not the RAM region directly but
> some container object (which contains the RAM region among
> other things). This is often a container that you're passing
> to the SoC anyway so it can put its devices in it.
> 
> (The direct answer to your chicken-and-egg problem is that you
> can create a container before creating the SoC, pass that to
> the SoC, then create the RAM afterwards and put it in the
> container after. But that feels a bit like a workaround for
> a less than ideal structure, somehow.)

couldn't we just initialize the RAM under the SoC ? We don't need
it at the machine level AFAICT, nor does the raspi model I think.

What is the rational behind putting the RAM region in a state 
structure at the machine level ? 

Thanks,

C.

Re: [Qemu-devel] [PATCH v2 03/11] hw/arm/aspeed: Add an Aspeed machine class

[Thomas Huth]

On 2018-09-21 18:19, Cédric Le Goater wrote:
> The code looks better, it removes duplicated lines and it will ease
> the introduction of common properties for the Aspeed machines.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/aspeed.h |  46 +++++++++
>  hw/arm/aspeed.c         | 212 +++++++++++++---------------------------
>  2 files changed, 116 insertions(+), 142 deletions(-)
>  create mode 100644 include/hw/arm/aspeed.h
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> new file mode 100644
> index 000000000000..325c091d09e4
> --- /dev/null
> +++ b/include/hw/arm/aspeed.h
> @@ -0,0 +1,46 @@
> +/*
> + * Aspeed Machines
> + *
> + * Copyright 2018 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef ARM_ASPEED_H
> +#define ARM_ASPEED_H
> +
> +#include "hw/boards.h"
> +
> +typedef struct AspeedBoardState AspeedBoardState;

 Hi Cédric,

this patch breaks compiling with clang v3.4.2 here:

 CC      aarch64-softmmu/hw/arm/aspeed.o
hw/arm/aspeed.c:36:3: error: redefinition of typedef 'AspeedBoardState' is a C11
      feature [-Werror,-Wtypedef-redefinition]
} AspeedBoardState;
  ^
include/hw/arm/aspeed.h:14:33: note: previous definition is here
typedef struct AspeedBoardState AspeedBoardState;
                                ^
1 error generated.
make[1]: *** [hw/arm/aspeed.o] Error 1
make: *** [subdir-aarch64-softmmu] Error 2

Seems like it can be fixed like this:

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -29,11 +29,11 @@ static struct arm_boot_info aspeed_board_binfo = {
     .nb_cpus = 1,
 };
 
-typedef struct AspeedBoardState {
+struct AspeedBoardState {
     AspeedSoCState soc;
     MemoryRegion ram;
     MemoryRegion max_ram;
-} AspeedBoardState;
+};

... does that look OK for you, too?

> +typedef struct AspeedBoardConfig {
> +    const char *name;
> +    const char *desc;
> +    const char *soc_name;
> +    uint32_t hw_strap1;
> +    const char *fmc_model;
> +    const char *spi_model;
> +    uint32_t num_cs;
> +    void (*i2c_init)(AspeedBoardState *bmc);
> +} AspeedBoardConfig;
> +
> +#define TYPE_ASPEED_MACHINE       MACHINE_TYPE_NAME("aspeed")
> +#define ASPEED_MACHINE(obj) \
> +    OBJECT_CHECK(AspeedMachine, (obj), TYPE_ASPEED_MACHINE)
> +
> +typedef struct AspeedMachine {
> +    MachineState parent_obj;
> +} AspeedMachine;
> +
> +#define ASPEED_MACHINE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AspeedMachineClass, (klass), TYPE_ASPEED_MACHINE)
> +#define ASPEED_MACHINE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AspeedMachineClass, (obj), TYPE_ASPEED_MACHINE)
> +
> +typedef struct AspeedMachineClass {
> +    MachineClass parent_obj;
> +    const AspeedBoardConfig *board;
> +} AspeedMachineClass;
> +
> +
> +#endif

 Thomas