Message ID | alpine.DEB.2.10.1711061412330.30448@sstabellini-ThinkPad-X260 |
---|---|
State | New |
Headers | show |
Series | [v2] aarch64: advertise the GIC system register interface | expand |
Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v2] aarch64: advertise the GIC system register interface Type: series Message-id: alpine.DEB.2.10.1711061412330.30448@sstabellini-ThinkPad-X260 === 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 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/alpine.DEB.2.10.1711061412330.30448@sstabellini-ThinkPad-X260 -> patchew/alpine.DEB.2.10.1711061412330.30448@sstabellini-ThinkPad-X260 Switched to a new branch 'test' d3e1cec549 aarch64: advertise the GIC system register interface === OUTPUT BEGIN === Checking PATCH 1/1: aarch64: advertise the GIC system register interface... ERROR: spaces required around that '|' (ctx:VxV) #67: FILE: target/arm/helper.c:4690: + .resetvalue = cpu->gicv3_sysregs ? (cpu->id_aa64pfr0|0x01000000) : ^ total: 1 errors, 0 warnings, 34 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
On 6 November 2017 at 22:16, Stefano Stabellini <sstabellini@kernel.org> wrote: > When QEMU emulates a GICv3, it needs to advertise the presence of the > system register interface, which is done via id_aa64pfr0. > > To do that, and at the same time to avoid advertising the presence of > the system register interface when it is actually not available, set a > boolean property in machvirt_init. Check on the boolean property from > register_cp_regs_for_features and set id_aa64pfr0 accordingly. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 9e18b41..369d36b 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1401,6 +1401,9 @@ static void machvirt_init(MachineState *machine) > object_property_set_link(cpuobj, OBJECT(secure_sysmem), > "secure-memory", &error_abort); > } > + if (vms->gic_version == 3) { > + object_property_set_bool(cpuobj, true, "gicv3-sysregs", NULL); > + } > > object_property_set_bool(cpuobj, true, "realized", NULL); > object_unref(cpuobj); I thought about this on the cycle into work this morning, and I think that rather than require every board that uses gicv3 to set a property on the CPU, we should change the definition of the id_aa64pfr0 register so that rather than being ARM_CP_CONST it has a readfn, and then at runtime we can get that readfn to add in the right bit if env->gicv3state is non-null. I'll put together a patch this afternoon. thanks -- PMM
On Tue, 7 Nov 2017, Peter Maydell wrote: > On 6 November 2017 at 22:16, Stefano Stabellini <sstabellini@kernel.org> wrote: > > When QEMU emulates a GICv3, it needs to advertise the presence of the > > system register interface, which is done via id_aa64pfr0. > > > > To do that, and at the same time to avoid advertising the presence of > > the system register interface when it is actually not available, set a > > boolean property in machvirt_init. Check on the boolean property from > > register_cp_regs_for_features and set id_aa64pfr0 accordingly. > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 9e18b41..369d36b 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1401,6 +1401,9 @@ static void machvirt_init(MachineState *machine) > > object_property_set_link(cpuobj, OBJECT(secure_sysmem), > > "secure-memory", &error_abort); > > } > > + if (vms->gic_version == 3) { > > + object_property_set_bool(cpuobj, true, "gicv3-sysregs", NULL); > > + } > > > > object_property_set_bool(cpuobj, true, "realized", NULL); > > object_unref(cpuobj); > > I thought about this on the cycle into work this morning, and I > think that rather than require every board that uses gicv3 > to set a property on the CPU, we should change the definition > of the id_aa64pfr0 register so that rather than being ARM_CP_CONST > it has a readfn, and then at runtime we can get that readfn to > add in the right bit if env->gicv3state is non-null. > > I'll put together a patch this afternoon. Great, please CC me when you do, I'll help you test the patch.
On 7 November 2017 at 17:57, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Tue, 7 Nov 2017, Peter Maydell wrote: >> I thought about this on the cycle into work this morning, and I >> think that rather than require every board that uses gicv3 >> to set a property on the CPU, we should change the definition >> of the id_aa64pfr0 register so that rather than being ARM_CP_CONST >> it has a readfn, and then at runtime we can get that readfn to >> add in the right bit if env->gicv3state is non-null. >> >> I'll put together a patch this afternoon. > > Great, please CC me when you do, I'll help you test the patch. http://patchwork.ozlabs.org/patch/835300/ -- should already be in your inbox somewhere... thanks -- PMM
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 9e18b41..369d36b 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1401,6 +1401,9 @@ static void machvirt_init(MachineState *machine) object_property_set_link(cpuobj, OBJECT(secure_sysmem), "secure-memory", &error_abort); } + if (vms->gic_version == 3) { + object_property_set_bool(cpuobj, true, "gicv3-sysregs", NULL); + } object_property_set_bool(cpuobj, true, "realized", NULL); object_unref(cpuobj); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 88578f3..259cad1 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1690,6 +1690,7 @@ static Property arm_cpu_properties[] = { DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, ARM64_AFFINITY_INVALID), DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), + DEFINE_PROP_BOOL("gicv3-sysregs", ARMCPU, gicv3_sysregs, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 89d49cd..0015b37 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -657,6 +657,9 @@ struct ARMCPU { /* Should CPU start in PSCI powered-off state? */ bool start_powered_off; + /* GICv3 sysregs present */ + bool gicv3_sysregs; + /* Current power state, access guarded by BQL */ ARMPSCIState power_state; diff --git a/target/arm/helper.c b/target/arm/helper.c index 37af750..6f21900 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -4687,7 +4687,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0, .access = PL1_R, .type = ARM_CP_CONST, - .resetvalue = cpu->id_aa64pfr0 }, + .resetvalue = cpu->gicv3_sysregs ? (cpu->id_aa64pfr0|0x01000000) : + cpu->id_aa64pfr0 }, { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST,
When QEMU emulates a GICv3, it needs to advertise the presence of the system register interface, which is done via id_aa64pfr0. To do that, and at the same time to avoid advertising the presence of the system register interface when it is actually not available, set a boolean property in machvirt_init. Check on the boolean property from register_cp_regs_for_features and set id_aa64pfr0 accordingly. Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>