diff mbox

[09/16] target-arm: Support multiple address spaces in page table walks

Message ID 1446747358-18214-10-git-send-email-peter.maydell@linaro.org
State Accepted
Headers show

Commit Message

Peter Maydell Nov. 5, 2015, 6:15 p.m. UTC 
If we have a secure address space, use it in page table walks:
 * when doing the physical accesses to read descriptors,
   make them through the correct address space
 * when the final result indicates a secure access, pass the
   correct address space index to tlb_set_page_with_attrs()

(The descriptor reads are the only direct physical accesses
made in target-arm/ for CPUs which might have TrustZone.)

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

---
 target-arm/cpu.h    | 29 +++++++++++++++++++++++++++++
 target-arm/helper.c | 10 +++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

-- 
1.9.1

Comments

Peter Maydell Nov. 9, 2015, 10:58 a.m. UTC | #1 
On 9 November 2015 at 10:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 05/11/2015 19:15, Peter Maydell wrote:

>> If we have a secure address space, use it in page table walks:

>>  * when doing the physical accesses to read descriptors,

>>    make them through the correct address space

>>  * when the final result indicates a secure access, pass the

>>    correct address space index to tlb_set_page_with_attrs()

>>

>> (The descriptor reads are the only direct physical accesses

>> made in target-arm/ for CPUs which might have TrustZone.)

>

> What is the case where you have no secure address space and you have

> TrustZone?  KVM doesn't have TrustZone, so it should never be in a

> secure regime, should it?


You mean "what is the case where is_secure but cpu->num_ases == 1" ?
That happens if you have a TrustZone CPU but the board has only
connected up one address space, because there is no difference
in the view from Secure and NonSecure. (vexpress is like this
in hardware, and most of our board models for TZ CPUS are like
that now even if the real h/w makes a distinction.)

I could have handled that by making the CPU init code always
register two ASes (using the same one twice if the board code
only passes one or using system_address_space twice if the
board code doesn't pass one at all), but that seemed a bit wasteful.

thanks
-- PMM
Peter Maydell Nov. 9, 2015, 11:09 a.m. UTC | #2 
On 9 November 2015 at 11:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 09/11/2015 11:58, Peter Maydell wrote:

>> You mean "what is the case where is_secure but cpu->num_ases == 1" ?

>> That happens if you have a TrustZone CPU but the board has only

>> connected up one address space, because there is no difference

>> in the view from Secure and NonSecure. (vexpress is like this

>> in hardware, and most of our board models for TZ CPUS are like

>> that now even if the real h/w makes a distinction.)

>>

>> I could have handled that by making the CPU init code always

>> register two ASes (using the same one twice if the board code

>> only passes one or using system_address_space twice if the

>> board code doesn't pass one at all), but that seemed a bit wasteful.

>

> I think it's simpler though.  Complicating the init code is better than

> handling the condition throughout all the helpers...


It was the overhead of having an extra AddressSpace that concerned
me (plus it shows up in things like 'info mtree' somewhat confusingly
if you didn't expect your board to really have 2 ASes). But I don't
feel very strongly about it (or have anything solid to base an
argument either way on).

thanks
-- PMM
Peter Maydell Nov. 9, 2015, 11:22 a.m. UTC | #3 
On 9 November 2015 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 09/11/2015 12:09, Peter Maydell wrote:

>>>> >> I could have handled that by making the CPU init code always

>>>> >> register two ASes (using the same one twice if the board code

>>>> >> only passes one or using system_address_space twice if the

>>>> >> board code doesn't pass one at all), but that seemed a bit wasteful.

>>> >

>>> > I think it's simpler though.  Complicating the init code is better than

>>> > handling the condition throughout all the helpers...

>> It was the overhead of having an extra AddressSpace that concerned

>> me (plus it shows up in things like 'info mtree' somewhat confusingly

>> if you didn't expect your board to really have 2 ASes).

>

> I don't think it shows up twice with address_space_init_shareable, does it?


Yes, looking at the code you're right.

-- PMM
Peter Maydell Nov. 13, 2015, 6:51 p.m. UTC | #4 
On 9 November 2015 at 10:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 November 2015 at 10:51, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>

>>

>> On 05/11/2015 19:15, Peter Maydell wrote:

>>> If we have a secure address space, use it in page table walks:

>>>  * when doing the physical accesses to read descriptors,

>>>    make them through the correct address space

>>>  * when the final result indicates a secure access, pass the

>>>    correct address space index to tlb_set_page_with_attrs()

>>>

>>> (The descriptor reads are the only direct physical accesses

>>> made in target-arm/ for CPUs which might have TrustZone.)

>>

>> What is the case where you have no secure address space and you have

>> TrustZone?  KVM doesn't have TrustZone, so it should never be in a

>> secure regime, should it?

>

> You mean "what is the case where is_secure but cpu->num_ases == 1" ?

> That happens if you have a TrustZone CPU but the board has only

> connected up one address space, because there is no difference

> in the view from Secure and NonSecure. (vexpress is like this

> in hardware, and most of our board models for TZ CPUS are like

> that now even if the real h/w makes a distinction.)


Looking more closely at my own code I was wrong here. The
case where you have only one AS is when the CPU is using KVM.
In that case in theory we should never end up being asked to
pick an AddressSpace for a set of MemTxAttrs that say "secure"
[in the new design where we figure out the asidx from the
txattrs], but it's a bit tricky to be absolutely certain that never
happens...

thanks
-- PMM
diff mbox

Patch

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 815fef8..8dbf4d4 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1720,6 +1720,12 @@  static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
     return el;
 }
 
+/* Indexes used when registering address spaces with cpu_address_space_init */
+typedef enum ARMASIdx {
+    ARMASIdx_NS = 0,
+    ARMASIdx_S = 1,
+} ARMASIdx;
+
 /* Return the Exception Level targeted by debug exceptions;
  * currently always EL1 since we don't implement EL2 or EL3.
  */
@@ -1991,4 +1997,27 @@  enum {
     QEMU_PSCI_CONDUIT_HVC = 2,
 };
 
+#ifndef CONFIG_USER_ONLY
+/* Return the address space index to use for a memory access
+ * (which depends on whether the access is S or NS, and whether
+ * the board gave us a separate AddressSpace for S accesses).
+ */
+static inline int arm_asidx(CPUState *cs, bool is_secure)
+{
+    if (is_secure && cs->num_ases > 1) {
+        return ARMASIdx_S;
+    }
+    return ARMASIdx_NS;
+}
+
+/* Return the AddressSpace to use for a memory access
+ * (which depends on whether the access is S or NS, and whether
+ * the board gave us a separate AddressSpace for S accesses).
+ */
+static inline AddressSpace *arm_addressspace(CPUState *cs, bool is_secure)
+{
+    return cpu_get_address_space(cs, arm_asidx(cs, is_secure));
+}
+#endif
+
 #endif
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 174371b..242928d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6260,13 +6260,14 @@  static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
+    AddressSpace *as = arm_addressspace(cs, is_secure);
 
     attrs.secure = is_secure;
     addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
     if (fi->s1ptw) {
         return 0;
     }
-    return address_space_ldl(cs->as, addr, attrs, NULL);
+    return address_space_ldl(as, addr, attrs, NULL);
 }
 
 static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
@@ -6276,13 +6277,14 @@  static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
+    AddressSpace *as = arm_addressspace(cs, is_secure);
 
     attrs.secure = is_secure;
     addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
     if (fi->s1ptw) {
         return 0;
     }
-    return address_space_ldq(cs->as, addr, attrs, NULL);
+    return address_space_ldq(as, addr, attrs, NULL);
 }
 
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
@@ -7307,6 +7309,7 @@  bool arm_tlb_fill(CPUState *cs, vaddr address,
     target_ulong page_size;
     int prot;
     int ret;
+    int asidx;
     MemTxAttrs attrs = {};
 
     ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
@@ -7315,7 +7318,8 @@  bool arm_tlb_fill(CPUState *cs, vaddr address,
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
         address &= TARGET_PAGE_MASK;
-        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
+        asidx = arm_asidx(cs, attrs.secure);
+        tlb_set_page_with_attrs(cs, address, asidx, phys_addr, attrs,
                                 prot, mmu_idx, page_size);
         return 0;
     }