Re: [Qemu-arm] [PATCH] Fix ast2500 protection register emulation

[Cédric Le Goater]

On 02/20/2018 02:26 PM, Hugo Landau wrote:
> Some register blocks of the ast2500 are protected by protection key
> registers which require the right magic value to be written to those
> registers to allow those registers to be mutated.
> 
> Register manuals indicate that writing the correct magic value to these
> registers should cause subsequent reads from those values to return 1,
> and writing any other value should cause subsequent reads to return 0.

Yes indeed.

OpenBMC has a similar patch in its QEMU tree : 

        https://github.com/openbmc/qemu/commit/0529fa5e5947

but this one is better.

> Previously, qemu implemented these registers incorrectly: the registers
> were handled as simple memory, meaning that writing some value x to a
> protection key register would result in subsequent reads from that
> register returning the same value x. The protection was implemented by
> ensuring that the current value of that register equaled the magic
> value.
> 
> This modifies qemu to have the correct behaviour: attempts to write to a
> ast2500 protection register results in a transition to 1 or 0 depending
> on whether the written value is the correct magic. The protection logic
> is updated to ensure that the value of the register is nonzero.
> 
> This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
> protectable register block, it attempts to lock it by writing the
> bitwise inverse of the correct magic value, and then spinning forever
> until the register reads as zero. Since qemu implemented writes to these
> registers as ordinary memory writes, writing the inverse of the magic
> value resulted in subsequent reads returning that value, leading to
> u-boot spinning forever.
> 
> Signed-off-by: Hugo Landau <address@hidden>

With a minor comment below, 

Reviewed-by: Cédric Le Goater <address@hidden> 

I also gave it a test on an OpenBMC romulus image. Looks fine, but that's 
an old custom U-Boot. Which defconfig did you use for U-Boot HEAD ? 

> ---
>  hw/misc/aspeed_scu.c  | 6 +++++-
>  hw/misc/aspeed_sdmc.c | 8 +++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 74537ce975..5e6d5744ee 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -191,7 +191,7 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, 
> uint64_t data,
>      }
>  
>      if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
> -            s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {
> +            !s->regs[PROT_KEY]) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
>          return;
>      }
> @@ -199,6 +199,10 @@ static void aspeed_scu_write(void *opaque, hwaddr 
> offset, uint64_t data,
>      trace_aspeed_scu_write(offset, size, data);
>  
>      switch (reg) {
> +    case PROT_KEY:
> +        s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
> +        return;
> +
>      case FREQ_CNTR_EVAL:
>      case VGA_SCRATCH1 ... VGA_SCRATCH8:
>      case RNG_DATA:
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index f0b3053fae..265171ee42 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -110,7 +110,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, 
> uint64_t data,
>          return;
>      }
>  
> -    if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> +    if (addr == R_PROT) {
> +      s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
> +      return;
> +    }
> +
> +    if (!s->regs[R_PROT]) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
>          return;
>      }
> @@ -123,6 +128,7 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, 
> uint64_t data,
>              data &= ~ASPEED_SDMC_READONLY_MASK;
>              break;
>          case AST2500_A0_SILICON_REV:
> +        case AST2500_A1_SILICON_REV:

That's unrelated to the commit log, but I don't think you need to 
resend for that.

Thanks,

C.
 
>              data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
>              break;
>          default:
>