[v2,08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate

Message ID 20180604152941.20374-9-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.
The final part of the Memory Protection Controller we need to
implement is actually using the BLK_LUT data programmed by the
guest to determine whether to block the transaction or not.

Since this means we now change transaction mappings when
the guest writes to BLK_LUT, we must also call the IOMMU
notifiers at that point.

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

---
 hw/misc/tz-mpc.c     | 50 ++++++++++++++++++++++++++++++++++++++++++--
 hw/misc/trace-events |  1 +
 2 files changed, 49 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Auger Eric June 15, 2018, 7:31 a.m. | #1
Hi Peter,

On 06/04/2018 05:29 PM, Peter Maydell wrote:
> The final part of the Memory Protection Controller we need to

> implement is actually using the BLK_LUT data programmed by the

> guest to determine whether to block the transaction or not.

> 

> Since this means we now change transaction mappings when

> the guest writes to BLK_LUT, we must also call the IOMMU

> notifiers at that point.

> 

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

> ---

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

>  hw/misc/trace-events |  1 +

>  2 files changed, 49 insertions(+), 2 deletions(-)

> 

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

> index 704bb3fb44d..7a6117c90b3 100644

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

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

> @@ -72,6 +72,50 @@ static void tz_mpc_irq_update(TZMPC *s)

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

>  }

>  

> +static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx,

> +                                uint32_t oldlut, uint32_t newlut)

> +{

> +    /* Called when the LUT word at lutidx has changed from oldlut to newlut;

> +     * must call the IOMMU notifiers for the changed blocks.

> +     */

> +    IOMMUTLBEntry entry = {

> +        .addr_mask = s->blocksize - 1,

> +    };

> +    hwaddr addr = lutidx * s->blocksize * 32;

> +    int i;

> +

> +    for (i = 0; i < 32; i++, addr += s->blocksize) {

> +        if (!((oldlut ^ newlut) & (1 << i))) {

> +            continue;

> +        }

> +        /* This changes the mappings for both the S and the NS space,

> +         * so we need to do four notifies: an UNMAP then a MAP for each.

> +         */

> +

> +        trace_tz_mpc_iommu_notify(addr);

> +        entry.iova = addr;

> +        entry.translated_addr = addr;

> +

> +        entry.perm = IOMMU_NONE;

> +        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);

> +        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);

> +

> +        entry.perm = IOMMU_RW;


> +        if (newlut & (1 << i)) {

Add a comment saying the block is configured as non-secure?
> +            entry.target_as = &s->blocked_io_as;

> +        } else {

> +            entry.target_as = &s->downstream_as;

so this disallows any secure access to non secure blocks. I have a
reduced knowledge of TZ but I would have thought that a secure access
would have sufficient priviledge to access NS memory?

Thanks

Eric
> +        }

> +        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);

> +        if (newlut & (1 << i)) {

> +            entry.target_as = &s->downstream_as;

> +        } else {

> +            entry.target_as = &s->blocked_io_as;

> +        }

> +        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);

> +    }

> +}

> +

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

>                                     uint64_t *pdata,

>                                     unsigned size, MemTxAttrs attrs)

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

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

>          break;

>      case A_BLK_LUT:

> +        tz_mpc_iommu_notify(s, s->blk_idx, s->blk_lut[s->blk_idx], value);

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

>          if (size == 4) {

>              s->blk_idx++;

> @@ -362,9 +407,10 @@ static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,

>      /* Look at the per-block configuration for this address, and

>       * return a TLB entry directing the transaction at either

>       * downstream_as or blocked_io_as, as appropriate.

> -     * For the moment, always permit accesses.

> +     * If the LUT cfg_ns bit is 1, only non-secure transactions

> +     * may pass. If the bit is 0, only secure transactions may pass.

>       */

> -    ok = true;

> +    ok = tz_mpc_cfg_ns(s, addr) == (iommu_idx == IOMMU_IDX_NS);

>  

>      trace_tz_mpc_translate(addr, flags,

>                             iommu_idx == IOMMU_IDX_S ? "S" : "NS",

> diff --git a/hw/misc/trace-events b/hw/misc/trace-events

> index 72bf9d57000..c956e1419b7 100644

> --- a/hw/misc/trace-events

> +++ b/hw/misc/trace-events

> @@ -90,6 +90,7 @@ tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs wri

>  tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"

>  tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"

>  tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"

> +tz_mpc_iommu_notify(uint64_t addr) "TZ MPC iommu: notifying UNMAP/MAP for 0x%" PRIx64

>  

>  # hw/misc/tz-ppc.c

>  tz_ppc_reset(void) "TZ PPC: reset"

>
Peter Maydell June 15, 2018, 4:07 p.m. | #2
On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,

>

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

>> The final part of the Memory Protection Controller we need to

>> implement is actually using the BLK_LUT data programmed by the

>> guest to determine whether to block the transaction or not.

>>

>> Since this means we now change transaction mappings when

>> the guest writes to BLK_LUT, we must also call the IOMMU

>> notifiers at that point.

>>

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


>> +        if (newlut & (1 << i)) {

> Add a comment saying the block is configured as non-secure?


Sure.

>> +            entry.target_as = &s->blocked_io_as;

>> +        } else {

>> +            entry.target_as = &s->downstream_as;

> so this disallows any secure access to non secure blocks. I have a

> reduced knowledge of TZ but I would have thought that a secure access

> would have sufficient priviledge to access NS memory?


The spec is not as clear as it could be on this point, but...

>> @@ -362,9 +407,10 @@ static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,

>>      /* Look at the per-block configuration for this address, and

>>       * return a TLB entry directing the transaction at either

>>       * downstream_as or blocked_io_as, as appropriate.

>> -     * For the moment, always permit accesses.

>> +     * If the LUT cfg_ns bit is 1, only non-secure transactions

>> +     * may pass. If the bit is 0, only secure transactions may pass.

>>       */


...as this comment says, the MPC doesn't permit wrong-secure-state
transactions either way. (This is the same behaviour as for the
Peripheral Protection Controller, whose spec is clearer.)

thanks
-- PMM
Peter Maydell June 15, 2018, 4:09 p.m. | #3
On 15 June 2018 at 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:

>> Hi Peter,

>>

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

>>> The final part of the Memory Protection Controller we need to

>>> implement is actually using the BLK_LUT data programmed by the

>>> guest to determine whether to block the transaction or not.

>>>

>>> Since this means we now change transaction mappings when

>>> the guest writes to BLK_LUT, we must also call the IOMMU

>>> notifiers at that point.

>>>

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

>

>>> +        if (newlut & (1 << i)) {

>> Add a comment saying the block is configured as non-secure?


...actually, how about instead having a new variable
   bool block_is_ns = newlut & (1 << i);

and then here and in the later if() having "if (block_is_ns)" ?
I think that clarifies without needing a comment.

thanks
-- PMM
Auger Eric June 18, 2018, 7:45 a.m. | #4
Hi Peter,
On 06/15/2018 06:09 PM, Peter Maydell wrote:
> On 15 June 2018 at 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:

>> On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:

>>> Hi Peter,

>>>

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

>>>> The final part of the Memory Protection Controller we need to

>>>> implement is actually using the BLK_LUT data programmed by the

>>>> guest to determine whether to block the transaction or not.

>>>>

>>>> Since this means we now change transaction mappings when

>>>> the guest writes to BLK_LUT, we must also call the IOMMU

>>>> notifiers at that point.

>>>>

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

>>

>>>> +        if (newlut & (1 << i)) {

>>> Add a comment saying the block is configured as non-secure?

> 

> ...actually, how about instead having a new variable

>    bool block_is_ns = newlut & (1 << i);

> 

> and then here and in the later if() having "if (block_is_ns)" ?

> I think that clarifies without needing a comment.

yes, looks good to me

Thanks

Eric
> 

> thanks

> -- PMM

>

Patch

diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index 704bb3fb44d..7a6117c90b3 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -72,6 +72,50 @@  static void tz_mpc_irq_update(TZMPC *s)
     qemu_set_irq(s->irq, s->int_stat && s->int_en);
 }
 
+static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx,
+                                uint32_t oldlut, uint32_t newlut)
+{
+    /* Called when the LUT word at lutidx has changed from oldlut to newlut;
+     * must call the IOMMU notifiers for the changed blocks.
+     */
+    IOMMUTLBEntry entry = {
+        .addr_mask = s->blocksize - 1,
+    };
+    hwaddr addr = lutidx * s->blocksize * 32;
+    int i;
+
+    for (i = 0; i < 32; i++, addr += s->blocksize) {
+        if (!((oldlut ^ newlut) & (1 << i))) {
+            continue;
+        }
+        /* This changes the mappings for both the S and the NS space,
+         * so we need to do four notifies: an UNMAP then a MAP for each.
+         */
+
+        trace_tz_mpc_iommu_notify(addr);
+        entry.iova = addr;
+        entry.translated_addr = addr;
+
+        entry.perm = IOMMU_NONE;
+        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
+        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
+
+        entry.perm = IOMMU_RW;
+        if (newlut & (1 << i)) {
+            entry.target_as = &s->blocked_io_as;
+        } else {
+            entry.target_as = &s->downstream_as;
+        }
+        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
+        if (newlut & (1 << i)) {
+            entry.target_as = &s->downstream_as;
+        } else {
+            entry.target_as = &s->blocked_io_as;
+        }
+        memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
+    }
+}
+
 static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
                                    uint64_t *pdata,
                                    unsigned size, MemTxAttrs attrs)
@@ -213,6 +257,7 @@  static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
         s->blk_idx = value % s->blk_max;
         break;
     case A_BLK_LUT:
+        tz_mpc_iommu_notify(s, s->blk_idx, s->blk_lut[s->blk_idx], value);
         s->blk_lut[s->blk_idx] = value;
         if (size == 4) {
             s->blk_idx++;
@@ -362,9 +407,10 @@  static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
     /* Look at the per-block configuration for this address, and
      * return a TLB entry directing the transaction at either
      * downstream_as or blocked_io_as, as appropriate.
-     * For the moment, always permit accesses.
+     * If the LUT cfg_ns bit is 1, only non-secure transactions
+     * may pass. If the bit is 0, only secure transactions may pass.
      */
-    ok = true;
+    ok = tz_mpc_cfg_ns(s, addr) == (iommu_idx == IOMMU_IDX_NS);
 
     trace_tz_mpc_translate(addr, flags,
                            iommu_idx == IOMMU_IDX_S ? "S" : "NS",
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 72bf9d57000..c956e1419b7 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -90,6 +90,7 @@  tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs wri
 tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
 tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
 tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
+tz_mpc_iommu_notify(uint64_t addr) "TZ MPC iommu: notifying UNMAP/MAP for 0x%" PRIx64
 
 # hw/misc/tz-ppc.c
 tz_ppc_reset(void) "TZ PPC: reset"