[Qemu-devel] [PATCH 0/4] aspeed: miscellaneous small fixes

[Qemu-devel] [PATCH 0/4] aspeed: miscellaneous small fixes

[Cédric Le Goater]

Hello,

Here is a short series fixing small issues in the SMC controller model
of the Aspeed SoC.

Thanks,

C. 

Cédric Le Goater (4):
  aspeed: check for negative values returned by blk_getlength()
  aspeed: remove useless comment on controller segment size
  aspeed/smc: handle dummies only in fast read mode
  aspeed/smc: use a modulo to check segment limits

 hw/arm/aspeed.c     | 22 +++++++++++++++-------
 hw/ssi/aspeed_smc.c | 13 ++++++++-----
 2 files changed, 23 insertions(+), 12 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] aspeed: check for negative values returned by blk_getlength()

[Cédric Le Goater]

write_boot_rom() does not check for negative values. This is more a
problem for coverity than the actual code as the size of the flash
device is checked when the m25p80 object is created. If there is
anything wrong with the backing file, we should not even reach that
path.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a92c2f1c362b..ac9cbd66b72a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -113,9 +113,19 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
 {
     BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
     uint8_t *storage;
+    int64_t size;
 
-    if (rom_size > blk_getlength(blk)) {
-        rom_size = blk_getlength(blk);
+    /* The block backend size should have already been 'validated' by
+     * the creation of the m25p80 object.
+     */
+    size = blk_getlength(blk);
+    if (size <= 0) {
+        error_setg(errp, "failed to get flash size");
+        return;
+    }
+
+    if (rom_size > size) {
+        rom_size = size;
     }
 
     storage = g_new0(uint8_t, rom_size);
-- 
2.7.4

[Qemu-devel] [PATCH 2/4] aspeed: remove useless comment on controller segment size

[Cédric Le Goater]

The flash devices used for the FMC controller (BMC firmware) are well
defined for each Aspeed machine and are all smaller than the default
mapping window size, at least for CE0 which is the chip the SoC boots
from.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ac9cbd66b72a..283c03881493 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -148,10 +148,6 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
         DriveInfo *dinfo = drive_get_next(IF_MTD);
         qemu_irq cs_line;
 
-        /*
-         * FIXME: check that we are not using a flash module exceeding
-         * the controller segment size
-         */
         fl->flash = ssi_create_slave_no_init(s->spi, flashtype);
         if (dinfo) {
             qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
@@ -210,7 +206,9 @@ static void aspeed_board_init(MachineState *machine,
 
         /*
          * create a ROM region using the default mapping window size of
-         * the flash module.
+         * the flash module. The window size is 64MB for the AST2400
+         * 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);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/4] aspeed: remove useless comment on controller segment size

[Philippe Mathieu-Daudé]

On 02/09/2017 10:47 AM, Cédric Le Goater wrote:
> The flash devices used for the FMC controller (BMC firmware) are well
> defined for each Aspeed machine and are all smaller than the default
> mapping window size, at least for CE0 which is the chip the SoC boots
> from.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/aspeed.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ac9cbd66b72a..283c03881493 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -148,10 +148,6 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>          DriveInfo *dinfo = drive_get_next(IF_MTD);
>          qemu_irq cs_line;
>
> -        /*
> -         * FIXME: check that we are not using a flash module exceeding
> -         * the controller segment size
> -         */
>          fl->flash = ssi_create_slave_no_init(s->spi, flashtype);
>          if (dinfo) {
>              qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo),
> @@ -210,7 +206,9 @@ static void aspeed_board_init(MachineState *machine,
>
>          /*
>           * create a ROM region using the default mapping window size of
> -         * the flash module.
> +         * the flash module. The window size is 64MB for the AST2400
> +         * 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);
>

[Qemu-devel] [PATCH 3/4] aspeed/smc: handle dummies only in fast read mode

[Cédric Le Goater]

HW works fine in normal read mode with dummy bytes being set. So let's
check this case to not transfer bytes.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 087b29e8da10..70177078a8f2 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -536,10 +536,13 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
         /*
          * Use fake transfers to model dummy bytes. The value should
          * be configured to some non-zero value in fast read mode and
-         * zero in read mode.
+         * zero in read mode. But, as the HW allows inconsistent
+         * settings, let's check for fast read mode.
          */
-        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
-            ssi_transfer(fl->controller->spi, 0xFF);
+        if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
+            for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
+                ssi_transfer(fl->controller->spi, 0xFF);
+            }
         }
 
         for (i = 0; i < size; i++) {
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] aspeed/smc: use a modulo to check segment limits

[Cédric Le Goater]

The size of a segment is not necessarily a power of 2.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 70177078a8f2..cb515730c5ad 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -475,15 +475,15 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
     AspeedSegments seg;
 
     aspeed_smc_reg_to_segment(s->regs[R_SEG_ADDR0 + fl->id], &seg);
-    if ((addr & (seg.size - 1)) != addr) {
+    if ((addr % seg.size) != addr) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid address 0x%08x for CS%d segment : "
                       "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
                       s->ctrl->name, addr, fl->id, seg.addr,
                       seg.addr + seg.size);
+        addr %= seg.size;
     }
 
-    addr &= seg.size - 1;
     return addr;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/4] aspeed/smc: use a modulo to check segment limits

[Philippe Mathieu-Daudé]

On 02/09/2017 10:47 AM, Cédric Le Goater wrote:
> The size of a segment is not necessarily a power of 2.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/ssi/aspeed_smc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 70177078a8f2..cb515730c5ad 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -475,15 +475,15 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
>      AspeedSegments seg;
>
>      aspeed_smc_reg_to_segment(s->regs[R_SEG_ADDR0 + fl->id], &seg);
> -    if ((addr & (seg.size - 1)) != addr) {
> +    if ((addr % seg.size) != addr) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: invalid address 0x%08x for CS%d segment : "
>                        "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
>                        s->ctrl->name, addr, fl->id, seg.addr,
>                        seg.addr + seg.size);
> +        addr %= seg.size;
>      }
>
> -    addr &= seg.size - 1;
>      return addr;
>  }
>
>

Re: [Qemu-devel] [PATCH 0/4] aspeed: miscellaneous small fixes

[Peter Maydell]

On 9 February 2017 at 13:47, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> Here is a short series fixing small issues in the SMC controller model
> of the Aspeed SoC.
>
> Thanks,
>
> C.
>
> Cédric Le Goater (4):
>   aspeed: check for negative values returned by blk_getlength()
>   aspeed: remove useless comment on controller segment size
>   aspeed/smc: handle dummies only in fast read mode
>   aspeed/smc: use a modulo to check segment limits



Applied to target-arm.next, thanks.

-- PMM