[Qemu-devel] [RFC PATCH 0/3] target-arm: Some fixes to page and TLB handling

[Qemu-devel] [RFC PATCH 0/3] target-arm: Some fixes to page and TLB handling

[Alex Bennée]

Hi,

While doing some performance analysis on aarch64 system emulation I
noticed a fairly high utilisation of cpu_arm_exec and the related find
next TB machinery. Peter pointed it this is probably not helped by
that fact TARGET_PAGE_BITS was set to 10 (1k pages) which would imply
less chaining of TBs than we should be able to get. However enabling
TARGET_PAGE_BITS 12 managed to shake out a bunch of bugs in the TLB
handing.

With TARGET_PAGE_BITS finally set to twelve I saw a drop in the % time
taken by cpu_arm_exec from 21.68% to 17.01% in my simple hand driven
android benchmark. I think if we are ever going to improve on this
further we need to consider alternative strategies to collecting,
invalidating and chaining together Translation Blocks.

I don't think this patch set is mergable as-is because we still
include a bunch of 32 bit ARM boards in the aarch64-softmmu build
which could be using an old enough ARM that has support for 1k page
tables (and may even use them?).

However review comments are welcome as well as any wider discussion on
reducing the time spent jumping between TBs.

Regards,


Alex Bennée (3):
  target-arm: don't hardcode mask values in arm_cpu_handle_mmu_fault
  target-arm: A64: fix TLB flush instructions
  target-arm: A64: fix use 12 bit page tables for aarch64

 target-arm/cpu.h    | 13 ++++++++++---
 target-arm/helper.c | 16 ++++++++++++----
 2 files changed, 22 insertions(+), 7 deletions(-)

-- 
2.0.2

[Qemu-devel] [PATCH 1/3] target-arm: don't hardcode mask values in arm_cpu_handle_mmu_fault

[Alex Bennée]

Otherwise we break quickly when we change TARGET_PAGE_SIZE.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/helper.c b/target-arm/helper.c
index a0e57cd..aa5d267 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4029,8 +4029,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
                         &page_size);
     if (ret == 0) {
         /* Map a single [sub]page.  */
-        phys_addr &= ~(hwaddr)0x3ff;
-        address &= ~(target_ulong)0x3ff;
+        phys_addr &= TARGET_PAGE_MASK;
+        address &= TARGET_PAGE_MASK;
         tlb_set_page(cs, address, phys_addr, prot, mmu_idx, page_size);
         return 0;
     }
-- 
2.0.2

[Qemu-devel] [PATCH 2/3] target-arm: A64: fix TLB flush instructions

[Alex Bennée]

According to the ARM ARM we weren't correctly flushing the TLB entries
where bits 63:56 didn't match bit 55 of the virtual address. This
exposed a problem when we switched QEMU's internal TARGET_PAGE_BITS to
12 for aarch64.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/helper.c b/target-arm/helper.c
index aa5d267..b0d0411 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1766,12 +1766,19 @@ static CPAccessResult aa64_cacheop_access(CPUARMState *env,
     return CP_ACCESS_OK;
 }
 
+/* See: D4.7.2 TLB maintenance requirements and the TLB maintenance instructions
+ * Page D4-1736 (DDI0487A.b) "For TLB maintenance instructions that
+ * take an address, the maintenance of VA[63:56] is interpreted as
+ * being the same as the maintenance of VA[55]"
+ */
+
 static void tlbi_aa64_va_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                uint64_t value)
 {
     /* Invalidate by VA (AArch64 version) */
     ARMCPU *cpu = arm_env_get_cpu(env);
-    uint64_t pageaddr = value << 12;
+    uint64_t pageaddr = sextract64(value << 12, 0, 56);
+
     tlb_flush_page(CPU(cpu), pageaddr);
 }
 
@@ -1780,7 +1787,8 @@ static void tlbi_aa64_vaa_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     /* Invalidate by VA, all ASIDs (AArch64 version) */
     ARMCPU *cpu = arm_env_get_cpu(env);
-    uint64_t pageaddr = value << 12;
+    uint64_t pageaddr = sextract64(value << 12, 0, 56);
+
     tlb_flush_page(CPU(cpu), pageaddr);
 }
 
-- 
2.0.2

[Qemu-devel] [PATCH 3/3] target-arm: A64: fix use 12 bit page tables for aarch64

[Alex Bennée]

The aarch64 architecture only support 4k+ pages so using a smaller value
for QEMU's internal page table handling only makes us less efficient.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c83f249..33359b9 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1051,11 +1051,18 @@ bool write_cpustate_to_list(ARMCPU *cpu);
 #if defined(CONFIG_USER_ONLY)
 #define TARGET_PAGE_BITS 12
 #else
-/* The ARM MMU allows 1k pages.  */
-/* ??? Linux doesn't actually use these, and they're deprecated in recent
-   architecture revisions.  Maybe a configure option to disable them.  */
+#if defined(TARGET_AARCH64)
+/* You can't configure 1k pages on aarch64 hardware */
+#define TARGET_PAGE_BITS 12
+#else
+/* The ARM MMU allows 1k pages - although they are not used by Linux
+ * FIXME?: they're deprecated in recent architecture revisions and
+ * this does create a performance hit. Maybe a configure option to
+ * disable them?
+ */
 #define TARGET_PAGE_BITS 10
 #endif
+#endif
 
 #if defined(TARGET_AARCH64)
 #  define TARGET_PHYS_ADDR_SPACE_BITS 48
-- 
2.0.2

Re: [Qemu-devel] [PATCH 2/3] target-arm: A64: fix TLB flush instructions

[Peter Maydell]

On 24 July 2014 16:52, Alex Bennée <alex.bennee@linaro.org> wrote:
> +/* See: D4.7.2 TLB maintenance requirements and the TLB maintenance instructions
> + * Page D4-1736 (DDI0487A.b) "For TLB maintenance instructions that
> + * take an address, the maintenance of VA[63:56] is interpreted as
> + * being the same as the maintenance of VA[55]"
> + */

I'd rather we didn't quote this bit of the ARM ARM, because it's
obviously mangled (I'm pretty sure it should say "the value of
VA[..]").

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] target-arm: don't hardcode mask values in arm_cpu_handle_mmu_fault

[Peter Maydell]

On 24 July 2014 16:52, Alex Bennée <alex.bennee@linaro.org> wrote:
> Otherwise we break quickly when we change TARGET_PAGE_SIZE.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index a0e57cd..aa5d267 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4029,8 +4029,8 @@ int arm_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
>                          &page_size);
>      if (ret == 0) {
>          /* Map a single [sub]page.  */
> -        phys_addr &= ~(hwaddr)0x3ff;
> -        address &= ~(target_ulong)0x3ff;
> +        phys_addr &= TARGET_PAGE_MASK;
> +        address &= TARGET_PAGE_MASK;
>          tlb_set_page(cs, address, phys_addr, prot, mmu_idx, page_size);
>          return 0;
>      }

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] target-arm: A64: fix use 12 bit page tables for aarch64

[Peter Maydell]

On 24 July 2014 16:52, Alex Bennée <alex.bennee@linaro.org> wrote:
> The aarch64 architecture only support 4k+ pages so using a smaller value
> for QEMU's internal page table handling only makes us less efficient.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c83f249..33359b9 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1051,11 +1051,18 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>  #if defined(CONFIG_USER_ONLY)
>  #define TARGET_PAGE_BITS 12
>  #else
> -/* The ARM MMU allows 1k pages.  */
> -/* ??? Linux doesn't actually use these, and they're deprecated in recent
> -   architecture revisions.  Maybe a configure option to disable them.  */
> +#if defined(TARGET_AARCH64)
> +/* You can't configure 1k pages on aarch64 hardware */

"AArch64" (here and in commit messages).

Also, qemu-system-aarch64 still supports all the 32 bit
CPUs, including the ARMv5 ones (v5 is the last revision
that supported 1K pages). So we'd have to at least remove
those CPUs from the TARGET_AARCH64 system emulator
compilation, if we can't come up with anything cleverer.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 0/3] target-arm: Some fixes to page and TLB handling

[Peter Maydell]

On 24 July 2014 16:52, Alex Bennée <alex.bennee@linaro.org> wrote:
> I don't think this patch set is mergable as-is because we still
> include a bunch of 32 bit ARM boards in the aarch64-softmmu build
> which could be using an old enough ARM that has support for 1k page
> tables (and may even use them?).

We can certainly merge patches 1 and 2, which are straight
bugfixes.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] target-arm: A64: fix TLB flush instructions

[Alex Bennée]

Peter Maydell writes:

> On 24 July 2014 16:52, Alex Bennée <alex.bennee@linaro.org> wrote:
>> +/* See: D4.7.2 TLB maintenance requirements and the TLB maintenance instructions
>> + * Page D4-1736 (DDI0487A.b) "For TLB maintenance instructions that
>> + * take an address, the maintenance of VA[63:56] is interpreted as
>> + * being the same as the maintenance of VA[55]"
>> + */
>
> I'd rather we didn't quote this bit of the ARM ARM, because it's
> obviously mangled (I'm pretty sure it should say "the value of
> VA[..]").

Is it OK to still reference the ARM ARM because otherwise the sign
extension would look a little weird without context (although obviously
we have a commit message to say we fixed something).

>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM

-- 
Alex Bennée