[RFC,v1,1/2] utils: Add helper to read arm MIDR_EL1 register

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-common.h |    1 +
 util/Makefile.objs    |    1 +
 util/cpuinfo.c        |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

----- Original Message -----
> From: "vijay kilari" <vijay.kilari@gmail.com>
> To: qemu-arm@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, pbonzini@redhat.com
> Cc: qemu-devel@nongnu.org, "Prasun Kapoor" <Prasun.Kapoor@cavium.com>, "vijay kilari" <vijay.kilari@gmail.com>,
> "Vijaya Kumar K" <Vijaya.Kumar@cavium.com>
> Sent: Tuesday, August 2, 2016 12:20:15 PM
> Subject: [RFC PATCH v1 1/2] utils: Add helper to read arm MIDR_EL1 register
> 
> 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-common.h |    1 +
>  util/Makefile.objs    |    1 +
>  util/cpuinfo.c        |   52
>  +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 1f2cb94..62ad674 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -134,4 +134,5 @@ void page_size_init(void);
>   * returned. */
>  bool dump_in_progress(void);
>  
> +long int qemu_read_cpuid_info(void);

First, please avoid adding to include/qemu-common.h (it really should
go away).

Second, this is too generic a name.  Please call it something like
qemu_read_aarch64_midr_el1.

Third, it's probably a bad idea to call this function from generic code, so
make it static and add the detection function from patch 2/2 already here.
By making it static, it's also possible to define it only if CONFIG_LINUX
is defined; the ThunderX detection will then return false if !CONFIG_LINUX.

Thanks,

Paolo

>  #endif
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 96cb1e0..9d25a72 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -35,3 +35,4 @@ util-obj-y += log.o
>  util-obj-y += qdist.o
>  util-obj-y += qht.o
>  util-obj-y += range.o
> +util-obj-y += cpuinfo.o
> diff --git a/util/cpuinfo.c b/util/cpuinfo.c
> new file mode 100644
> index 0000000..3ba7194
> --- /dev/null
> +++ b/util/cpuinfo.c
> @@ -0,0 +1,52 @@
> +/*
> + * 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 "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/cutils.h"
> +
> +#if defined(__aarch64__)
> +
> +long int qemu_read_cpuid_info(void)
> +{
> +    FILE *fp;
> +    char *buf;
> +    long int midr = 0;
> +#define BUF_SIZE 32
> +
> +    fp = fopen("/sys/devices/system/cpu/cpu0/regs/identification/midr_el1",
> +               "r");
> +    if (!fp) {
> +        return 0;
> +    }
> +
> +    buf = g_malloc0(BUF_SIZE);
> +    if (!buf) {
> +        fclose(fp);
> +        return 0;
> +    }
> +
> +    if (buf != fgets(buf, BUF_SIZE - 1, fp)) {
> +        goto out;
> +    }
> +
> +    if (qemu_strtol(buf, NULL, 0, &midr) < 0) {
> +        goto out;
> +    }
> +
> +out:
> +    g_free(buf);
> +    fclose(fp);
> +
> +    return midr;
> +}
> +#endif
> --
> 1.7.9.5
> 
>

On 2 August 2016 at 11:20,  <vijay.kilari@gmail.com> wrote:
> +long int qemu_read_cpuid_info(void)

Don't use "long" here, it might be 32 or 64 bits.
The kernel ABI for the /sys/ file we're reading says it
is a 64-bit value, so uint64_t is what you want.

> +{
> +    FILE *fp;
> +    char *buf;
> +    long int midr = 0;
> +#define BUF_SIZE 32
> +
> +    fp = fopen("/sys/devices/system/cpu/cpu0/regs/identification/midr_el1",
> +               "r");
> +    if (!fp) {
> +        return 0;
> +    }
> +
> +    buf = g_malloc0(BUF_SIZE);
> +    if (!buf) {
> +        fclose(fp);
> +        return 0;
> +    }
> +
> +    if (buf != fgets(buf, BUF_SIZE - 1, fp)) {
> +        goto out;
> +    }

g_file_get_contents() is probably easier than manually
opening the file and reading it into an allocated buffer.

> +
> +    if (qemu_strtol(buf, NULL, 0, &midr) < 0) {

qemu_strtoull().

> +        goto out;
> +    }
> +
> +out:
> +    g_free(buf);
> +    fclose(fp);
> +
> +    return midr;
> +}
> +#endif
> --
> 1.7.9.5
>

thanks
-- PMM

Hi Paolo,

On Tue, Aug 2, 2016 at 4:18 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> ----- Original Message -----
>> From: "vijay kilari" <vijay.kilari@gmail.com>
>> To: qemu-arm@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, pbonzini@redhat.com
>> Cc: qemu-devel@nongnu.org, "Prasun Kapoor" <Prasun.Kapoor@cavium.com>, "vijay kilari" <vijay.kilari@gmail.com>,
>> "Vijaya Kumar K" <Vijaya.Kumar@cavium.com>
>> Sent: Tuesday, August 2, 2016 12:20:15 PM
>> Subject: [RFC PATCH v1 1/2] utils: Add helper to read arm MIDR_EL1 register
>>
>> 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-common.h |    1 +
>>  util/Makefile.objs    |    1 +
>>  util/cpuinfo.c        |   52
>>  +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 54 insertions(+)
>>
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index 1f2cb94..62ad674 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -134,4 +134,5 @@ void page_size_init(void);
>>   * returned. */
>>  bool dump_in_progress(void);
>>
>> +long int qemu_read_cpuid_info(void);
>
> First, please avoid adding to include/qemu-common.h (it really should
> go away).
>
> Second, this is too generic a name.  Please call it something like
> qemu_read_aarch64_midr_el1.

OK
>
> Third, it's probably a bad idea to call this function from generic code, so
> make it static and add the detection function from patch 2/2 already here.
> By making it static, it's also possible to define it only if CONFIG_LINUX
> is defined; the ThunderX detection will then return false if !CONFIG_LINUX.
>

  You mean to say, move contents of this patch to util/cutils.c and make it
static and define under __aarch64__ and CONFIG_LINUX?.

> Thanks,
>
> Paolo
>
>>  #endif
>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>> index 96cb1e0..9d25a72 100644
>> --- a/util/Makefile.objs
>> +++ b/util/Makefile.objs
>> @@ -35,3 +35,4 @@ util-obj-y += log.o
>>  util-obj-y += qdist.o
>>  util-obj-y += qht.o
>>  util-obj-y += range.o
>> +util-obj-y += cpuinfo.o
>> diff --git a/util/cpuinfo.c b/util/cpuinfo.c
>> new file mode 100644
>> index 0000000..3ba7194
>> --- /dev/null
>> +++ b/util/cpuinfo.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * 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 "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qemu/cutils.h"
>> +
>> +#if defined(__aarch64__)
>> +
>> +long int qemu_read_cpuid_info(void)
>> +{
>> +    FILE *fp;
>> +    char *buf;
>> +    long int midr = 0;
>> +#define BUF_SIZE 32
>> +
>> +    fp = fopen("/sys/devices/system/cpu/cpu0/regs/identification/midr_el1",
>> +               "r");
>> +    if (!fp) {
>> +        return 0;
>> +    }
>> +
>> +    buf = g_malloc0(BUF_SIZE);
>> +    if (!buf) {
>> +        fclose(fp);
>> +        return 0;
>> +    }
>> +
>> +    if (buf != fgets(buf, BUF_SIZE - 1, fp)) {
>> +        goto out;
>> +    }
>> +
>> +    if (qemu_strtol(buf, NULL, 0, &midr) < 0) {
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    g_free(buf);
>> +    fclose(fp);
>> +
>> +    return midr;
>> +}
>> +#endif
>> --
>> 1.7.9.5
>>
>>

> > Third, it's probably a bad idea to call this function from generic code, so
> > make it static and add the detection function from patch 2/2 already here.
> > By making it static, it's also possible to define it only if CONFIG_LINUX
> > is defined; the ThunderX detection will then return false if !CONFIG_LINUX.
> >
> 
>   You mean to say, move contents of this patch to util/cutils.c and make it
> static and define under __aarch64__ and CONFIG_LINUX?.

I don't think util/cutils.c is the right file.  It should be a new file, something
like util/aarch64-cpuid.c.

If CONFIG_LINUX is not defined, the ThunderX detection function should return
zero.

If __aarch64__ is not defined, the function should not be defined at all.

Paolo