Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Peter Maydell]

On 19 March 2018 at 15:24, Aaron Lindsay  wrote:
> Phil,
>
> On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
>> Hi Aaron,
>>
>> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
>> > This is a bug fix to ensure 64-bit reads of this register don't read
>> > adjacent data.
>> >
>> > Signed-off-by: Aaron Lindsay 
>> > ---
>> >  target/arm/cpu.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> > index 9c3b5ef..fb2f983 100644
>> > --- a/target/arm/cpu.h
>> > +++ b/target/arm/cpu.h
>> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>> >  uint32_t c9_data;
>> >  uint64_t c9_pmcr; /* performance monitor control register */
>> >  uint64_t c9_pmcnten; /* perf monitor counter enables */
>> > -uint32_t c9_pmovsr; /* perf monitor overflow status */
>> > +uint64_t c9_pmovsr; /* perf monitor overflow status */
>>
>> This doesn't look correct, since this reg is 32b.
>>
>> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
>>
>> { .name = "PMOVSR", ...
>> - ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
>> + ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>>   .accessfn = pmreg_access,
>>   .writefn = pmovsr_write,
>>   .raw_writefn = raw_write },
>
> Nearly all of these PMU registers are 32 bits wide, but most of them are
> implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
> few examples I see in this patch's context). My understanding is that
> AArch64 register accesses are handled as 64 bits, even if the register
> itself isn't that wide (though I haven't personally verified this).

Correct. Technically there's no such thing as a 32-bit wide AArch64
system register -- that is just a shorthand in the Arm ARM for
"64-bit wide with the top 32-bits being RES0".

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

Phil,

On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > This is a bug fix to ensure 64-bit reads of this register don't read
> > adjacent data.
> > 
> > Signed-off-by: Aaron Lindsay 
> > ---
> >  target/arm/cpu.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9c3b5ef..fb2f983 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
> >  uint32_t c9_data;
> >  uint64_t c9_pmcr; /* performance monitor control register */
> >  uint64_t c9_pmcnten; /* perf monitor counter enables */
> > -uint32_t c9_pmovsr; /* perf monitor overflow status */
> > +uint64_t c9_pmovsr; /* perf monitor overflow status */
> 
> This doesn't look correct, since this reg is 32b.
> 
> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
> 
> { .name = "PMOVSR", ...
> - ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> + ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>   .accessfn = pmreg_access,
>   .writefn = pmovsr_write,
>   .raw_writefn = raw_write },

Nearly all of these PMU registers are 32 bits wide, but most of them are
implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
few examples I see in this patch's context). My understanding is that
AArch64 register accesses are handled as 64 bits, even if the register
itself isn't that wide (though I haven't personally verified this). See
an earlier email from Peter from v2 of this patchset:

https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03983.html

Does this still look wrong to you? If so, I'll take a more thorough look
into how these accesses work.

> >  uint32_t c9_pmuserenr; /* perf monitor user enable */

Whatever we decide should likely be done to PMUSERENR too - I think I
overlooked this one before.

> >  uint64_t c9_pmselr; /* perf monitor counter selection register */
> >  uint64_t c9_pminten; /* perf monitor interrupt enables */
> > 
> 
> Regards,
> 
> Phil.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Philippe Mathieu-Daudé]

Hi Aaron,

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> This is a bug fix to ensure 64-bit reads of this register don't read
> adjacent data.
> 
> Signed-off-by: Aaron Lindsay 
> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9c3b5ef..fb2f983 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>  uint32_t c9_data;
>  uint64_t c9_pmcr; /* performance monitor control register */
>  uint64_t c9_pmcnten; /* perf monitor counter enables */
> -uint32_t c9_pmovsr; /* perf monitor overflow status */
> +uint64_t c9_pmovsr; /* perf monitor overflow status */

This doesn't look correct, since this reg is 32b.

I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:

{ .name = "PMOVSR", ...
- ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+ ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
  .accessfn = pmreg_access,
  .writefn = pmovsr_write,
  .raw_writefn = raw_write },

>  uint32_t c9_pmuserenr; /* perf monitor user enable */
>  uint64_t c9_pmselr; /* perf monitor counter selection register */
>  uint64_t c9_pminten; /* perf monitor interrupt enables */
> 

Regards,

Phil.

[Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

This is a bug fix to ensure 64-bit reads of this register don't read
adjacent data.

Signed-off-by: Aaron Lindsay 
---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9c3b5ef..fb2f983 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -367,7 +367,7 @@ typedef struct CPUARMState {
 uint32_t c9_data;
 uint64_t c9_pmcr; /* performance monitor control register */
 uint64_t c9_pmcnten; /* perf monitor counter enables */
-uint32_t c9_pmovsr; /* perf monitor overflow status */
+uint64_t c9_pmovsr; /* perf monitor overflow status */
 uint32_t c9_pmuserenr; /* perf monitor user enable */
 uint64_t c9_pmselr; /* perf monitor counter selection register */
 uint64_t c9_pminten; /* perf monitor interrupt enables */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.