[v2,06/13] hw/misc/tz-mpc.c: Implement registers

Message ID 20180604152941.20374-7-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • iommu: support txattrs, support TCG execution, implement TZ MPC
Related show

Commit Message

Peter Maydell June 4, 2018, 3:29 p.m.
Implement the missing registers for the TZ MPC.

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

---
 include/hw/misc/tz-mpc.h |  10 +++
 hw/misc/tz-mpc.c         | 137 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 144 insertions(+), 3 deletions(-)

-- 
2.17.1

Comments

Eric Auger June 14, 2018, 8:14 p.m. | #1
Hi Peter,

On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.

> 

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

> ---

>  include/hw/misc/tz-mpc.h |  10 +++

>  hw/misc/tz-mpc.c         | 137 ++++++++++++++++++++++++++++++++++++++-

>  2 files changed, 144 insertions(+), 3 deletions(-)

> 

> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h

> index b5eaf1699ea..1fff4d6029a 100644

> --- a/include/hw/misc/tz-mpc.h

> +++ b/include/hw/misc/tz-mpc.h

> @@ -48,6 +48,16 @@ struct TZMPC {

>  

>      /*< public >*/

>  

> +    /* State */

> +    uint32_t ctrl;

> +    uint32_t blk_idx;

> +    uint32_t int_stat;

> +    uint32_t int_en;

> +    uint32_t int_info1;

> +    uint32_t int_info2;

> +

> +    uint32_t *blk_lut;

> +

>      qemu_irq irq;

>  

>      /* Properties */

> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c

> index d4467ccc3b2..9db55e23daf 100644

> --- a/hw/misc/tz-mpc.c

> +++ b/hw/misc/tz-mpc.c

> @@ -28,16 +28,23 @@ enum {

>  

>  /* Config registers */

>  REG32(CTRL, 0x00)

> +    FIELD(CTRL, SEC_RESP, 4, 1)

> +    FIELD(CTRL, AUTOINC, 8, 1)

> +    FIELD(CTRL, LOCKDOWN, 31, 1)

>  REG32(BLK_MAX, 0x10)

>  REG32(BLK_CFG, 0x14)

>  REG32(BLK_IDX, 0x18)

>  REG32(BLK_LUT, 0x1c)

>  REG32(INT_STAT, 0x20)

> +    FIELD(INT_STAT, IRQ, 0, 1)

>  REG32(INT_CLEAR, 0x24)

> +    FIELD(INT_CLEAR, IRQ, 0, 1)

>  REG32(INT_EN, 0x28)

> +    FIELD(INT_EN, IRQ, 0, 1)

>  REG32(INT_INFO1, 0x2c)

>  REG32(INT_INFO2, 0x30)

>  REG32(INT_SET, 0x34)

> +    FIELD(INT_SET, IRQ, 0, 1)

>  REG32(PIDR4, 0xfd0)

>  REG32(PIDR5, 0xfd4)

>  REG32(PIDR6, 0xfd8)

> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {

>      0x0d, 0xf0, 0x05, 0xb1,

>  };

>  

> +static void tz_mpc_irq_update(TZMPC *s)

> +{

> +    qemu_set_irq(s->irq, s->int_stat && s->int_en);

> +}

> +

>  static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,

>                                     uint64_t *pdata,

>                                     unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint64_t r;

>      uint32_t offset = addr & ~0x3;

I don't get where do we check if the access is secure or not. Shouldn't
we test attrs.secure somewhere (PPROT[1] == 0)? Also the spec says
IDregs can be read by any type of access. Where is this differentiation
done?

Thanks

Eric
>  

>      switch (offset) {

> +    case A_CTRL:

> +        r = s->ctrl;

> +        break;

> +    case A_BLK_MAX:

> +        r = s->blk_max;

> +        break;

> +    case A_BLK_CFG:

> +        /* We are never in "init in progress state", so this just indicates

> +         * the block size. s->blocksize == (1 << BLK_CFG + 5), so

> +         * BLK_CFG == ctz32(s->blocksize) - 5

> +         */

> +        r = ctz32(s->blocksize) - 5;

> +        break;

> +    case A_BLK_IDX:

> +        r = s->blk_idx;

> +        break;

> +    case A_BLK_LUT:

> +        r = s->blk_lut[s->blk_idx];

> +        if (size == 4) {

> +            s->blk_idx++;

> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_STAT:

> +        r = s->int_stat;

> +        break;

> +    case A_INT_EN:

> +        r = s->int_en;

> +        break;

> +    case A_INT_INFO1:

> +        r = s->int_info1;

> +        break;

> +    case A_INT_INFO2:

> +        r = s->int_info2;

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>                                      uint64_t value,

>                                      unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint32_t offset = addr & ~0x3;

>  

>      trace_tz_mpc_reg_write(addr, value, size);

> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          uint32_t oldval;

>  

>          switch (offset) {

> -            /* As we add support for registers which need expansions

> -             * other than zeroes we'll fill in cases here.

> -             */

> +        case A_CTRL:

> +            oldval = s->ctrl;

> +            break;

> +        case A_BLK_IDX:

> +            oldval = s->blk_idx;

> +            break;

> +        case A_BLK_LUT:

> +            oldval = s->blk_lut[s->blk_idx];

> +            break;

>          default:

>              oldval = 0;

>              break;

> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          value = deposit32(oldval, (addr & 3) * 8, size * 8, value);

>      }

>  

> +    if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&

> +        (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {

> +        /* Lockdown mode makes these three registers read-only, and

> +         * the only way out of it is to reset the device.

> +         */

> +        qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "

> +                      "while MPC is in lockdown mode\n", offset);

> +        return MEMTX_OK;

> +    }

> +

>      switch (offset) {

> +    case A_CTRL:

> +        /* We don't implement the 'data gating' feature so all other bits

> +         * are reserved and we make them RAZ/WI.

> +         */

> +        s->ctrl = value & (R_CTRL_SEC_RESP_MASK |

> +                           R_CTRL_AUTOINC_MASK |

> +                           R_CTRL_LOCKDOWN_MASK);

> +        break;

> +    case A_BLK_IDX:

> +        s->blk_idx = value % s->blk_max;

> +        break;

> +    case A_BLK_LUT:

> +        s->blk_lut[s->blk_idx] = value;

> +        if (size == 4) {

> +            s->blk_idx++;

> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_CLEAR:

> +        if (value & R_INT_CLEAR_IRQ_MASK) {

> +            s->int_stat = 0;

> +            tz_mpc_irq_update(s);

> +        }

> +        break;

> +    case A_INT_EN:

> +        s->int_en = value & R_INT_EN_IRQ_MASK;

> +        tz_mpc_irq_update(s);

> +        break;

> +    case A_INT_SET:

> +        if (value & R_INT_SET_IRQ_MASK) {

> +            s->int_stat = R_INT_STAT_IRQ_MASK;

> +            tz_mpc_irq_update(s);

> +        }

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)

>  

>  static void tz_mpc_reset(DeviceState *dev)

>  {

> +    TZMPC *s = TZ_MPC(dev);

> +

> +    s->ctrl = 0x00000100;

> +    s->blk_idx = 0;

> +    s->int_stat = 0;

> +    s->int_en = 1;

> +    s->int_info1 = 0;

> +    s->int_info2 = 0;

> +

> +    memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));

>  }

>  

>  static void tz_mpc_init(Object *obj)

> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)

>                         "tz-mpc-downstream");

>      address_space_init(&s->blocked_io_as, &s->blocked_io,

>                         "tz-mpc-blocked-io");

> +

> +    s->blk_lut = g_new(uint32_t, s->blk_max);

> +}

> +

> +static int tz_mpc_post_load(void *opaque, int version_id)

> +{

> +    TZMPC *s = TZ_MPC(opaque);

> +

> +    /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */

> +    if (s->blk_idx >= s->blk_max) {

> +        return -1;

> +    }

> +    return 0;

>  }

>  

>  static const VMStateDescription tz_mpc_vmstate = {

>      .name = "tz-mpc",

>      .version_id = 1,

>      .minimum_version_id = 1,

> +    .post_load = tz_mpc_post_load,

>      .fields = (VMStateField[]) {

> +        VMSTATE_UINT32(ctrl, TZMPC),

> +        VMSTATE_UINT32(blk_idx, TZMPC),

> +        VMSTATE_UINT32(int_stat, TZMPC),

> +        VMSTATE_UINT32(int_en, TZMPC),

> +        VMSTATE_UINT32(int_info1, TZMPC),

> +        VMSTATE_UINT32(int_info2, TZMPC),

> +        VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,

> +                              0, vmstate_info_uint32, uint32_t),

>          VMSTATE_END_OF_LIST()

>      }

>  };

>
Eric Auger June 14, 2018, 8:36 p.m. | #2
Hi Peter,

On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.

> 

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

> ---

>  include/hw/misc/tz-mpc.h |  10 +++

>  hw/misc/tz-mpc.c         | 137 ++++++++++++++++++++++++++++++++++++++-

>  2 files changed, 144 insertions(+), 3 deletions(-)

> 

> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h

> index b5eaf1699ea..1fff4d6029a 100644

> --- a/include/hw/misc/tz-mpc.h

> +++ b/include/hw/misc/tz-mpc.h

> @@ -48,6 +48,16 @@ struct TZMPC {

>  

>      /*< public >*/

>  

> +    /* State */

> +    uint32_t ctrl;

> +    uint32_t blk_idx;

> +    uint32_t int_stat;

> +    uint32_t int_en;

> +    uint32_t int_info1;

> +    uint32_t int_info2;

> +

> +    uint32_t *blk_lut;

> +

>      qemu_irq irq;

>  

>      /* Properties */

> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c

> index d4467ccc3b2..9db55e23daf 100644

> --- a/hw/misc/tz-mpc.c

> +++ b/hw/misc/tz-mpc.c

> @@ -28,16 +28,23 @@ enum {

>  

>  /* Config registers */

>  REG32(CTRL, 0x00)

> +    FIELD(CTRL, SEC_RESP, 4, 1)

> +    FIELD(CTRL, AUTOINC, 8, 1)

> +    FIELD(CTRL, LOCKDOWN, 31, 1)

>  REG32(BLK_MAX, 0x10)

>  REG32(BLK_CFG, 0x14)

>  REG32(BLK_IDX, 0x18)

>  REG32(BLK_LUT, 0x1c)

>  REG32(INT_STAT, 0x20)

> +    FIELD(INT_STAT, IRQ, 0, 1)

>  REG32(INT_CLEAR, 0x24)

> +    FIELD(INT_CLEAR, IRQ, 0, 1)

>  REG32(INT_EN, 0x28)

> +    FIELD(INT_EN, IRQ, 0, 1)

>  REG32(INT_INFO1, 0x2c)

>  REG32(INT_INFO2, 0x30)

>  REG32(INT_SET, 0x34)

> +    FIELD(INT_SET, IRQ, 0, 1)

>  REG32(PIDR4, 0xfd0)

>  REG32(PIDR5, 0xfd4)

>  REG32(PIDR6, 0xfd8)

> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {

>      0x0d, 0xf0, 0x05, 0xb1,

>  };

>  

> +static void tz_mpc_irq_update(TZMPC *s)

> +{

> +    qemu_set_irq(s->irq, s->int_stat && s->int_en);

> +}

> +

>  static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,

>                                     uint64_t *pdata,

>                                     unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint64_t r;

>      uint32_t offset = addr & ~0x3;

>  

>      switch (offset) {

> +    case A_CTRL:

> +        r = s->ctrl;

> +        break;

> +    case A_BLK_MAX:

> +        r = s->blk_max;

> +        break;

> +    case A_BLK_CFG:

> +        /* We are never in "init in progress state", so this just indicates

> +         * the block size. s->blocksize == (1 << BLK_CFG + 5), so

> +         * BLK_CFG == ctz32(s->blocksize) - 5

> +         */

> +        r = ctz32(s->blocksize) - 5;

> +        break;

> +    case A_BLK_IDX:

> +        r = s->blk_idx;

> +        break;

> +    case A_BLK_LUT:

> +        r = s->blk_lut[s->blk_idx];

> +        if (size == 4) {

> +            s->blk_idx++;

> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_STAT:

> +        r = s->int_stat;

> +        break;

> +    case A_INT_EN:

> +        r = s->int_en;

> +        break;

> +    case A_INT_INFO1:

> +        r = s->int_info1;

> +        break;

> +    case A_INT_INFO2:

> +        r = s->int_info2;

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>                                      uint64_t value,

>                                      unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint32_t offset = addr & ~0x3;

>  

>      trace_tz_mpc_reg_write(addr, value, size);

> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          uint32_t oldval;

>  

>          switch (offset) {

> -            /* As we add support for registers which need expansions

> -             * other than zeroes we'll fill in cases here.

> -             */

> +        case A_CTRL:

> +            oldval = s->ctrl;

> +            break;

> +        case A_BLK_IDX:

> +            oldval = s->blk_idx;

> +            break;

> +        case A_BLK_LUT:

> +            oldval = s->blk_lut[s->blk_idx];

> +            break;

>          default:

>              oldval = 0;

>              break;

> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          value = deposit32(oldval, (addr & 3) * 8, size * 8, value);

>      }

>  

> +    if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&

> +        (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {

> +        /* Lockdown mode makes these three registers read-only, and

> +         * the only way out of it is to reset the device.

> +         */

> +        qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "

> +                      "while MPC is in lockdown mode\n", offset);

> +        return MEMTX_OK;

> +    }

> +

>      switch (offset) {

> +    case A_CTRL:

> +        /* We don't implement the 'data gating' feature so all other bits

> +         * are reserved and we make them RAZ/WI.

> +         */

> +        s->ctrl = value & (R_CTRL_SEC_RESP_MASK |

> +                           R_CTRL_AUTOINC_MASK |

> +                           R_CTRL_LOCKDOWN_MASK);

> +        break;

> +    case A_BLK_IDX:

> +        s->blk_idx = value % s->blk_max;

> +        break;

> +    case A_BLK_LUT:

> +        s->blk_lut[s->blk_idx] = value;

> +        if (size == 4) {

> +            s->blk_idx++;

> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_CLEAR:

> +        if (value & R_INT_CLEAR_IRQ_MASK) {

> +            s->int_stat = 0;

> +            tz_mpc_irq_update(s);

don't you need to clear the info regs. spec says:
the [info] register retains its value until mpc_irq is cleared.

Thanks

Eric

> +        }

> +        break;

> +    case A_INT_EN:

> +        s->int_en = value & R_INT_EN_IRQ_MASK;

> +        tz_mpc_irq_update(s);

> +        break;

> +    case A_INT_SET:

> +        if (value & R_INT_SET_IRQ_MASK) {

> +            s->int_stat = R_INT_STAT_IRQ_MASK;

> +            tz_mpc_irq_update(s);

> +        }

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)

>  

>  static void tz_mpc_reset(DeviceState *dev)

>  {

> +    TZMPC *s = TZ_MPC(dev);

> +

> +    s->ctrl = 0x00000100;

> +    s->blk_idx = 0;

> +    s->int_stat = 0;

> +    s->int_en = 1;

> +    s->int_info1 = 0;

> +    s->int_info2 = 0;

> +

> +    memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));

>  }

>  

>  static void tz_mpc_init(Object *obj)

> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)

>                         "tz-mpc-downstream");

>      address_space_init(&s->blocked_io_as, &s->blocked_io,

>                         "tz-mpc-blocked-io");

> +

> +    s->blk_lut = g_new(uint32_t, s->blk_max);

> +}

> +

> +static int tz_mpc_post_load(void *opaque, int version_id)

> +{

> +    TZMPC *s = TZ_MPC(opaque);

> +

> +    /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */

> +    if (s->blk_idx >= s->blk_max) {

> +        return -1;

> +    }

> +    return 0;

>  }

>  

>  static const VMStateDescription tz_mpc_vmstate = {

>      .name = "tz-mpc",

>      .version_id = 1,

>      .minimum_version_id = 1,

> +    .post_load = tz_mpc_post_load,

>      .fields = (VMStateField[]) {

> +        VMSTATE_UINT32(ctrl, TZMPC),

> +        VMSTATE_UINT32(blk_idx, TZMPC),

> +        VMSTATE_UINT32(int_stat, TZMPC),

> +        VMSTATE_UINT32(int_en, TZMPC),

> +        VMSTATE_UINT32(int_info1, TZMPC),

> +        VMSTATE_UINT32(int_info2, TZMPC),

> +        VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,

> +                              0, vmstate_info_uint32, uint32_t),

>          VMSTATE_END_OF_LIST()

>      }

>  };

>
Eric Auger June 15, 2018, 7:23 a.m. | #3
Hi Peter,

On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.

> 

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

> ---

>  include/hw/misc/tz-mpc.h |  10 +++

>  hw/misc/tz-mpc.c         | 137 ++++++++++++++++++++++++++++++++++++++-

>  2 files changed, 144 insertions(+), 3 deletions(-)

> 

> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h

> index b5eaf1699ea..1fff4d6029a 100644

> --- a/include/hw/misc/tz-mpc.h

> +++ b/include/hw/misc/tz-mpc.h

> @@ -48,6 +48,16 @@ struct TZMPC {

>  

>      /*< public >*/

>  

> +    /* State */

> +    uint32_t ctrl;

> +    uint32_t blk_idx;

> +    uint32_t int_stat;

> +    uint32_t int_en;

> +    uint32_t int_info1;

> +    uint32_t int_info2;

> +

> +    uint32_t *blk_lut;

> +

>      qemu_irq irq;

>  

>      /* Properties */

> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c

> index d4467ccc3b2..9db55e23daf 100644

> --- a/hw/misc/tz-mpc.c

> +++ b/hw/misc/tz-mpc.c

> @@ -28,16 +28,23 @@ enum {

>  

>  /* Config registers */

>  REG32(CTRL, 0x00)

> +    FIELD(CTRL, SEC_RESP, 4, 1)

> +    FIELD(CTRL, AUTOINC, 8, 1)

> +    FIELD(CTRL, LOCKDOWN, 31, 1)

>  REG32(BLK_MAX, 0x10)

>  REG32(BLK_CFG, 0x14)

>  REG32(BLK_IDX, 0x18)

>  REG32(BLK_LUT, 0x1c)

>  REG32(INT_STAT, 0x20)

> +    FIELD(INT_STAT, IRQ, 0, 1)

>  REG32(INT_CLEAR, 0x24)

> +    FIELD(INT_CLEAR, IRQ, 0, 1)

>  REG32(INT_EN, 0x28)

> +    FIELD(INT_EN, IRQ, 0, 1)

>  REG32(INT_INFO1, 0x2c)

>  REG32(INT_INFO2, 0x30)

>  REG32(INT_SET, 0x34)

> +    FIELD(INT_SET, IRQ, 0, 1)

>  REG32(PIDR4, 0xfd0)

>  REG32(PIDR5, 0xfd4)

>  REG32(PIDR6, 0xfd8)

> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {

>      0x0d, 0xf0, 0x05, 0xb1,

>  };

>  

> +static void tz_mpc_irq_update(TZMPC *s)

> +{

> +    qemu_set_irq(s->irq, s->int_stat && s->int_en);

> +}

> +

>  static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,

>                                     uint64_t *pdata,

>                                     unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint64_t r;

>      uint32_t offset = addr & ~0x3;

>  

>      switch (offset) {

> +    case A_CTRL:

> +        r = s->ctrl;

> +        break;

> +    case A_BLK_MAX:

> +        r = s->blk_max;

> +        break;

> +    case A_BLK_CFG:

> +        /* We are never in "init in progress state", so this just indicates

> +         * the block size. s->blocksize == (1 << BLK_CFG + 5), so

> +         * BLK_CFG == ctz32(s->blocksize) - 5

> +         */

> +        r = ctz32(s->blocksize) - 5;

> +        break;

> +    case A_BLK_IDX:

> +        r = s->blk_idx;

> +        break;

> +    case A_BLK_LUT:

> +        r = s->blk_lut[s->blk_idx];

> +        if (size == 4) {

don't you need to test CTRL[8] before doing the auto-increment?

spec says:
"A full word write or read to this register
automatically increments the BLK_IDX by one if
enabled by CTRL[8]."
> +            s->blk_idx++;

> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_STAT:

> +        r = s->int_stat;

> +        break;

> +    case A_INT_EN:

> +        r = s->int_en;

> +        break;

> +    case A_INT_INFO1:

> +        r = s->int_info1;

> +        break;

> +    case A_INT_INFO2:

> +        r = s->int_info2;

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>                                      uint64_t value,

>                                      unsigned size, MemTxAttrs attrs)

>  {

> +    TZMPC *s = TZ_MPC(opaque);

>      uint32_t offset = addr & ~0x3;

>  

>      trace_tz_mpc_reg_write(addr, value, size);

> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          uint32_t oldval;

>  

>          switch (offset) {

> -            /* As we add support for registers which need expansions

> -             * other than zeroes we'll fill in cases here.

> -             */

> +        case A_CTRL:

> +            oldval = s->ctrl;

> +            break;

> +        case A_BLK_IDX:

> +            oldval = s->blk_idx;

> +            break;

> +        case A_BLK_LUT:

> +            oldval = s->blk_lut[s->blk_idx];

> +            break;

>          default:

>              oldval = 0;

>              break;

> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,

>          value = deposit32(oldval, (addr & 3) * 8, size * 8, value);

>      }

>  

> +    if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&

> +        (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {

> +        /* Lockdown mode makes these three registers read-only, and

> +         * the only way out of it is to reset the device.

> +         */

> +        qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "

> +                      "while MPC is in lockdown mode\n", offset);

> +        return MEMTX_OK;

> +    }

> +

>      switch (offset) {

> +    case A_CTRL:

> +        /* We don't implement the 'data gating' feature so all other bits

> +         * are reserved and we make them RAZ/WI.

> +         */

> +        s->ctrl = value & (R_CTRL_SEC_RESP_MASK |

> +                           R_CTRL_AUTOINC_MASK |

> +                           R_CTRL_LOCKDOWN_MASK);

> +        break;

> +    case A_BLK_IDX:

> +        s->blk_idx = value % s->blk_max;

> +        break;

> +    case A_BLK_LUT:

> +        s->blk_lut[s->blk_idx] = value;

> +        if (size == 4) {

> +            s->blk_idx++;

same?

Thanks

Eric
> +            s->blk_idx %= s->blk_max;

> +        }

> +        break;

> +    case A_INT_CLEAR:

> +        if (value & R_INT_CLEAR_IRQ_MASK) {

> +            s->int_stat = 0;

> +            tz_mpc_irq_update(s);

> +        }

> +        break;

> +    case A_INT_EN:

> +        s->int_en = value & R_INT_EN_IRQ_MASK;

> +        tz_mpc_irq_update(s);

> +        break;

> +    case A_INT_SET:

> +        if (value & R_INT_SET_IRQ_MASK) {

> +            s->int_stat = R_INT_STAT_IRQ_MASK;

> +            tz_mpc_irq_update(s);

> +        }

> +        break;

>      case A_PIDR4:

>      case A_PIDR5:

>      case A_PIDR6:

> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)

>  

>  static void tz_mpc_reset(DeviceState *dev)

>  {

> +    TZMPC *s = TZ_MPC(dev);

> +

> +    s->ctrl = 0x00000100;

> +    s->blk_idx = 0;

> +    s->int_stat = 0;

> +    s->int_en = 1;

> +    s->int_info1 = 0;

> +    s->int_info2 = 0;

> +

> +    memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));

>  }

>  

>  static void tz_mpc_init(Object *obj)

> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)

>                         "tz-mpc-downstream");

>      address_space_init(&s->blocked_io_as, &s->blocked_io,

>                         "tz-mpc-blocked-io");

> +

> +    s->blk_lut = g_new(uint32_t, s->blk_max);

> +}

> +

> +static int tz_mpc_post_load(void *opaque, int version_id)

> +{

> +    TZMPC *s = TZ_MPC(opaque);

> +

> +    /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */

> +    if (s->blk_idx >= s->blk_max) {

> +        return -1;

> +    }

> +    return 0;

>  }

>  

>  static const VMStateDescription tz_mpc_vmstate = {

>      .name = "tz-mpc",

>      .version_id = 1,

>      .minimum_version_id = 1,

> +    .post_load = tz_mpc_post_load,

>      .fields = (VMStateField[]) {

> +        VMSTATE_UINT32(ctrl, TZMPC),

> +        VMSTATE_UINT32(blk_idx, TZMPC),

> +        VMSTATE_UINT32(int_stat, TZMPC),

> +        VMSTATE_UINT32(int_en, TZMPC),

> +        VMSTATE_UINT32(int_info1, TZMPC),

> +        VMSTATE_UINT32(int_info2, TZMPC),

> +        VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,

> +                              0, vmstate_info_uint32, uint32_t),

>          VMSTATE_END_OF_LIST()

>      }

>  };

>
Peter Maydell June 15, 2018, 8:59 a.m. | #4
On 14 June 2018 at 21:14, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,

>

> On 06/04/2018 05:29 PM, Peter Maydell wrote:

>> Implement the missing registers for the TZ MPC.

>>

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


>>  static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,

>>                                     uint64_t *pdata,

>>                                     unsigned size, MemTxAttrs attrs)

>>  {

>> +    TZMPC *s = TZ_MPC(opaque);

>>      uint64_t r;

>>      uint32_t offset = addr & ~0x3;

> I don't get where do we check if the access is secure or not. Shouldn't

> we test attrs.secure somewhere (PPROT[1] == 0)? Also the spec says

> IDregs can be read by any type of access. Where is this differentiation

> done?


I'd missed that bit of the spec -- yes, we should be checking
attrs.secure to see if we should allow access to the non-ID registers.
For the MPS2 boards it doesn't matter because the registers are
in a part of the memory map that is enforced as secure-access-only
by the IDAU.

thanks
-- PMM
Peter Maydell June 15, 2018, 9:04 a.m. | #5
On 14 June 2018 at 21:36, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,

>

> On 06/04/2018 05:29 PM, Peter Maydell wrote:

>> Implement the missing registers for the TZ MPC.

>>

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


>> +    case A_INT_CLEAR:

>> +        if (value & R_INT_CLEAR_IRQ_MASK) {

>> +            s->int_stat = 0;

>> +            tz_mpc_irq_update(s);

> don't you need to clear the info regs. spec says:

> the [info] register retains its value until mpc_irq is cleared.


The full sentence is "Subsequent security violating transfers
remain blocked, that is, not captured in this register
and the register retains its value until mpc_irq is cleared."
I interpret "until mpc_irq is cleared" as applying to the
entire thing, ie mpc_irq being cleared is what allows a
subsequent transfer to be captured in this register.
(Hardware actively clearing itself to zero is unlikely,
because that costs extra gates which designers don't tend
to do unless there's a reason for it.)

From a guest point of view (which is kind of the pov the
docs are written from), the guest can't rely on the register
value once mpc_irq is cleared (because another transaction
might come along and cause the value to be overwritten).

thanks
-- PMM
Peter Maydell June 15, 2018, 9:05 a.m. | #6
On 15 June 2018 at 08:23, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,

>

> On 06/04/2018 05:29 PM, Peter Maydell wrote:

>> Implement the missing registers for the TZ MPC.

>>

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


>> +    case A_BLK_LUT:

>> +        r = s->blk_lut[s->blk_idx];

>> +        if (size == 4) {

> don't you need to test CTRL[8] before doing the auto-increment?

>

> spec says:

> "A full word write or read to this register

> automatically increments the BLK_IDX by one if

> enabled by CTRL[8]."


Oops, yes, definitely.

thanks
-- PMM
Eric Auger June 15, 2018, 1:24 p.m. | #7
Hi Peter,

On 06/15/2018 11:04 AM, Peter Maydell wrote:
> On 14 June 2018 at 21:36, Auger Eric <eric.auger@redhat.com> wrote:

>> Hi Peter,

>>

>> On 06/04/2018 05:29 PM, Peter Maydell wrote:

>>> Implement the missing registers for the TZ MPC.

>>>

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

> 

>>> +    case A_INT_CLEAR:

>>> +        if (value & R_INT_CLEAR_IRQ_MASK) {

>>> +            s->int_stat = 0;

>>> +            tz_mpc_irq_update(s);

>> don't you need to clear the info regs. spec says:

>> the [info] register retains its value until mpc_irq is cleared.

> 

> The full sentence is "Subsequent security violating transfers

> remain blocked, that is, not captured in this register

> and the register retains its value until mpc_irq is cleared."

> I interpret "until mpc_irq is cleared" as applying to the

> entire thing, ie mpc_irq being cleared is what allows a

> subsequent transfer to be captured in this register.

> (Hardware actively clearing itself to zero is unlikely,

> because that costs extra gates which designers don't tend

> to do unless there's a reason for it.)

> 

> From a guest point of view (which is kind of the pov the

> docs are written from), the guest can't rely on the register

> value once mpc_irq is cleared (because another transaction

> might come along and cause the value to be overwritten).

OK

Thanks

Eric
> 

> thanks

> -- PMM

>

Patch

diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
index b5eaf1699ea..1fff4d6029a 100644
--- a/include/hw/misc/tz-mpc.h
+++ b/include/hw/misc/tz-mpc.h
@@ -48,6 +48,16 @@  struct TZMPC {
 
     /*< public >*/
 
+    /* State */
+    uint32_t ctrl;
+    uint32_t blk_idx;
+    uint32_t int_stat;
+    uint32_t int_en;
+    uint32_t int_info1;
+    uint32_t int_info2;
+
+    uint32_t *blk_lut;
+
     qemu_irq irq;
 
     /* Properties */
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index d4467ccc3b2..9db55e23daf 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -28,16 +28,23 @@  enum {
 
 /* Config registers */
 REG32(CTRL, 0x00)
+    FIELD(CTRL, SEC_RESP, 4, 1)
+    FIELD(CTRL, AUTOINC, 8, 1)
+    FIELD(CTRL, LOCKDOWN, 31, 1)
 REG32(BLK_MAX, 0x10)
 REG32(BLK_CFG, 0x14)
 REG32(BLK_IDX, 0x18)
 REG32(BLK_LUT, 0x1c)
 REG32(INT_STAT, 0x20)
+    FIELD(INT_STAT, IRQ, 0, 1)
 REG32(INT_CLEAR, 0x24)
+    FIELD(INT_CLEAR, IRQ, 0, 1)
 REG32(INT_EN, 0x28)
+    FIELD(INT_EN, IRQ, 0, 1)
 REG32(INT_INFO1, 0x2c)
 REG32(INT_INFO2, 0x30)
 REG32(INT_SET, 0x34)
+    FIELD(INT_SET, IRQ, 0, 1)
 REG32(PIDR4, 0xfd0)
 REG32(PIDR5, 0xfd4)
 REG32(PIDR6, 0xfd8)
@@ -57,14 +64,55 @@  static const uint8_t tz_mpc_idregs[] = {
     0x0d, 0xf0, 0x05, 0xb1,
 };
 
+static void tz_mpc_irq_update(TZMPC *s)
+{
+    qemu_set_irq(s->irq, s->int_stat && s->int_en);
+}
+
 static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
                                    uint64_t *pdata,
                                    unsigned size, MemTxAttrs attrs)
 {
+    TZMPC *s = TZ_MPC(opaque);
     uint64_t r;
     uint32_t offset = addr & ~0x3;
 
     switch (offset) {
+    case A_CTRL:
+        r = s->ctrl;
+        break;
+    case A_BLK_MAX:
+        r = s->blk_max;
+        break;
+    case A_BLK_CFG:
+        /* We are never in "init in progress state", so this just indicates
+         * the block size. s->blocksize == (1 << BLK_CFG + 5), so
+         * BLK_CFG == ctz32(s->blocksize) - 5
+         */
+        r = ctz32(s->blocksize) - 5;
+        break;
+    case A_BLK_IDX:
+        r = s->blk_idx;
+        break;
+    case A_BLK_LUT:
+        r = s->blk_lut[s->blk_idx];
+        if (size == 4) {
+            s->blk_idx++;
+            s->blk_idx %= s->blk_max;
+        }
+        break;
+    case A_INT_STAT:
+        r = s->int_stat;
+        break;
+    case A_INT_EN:
+        r = s->int_en;
+        break;
+    case A_INT_INFO1:
+        r = s->int_info1;
+        break;
+    case A_INT_INFO2:
+        r = s->int_info2;
+        break;
     case A_PIDR4:
     case A_PIDR5:
     case A_PIDR6:
@@ -110,6 +158,7 @@  static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
                                     uint64_t value,
                                     unsigned size, MemTxAttrs attrs)
 {
+    TZMPC *s = TZ_MPC(opaque);
     uint32_t offset = addr & ~0x3;
 
     trace_tz_mpc_reg_write(addr, value, size);
@@ -122,9 +171,15 @@  static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
         uint32_t oldval;
 
         switch (offset) {
-            /* As we add support for registers which need expansions
-             * other than zeroes we'll fill in cases here.
-             */
+        case A_CTRL:
+            oldval = s->ctrl;
+            break;
+        case A_BLK_IDX:
+            oldval = s->blk_idx;
+            break;
+        case A_BLK_LUT:
+            oldval = s->blk_lut[s->blk_idx];
+            break;
         default:
             oldval = 0;
             break;
@@ -132,7 +187,51 @@  static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
         value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
     }
 
+    if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&
+        (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {
+        /* Lockdown mode makes these three registers read-only, and
+         * the only way out of it is to reset the device.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "
+                      "while MPC is in lockdown mode\n", offset);
+        return MEMTX_OK;
+    }
+
     switch (offset) {
+    case A_CTRL:
+        /* We don't implement the 'data gating' feature so all other bits
+         * are reserved and we make them RAZ/WI.
+         */
+        s->ctrl = value & (R_CTRL_SEC_RESP_MASK |
+                           R_CTRL_AUTOINC_MASK |
+                           R_CTRL_LOCKDOWN_MASK);
+        break;
+    case A_BLK_IDX:
+        s->blk_idx = value % s->blk_max;
+        break;
+    case A_BLK_LUT:
+        s->blk_lut[s->blk_idx] = value;
+        if (size == 4) {
+            s->blk_idx++;
+            s->blk_idx %= s->blk_max;
+        }
+        break;
+    case A_INT_CLEAR:
+        if (value & R_INT_CLEAR_IRQ_MASK) {
+            s->int_stat = 0;
+            tz_mpc_irq_update(s);
+        }
+        break;
+    case A_INT_EN:
+        s->int_en = value & R_INT_EN_IRQ_MASK;
+        tz_mpc_irq_update(s);
+        break;
+    case A_INT_SET:
+        if (value & R_INT_SET_IRQ_MASK) {
+            s->int_stat = R_INT_STAT_IRQ_MASK;
+            tz_mpc_irq_update(s);
+        }
+        break;
     case A_PIDR4:
     case A_PIDR5:
     case A_PIDR6:
@@ -248,6 +347,16 @@  static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
 
 static void tz_mpc_reset(DeviceState *dev)
 {
+    TZMPC *s = TZ_MPC(dev);
+
+    s->ctrl = 0x00000100;
+    s->blk_idx = 0;
+    s->int_stat = 0;
+    s->int_en = 1;
+    s->int_info1 = 0;
+    s->int_info2 = 0;
+
+    memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));
 }
 
 static void tz_mpc_init(Object *obj)
@@ -321,13 +430,35 @@  static void tz_mpc_realize(DeviceState *dev, Error **errp)
                        "tz-mpc-downstream");
     address_space_init(&s->blocked_io_as, &s->blocked_io,
                        "tz-mpc-blocked-io");
+
+    s->blk_lut = g_new(uint32_t, s->blk_max);
+}
+
+static int tz_mpc_post_load(void *opaque, int version_id)
+{
+    TZMPC *s = TZ_MPC(opaque);
+
+    /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */
+    if (s->blk_idx >= s->blk_max) {
+        return -1;
+    }
+    return 0;
 }
 
 static const VMStateDescription tz_mpc_vmstate = {
     .name = "tz-mpc",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = tz_mpc_post_load,
     .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ctrl, TZMPC),
+        VMSTATE_UINT32(blk_idx, TZMPC),
+        VMSTATE_UINT32(int_stat, TZMPC),
+        VMSTATE_UINT32(int_en, TZMPC),
+        VMSTATE_UINT32(int_info1, TZMPC),
+        VMSTATE_UINT32(int_info2, TZMPC),
+        VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,
+                              0, vmstate_info_uint32, uint32_t),
         VMSTATE_END_OF_LIST()
     }
 };