[Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[vijayak]

From: Vijay <vijayak@cavium.com>

Set target page size to minimum 4K for aarch64.
This helps to reduce live migration downtime significantly.

Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
---
 target-arm/cpu.h |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 066ff67..2e4b48f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1562,11 +1562,18 @@ bool write_cpustate_to_list(ARMCPU *cpu);
 #if defined(CONFIG_USER_ONLY)
 #define TARGET_PAGE_BITS 12
 #else
+/*
+ * Aarch64 support minimum 4K page size
+ */
+#if defined(TARGET_AARCH64)
+#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.  */
 #define TARGET_PAGE_BITS 10
 #endif
+#endif
 
 #if defined(TARGET_AARCH64)
 #  define TARGET_PHYS_ADDR_SPACE_BITS 48
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Peter Maydell]

On 4 April 2016 at 14:39,  <vijayak@caviumnetworks.com> wrote:
> From: Vijay <vijayak@cavium.com>
>
> Set target page size to minimum 4K for aarch64.
> This helps to reduce live migration downtime significantly.
>
> Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
> ---
>  target-arm/cpu.h |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 066ff67..2e4b48f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1562,11 +1562,18 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>  #if defined(CONFIG_USER_ONLY)
>  #define TARGET_PAGE_BITS 12
>  #else
> +/*
> + * Aarch64 support minimum 4K page size
> + */
> +#if defined(TARGET_AARCH64)
> +#define TARGET_PAGE_BITS 12

I agree that this would definitely improve performance (both for
migration and for emulated guests), but I'm afraid this breaks
running 32-bit ARMv5 and ARMv7M guests with this QEMU binary,
so we can't do this. If we want to allow the minimum page size to
be bigger than 1K for AArch64 CPUs then we need to make it a
runtime settable thing rather than compile-time (which is not
an entirely trivial thing).

> +#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.  */
>  #define TARGET_PAGE_BITS 10
>  #endif
> +#endif
>
>  #if defined(TARGET_AARCH64)
>  #  define TARGET_PHYS_ADDR_SPACE_BITS 48

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Vijay Kilari]

On Mon, Apr 4, 2016 at 7:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 April 2016 at 14:39,  <vijayak@caviumnetworks.com> wrote:
>> From: Vijay <vijayak@cavium.com>
>>
>> Set target page size to minimum 4K for aarch64.
>> This helps to reduce live migration downtime significantly.
>>
>> Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
>> ---
>>  target-arm/cpu.h |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 066ff67..2e4b48f 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1562,11 +1562,18 @@ bool write_cpustate_to_list(ARMCPU *cpu);
>>  #if defined(CONFIG_USER_ONLY)
>>  #define TARGET_PAGE_BITS 12
>>  #else
>> +/*
>> + * Aarch64 support minimum 4K page size
>> + */
>> +#if defined(TARGET_AARCH64)
>> +#define TARGET_PAGE_BITS 12
>
> I agree that this would definitely improve performance (both for
> migration and for emulated guests), but I'm afraid this breaks
> running 32-bit ARMv5 and ARMv7M guests with this QEMU binary,
> so we can't do this. If we want to allow the minimum page size to
> be bigger than 1K for AArch64 CPUs then we need to make it a
> runtime settable thing rather than compile-time (which is not
> an entirely trivial thing).

Do you mean to say that based on -cpu type qemu option
choose the page size at runtime?

>
>> +#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.  */
>>  #define TARGET_PAGE_BITS 10
>>  #endif
>> +#endif
>>
>>  #if defined(TARGET_AARCH64)
>>  #  define TARGET_PHYS_ADDR_SPACE_BITS 48
>
> thanks
> -- PMM

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Peter Maydell]

On 4 April 2016 at 17:40, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Mon, Apr 4, 2016 at 7:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I agree that this would definitely improve performance (both for
>> migration and for emulated guests), but I'm afraid this breaks
>> running 32-bit ARMv5 and ARMv7M guests with this QEMU binary,
>> so we can't do this. If we want to allow the minimum page size to
>> be bigger than 1K for AArch64 CPUs then we need to make it a
>> runtime settable thing rather than compile-time (which is not
>> an entirely trivial thing).
>
> Do you mean to say that based on -cpu type qemu option
> choose the page size at runtime?

If you want to avoid defining TARGET_PAGE_SIZE to the
lowest-common-denominator 1K, then yes, you'd need to
choose it at runtime. That could be painful to implement.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Vijay Kilari]

On Mon, Apr 4, 2016 at 10:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 April 2016 at 17:40, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Mon, Apr 4, 2016 at 7:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I agree that this would definitely improve performance (both for
>>> migration and for emulated guests), but I'm afraid this breaks
>>> running 32-bit ARMv5 and ARMv7M guests with this QEMU binary,
>>> so we can't do this. If we want to allow the minimum page size to
>>> be bigger than 1K for AArch64 CPUs then we need to make it a
>>> runtime settable thing rather than compile-time (which is not
>>> an entirely trivial thing).
>>
>> Do you mean to say that based on -cpu type qemu option
>> choose the page size at runtime?
>
> If you want to avoid defining TARGET_PAGE_SIZE to the
> lowest-common-denominator 1K, then yes, you'd need to
> choose it at runtime. That could be painful to implement.

Had a look at it. Needs some changes in common code as well.
I will send this as a separate patch series and drop this patch
from this series.

>
> thanks
> -- PMM

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Vijay Kilari]

Hi Peter

On Wed, Apr 6, 2016 at 8:31 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Mon, Apr 4, 2016 at 10:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 4 April 2016 at 17:40, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>>> On Mon, Apr 4, 2016 at 7:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> I agree that this would definitely improve performance (both for
>>>> migration and for emulated guests), but I'm afraid this breaks
>>>> running 32-bit ARMv5 and ARMv7M guests with this QEMU binary,
>>>> so we can't do this. If we want to allow the minimum page size to
>>>> be bigger than 1K for AArch64 CPUs then we need to make it a
>>>> runtime settable thing rather than compile-time (which is not
>>>> an entirely trivial thing).
>>>
>>> Do you mean to say that based on -cpu type qemu option
>>> choose the page size at runtime?
>>
>> If you want to avoid defining TARGET_PAGE_SIZE to the
>> lowest-common-denominator 1K, then yes, you'd need to
>> choose it at runtime. That could be painful to implement.
>
> Had a look at it. Needs some changes in common code as well.
> I will send this as a separate patch series and drop this patch
> from this series.

The L1 page table size, L1 shift are dependent on TARGET_PAGE_BITS(page size).
as shown in snippet code below from translate-all.c

/* The bits remaining after N lower levels of page tables.  */
#define V_L1_BITS_REM \
    ((L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS) % V_L2_BITS)

#if V_L1_BITS_REM < 4
#define V_L1_BITS  (V_L1_BITS_REM + V_L2_BITS)
#else
#define V_L1_BITS  V_L1_BITS_REM
#endif

#define V_L1_SIZE  ((target_ulong)1 << V_L1_BITS)

#define V_L1_SHIFT (L1_MAP_ADDR_SPACE_BITS - TARGET_PAGE_BITS - V_L1_BITS)

/* The bottom level has pointers to PageDesc */
static void *l1_map[V_L1_SIZE];

How about adding CONFIG_PAGE_SIZE option to configure?.

Re: [Qemu-devel] [RFC PATCH v1 1/2] target-arm: Update page size for aarch64

[Peter Maydell]

On 31 May 2016 at 10:04, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Wed, Apr 6, 2016 at 8:31 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Mon, Apr 4, 2016 at 10:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> If you want to avoid defining TARGET_PAGE_SIZE to the
>>> lowest-common-denominator 1K, then yes, you'd need to
>>> choose it at runtime. That could be painful to implement.

> The L1 page table size, L1 shift are dependent on TARGET_PAGE_BITS(page size).
> as shown in snippet code below from translate-all.c

Yes, that's why I said "painful to implement" :-)

> How about adding CONFIG_PAGE_SIZE option to configure?.

I don't want this to be a configure option, because QEMU
should just work for everybody. Otherwise we have some QEMU
binaries which silently don't implement the architecture/CPU
that they ought to.

thanks
-- PMM

[Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[vijayak]

From: Vijay <vijayak@cavium.com>

Use Neon instructions to perform zero checking of
buffer. This is helps in reducing downtime during
live migration.

Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
---
 util/cutils.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/util/cutils.c b/util/cutils.c
index 43d1afb..d343b9a 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -352,6 +352,87 @@ static void *can_use_buffer_find_nonzero_offset_ifunc(void)
     return func;
 }
 #pragma GCC pop_options
+
+#elif defined __aarch64__
+#include "arm_neon.h"
+
+#define NEON_VECTYPE               uint64x2_t
+#define NEON_LOAD_N_ORR(v1, v2)    vorrq_u64(vld1q_u64(&v1), vld1q_u64(&v2))
+#define NEON_ORR(v1, v2)           vorrq_u64(v1, v2)
+#define NEON_EQ_ZERO(v1) \
+        ((vgetq_lane_u64(vceqzq_u64(v1), 0) == 0) || \
+         (vgetq_lane_u64(vceqzq_u64(v1), 1)) == 0)
+
+#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON 16
+
+/*
+ * Zero page/buffer checking using SIMD(Neon)
+ */
+
+static bool
+can_use_buffer_find_nonzero_offset_neon(const void *buf, size_t len)
+{
+    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON
+                   * sizeof(NEON_VECTYPE)) == 0
+            && ((uintptr_t) buf) % sizeof(NEON_VECTYPE) == 0);
+}
+
+static size_t buffer_find_nonzero_offset_neon(const void *buf, size_t len)
+{
+    size_t i;
+    NEON_VECTYPE d0, d1, d2, d3, d4, d5, d6;
+    NEON_VECTYPE d7, d8, d9, d10, d11, d12, d13, d14;
+    uint64_t const *data = buf;
+
+    assert(can_use_buffer_find_nonzero_offset_neon(buf, len));
+    len /= sizeof(unsigned long);
+
+    for (i = 0; i < len; i += 32) {
+        d0 = NEON_LOAD_N_ORR(data[i], data[i + 2]);
+        d1 = NEON_LOAD_N_ORR(data[i + 4], data[i + 6]);
+        d2 = NEON_LOAD_N_ORR(data[i + 8], data[i + 10]);
+        d3 = NEON_LOAD_N_ORR(data[i + 12], data[i + 14]);
+        d4 = NEON_ORR(d0, d1);
+        d5 = NEON_ORR(d2, d3);
+        d6 = NEON_ORR(d4, d5);
+
+        d7 = NEON_LOAD_N_ORR(data[i + 16], data[i + 18]);
+        d8 = NEON_LOAD_N_ORR(data[i + 20], data[i + 22]);
+        d9 = NEON_LOAD_N_ORR(data[i + 24], data[i + 26]);
+        d10 = NEON_LOAD_N_ORR(data[i + 28], data[i + 30]);
+        d11 = NEON_ORR(d7, d8);
+        d12 = NEON_ORR(d9, d10);
+        d13 = NEON_ORR(d11, d12);
+
+        d14 = NEON_ORR(d6, d13);
+        if (NEON_EQ_ZERO(d14)) {
+            break;
+        }
+    }
+
+    return i * sizeof(unsigned long);
+}
+
+static inline bool neon_support(void)
+{
+    /*
+     * Check if neon feature is supported.
+     * By default neon is supported for aarch64.
+     */
+    return true;
+}
+
+bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    return neon_support() ? can_use_buffer_find_nonzero_offset_neon(buf, len) :
+           can_use_buffer_find_nonzero_offset_inner(buf, len);
+}
+
+size_t buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    return neon_support() ? buffer_find_nonzero_offset_neon(buf, len) :
+           buffer_find_nonzero_offset_inner(buf, len);
+}
 #else
 bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
 {
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[Peter Maydell]

On 4 April 2016 at 14:39,  <vijayak@caviumnetworks.com> wrote:
> From: Vijay <vijayak@cavium.com>
>
> Use Neon instructions to perform zero checking of
> buffer. This is helps in reducing downtime during
> live migration.
>
> Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
> ---
>  util/cutils.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
> diff --git a/util/cutils.c b/util/cutils.c
> index 43d1afb..d343b9a 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -352,6 +352,87 @@ static void *can_use_buffer_find_nonzero_offset_ifunc(void)
>      return func;
>  }
>  #pragma GCC pop_options
> +
> +#elif defined __aarch64__
> +#include "arm_neon.h"

Can we rely on all compilers having this, or do we need to
test in configure?

> +
> +#define NEON_VECTYPE               uint64x2_t
> +#define NEON_LOAD_N_ORR(v1, v2)    vorrq_u64(vld1q_u64(&v1), vld1q_u64(&v2))
> +#define NEON_ORR(v1, v2)           vorrq_u64(v1, v2)
> +#define NEON_EQ_ZERO(v1) \
> +        ((vgetq_lane_u64(vceqzq_u64(v1), 0) == 0) || \
> +         (vgetq_lane_u64(vceqzq_u64(v1), 1)) == 0)

The intrinsics are a bit confusing, but shouldn't we be
testing that both lanes of v1 are 0, rather than whether
either of them is? (so "&&", not "||").

> +
> +#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON 16
> +
> +/*
> + * Zero page/buffer checking using SIMD(Neon)
> + */
> +
> +static bool
> +can_use_buffer_find_nonzero_offset_neon(const void *buf, size_t len)
> +{
> +    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON
> +                   * sizeof(NEON_VECTYPE)) == 0
> +            && ((uintptr_t) buf) % sizeof(NEON_VECTYPE) == 0);
> +}
> +
> +static size_t buffer_find_nonzero_offset_neon(const void *buf, size_t len)
> +{
> +    size_t i;
> +    NEON_VECTYPE d0, d1, d2, d3, d4, d5, d6;
> +    NEON_VECTYPE d7, d8, d9, d10, d11, d12, d13, d14;
> +    uint64_t const *data = buf;
> +
> +    assert(can_use_buffer_find_nonzero_offset_neon(buf, len));
> +    len /= sizeof(unsigned long);
> +
> +    for (i = 0; i < len; i += 32) {
> +        d0 = NEON_LOAD_N_ORR(data[i], data[i + 2]);
> +        d1 = NEON_LOAD_N_ORR(data[i + 4], data[i + 6]);
> +        d2 = NEON_LOAD_N_ORR(data[i + 8], data[i + 10]);
> +        d3 = NEON_LOAD_N_ORR(data[i + 12], data[i + 14]);
> +        d4 = NEON_ORR(d0, d1);
> +        d5 = NEON_ORR(d2, d3);
> +        d6 = NEON_ORR(d4, d5);
> +
> +        d7 = NEON_LOAD_N_ORR(data[i + 16], data[i + 18]);
> +        d8 = NEON_LOAD_N_ORR(data[i + 20], data[i + 22]);
> +        d9 = NEON_LOAD_N_ORR(data[i + 24], data[i + 26]);
> +        d10 = NEON_LOAD_N_ORR(data[i + 28], data[i + 30]);
> +        d11 = NEON_ORR(d7, d8);
> +        d12 = NEON_ORR(d9, d10);
> +        d13 = NEON_ORR(d11, d12);
> +
> +        d14 = NEON_ORR(d6, d13);
> +        if (NEON_EQ_ZERO(d14)) {
> +            break;
> +        }
> +    }

Both the other optimised find_nonzero implementations in this
file have two loops, not just one. Is it OK that this
implementation has only a single loop?

Paolo: do you know why we have two loops in the other
implementations?

> +
> +    return i * sizeof(unsigned long);
> +}
> +
> +static inline bool neon_support(void)
> +{
> +    /*
> +     * Check if neon feature is supported.
> +     * By default neon is supported for aarch64.
> +     */
> +    return true;
> +}

There doesn't seem much point in this. We can assume Neon exists
on any CPU we're going to run on (it's part of the ABI, the kernel
assumes it, etc etc). So you can just implement the functions without
the indirection functions below.

> +
> +bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
> +{
> +    return neon_support() ? can_use_buffer_find_nonzero_offset_neon(buf, len) :
> +           can_use_buffer_find_nonzero_offset_inner(buf, len);
> +}
> +
> +size_t buffer_find_nonzero_offset(const void *buf, size_t len)
> +{
> +    return neon_support() ? buffer_find_nonzero_offset_neon(buf, len) :
> +           buffer_find_nonzero_offset_inner(buf, len);
> +}
>  #else
>  bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
>  {
> --

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[Paolo Bonzini]

On 05/04/2016 16:36, Peter Maydell wrote:
>> > +
>> > +#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON 16
>> > +
>> > +/*
>> > + * Zero page/buffer checking using SIMD(Neon)
>> > + */
>> > +
>> > +static bool
>> > +can_use_buffer_find_nonzero_offset_neon(const void *buf, size_t len)
>> > +{
>> > +    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON
>> > +                   * sizeof(NEON_VECTYPE)) == 0
>> > +            && ((uintptr_t) buf) % sizeof(NEON_VECTYPE) == 0);
>> > +}
>> > +
>> > +static size_t buffer_find_nonzero_offset_neon(const void *buf, size_t len)
>> > +{
>> > +    size_t i;
>> > +    NEON_VECTYPE d0, d1, d2, d3, d4, d5, d6;
>> > +    NEON_VECTYPE d7, d8, d9, d10, d11, d12, d13, d14;
>> > +    uint64_t const *data = buf;
>> > +
>> > +    assert(can_use_buffer_find_nonzero_offset_neon(buf, len));
>> > +    len /= sizeof(unsigned long);
>> > +
>> > +    for (i = 0; i < len; i += 32) {
>> > +        d0 = NEON_LOAD_N_ORR(data[i], data[i + 2]);
>> > +        d1 = NEON_LOAD_N_ORR(data[i + 4], data[i + 6]);
>> > +        d2 = NEON_LOAD_N_ORR(data[i + 8], data[i + 10]);
>> > +        d3 = NEON_LOAD_N_ORR(data[i + 12], data[i + 14]);
>> > +        d4 = NEON_ORR(d0, d1);
>> > +        d5 = NEON_ORR(d2, d3);
>> > +        d6 = NEON_ORR(d4, d5);
>> > +
>> > +        d7 = NEON_LOAD_N_ORR(data[i + 16], data[i + 18]);
>> > +        d8 = NEON_LOAD_N_ORR(data[i + 20], data[i + 22]);
>> > +        d9 = NEON_LOAD_N_ORR(data[i + 24], data[i + 26]);
>> > +        d10 = NEON_LOAD_N_ORR(data[i + 28], data[i + 30]);
>> > +        d11 = NEON_ORR(d7, d8);
>> > +        d12 = NEON_ORR(d9, d10);
>> > +        d13 = NEON_ORR(d11, d12);
>> > +
>> > +        d14 = NEON_ORR(d6, d13);
>> > +        if (NEON_EQ_ZERO(d14)) {
>> > +            break;
>> > +        }
>> > +    }
> Both the other optimised find_nonzero implementations in this
> file have two loops, not just one. Is it OK that this
> implementation has only a single loop?
> 
> Paolo: do you know why we have two loops in the other
> implementations?

Because usually the first one or two iterations are enough to exit the
function if the page is nonzero.  It's measurably slower to go through
the unrolled loop in that case.  On the other hand, once the first few
iterations found only zero bytes, the buffer is very likely entirely
zero and the unrolled loop helps.

But in theory it should be enough to add a new #elif branch like this:

#include "arm_neon.h"
#define VECTYPE   uint64x2_t
#define VEC_OR(a, b) ((a) | (b))
#define ALL_EQ(a, b) /* ??? :) */

around the

/* vector definitions */

comment in util/cutils.c.  GCC should do everything else.

Paolo

Re: [Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[Peter Maydell]

On 5 April 2016 at 16:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
> But in theory it should be enough to add a new #elif branch like this:
>
> #include "arm_neon.h"
> #define VECTYPE   uint64x2_t
> #define VEC_OR(a, b) ((a) | (b))
> #define ALL_EQ(a, b) /* ??? :) */

#define ALL_EQ(a, b) (vgetq_lane_u64(a, 0) == vgetq_lane_u64(b, 0) && \
                      vgetq_lane_u64(a, 1) == vgetq_lane_u64(b, 1))

will do I think (probably suboptimal for a true vector compare but
works OK here as we're actually only interested in comparing against
constant zero; the compiler generates "load 64bit value from vector
register to integer; cbnz" for each half of the vector).

Worth benchmarking that (and the variant where we use the C code
but move the loop unrolling out to 16) against the handwritten
intrinsics version.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] about armv8's prefetch decode

[Pranith Kumar]

Hi Jed,

On Mon, Mar 20, 2017 at 2:35 AM, Wangjintang <wangjintang@huawei.com> wrote:
> Hi,
>
>         We see that armv8's prefetch instruction decode have been skipped in qemu. For some user, they need prefetch instruction, for example, they use qemu to generate the instruction trace. We want to merge this patch to community, it's ok or not?  Thanks.
>

Your patch seems to be missing. Can you retry with the content of the
patch pasted in the email?

Thanks,
--
Pranith

Re: [Qemu-devel] [Qemu-arm] [patch 1/1]about armv8's prefetch decode

[Wangjintang]

Hi Pranith,
 
 	Thanks for your reply. patch as below, new added code default is off, please review. 
The major thinking is about translate Armv8's prefetch instruction into intermediate code, at the same time don't effect the rm/rn register. 


diff --git a/translate-a64.c b/translate-a64.c
index 814f30f..86da8ee 100644
--- a/translate-a64.c
+++ b/translate-a64.c
@@ -2061,7 +2061,11 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
     } else {
         if (opc == 3) {
             /* PRFM (literal) : prefetch */
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            ;
+            #else
             return;
+            #endif
         }
         size = 2 + extract32(opc, 0, 1);
         is_signed = extract32(opc, 1, 1);
@@ -2075,9 +2079,19 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
     } else {
         /* Only unsigned 32bit loads target 32bit registers.  */
         bool iss_sf = opc != 0;
-
-        do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false,
-                  true, rt, iss_sf, false);
+        #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+        if (opc == 3) {
+            TCGv_i64 v = tcg_temp_new_i64();
+            do_gpr_ld(s, v, tcg_addr, size, is_signed, false,
+                      true, rt, iss_sf, false);
+            tcg_temp_free_i64(v);
+        } else {
+        #endif
+            do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false,
+                      true, rt, iss_sf, false);
+        #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+        }
+        #endif
     }
     tcg_temp_free_i64(tcg_addr);
 }
@@ -2283,7 +2297,11 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
                 unallocated_encoding(s);
                 return;
             }
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            rn = 31;
+            #else
             return;
+            #endif
         }
         if (opc == 3 && size > 1) {
             unallocated_encoding(s);
@@ -2334,9 +2352,21 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
             do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx,
                              iss_valid, rt, iss_sf, false);
         } else {
-            do_gpr_ld_memidx(s, tcg_rt, tcg_addr, size,
-                             is_signed, is_extended, memidx,
-                             iss_valid, rt, iss_sf, false);
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            if (size == 3 && opc == 2) {
+                TCGv_i64 v = tcg_temp_new_i64();
+                do_gpr_ld_memidx(s, v, tcg_addr, size,
+                                  is_signed, is_extended, memidx,
+                                  iss_valid, rt, iss_sf, false);
+                tcg_temp_free_i64(v);
+             } else {
+             #endif
+                 do_gpr_ld_memidx(s, tcg_rt, tcg_addr, size,
+                                  is_signed, is_extended, memidx,
+                                  iss_valid, rt, iss_sf, false);
+             #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+             }
+             #endif
         }
     }

@@ -2383,6 +2413,9 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
     bool is_signed = false;
     bool is_store = false;
     bool is_extended = false;
+    #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+    TCGv_i64 v = tcg_temp_new_i64();
+    #endif

     TCGv_i64 tcg_rm;
     TCGv_i64 tcg_addr;
@@ -2405,7 +2438,11 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
     } else {
         if (size == 3 && opc == 2) {
             /* PRFM - prefetch */
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            rn = 31;
+            #else
             return;
+            #endif
         }
         if (opc == 3 && size > 1) {
             unallocated_encoding(s);
@@ -2422,9 +2459,17 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
     tcg_addr = read_cpu_reg_sp(s, rn, 1);

     tcg_rm = read_cpu_reg(s, rm, 1);
-    ext_and_shift_reg(tcg_rm, tcg_rm, opt, shift ? size : 0);
-
-    tcg_gen_add_i64(tcg_addr, tcg_addr, tcg_rm);
+    #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+    if ((!is_vector) &&(size == 3 && opc == 2)) {
+        ext_and_shift_reg(v, tcg_rm, opt, shift ? size : 0);
+        tcg_gen_add_i64(tcg_addr, tcg_addr, v);
+    } else {
+    #endif
+        ext_and_shift_reg(tcg_rm, tcg_rm, opt, shift ? size : 0);
+        tcg_gen_add_i64(tcg_addr, tcg_addr, tcg_rm);
+    #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+    }
+    #endif

     if (is_vector) {
         if (is_store) {
@@ -2439,11 +2484,25 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
             do_gpr_st(s, tcg_rt, tcg_addr, size,
                       true, rt, iss_sf, false);
         } else {
-            do_gpr_ld(s, tcg_rt, tcg_addr, size,
-                      is_signed, is_extended,
-                      true, rt, iss_sf, false);
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            if (size == 3 && opc == 2) {
+                is_signed = false;
+                do_gpr_ld(s, v, tcg_addr, size,
+                          is_signed, is_extended,
+                          true, rt, iss_sf, false);
+            } else {
+            #endif
+                do_gpr_ld(s, tcg_rt, tcg_addr, size,
+                          is_signed, is_extended,
+                          true, rt, iss_sf, false);
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            }
+            #endif
         }
     }
+    #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+    tcg_temp_free_i64(v);
+    #endif
 }

 /*
@@ -2492,7 +2551,11 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
     } else {
         if (size == 3 && opc == 2) {
             /* PRFM - prefetch */
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            rn = 31;
+            #else
             return;
+            #endif
         }
         if (opc == 3 && size > 1) {
             unallocated_encoding(s);
@@ -2523,8 +2586,20 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
             do_gpr_st(s, tcg_rt, tcg_addr, size,
                       true, rt, iss_sf, false);
         } else {
-            do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended,
-                      true, rt, iss_sf, false);
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            if (size == 3 && opc == 2) {
+                TCGv_i64 v = tcg_temp_new_i64();
+                is_signed = false;
+                do_gpr_ld(s, v, tcg_addr, size, is_signed, is_extended,
+                          true, rt, iss_sf, false);
+                tcg_temp_free_i64(v);
+            } else {
+            #endif
+                do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended,
+                          true, rt, iss_sf, false);
+            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
+            }
+            #endif
         }
     }
 }
diff --git a/translate.h b/translate.h
index 2cc4412..6bb1e24 100644
--- a/translate.h
+++ b/translate.h
@@ -135,6 +135,8 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
 #define DISAS_SMC 9
 #define DISAS_YIELD 10

+#undef  TCG_AARCH64_PREFETCH_TRANSLATE
+
 #ifdef TARGET_AARCH64
 void a64_translate_init(void);
 void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb);


Best Regards,
Wang jintang / Jed



Huawei Technologies Co., Ltd. 
Email: wangjintang@huawei.com
Building Z8, Huawei R&D center, Jiangshu Rd 360, Binjiang District,Hangzhou 310051, P.R.China
http://www.huawei.com 

> -----Original Message-----
> From: Pranith Kumar [mailto:bobby.prani@gmail.com]
> Sent: Friday, March 24, 2017 12:56 AM
> To: Wangjintang
> Cc: Peter Maydell; Shlomo Pongratz (A); qemu-arm; Ori Chalak (A);
> qemu-devel
> Subject: Re: [Qemu-arm] about armv8's prefetch decode
> 
> Hi Jed,
> 
> On Mon, Mar 20, 2017 at 2:35 AM, Wangjintang <wangjintang@huawei.com>
> wrote:
> > Hi,
> >
> >         We see that armv8's prefetch instruction decode have been
> skipped in qemu. For some user, they need prefetch instruction, for example,
> they use qemu to generate the instruction trace. We want to merge this
> patch to community, it's ok or not?  Thanks.
> >
> 
> Your patch seems to be missing. Can you retry with the content of the
> patch pasted in the email?
> 
> Thanks,
> --
> Pranith

Re: [Qemu-devel] [Qemu-arm] [patch 1/1]about armv8's prefetch decode

[Peter Maydell]

On 24 March 2017 at 06:14, Wangjintang <wangjintang@huawei.com> wrote:
> Hi Pranith,
>
>         Thanks for your reply. patch as below, new added code default is off, please review.
> The major thinking is about translate Armv8's prefetch instruction into intermediate code, at the same time don't effect the rm/rn register.
>
>
> diff --git a/translate-a64.c b/translate-a64.c
> index 814f30f..86da8ee 100644
> --- a/translate-a64.c
> +++ b/translate-a64.c
> @@ -2061,7 +2061,11 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
>      } else {
>          if (opc == 3) {
>              /* PRFM (literal) : prefetch */
> +            #ifdef TCG_AARCH64_PREFETCH_TRANSLATE
> +            ;
> +            #else
>              return;
> +            #endif
>          }

No, these changes look wrong. PRFM instructions do not need to
do anything and should definitely not be emitting any intermediate
code. In particular if you let execution fall through and try
do_gpr_ld() then it will really do a load, which might cause
an exception -- this is specifically forbidden for PRFM.
Architecturally the ARM ARM says "it is valid for the PE to
treat any or all prefetch instructions as a NOP", which is
what QEMU does.

The existing code is correct. In general you should not
expect to be able to deduce the guest instructions from
the intermediate code representation.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [patch 1/1]about armv8's prefetch decode

[Wangjintang]

Hi Peter,
	More detail illustration at below.

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Friday, March 24, 2017 6:06 PM
> To: Wangjintang
> Cc: Pranith Kumar; Shlomo Pongratz (A); Wanghaibin (Benjamin); qemu-arm;
> qemu-devel; Ori Chalak (A)
> Subject: Re: [Qemu-arm] [patch 1/1]about armv8's prefetch decode
> No, these changes look wrong. PRFM instructions do not need to
> do anything and should definitely not be emitting any intermediate
> code. In particular if you let execution fall through and try
> do_gpr_ld() then it will really do a load, which might cause
> an exception -- this is specifically forbidden for PRFM.
> Architecturally the ARM ARM says "it is valid for the PE to
> treat any or all prefetch instructions as a NOP", which is
> what QEMU does.
> 
> The existing code is correct. In general you should not
> expect to be able to deduce the guest instructions from
> the intermediate code representation.
> 

"it is valid for the PE to treat any or all prefetch instructions as a NOP", 
from software view, it's right.
the patch regard the prefetch as load instruction, at the same time 
don't affect rm/rt register. Only the PRFM instruction been emitted to
intermediate code and do a really load, then we can get the memory 
address relative to the prefetch instruction. Because the rm/rt register 
don't been modified, so the application can run correctly. 
BTW, the new added code default is disable. So for the common user, have no 
affect to them.

In our case, we need all the instruction trace & ld/st instruction's 
access memory address, the trace as the input for chip cycle-accurate 
model. Similar with flexus + qemu. 
Current code that skip generate prefetch instructions' intermediate code, 
So we can get prefetch instruction, but can't get the prefetch instruction 
relative memory address. 
We have tested that the ratio of prefetch instructions is about 2%~3% during 
run Dhrystone in system mode. The ratio is high.
________________                       ________________
|                |                     |                |
|                |                     |                |
|   Qemu        |                     |  chip          |
|                |   instruction trace    | cycle-accurate   |
|                |    ----------------->      | model          |
|                |   memory trace      |                |
|________________|                     |________________|



Ori Chalak's explain this as below:
" Indeed, prefetch instruction affects only the micro architecture, 
and hence not needed for running correctly the generated code.
However, we developed a performance simulator for a detailed 
ARMv8 CPU model, and use Qemu to resolve the functionality.
And for this purpose we need to translate all instructions that 
may affect the pipeline behavior, caches, etc.

This is not the major usage of Qemu, however there may be 
others doing this and it may help them.
http://www.linux-kvm.org/images/4/45/01x09-Christopher_Covington-Using_Upstream_QEMU_for_CASS.pdf "


Best Regards,
Wang jintang / Jed
Huawei Technologies Co., Ltd. 
Email: wangjintang@huawei.com
http://www.huawei.com

Re: [Qemu-devel] [Qemu-arm] [patch 1/1]about armv8's prefetch decode

[Peter Maydell]

On 25 March 2017 at 02:22, Wangjintang <wangjintang@huawei.com> wrote:
> the patch regard the prefetch as load instruction, at the same time
> don't affect rm/rt register. Only the PRFM instruction been emitted to
> intermediate code and do a really load, then we can get the memory
> address relative to the prefetch instruction. Because the rm/rt register
> don't been modified, so the application can run correctly.

It will still fault if the address is not valid, which is
not a permitted behaviour.

> In our case, we need all the instruction trace & ld/st instruction's
> access memory address, the trace as the input for chip cycle-accurate
> model. Similar with flexus + qemu.
> Current code that skip generate prefetch instructions' intermediate code,
> So we can get prefetch instruction, but can't get the prefetch instruction
> relative memory address.

I understand the use case you would like, but if we want
to support that kind of thing we should do it with a much
more significant and consistent degree of support for
tracing of guest code actions, not with a small ad-hoc
change that happens to fix the immediate thing you're
running into for your specific problem.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[Vijay Kilari]

On Tue, Apr 5, 2016 at 8:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 April 2016 at 14:39,  <vijayak@caviumnetworks.com> wrote:
>> From: Vijay <vijayak@cavium.com>
>>
>> Use Neon instructions to perform zero checking of
>> buffer. This is helps in reducing downtime during
>> live migration.
>>
>> Signed-off-by: Vijaya Kumar K <vijayak@caviumnetworks.com>
>> ---
>>  util/cutils.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 81 insertions(+)
>>
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 43d1afb..d343b9a 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -352,6 +352,87 @@ static void *can_use_buffer_find_nonzero_offset_ifunc(void)
>>      return func;
>>  }
>>  #pragma GCC pop_options
>> +
>> +#elif defined __aarch64__
>> +#include "arm_neon.h"
>
> Can we rely on all compilers having this, or do we need to
> test in configure?

GCC and armcc support the same intrinsics. Both needs inclusion
of arm_neon.h.

>
>> +
>> +#define NEON_VECTYPE               uint64x2_t
>> +#define NEON_LOAD_N_ORR(v1, v2)    vorrq_u64(vld1q_u64(&v1), vld1q_u64(&v2))
>> +#define NEON_ORR(v1, v2)           vorrq_u64(v1, v2)
>> +#define NEON_EQ_ZERO(v1) \
>> +        ((vgetq_lane_u64(vceqzq_u64(v1), 0) == 0) || \
>> +         (vgetq_lane_u64(vceqzq_u64(v1), 1)) == 0)
>
> The intrinsics are a bit confusing, but shouldn't we be
> testing that both lanes of v1 are 0, rather than whether
> either of them is? (so "&&", not "||").

Above check is correct. vceqzq() sets all bits to 1 if value is 0.
So if one lane is 0, then it means it is non-zero buffer. I think
redefining this macro as below would be better and avoid
vceqzq_u64()

#define NEON_NOT_EQ_ZERO(v1) \
        ((vgetq_lane_u64(v1, 0) != 0) || (vgetq_lane_u64(v1, 1)) != 0)

>
>> +
>> +#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON 16
>> +
>> +/*
>> + * Zero page/buffer checking using SIMD(Neon)
>> + */
>> +
>> +static bool
>> +can_use_buffer_find_nonzero_offset_neon(const void *buf, size_t len)
>> +{
>> +    return (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR_NEON
>> +                   * sizeof(NEON_VECTYPE)) == 0
>> +            && ((uintptr_t) buf) % sizeof(NEON_VECTYPE) == 0);
>> +}
>> +
>> +static size_t buffer_find_nonzero_offset_neon(const void *buf, size_t len)
>> +{
>> +    size_t i;
>> +    NEON_VECTYPE d0, d1, d2, d3, d4, d5, d6;
>> +    NEON_VECTYPE d7, d8, d9, d10, d11, d12, d13, d14;
>> +    uint64_t const *data = buf;
>> +
>> +    assert(can_use_buffer_find_nonzero_offset_neon(buf, len));
>> +    len /= sizeof(unsigned long);
>> +
>> +    for (i = 0; i < len; i += 32) {
>> +        d0 = NEON_LOAD_N_ORR(data[i], data[i + 2]);
>> +        d1 = NEON_LOAD_N_ORR(data[i + 4], data[i + 6]);
>> +        d2 = NEON_LOAD_N_ORR(data[i + 8], data[i + 10]);
>> +        d3 = NEON_LOAD_N_ORR(data[i + 12], data[i + 14]);
>> +        d4 = NEON_ORR(d0, d1);
>> +        d5 = NEON_ORR(d2, d3);
>> +        d6 = NEON_ORR(d4, d5);
>> +
>> +        d7 = NEON_LOAD_N_ORR(data[i + 16], data[i + 18]);
>> +        d8 = NEON_LOAD_N_ORR(data[i + 20], data[i + 22]);
>> +        d9 = NEON_LOAD_N_ORR(data[i + 24], data[i + 26]);
>> +        d10 = NEON_LOAD_N_ORR(data[i + 28], data[i + 30]);
>> +        d11 = NEON_ORR(d7, d8);
>> +        d12 = NEON_ORR(d9, d10);
>> +        d13 = NEON_ORR(d11, d12);
>> +
>> +        d14 = NEON_ORR(d6, d13);
>> +        if (NEON_EQ_ZERO(d14)) {
>> +            break;
>> +        }
>> +    }
>
> Both the other optimised find_nonzero implementations in this
> file have two loops, not just one. Is it OK that this
> implementation has only a single loop?
>
> Paolo: do you know why we have two loops in the other
> implementations?

Paolo was right as he mentioned in the previous email.
But with two loops, I don't see much benefit. So restricted to
one loop.

>
>> +
>> +    return i * sizeof(unsigned long);
>> +}
>> +
>> +static inline bool neon_support(void)
>> +{
>> +    /*
>> +     * Check if neon feature is supported.
>> +     * By default neon is supported for aarch64.
>> +     */
>> +    return true;
>> +}
>
> There doesn't seem much point in this. We can assume Neon exists
> on any CPU we're going to run on (it's part of the ABI, the kernel
> assumes it, etc etc). So you can just implement the functions without
> the indirection functions below.
>
 Hmm. One reason was compilation fails if we don't call
can_use_buffer_find_nonzero_offset_inner() function from inside neon
implementation.
So I added this similar to AVX2 intel. Also thought if any platform
does not implement
Neon, then can simply skip changes this function.

>> +
>> +bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
>> +{
>> +    return neon_support() ? can_use_buffer_find_nonzero_offset_neon(buf, len) :
>> +           can_use_buffer_find_nonzero_offset_inner(buf, len);
>> +}
>> +
>> +size_t buffer_find_nonzero_offset(const void *buf, size_t len)
>> +{
>> +    return neon_support() ? buffer_find_nonzero_offset_neon(buf, len) :
>> +           buffer_find_nonzero_offset_inner(buf, len);
>> +}
>>  #else
>>  bool can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
>>  {
>> --
>
> thanks
> -- PMM

Re: [Qemu-devel] [RFC PATCH v1 2/2] target-arm: Use Neon for zero checking

[Peter Maydell]

On 4 April 2016 at 14:39,  <vijayak@caviumnetworks.com> wrote:
> From: Vijay <vijayak@cavium.com>
>
> Use Neon instructions to perform zero checking of
> buffer. This is helps in reducing downtime during
> live migration.

One other comment I forgot:

> +#define NEON_VECTYPE               uint64x2_t

This is a 128-bit type...

> +static size_t buffer_find_nonzero_offset_neon(const void *buf, size_t len)
> +{
> +    size_t i;
> +    NEON_VECTYPE d0, d1, d2, d3, d4, d5, d6;
> +    NEON_VECTYPE d7, d8, d9, d10, d11, d12, d13, d14;

...so it's a bit confusing to use d0, d1, etc, which implies
a 64-bit value.

thanks
-- PMM