Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Peter Maydell]

On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Add helper API to read MIDR_EL1 registers to fetch
> cpu identification information. This helps in
> adding errata's and architecture specific features.
>
> This is implemented only for arm architecture.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  include/qemu/aarch64-cpuid.h | 29 +++++++++++++++++++++
>  util/Makefile.objs           |  1 +
>  util/aarch64-cpuid.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>
> diff --git a/include/qemu/aarch64-cpuid.h b/include/qemu/aarch64-cpuid.h
> new file mode 100644
> index 0000000..8f776e8
> --- /dev/null
> +++ b/include/qemu/aarch64-cpuid.h
> @@ -0,0 +1,29 @@
> +#ifndef QEMU_AARCH64_CPUID_H
> +#define QEMU_AARCH64_CPUID_H
> +
> +#if defined(__aarch64__)
> +#define MIDR_IMPLEMENTER_SHIFT  24
> +#define MIDR_IMPLEMENTER_MASK   (0xffULL << MIDR_IMPLEMENTER_SHIFT)
> +#define MIDR_ARCHITECTURE_SHIFT 16
> +#define MIDR_ARCHITECTURE_MASK  (0xf << MIDR_ARCHITECTURE_SHIFT)
> +#define MIDR_PARTNUM_SHIFT      4
> +#define MIDR_PARTNUM_MASK       (0xfff << MIDR_PARTNUM_SHIFT)
> +
> +#define MIDR_CPU_PART(imp, partnum) \
> +        (((imp)                 << MIDR_IMPLEMENTER_SHIFT)  | \
> +        (0xf                    << MIDR_ARCHITECTURE_SHIFT) | \
> +        ((partnum)              << MIDR_PARTNUM_SHIFT))
> +
> +#define ARM_CPU_IMP_CAVIUM        0x43
> +#define CAVIUM_CPU_PART_THUNDERX  0x0A1
> +
> +#define MIDR_THUNDERX_PASS2  \
> +               MIDR_CPU_PART(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
> +#define CPU_MODEL_MASK  (MIDR_IMPLEMENTER_MASK | MIDR_ARCHITECTURE_MASK | \
> +                         MIDR_PARTNUM_MASK)
> +
> +uint64_t get_aarch64_cpu_id(void);
> +bool is_thunderx_pass2_cpu(void);
> +#endif
> +
> +#endif
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 36c7dcc..d14a455 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -37,3 +37,4 @@ util-obj-y += log.o
>  util-obj-y += qdist.o
>  util-obj-y += qht.o
>  util-obj-y += range.o
> +util-obj-y += aarch64-cpuid.o
> diff --git a/util/aarch64-cpuid.c b/util/aarch64-cpuid.c
> new file mode 100644
> index 0000000..536ece1
> --- /dev/null
> +++ b/util/aarch64-cpuid.c
> @@ -0,0 +1,61 @@
> +/*
> + * Dealing with arm cpu identification information.
> + *
> + * Copyright (C) 2016 Cavium, Inc.
> + *
> + * Authors:
> + *  Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1
> + * or later.  See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include <math.h>
> +#include "qemu/osdep.h"

osdep.h must always be the first #include, before anything else.

What do we need math.h for anyway?

> +#include "qemu-common.h"

What do we need qemu-common.h for ?

> +#include "qemu/cutils.h"
> +#include "qemu/aarch64-cpuid.h"
> +
> +#if defined(__aarch64__)
> +static uint64_t qemu_read_aarch64_midr_el1(void)
> +{
> +#ifdef CONFIG_LINUX

When will CONFIG_LINUX not be defined but __aarch64__ is?

> +    const char *file = "/sys/devices/system/cpu/cpu0/regs/identification/midr_el1";
> +    char *buf;
> +    uint64_t midr = 0;
> +
> +    if (!g_file_get_contents(file, &buf, 0, NULL)) {
> +        goto out;
> +    }
> +
> +    if (qemu_strtoull(buf, NULL, 0, &midr) < 0) {
> +        goto out;
> +    }
> +
> +out:

Why do we do a goto to the immediately following statement?

You have an inconsistency in that for one error condition
(if g_file_get_contents() fails) this function will
return 0, but for another error condition (qemu_strtoull()
fails) you'll return -1.

> +    g_free(buf);
> +
> +    return midr;
> +#else
> +    return 0;
> +#endif
> +}
> +
> +static uint64_t aarch64_midr_val;
> +uint64_t get_aarch64_cpu_id(void)
> +{
> +#ifdef CONFIG_LINUX
> +    aarch64_midr_val = qemu_read_aarch64_midr_el1();
> +    aarch64_midr_val &= CPU_MODEL_MASK;
> +
> +    return aarch64_midr_val;
> +#else
> +    return 0;
> +#endif
> +}
> +
> +bool is_thunderx_pass2_cpu(void)
> +{
> +    return aarch64_midr_val == MIDR_THUNDERX_PASS2;
> +}
> +#endif

> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Vijay Kilari]

On Thu, Oct 27, 2016 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> Add helper API to read MIDR_EL1 registers to fetch
>> cpu identification information. This helps in
>> adding errata's and architecture specific features.
>>
>> This is implemented only for arm architecture.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> ---
>>  include/qemu/aarch64-cpuid.h | 29 +++++++++++++++++++++
>>  util/Makefile.objs           |  1 +
>>  util/aarch64-cpuid.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 91 insertions(+)
>>
>> diff --git a/include/qemu/aarch64-cpuid.h b/include/qemu/aarch64-cpuid.h
>> new file mode 100644
>> index 0000000..8f776e8
>> --- /dev/null
>> +++ b/include/qemu/aarch64-cpuid.h
>> @@ -0,0 +1,29 @@
>> +#ifndef QEMU_AARCH64_CPUID_H
>> +#define QEMU_AARCH64_CPUID_H
>> +
>> +#if defined(__aarch64__)
>> +#define MIDR_IMPLEMENTER_SHIFT  24
>> +#define MIDR_IMPLEMENTER_MASK   (0xffULL << MIDR_IMPLEMENTER_SHIFT)
>> +#define MIDR_ARCHITECTURE_SHIFT 16
>> +#define MIDR_ARCHITECTURE_MASK  (0xf << MIDR_ARCHITECTURE_SHIFT)
>> +#define MIDR_PARTNUM_SHIFT      4
>> +#define MIDR_PARTNUM_MASK       (0xfff << MIDR_PARTNUM_SHIFT)
>> +
>> +#define MIDR_CPU_PART(imp, partnum) \
>> +        (((imp)                 << MIDR_IMPLEMENTER_SHIFT)  | \
>> +        (0xf                    << MIDR_ARCHITECTURE_SHIFT) | \
>> +        ((partnum)              << MIDR_PARTNUM_SHIFT))
>> +
>> +#define ARM_CPU_IMP_CAVIUM        0x43
>> +#define CAVIUM_CPU_PART_THUNDERX  0x0A1
>> +
>> +#define MIDR_THUNDERX_PASS2  \
>> +               MIDR_CPU_PART(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
>> +#define CPU_MODEL_MASK  (MIDR_IMPLEMENTER_MASK | MIDR_ARCHITECTURE_MASK | \
>> +                         MIDR_PARTNUM_MASK)
>> +
>> +uint64_t get_aarch64_cpu_id(void);
>> +bool is_thunderx_pass2_cpu(void);
>> +#endif
>> +
>> +#endif
>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>> index 36c7dcc..d14a455 100644
>> --- a/util/Makefile.objs
>> +++ b/util/Makefile.objs
>> @@ -37,3 +37,4 @@ util-obj-y += log.o
>>  util-obj-y += qdist.o
>>  util-obj-y += qht.o
>>  util-obj-y += range.o
>> +util-obj-y += aarch64-cpuid.o
>> diff --git a/util/aarch64-cpuid.c b/util/aarch64-cpuid.c
>> new file mode 100644
>> index 0000000..536ece1
>> --- /dev/null
>> +++ b/util/aarch64-cpuid.c
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Dealing with arm cpu identification information.
>> + *
>> + * Copyright (C) 2016 Cavium, Inc.
>> + *
>> + * Authors:
>> + *  Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1
>> + * or later.  See the COPYING.LIB file in the top-level directory.
>> + */
>> +
>> +#include <math.h>
>> +#include "qemu/osdep.h"
>
> osdep.h must always be the first #include, before anything else.
>
> What do we need math.h for anyway?
>
>> +#include "qemu-common.h"
>
> What do we need qemu-common.h for ?
>
>> +#include "qemu/cutils.h"
>> +#include "qemu/aarch64-cpuid.h"
>> +
>> +#if defined(__aarch64__)
>> +static uint64_t qemu_read_aarch64_midr_el1(void)
>> +{
>> +#ifdef CONFIG_LINUX
>
> When will CONFIG_LINUX not be defined but __aarch64__ is?
  The contents of this file is compiled only for aarch64 and hence
all the contents are under this __aarch64__.
Also the code is only for linux, have added CONFIG_LINUX.

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Peter Maydell]

On 28 October 2016 at 08:00, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Thu, Oct 27, 2016 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>
>>> Add helper API to read MIDR_EL1 registers to fetch
>>> cpu identification information. This helps in
>>> adding errata's and architecture specific features.
>>>
>>> This is implemented only for arm architecture.
>>>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

>>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>>> index 36c7dcc..d14a455 100644
>>> --- a/util/Makefile.objs
>>> +++ b/util/Makefile.objs
>>> @@ -37,3 +37,4 @@ util-obj-y += log.o
>>>  util-obj-y += qdist.o
>>>  util-obj-y += qht.o
>>>  util-obj-y += range.o
>>> +util-obj-y += aarch64-cpuid.o

>>
>>> +#include "qemu/cutils.h"
>>> +#include "qemu/aarch64-cpuid.h"
>>> +
>>> +#if defined(__aarch64__)
>>> +static uint64_t qemu_read_aarch64_midr_el1(void)
>>> +{
>>> +#ifdef CONFIG_LINUX
>>
>> When will CONFIG_LINUX not be defined but __aarch64__ is?
>   The contents of this file is compiled only for aarch64

Your makefile change compiles it for everything.

> and hence
> all the contents are under this __aarch64__.
> Also the code is only for linux, have added CONFIG_LINUX.

...and you haven't answered the question: in what
circumstances could __aarch64__ be defined but
CONFIG_LINUX is not, ie why is there any point in
checking both defines?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Vijay Kilari]

On Fri, Oct 28, 2016 at 2:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 October 2016 at 08:00, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Thu, Oct 27, 2016 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> Add helper API to read MIDR_EL1 registers to fetch
>>>> cpu identification information. This helps in
>>>> adding errata's and architecture specific features.
>>>>
>>>> This is implemented only for arm architecture.
>>>>
>>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
>>>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>>>> index 36c7dcc..d14a455 100644
>>>> --- a/util/Makefile.objs
>>>> +++ b/util/Makefile.objs
>>>> @@ -37,3 +37,4 @@ util-obj-y += log.o
>>>>  util-obj-y += qdist.o
>>>>  util-obj-y += qht.o
>>>>  util-obj-y += range.o
>>>> +util-obj-y += aarch64-cpuid.o
>
>>>
>>>> +#include "qemu/cutils.h"
>>>> +#include "qemu/aarch64-cpuid.h"
>>>> +
>>>> +#if defined(__aarch64__)
>>>> +static uint64_t qemu_read_aarch64_midr_el1(void)
>>>> +{
>>>> +#ifdef CONFIG_LINUX
>>>
>>> When will CONFIG_LINUX not be defined but __aarch64__ is?
>>   The contents of this file is compiled only for aarch64
>
> Your makefile change compiles it for everything.
>
>> and hence
>> all the contents are under this __aarch64__.
>> Also the code is only for linux, have added CONFIG_LINUX.
>
> ...and you haven't answered the question: in what
> circumstances could __aarch64__ be defined but
> CONFIG_LINUX is not, ie why is there any point in
> checking both defines?

Ok. You mean __aarch64__ and __linux__ both are defined by gcc.
we can rely on __aarch64__ define  here?.

AFAIK, the caller of this function bufferiszero.c is compiled
for everything, I case of bufferiszero.c is compiled for other than
linux for aarch64, compilation might fail. In such case, the header file
needs to have dummy/empty functions.

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Vijay Kilari]

Hi Peter

On Fri, Oct 28, 2016 at 3:39 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Fri, Oct 28, 2016 at 2:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 28 October 2016 at 08:00, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>>> On Thu, Oct 27, 2016 at 9:33 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
>>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>>
>>>>> Add helper API to read MIDR_EL1 registers to fetch
>>>>> cpu identification information. This helps in
>>>>> adding errata's and architecture specific features.
>>>>>
>>>>> This is implemented only for arm architecture.
>>>>>
>>>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>>>>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>>>>> index 36c7dcc..d14a455 100644
>>>>> --- a/util/Makefile.objs
>>>>> +++ b/util/Makefile.objs
>>>>> @@ -37,3 +37,4 @@ util-obj-y += log.o
>>>>>  util-obj-y += qdist.o
>>>>>  util-obj-y += qht.o
>>>>>  util-obj-y += range.o
>>>>> +util-obj-y += aarch64-cpuid.o
>>
>>>>
>>>>> +#include "qemu/cutils.h"
>>>>> +#include "qemu/aarch64-cpuid.h"
>>>>> +
>>>>> +#if defined(__aarch64__)
>>>>> +static uint64_t qemu_read_aarch64_midr_el1(void)
>>>>> +{
>>>>> +#ifdef CONFIG_LINUX
>>>>
>>>> When will CONFIG_LINUX not be defined but __aarch64__ is?
>>>   The contents of this file is compiled only for aarch64
>>
>> Your makefile change compiles it for everything.
>>
>>> and hence
>>> all the contents are under this __aarch64__.
>>> Also the code is only for linux, have added CONFIG_LINUX.
>>
>> ...and you haven't answered the question: in what
>> circumstances could __aarch64__ be defined but
>> CONFIG_LINUX is not, ie why is there any point in
>> checking both defines?
>
> Ok. You mean __aarch64__ and __linux__ both are defined by gcc.
> we can rely on __aarch64__ define  here?.
>
> AFAIK, the caller of this function bufferiszero.c is compiled
> for everything, I case of bufferiszero.c is compiled for other than
> linux for aarch64, compilation might fail. In such case, the header file
> needs to have dummy/empty functions.

What do you suggest for this?.

Regards
Vijay

Re: [Qemu-devel] [PATCH v4 2/3] utils: Add helper to read arm MIDR_EL1 register

[Peter Maydell]

On 4 November 2016 at 14:54, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> What do you suggest for this?.

Some general principles:
 * be consistent
 * don't check things you don't need to
 * prefer functions that exist in all configs (which may be
   dummy functions in some cases) rather than guarding
   callsites and #include lines with #ifdefs

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 3/3] utils: Add prefetch for Thunderx platform

[Peter Maydell]

On 25 October 2016 at 13:12,  <vijay.kilari@gmail.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Thunderx pass2 chip requires explicit prefetch
> instruction to give prefetch hint.
>
> To speed up live migration on Thunderx platform,
> prefetch instruction is added in zero buffer check
> function.The below results show live migration time improvement
> with prefetch instruction. VM with 4 VCPUs, 8GB RAM is migrated.
>
> Code for decoding cache size is taken from Richard's patch.

> +#if defined(__aarch64__)
> +#include "qemu/aarch64-cpuid.h"
> +
> +static void __attribute__((constructor)) aarch64_init_cache_size(void)
> +{
> +    uint64_t t;
> +
> +    /* Use the DZP block size as a proxy for the cacheline size,
> +       since the later is not available to userspace.  This seems
> +       to work in practice for existing implementations.  */
> +    asm("mrs %0, dczid_el0" : "=r"(t));
> +    if (pow(2, (t & 0xf)) * 4 >= 128) {

Converting to double and calling a function in the math library
just to determine 2^x seems a bit silly when that is (1 << x).

   dzp_blocksize_bytes = 1 << ((t & 0xf) + 2);

   if (dzp_blocksize_bytes >= 128) {

> +        cache_line_size = 128;
> +    } else {
> +        cache_line_size = 64;
> +    }
> +
> +    get_aarch64_cpu_id();
> +    if (is_thunderx_pass2_cpu()) {
> +        prefetch_line_dist = 3;
> +        prefetch_distance = (prefetch_line_dist * cache_line_size) /
> +                             sizeof(uint64_t);
> +    }
> +}
> +#endif

thanks
-- PMM