diff mbox series

linux-user: implement HWCAP bits on MIPS

Message ID 20180314153121.23838-1-james.cowgill@mips.com
State New
Headers show
Series linux-user: implement HWCAP bits on MIPS | expand

Commit Message

James Cowgill March 14, 2018, 3:31 p.m. UTC 
Add support for the two currently defined HWCAP bits on MIPS - R6 and
MSA.

Buglink: https://bugs.launchpad.net/qemu/+bug/1754372
Signed-off-by: James Cowgill <james.cowgill@mips.com>
---
This was resent because I think I messed up my email config. Apologies if you
receive this twice.

 linux-user/elfload.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

no-reply@patchew.org March 14, 2018, 3:38 p.m. UTC | #1 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180314153121.23838-1-james.cowgill@mips.com
Subject: [Qemu-devel] [PATCH] linux-user: implement HWCAP bits on MIPS

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180314153121.23838-1-james.cowgill@mips.com -> patchew/20180314153121.23838-1-james.cowgill@mips.com
Switched to a new branch 'test'
4b00115726 linux-user: implement HWCAP bits on MIPS

=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: implement HWCAP bits on MIPS...
ERROR: braces {} are necessary for all arms of this statement
#35: FILE: linux-user/elfload.c:967:
+    do { if (cpu->env.insn_flags & (flag)) { hwcaps |= hwcap; } } while (0)
[...]

total: 1 errors, 0 warnings, 30 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Laurent Vivier March 14, 2018, 4:13 p.m. UTC | #2 
Le 14/03/2018 à 16:31, James Cowgill a écrit :
> Add support for the two currently defined HWCAP bits on MIPS - R6 and
> MSA.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1754372
> Signed-off-by: James Cowgill <james.cowgill@mips.com>
> ---
> This was resent because I think I messed up my email config. Apologies if you
> receive this twice.
> 
>  linux-user/elfload.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 5fc130cc20..747b0ed10b 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -950,6 +950,30 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
>  #define USE_ELF_CORE_DUMP
>  #define ELF_EXEC_PAGESIZE        4096
>  
> +/* See arch/mips/include/uapi/hwcap.h.  */

in fact arch/mips/include/uapi/asm/hwcap.h

> +enum {
> +    HWCAP_MIPS_R6           = (1 << 0),
> +    HWCAP_MIPS_MSA          = (1 << 1),
> +};

We have this for ARM only in elfload.c since:

    afce2927aa Arm AT_HWCAP AUXV entry (Paul Brook) [2005]

but they have been added in include/elf.h since:

    41d9ea80ac tcg-arm: Use qemu_getauxval [Richard Henderson, 2013]

and I think we should remove them (they are prefixed by ARM_)

So the MIPS ones should be in include/elf.h (with the #define form).

Richard, any comment?

Thanks,
Laurent
James Cowgill March 15, 2018, 10:52 a.m. UTC | #3 
Hi,

On 14/03/18 16:13, Laurent Vivier wrote:
> Le 14/03/2018 à 16:31, James Cowgill a écrit :
>> Add support for the two currently defined HWCAP bits on MIPS - R6 and
>> MSA.
>>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1754372
>> Signed-off-by: James Cowgill <james.cowgill@mips.com>
>> ---
>> This was resent because I think I messed up my email config. Apologies if you
>> receive this twice.
>>
>>  linux-user/elfload.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 5fc130cc20..747b0ed10b 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -950,6 +950,30 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
>>  #define USE_ELF_CORE_DUMP
>>  #define ELF_EXEC_PAGESIZE        4096
>>  
>> +/* See arch/mips/include/uapi/hwcap.h.  */
> 
> in fact arch/mips/include/uapi/asm/hwcap.h

Woops.

>> +enum {
>> +    HWCAP_MIPS_R6           = (1 << 0),
>> +    HWCAP_MIPS_MSA          = (1 << 1),
>> +};
> 
> We have this for ARM only in elfload.c since:
> 
>     afce2927aa Arm AT_HWCAP AUXV entry (Paul Brook) [2005]
> 
> but they have been added in include/elf.h since:
> 
>     41d9ea80ac tcg-arm: Use qemu_getauxval [Richard Henderson, 2013]
> 
> and I think we should remove them (they are prefixed by ARM_)
> 
> So the MIPS ones should be in include/elf.h (with the #define form).

I can do that, although I think it's a bit unusual. The HWCAP bits are
specific to the Linux kernel and not to "the MIPS ELF format" so it
doesn't make sense to me to put them in elf.h.

James
Laurent Vivier March 15, 2018, 1 p.m. UTC | #4 
Le 15/03/2018 à 11:52, James Cowgill a écrit :
> Hi,
> 
> On 14/03/18 16:13, Laurent Vivier wrote:
>> Le 14/03/2018 à 16:31, James Cowgill a écrit :
>>> Add support for the two currently defined HWCAP bits on MIPS - R6 and
>>> MSA.
>>>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1754372
>>> Signed-off-by: James Cowgill <james.cowgill@mips.com>
>>> ---
>>> This was resent because I think I messed up my email config. Apologies if you
>>> receive this twice.
>>>
>>>  linux-user/elfload.c | 24 ++++++++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>>> index 5fc130cc20..747b0ed10b 100644
>>> --- a/linux-user/elfload.c
>>> +++ b/linux-user/elfload.c
>>> @@ -950,6 +950,30 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
>>>  #define USE_ELF_CORE_DUMP
>>>  #define ELF_EXEC_PAGESIZE        4096
>>>  
>>> +/* See arch/mips/include/uapi/hwcap.h.  */
>>
>> in fact arch/mips/include/uapi/asm/hwcap.h
> 
> Woops.
> 
>>> +enum {
>>> +    HWCAP_MIPS_R6           = (1 << 0),
>>> +    HWCAP_MIPS_MSA          = (1 << 1),
>>> +};
>>
>> We have this for ARM only in elfload.c since:
>>
>>     afce2927aa Arm AT_HWCAP AUXV entry (Paul Brook) [2005]
>>
>> but they have been added in include/elf.h since:
>>
>>     41d9ea80ac tcg-arm: Use qemu_getauxval [Richard Henderson, 2013]
>>
>> and I think we should remove them (they are prefixed by ARM_)
>>
>> So the MIPS ones should be in include/elf.h (with the #define form).
> 
> I can do that, although I think it's a bit unusual. The HWCAP bits are
> specific to the Linux kernel and not to "the MIPS ELF format" so it
> doesn't make sense to me to put them in elf.h.

In fact, in a system, they come with the glibc <sys/auxv.h>. auxv.h
includes <elf.h> and <bits/hwcap.h>. They are both part of glibc.

They can be used with qemu_getauxval() so it's better to have them in a
header file. elfoad.c or elf.h, it's in both cases an ELF file.

Thanks,
Laurent
James Cowgill March 15, 2018, 2:19 p.m. UTC | #5 
Hi,

On 15/03/18 13:00, Laurent Vivier wrote:
> Le 15/03/2018 à 11:52, James Cowgill a écrit :
>> Hi,
>>
>> On 14/03/18 16:13, Laurent Vivier wrote:
>>> Le 14/03/2018 à 16:31, James Cowgill a écrit :
>>>> +enum {
>>>> +    HWCAP_MIPS_R6           = (1 << 0),
>>>> +    HWCAP_MIPS_MSA          = (1 << 1),
>>>> +};
>>>
>>> We have this for ARM only in elfload.c since:
>>>
>>>     afce2927aa Arm AT_HWCAP AUXV entry (Paul Brook) [2005]
>>>
>>> but they have been added in include/elf.h since:
>>>
>>>     41d9ea80ac tcg-arm: Use qemu_getauxval [Richard Henderson, 2013]
>>>
>>> and I think we should remove them (they are prefixed by ARM_)
>>>
>>> So the MIPS ones should be in include/elf.h (with the #define form).
>>
>> I can do that, although I think it's a bit unusual. The HWCAP bits are
>> specific to the Linux kernel and not to "the MIPS ELF format" so it
>> doesn't make sense to me to put them in elf.h.
> 
> In fact, in a system, they come with the glibc <sys/auxv.h>. auxv.h
> includes <elf.h> and <bits/hwcap.h>. They are both part of glibc.

None of the headers you mention contain hwcap bits. They are usually
provided in an arch specific kernel header - on MIPS they are in
<asm/hwcap.h> which must be included separately.

> They can be used with qemu_getauxval() so it's better to have them in a
> header file. elfoad.c or elf.h, it's in both cases an ELF file.

I agree with that.

I tried to move them to elf.h but hit a slight problem. In elfload.c,
elf.h is included after all the target specific stuff so the
get_elf_hwcap function cannot use anything from elf.h. I think this has
lead to all architectures replicating the list of hwcap bits in both
elf.h and elfload.c. You mentioned arm before, but I can also see
aarch64, powerpc and sh4 do this. Some of these architectures also have
their bits (with different constant names) in elf.h and some do not.

James
Laurent Vivier March 15, 2018, 2:35 p.m. UTC | #6 
Le 15/03/2018 à 15:19, James Cowgill a écrit :
> Hi,
> 
> On 15/03/18 13:00, Laurent Vivier wrote:
>> Le 15/03/2018 à 11:52, James Cowgill a écrit :
>>> Hi,
>>>
>>> On 14/03/18 16:13, Laurent Vivier wrote:
>>>> Le 14/03/2018 à 16:31, James Cowgill a écrit :
>>>>> +enum {
>>>>> +    HWCAP_MIPS_R6           = (1 << 0),
>>>>> +    HWCAP_MIPS_MSA          = (1 << 1),
>>>>> +};
>>>>
>>>> We have this for ARM only in elfload.c since:
>>>>
>>>>     afce2927aa Arm AT_HWCAP AUXV entry (Paul Brook) [2005]
>>>>
>>>> but they have been added in include/elf.h since:
>>>>
>>>>     41d9ea80ac tcg-arm: Use qemu_getauxval [Richard Henderson, 2013]
>>>>
>>>> and I think we should remove them (they are prefixed by ARM_)
>>>>
>>>> So the MIPS ones should be in include/elf.h (with the #define form).
>>>
>>> I can do that, although I think it's a bit unusual. The HWCAP bits are
>>> specific to the Linux kernel and not to "the MIPS ELF format" so it
>>> doesn't make sense to me to put them in elf.h.
>>
>> In fact, in a system, they come with the glibc <sys/auxv.h>. auxv.h
>> includes <elf.h> and <bits/hwcap.h>. They are both part of glibc.
> 
> None of the headers you mention contain hwcap bits. They are usually
> provided in an arch specific kernel header - on MIPS they are in
> <asm/hwcap.h> which must be included separately.
> 
>> They can be used with qemu_getauxval() so it's better to have them in a
>> header file. elfoad.c or elf.h, it's in both cases an ELF file.
> 
> I agree with that.
> 
> I tried to move them to elf.h but hit a slight problem. In elfload.c,
> elf.h is included after all the target specific stuff so the
> get_elf_hwcap function cannot use anything from elf.h. I think this has
> lead to all architectures replicating the list of hwcap bits in both
> elf.h and elfload.c. You mentioned arm before, but I can also see
> aarch64, powerpc and sh4 do this. Some of these architectures also have
> their bits (with different constant names) in elf.h and some do not.

OK, you're right. That's sad to have duplicated values...

So your patch is correct. Perhaps just fix the typo in the path name for
hwcap.h.

Thanks,
Laurent
diff mbox series

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5fc130cc20..747b0ed10b 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -950,6 +950,30 @@  static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUMIPSState *e
 #define USE_ELF_CORE_DUMP
 #define ELF_EXEC_PAGESIZE        4096
 
+/* See arch/mips/include/uapi/hwcap.h.  */
+enum {
+    HWCAP_MIPS_R6           = (1 << 0),
+    HWCAP_MIPS_MSA          = (1 << 1),
+};
+
+#define ELF_HWCAP get_elf_hwcap()
+
+static uint32_t get_elf_hwcap(void)
+{
+    MIPSCPU *cpu = MIPS_CPU(thread_cpu);
+    uint32_t hwcaps = 0;
+
+#define GET_FEATURE(flag, hwcap) \
+    do { if (cpu->env.insn_flags & (flag)) { hwcaps |= hwcap; } } while (0)
+
+    GET_FEATURE(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
+    GET_FEATURE(ASE_MSA, HWCAP_MIPS_MSA);
+
+#undef GET_FEATURE
+
+    return hwcaps;
+}
+
 #endif /* TARGET_MIPS */
 
 #ifdef TARGET_MICROBLAZE