[PATCH qemu 25/38] aspeed/sdmc: rework the locking register of the memory controller

[Cédric Le Goater]

On 11/28/2016 02:55 AM, Andrew Jeffery wrote:
> On Fri, 2016-11-18 at 15:22 +0100, Cédric Le Goater wrote:
>> The lock register is unlocked with a write of a special value and
>> locked with a write of any other value. When unlocked, reads return
>> one and when locked a zero.
>>
>>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
>> ---
>>  hw/misc/aspeed_sdmc.c         | 28 +++++++++++++++++++---------
>>  include/hw/misc/aspeed_sdmc.h |  1 +
>>  2 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>> index 5f3ac0b6f608..867caa2d64dc 100644
>> --- a/hw/misc/aspeed_sdmc.c
>> +++ b/hw/misc/aspeed_sdmc.c
>> @@ -83,17 +83,22 @@
>>  static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr, unsigned size)
>>  {
>>      AspeedSDMCState *s = ASPEED_SDMC(opaque);
>> +    uint64_t val = 0;
>>  
>>      addr >>= 2;
>>  
>> -    if (addr >= ARRAY_SIZE(s->regs)) {
>> +    switch (addr) {
>> +    case R_PROT:
>> +        val = s->unlocked;
>> +        break;
>> +    default:
>>          qemu_log_mask(LOG_GUEST_ERROR,
>>                        "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
>>                        __func__, addr);
>> -        return 0;
>> +        break;
>>      }
>>  
>> -    return s->regs[addr];
>> +    return val;
> 
> This suggests we only care about R_PROT - the change returns 0 for the
> value of other registers. That doesn't seem right.

Well, the read already returned 0 and that is what you get most of the 
time on real HW. Some time you get a 0xFFFFFFFF. 

Thanks,

C.


> Andrew
> 
>>  }
>>  
>>  static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>> @@ -103,19 +108,18 @@ static void aspeed_sdmc_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 offset 0x%" HWADDR_PRIx "\n",
>> -                      __func__, addr);
>> +    if (addr == R_PROT) {
>> +        s->unlocked = (data == PROT_KEY_UNLOCK);
>>          return;
>>      }
>>  
>> -    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
>> +    if (!s->unlocked) { /* TODO protect : MCR04 ∼ MCR7C */
>>          qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
>>          return;
>>      }
>>  
>> -    if (addr == R_CONF) {
>> +    switch (addr) {
>> +    case R_CONF:
>>          /* Make sure readonly bits are kept */
>>          switch (s->silicon_rev) {
>>          case AST2400_A0_SILICON_REV:
>> @@ -128,6 +132,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
>>          default:
>>              g_assert_not_reached();
>>          }
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
>> +                      __func__, addr << 2);
>> +        return;
>>      }
>>  
>>      s->regs[addr] = data;
>> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
>> index 551c8afdf4be..a4415d9efc2f 100644
>> --- a/include/hw/misc/aspeed_sdmc.h
>> +++ b/include/hw/misc/aspeed_sdmc.h
>> @@ -28,6 +28,7 @@ typedef struct AspeedSDMCState {
>>      uint32_t ram_bits;
>>      uint64_t ram_size;
>>  
>> +    bool unlocked;
>>  } AspeedSDMCState;
>>  
>>  #endif /* ASPEED_SDMC_H */