[PATCH qemu 21/38] aspeed/smc: adjust the size of the register region

[Cédric Le Goater]

On 11/28/2016 02:44 AM, Andrew Jeffery wrote:
> On Fri, 2016-11-18 at 15:22 +0100, Cédric Le Goater wrote:
>> The SPI controller of the AST2400 SoC does not support DMAs and
>> segment registers, hence it has less registers. So we can adjust the
>> size of the memory region holding the registers depending on the
>> controller type. We can also remove the guest_error logging which is
>> useless as the range of the region is strict enough.
>>
>>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>> ---
>>  hw/ssi/aspeed_smc.c         | 23 ++++++++---------------
>>  include/hw/ssi/aspeed_smc.h |  1 +
>>  2 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index c0ae07ef8c96..7d7800d96271 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -135,6 +135,8 @@
>>  #define R_SPI_MISC_CTRL   (0x10 / 4)
>>  #define R_SPI_TIMINGS     (0x14 / 4)
>>  
>> +#define R_SPI_MAX         (0x20 / 4)
>> +
>>  #define ASPEED_SOC_SMC_FLASH_BASE   0x10000000
>>  #define ASPEED_SOC_FMC_FLASH_BASE   0x20000000
>>  #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
>> @@ -221,6 +223,7 @@ static const AspeedSMCController controllers[] = {
>>          .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
>>          .flash_window_size = 0x10000000,
>>          .has_dma           = true,
>> +        .nregs             = ASPEED_SMC_R_MAX,
>>      }, {
>>          .name              = "aspeed.smc.spi",
>>          .r_conf            = R_SPI_CONF,
>> @@ -233,6 +236,7 @@ static const AspeedSMCController controllers[] = {
>>          .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
>>          .flash_window_size = 0x10000000,
>>          .has_dma           = false,
>> +        .nregs             = R_SPI_MAX,
>>      }, {
>>          .name              = "aspeed.smc.ast2500-fmc",
>>          .r_conf            = R_CONF,
>> @@ -245,6 +249,7 @@ static const AspeedSMCController controllers[] = {
>>          .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
>>          .flash_window_size = 0x10000000,
>>          .has_dma           = true,
>> +        .nregs             = ASPEED_SMC_R_MAX,
>>      }, {
>>          .name              = "aspeed.smc.ast2500-spi1",
>>          .r_conf            = R_CONF,
>> @@ -257,6 +262,7 @@ static const AspeedSMCController controllers[] = {
>>          .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
>>          .flash_window_size = 0x8000000,
>>          .has_dma           = false,
>> +        .nregs             = ASPEED_SMC_R_MAX,
>>      }, {
>>          .name              = "aspeed.smc.ast2500-spi2",
>>          .r_conf            = R_CONF,
>> @@ -269,6 +275,7 @@ static const AspeedSMCController controllers[] = {
>>          .flash_window_base = ASPEED_SOC_SPI2_FLASH_BASE,
>>          .flash_window_size = 0x8000000,
>>          .has_dma           = false,
>> +        .nregs             = ASPEED_SMC_R_MAX,
>>      },
>>  };
>>  
>> @@ -712,13 +719,6 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>>  
>>      addr >>= 2;
>>  
>> -    if (addr >= ARRAY_SIZE(s->regs)) {
>> -        qemu_log_mask(LOG_GUEST_ERROR,
>> -                      "%s: Out-of-bounds read at 0x%" HWADDR_PRIx "\n",
>> -                      __func__, addr);
>> -        return 0;
>> -    }
>> -
>>      if (addr == s->r_conf ||
>>          addr == s->r_timings ||
>>          addr == s->r_ce_ctrl ||
>> @@ -942,13 +942,6 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>>  
>>      addr >>= 2;
>>  
>> -    if (addr >= ARRAY_SIZE(s->regs)) {
>> -        qemu_log_mask(LOG_GUEST_ERROR,
>> -                      "%s: Out-of-bounds write at 0x%" HWADDR_PRIx "\n",
>> -                      __func__, addr);
>> -        return;
>> -    }
>> -
>>      if (addr == s->r_conf ||
>>          addr == s->r_timings ||
>>          addr == s->r_ce_ctrl) {
>> @@ -1030,7 +1023,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
>>  
>>      /* The memory region for the controller registers */
>>      memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
>> -                          s->ctrl->name, ASPEED_SMC_R_MAX * 4);
>> +                          s->ctrl->name, s->ctrl->nregs * 4);
> 
> It looks like you missed defining nregs for the "aspeed.smc.smc"
> controller. This would suggest we need to.

Ah yes. we don't instantiate the legacy controller so I didn't see it.

Thanks,

C. 


> Andrew
> 
>>      sysbus_init_mmio(sbd, &s->mmio);
>>  
>>      /*
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 3ae0a369073d..91bad82e9c65 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -45,6 +45,7 @@ typedef struct AspeedSMCController {
>>      hwaddr flash_window_base;
>>      uint32_t flash_window_size;
>>      bool has_dma;
>> +    uint32_t nregs;
>>  } AspeedSMCController;
>>  
>>  typedef struct AspeedSMCFlash {