Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
On Thu, 23 Apr 2020 at 14:08, Laurent Desnogues
wrote:
> On Thu, Apr 23, 2020 at 2:44 PM Philippe Mathieu-Daudé
> wrote:
> >
> > MIDR_EL1 is a 32-bit register.
>
> In fact MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.
>
> So the right fix might be to change midr field size, just to be future proof
> :-)
Yes, I think I prefer changing the midr field size. Looking at the
code this should just be a matter of updating the 'uint32_t midr' in
the CPU struct to 'uint64_t midr' and changing the
DEFINE_PROP_UINT32("midr",...)
in cpu.c to UINT64. (The one user of the property in xlnx-zynqmp.c
doesn't need to change because object_property_set_int() works on
both 32-bit and 64-bit integer properties.)
Mostly we have been fixing up these ID register field size values as
we move them from being top-level ARMCPU fields to being in the
ARMISARegisters struct, but I think midr is unlikely to ever need
to move there because no CPU feature is gated on the MIDR value.
thanks
-- PMM
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Patchew URL: https://patchew.org/QEMU/20200423124305.14718-1-f4...@amsat.org/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200423124305.14718-1-f4...@amsat.org/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Patchew URL: https://patchew.org/QEMU/20200423124305.14718-1-f4...@amsat.org/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200423124305.14718-1-f4...@amsat.org/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Patchew URL: https://patchew.org/QEMU/20200423124305.14718-1-f4...@amsat.org/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200423124305.14718-1-f4...@amsat.org/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Patchew URL: https://patchew.org/QEMU/20200423124305.14718-1-f4...@amsat.org/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1 Message-id: 20200423124305.14718-1-f4...@amsat.org Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 fatal: unable to access 'https://github.com/patchew-project/qemu/': The requested URL returned error: 504 error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384 Traceback (most recent call last): File "patchew-tester/src/patchew-cli", line 521, in test_one git_clone_repo(clone, r["repo"], r["head"], logf, True) File "patchew-tester/src/patchew-cli", line 48, in git_clone_repo stdout=logf, stderr=logf) File "/opt/rh/rh-python36/root/usr/lib64/python3.6/subprocess.py", line 291, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1. The full log is available at http://patchew.org/logs/20200423124305.14718-1-f4...@amsat.org/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
Patchew URL: https://patchew.org/QEMU/20200423124305.14718-1-f4...@amsat.org/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200423124305.14718-1-f4...@amsat.org/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
On Thu, Apr 23, 2020 at 2:44 PM Philippe Mathieu-Daudé wrote:
>
> MIDR_EL1 is a 32-bit register.
In fact MIDR_EL1 a 64-bit system register with the top 32-bit being RES0.
So the right fix might be to change midr field size, just to be future proof :-)
But if we stick to a 32-bit midr then:
Reviewed-by: Laurent Desnogues
Thanks,
Laurent
> This fixes when compiling with -Werror=conversion:
>
> target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
> target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long
> unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value
> [-Werror=conversion]
> 628 | cpu->midr = t;
> | ^
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/arm/cpu64.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 95d0c8c101..4eb0a9030e 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -620,12 +620,12 @@ static void aarch64_max_initfn(Object *obj)
> * code needs to distinguish this QEMU CPU from other software
> * implementations, though this shouldn't be needed.
> */
> -t = FIELD_DP64(0, MIDR_EL1, IMPLEMENTER, 0);
> -t = FIELD_DP64(t, MIDR_EL1, ARCHITECTURE, 0xf);
> -t = FIELD_DP64(t, MIDR_EL1, PARTNUM, 'Q');
> -t = FIELD_DP64(t, MIDR_EL1, VARIANT, 0);
> -t = FIELD_DP64(t, MIDR_EL1, REVISION, 0);
> -cpu->midr = t;
> +u = FIELD_DP32(0, MIDR_EL1, IMPLEMENTER, 0);
> +u = FIELD_DP32(u, MIDR_EL1, ARCHITECTURE, 0xf);
> +u = FIELD_DP32(u, MIDR_EL1, PARTNUM, 'Q');
> +u = FIELD_DP32(u, MIDR_EL1, VARIANT, 0);
> +u = FIELD_DP32(u, MIDR_EL1, REVISION, 0);
> +cpu->midr = u;
>
> t = cpu->isar.id_aa64isar0;
> t = FIELD_DP64(t, ID_AA64ISAR0, AES, 2); /* AES + PMULL */
> --
> 2.21.1
>
>
[PATCH] target/arm: Use correct variable for setting 'max' cpu's MIDR_EL1
MIDR_EL1 is a 32-bit register.
This fixes when compiling with -Werror=conversion:
target/arm/cpu64.c: In function ‘aarch64_max_initfn’:
target/arm/cpu64.c:628:21: error: conversion from ‘uint64_t’ {aka ‘long
unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value
[-Werror=conversion]
628 | cpu->midr = t;
| ^
Signed-off-by: Philippe Mathieu-Daudé
---
target/arm/cpu64.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 95d0c8c101..4eb0a9030e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -620,12 +620,12 @@ static void aarch64_max_initfn(Object *obj)
* code needs to distinguish this QEMU CPU from other software
* implementations, though this shouldn't be needed.
*/
-t = FIELD_DP64(0, MIDR_EL1, IMPLEMENTER, 0);
-t = FIELD_DP64(t, MIDR_EL1, ARCHITECTURE, 0xf);
-t = FIELD_DP64(t, MIDR_EL1, PARTNUM, 'Q');
-t = FIELD_DP64(t, MIDR_EL1, VARIANT, 0);
-t = FIELD_DP64(t, MIDR_EL1, REVISION, 0);
-cpu->midr = t;
+u = FIELD_DP32(0, MIDR_EL1, IMPLEMENTER, 0);
+u = FIELD_DP32(u, MIDR_EL1, ARCHITECTURE, 0xf);
+u = FIELD_DP32(u, MIDR_EL1, PARTNUM, 'Q');
+u = FIELD_DP32(u, MIDR_EL1, VARIANT, 0);
+u = FIELD_DP32(u, MIDR_EL1, REVISION, 0);
+cpu->midr = u;
t = cpu->isar.id_aa64isar0;
t = FIELD_DP64(t, ID_AA64ISAR0, AES, 2); /* AES + PMULL */
--
2.21.1
