| Message ID | 20181010203735.27918-13-aclindsa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
On 10/10/18 1:37 PM, Aaron Lindsay wrote: > This both advertises that we support four counters and enables them > because the pmu_num_counters() reads this value from PMCR. > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > target/arm/helper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) It looks like this should have been folded into patch 10, or patch 10 split to include the code changes that go with these comment changes. r~
On Oct 16 17:09, Richard Henderson wrote: > On 10/10/18 1:37 PM, Aaron Lindsay wrote: > > This both advertises that we support four counters and enables them > > because the pmu_num_counters() reads this value from PMCR. > > > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > > --- > > target/arm/helper.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > It looks like this should have been folded into patch 10, > or patch 10 split to include the code changes that go with > these comment changes. By setting PMCR.N to 4, this commit is what causes all the previous implementation of non-PMCCNTR counters to become usable by the guest - both because the OS can now detect their presence in PMCR, and because the system registers used to setup/access the counters are gated on pmu_num_counters(), which uses PMCR.N to determine if a counter is implemented. That said, I'm not against merging the two patches if you feel that is cleaner. -Aaron
On 10/17/18 12:20 PM, Aaron Lindsay wrote: > On Oct 16 17:09, Richard Henderson wrote: >> On 10/10/18 1:37 PM, Aaron Lindsay wrote: >>> This both advertises that we support four counters and enables them >>> because the pmu_num_counters() reads this value from PMCR. >>> >>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> >>> --- >>> target/arm/helper.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> It looks like this should have been folded into patch 10, >> or patch 10 split to include the code changes that go with >> these comment changes. > > By setting PMCR.N to 4, this commit is what causes all the previous > implementation of non-PMCCNTR counters to become usable by the guest - > both because the OS can now detect their presence in PMCR, and because > the system registers used to setup/access the counters are gated on > pmu_num_counters(), which uses PMCR.N to determine if a counter is > implemented. But e.g. > @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } > if (arm_feature(env, ARM_FEATURE_V7)) { > /* v7 performance monitor control register: same implementor > - * field as main ID register, and we implement only the cycle > - * count register. > + * field as main ID register, and we implement four counters in > + * addition to the cycle count register. > */ > unsigned int i, pmcrn = 4; > ARMCPRegInfo pmcr = { clearly belongs with the previous patch, since pmcrn is already 4. > @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > .access = PL1_W, .type = ARM_CP_NOP }, > /* Performance monitors are implementation defined in v7, > * but with an ARM recommended set of registers, which we > - * follow (although we don't actually implement any counters) > + * follow. > * > * Performance registers fall into three categories: > * (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR) And this is out of date as well, and probably belonged elsewhere. Which leaves only > @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) > .access = PL0_RW, .accessfn = pmreg_access, > .type = ARM_CP_IO, > .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), > - .resetvalue = cpu->midr & 0xff000000, > + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), > .writefn = pmcr_write, .raw_writefn = raw_write, > }; which suggests that this one line belonged to the patch that set pmcrn in the first place. r~
On Oct 17 12:34, Richard Henderson wrote: > On 10/17/18 12:20 PM, Aaron Lindsay wrote: > > On Oct 16 17:09, Richard Henderson wrote: > >> On 10/10/18 1:37 PM, Aaron Lindsay wrote: > >>> This both advertises that we support four counters and enables them > >>> because the pmu_num_counters() reads this value from PMCR. > >>> > >>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > >>> --- > >>> target/arm/helper.c | 8 ++++---- > >>> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> It looks like this should have been folded into patch 10, > >> or patch 10 split to include the code changes that go with > >> these comment changes. > > > > By setting PMCR.N to 4, this commit is what causes all the previous > > implementation of non-PMCCNTR counters to become usable by the guest - > > both because the OS can now detect their presence in PMCR, and because > > the system registers used to setup/access the counters are gated on > > pmu_num_counters(), which uses PMCR.N to determine if a counter is > > implemented. > > But e.g. > > > @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) > > } > > if (arm_feature(env, ARM_FEATURE_V7)) { > > /* v7 performance monitor control register: same implementor > > - * field as main ID register, and we implement only the cycle > > - * count register. > > + * field as main ID register, and we implement four counters in > > + * addition to the cycle count register. > > */ > > unsigned int i, pmcrn = 4; > > ARMCPRegInfo pmcr = { > > clearly belongs with the previous patch, since pmcrn is already 4. I suppose pmcrn (the local variable) should've been set to 0 before this patch and updated here to be 4. However, even though it was already set to 4 my amendment to the comment was still not true until this patch, because it wasn't written into PMCR.N (the CP register). > > @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > > .access = PL1_W, .type = ARM_CP_NOP }, > > /* Performance monitors are implementation defined in v7, > > * but with an ARM recommended set of registers, which we > > - * follow (although we don't actually implement any counters) > > + * follow. > > * > > * Performance registers fall into three categories: > > * (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR) > > And this is out of date as well, and probably belonged elsewhere. > Which leaves only Why is this out of date? Because PMCR.N still contained zero, no counters were implemented from the guest's standpoint until this patch. > > @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) > > .access = PL0_RW, .accessfn = pmreg_access, > > .type = ARM_CP_IO, > > .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), > > - .resetvalue = cpu->midr & 0xff000000, > > + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), > > .writefn = pmcr_write, .raw_writefn = raw_write, > > }; > > which suggests that this one line belonged to the patch that set pmcrn in the > first place. I get the feeling we may be talking past each other/misunderstanding somehow, but I'm not sure why/how. Maybe I'm thinking about whether or not counters are implemented from the guest's standpoint and you're thinking about it from the standpoint of whether the code to implement them exists in QEMU? Anyway, by splitting these patches apart, my intention was to separate the plumbing work from turning on the water. If you believe it is better to do both in the same patch, I'll make that change for v7. -Aaron
On 10/17/18 1:25 PM, Aaron Lindsay wrote: > I suppose pmcrn (the local variable) should've been set to 0 before this > patch and updated here to be 4. That's plausible. > Anyway, by splitting these patches apart, my intention was to separate > the plumbing work from turning on the water. If you believe it is better > to do both in the same patch, I'll make that change for v7. Fair. I'll let Peter weigh in for final decision. r~
On 17 October 2018 at 22:14, Richard Henderson <richard.henderson@linaro.org> wrote: > On 10/17/18 1:25 PM, Aaron Lindsay wrote: >> I suppose pmcrn (the local variable) should've been set to 0 before this >> patch and updated here to be 4. > > That's plausible. > >> Anyway, by splitting these patches apart, my intention was to separate >> the plumbing work from turning on the water. If you believe it is better >> to do both in the same patch, I'll make that change for v7. > > Fair. I'll let Peter weigh in for final decision. No strong opinion -- rth has reviewed this set, not me. thanks -- PMM
On Oct 17 14:14, Richard Henderson wrote: > On 10/17/18 1:25 PM, Aaron Lindsay wrote: > > I suppose pmcrn (the local variable) should've been set to 0 before this > > patch and updated here to be 4. > > That's plausible. > > > Anyway, by splitting these patches apart, my intention was to separate > > the plumbing work from turning on the water. If you believe it is better > > to do both in the same patch, I'll make that change for v7. > > Fair. I'll let Peter weigh in for final decision. At a minimum, I've updated my local tree so that pmcrn is initially set to 0 in 'target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER' and updated to 4 in this patch. And I'll squash them instead if that ends up being the decision. -Aaron
diff --git a/target/arm/helper.c b/target/arm/helper.c index d6501de1ba..89ceb34cb9 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL1_W, .type = ARM_CP_NOP }, /* Performance monitors are implementation defined in v7, * but with an ARM recommended set of registers, which we - * follow (although we don't actually implement any counters) + * follow. * * Performance registers fall into three categories: * (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR) @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) } if (arm_feature(env, ARM_FEATURE_V7)) { /* v7 performance monitor control register: same implementor - * field as main ID register, and we implement only the cycle - * count register. + * field as main ID register, and we implement four counters in + * addition to the cycle count register. */ unsigned int i, pmcrn = 4; ARMCPRegInfo pmcr = { @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) .access = PL0_RW, .accessfn = pmreg_access, .type = ARM_CP_IO, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), - .resetvalue = cpu->midr & 0xff000000, + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), .writefn = pmcr_write, .raw_writefn = raw_write, }; define_one_arm_cp_reg(cpu, &pmcr);
This both advertises that we support four counters and enables them because the pmu_num_counters() reads this value from PMCR. Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> --- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)