[PATCH] target/arm: Define raw write for PMU CLR registers

[PATCH] target/arm: Define raw write for PMU CLR registers

[Akihiko Odaki]

PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
alternative raw write functions, which will be used to copy KVM kernel
coprocessor state into userspace.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f0ead22937bf..30883cd3a989 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1907,7 +1907,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fgt = FGT_PMCNTEN,
       .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
-      .writefn = pmcntenclr_write },
+      .writefn = pmcntenclr_write,
+      .raw_writefn = raw_write },
     { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
       .access = PL0_RW, .type = ARM_CP_IO,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
@@ -2033,7 +2034,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fgt = FGT_PMINTEN,
       .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-      .writefn = pmintenclr_write },
+      .writefn = pmintenclr_write,
+      .raw_writefn = raw_write },
     { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_R,

---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250313-clr-6a34831628b7

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH] target/arm: Define raw write for PMU CLR registers

[Peter Maydell]

On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
> alternative raw write functions, which will be used to copy KVM kernel
> coprocessor state into userspace.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Nice catch. Did you find this because it caused a visible
problem/bug, or was it just code inspection?

thanks
-- PMM

Re: [PATCH] target/arm: Define raw write for PMU CLR registers

[Akihiko Odaki]

On 2025/03/13 19:13, Peter Maydell wrote:
> On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
>> alternative raw write functions, which will be used to copy KVM kernel
>> coprocessor state into userspace.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Nice catch. Did you find this because it caused a visible
> problem/bug, or was it just code inspection?

I found GDB always says PMCNTENSET_EL0 and PMCNTENCLR_EL0 are zero 
regardless of the actual value. This is because the value gets cleared 
when PMCNTENCLR_EL0 is written back from KVM.

Regards,
Akihiko Odaki

> 
> thanks
> -- PMM

Re: [PATCH] target/arm: Define raw write for PMU CLR registers

[Peter Maydell]

On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
> alternative raw write functions, which will be used to copy KVM kernel
> coprocessor state into userspace.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/arm/helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f0ead22937bf..30883cd3a989 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1907,7 +1907,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fgt = FGT_PMCNTEN,
>        .type = ARM_CP_ALIAS | ARM_CP_IO,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> -      .writefn = pmcntenclr_write },
> +      .writefn = pmcntenclr_write,
> +      .raw_writefn = raw_write },
>      { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
>        .access = PL0_RW, .type = ARM_CP_IO,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> @@ -2033,7 +2034,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fgt = FGT_PMINTEN,
>        .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> -      .writefn = pmintenclr_write },
> +      .writefn = pmintenclr_write,
> +      .raw_writefn = raw_write },
>      { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
>        .access = PL1_R,

Hmm, looking more closely at this, I think this second one should
not need a raw_writefn, because it's marked as ARM_CP_NO_RAW
(meaning nothing should try to do a raw write to it).

And the first one is marked ARM_CP_ALIAS, so I'm not
sure why we would be using it in KVM register sync:
add_cpreg_to_list() skips ARM_CP_ALIAS (and ARM_CP_NO_RAW)
registers when we construct the cpreg_tuples[] array that
defines which sysregs we sync to and from KVM.

(We should arguably be consistent about our usage of the
NO_RAW flag between the pmintenclr and pmcntenclr registers.)

thanks
-- PMM

Re: [PATCH] target/arm: Define raw write for PMU CLR registers

[Akihiko Odaki]

On 2025/03/14 3:34, Peter Maydell wrote:
> On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
>> alternative raw write functions, which will be used to copy KVM kernel
>> coprocessor state into userspace.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   target/arm/helper.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index f0ead22937bf..30883cd3a989 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -1907,7 +1907,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>         .fgt = FGT_PMCNTEN,
>>         .type = ARM_CP_ALIAS | ARM_CP_IO,
>>         .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>> -      .writefn = pmcntenclr_write },
>> +      .writefn = pmcntenclr_write,
>> +      .raw_writefn = raw_write },
>>       { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
>>         .access = PL0_RW, .type = ARM_CP_IO,
>>         .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>> @@ -2033,7 +2034,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>         .fgt = FGT_PMINTEN,
>>         .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
>>         .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>> -      .writefn = pmintenclr_write },
>> +      .writefn = pmintenclr_write,
>> +      .raw_writefn = raw_write },
>>       { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
>>         .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
>>         .access = PL1_R,
> 
> Hmm, looking more closely at this, I think this second one should
> not need a raw_writefn, because it's marked as ARM_CP_NO_RAW
> (meaning nothing should try to do a raw write to it).

Good catch; I didn't notice ARM_CP_NO_RAW.

> 
> And the first one is marked ARM_CP_ALIAS, so I'm not
> sure why we would be using it in KVM register sync:
> add_cpreg_to_list() skips ARM_CP_ALIAS (and ARM_CP_NO_RAW)
> registers when we construct the cpreg_tuples[] array that
> defines which sysregs we sync to and from KVM.

The register list is initialized with kvm_arm_init_cpreg_list() for KVM, 
which ignores those flags.

target/arm/cpregs.h explicitly says: "registers marked ARM_CP_ALIAS will 
not be migrated but may have their state set by syncing of register 
state from KVM."

ARM_CP_NO_RAW is still respected for KVM by write_cpustate_to_list() and 
write_list_to_cpustate().

> 
> (We should arguably be consistent about our usage of the
> NO_RAW flag between the pmintenclr and pmcntenclr registers.)

I sent v2 to drop the flag. target/arm/cpregs.h suggests ARM_CP_NO_RAW 
is not a flag for these registers:
 > Flag: Register has no underlying state and does not support raw access
 > for state saving/loading; it will not be used for either migration or
 > KVM state synchronization. Typically this is for "registers" which are
 > actually used as instructions for cache maintenance and so on.

These registers have underlying states and can support raw access.

Regards,
Akihiko Odaki

> 
> thanks
> -- PMM

Re: [PATCH] target/arm: Define raw write for PMU CLR registers

[Peter Maydell]

On Fri, 14 Mar 2025 at 08:15, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/14 3:34, Peter Maydell wrote:
> > On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> PMCNTENCLR_EL0 and PMINTENCLR_EL1 clears written bits so we need an
> >> alternative raw write functions, which will be used to copy KVM kernel
> >> coprocessor state into userspace.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   target/arm/helper.c | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/arm/helper.c b/target/arm/helper.c
> >> index f0ead22937bf..30883cd3a989 100644
> >> --- a/target/arm/helper.c
> >> +++ b/target/arm/helper.c
> >> @@ -1907,7 +1907,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >>         .fgt = FGT_PMCNTEN,
> >>         .type = ARM_CP_ALIAS | ARM_CP_IO,
> >>         .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> >> -      .writefn = pmcntenclr_write },
> >> +      .writefn = pmcntenclr_write,
> >> +      .raw_writefn = raw_write },
> >>       { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
> >>         .access = PL0_RW, .type = ARM_CP_IO,
> >>         .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> >> @@ -2033,7 +2034,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >>         .fgt = FGT_PMINTEN,
> >>         .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
> >>         .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> >> -      .writefn = pmintenclr_write },
> >> +      .writefn = pmintenclr_write,
> >> +      .raw_writefn = raw_write },
> >>       { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
> >>         .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
> >>         .access = PL1_R,
> >
> > Hmm, looking more closely at this, I think this second one should
> > not need a raw_writefn, because it's marked as ARM_CP_NO_RAW
> > (meaning nothing should try to do a raw write to it).
>
> Good catch; I didn't notice ARM_CP_NO_RAW.
>
> >
> > And the first one is marked ARM_CP_ALIAS, so I'm not
> > sure why we would be using it in KVM register sync:
> > add_cpreg_to_list() skips ARM_CP_ALIAS (and ARM_CP_NO_RAW)
> > registers when we construct the cpreg_tuples[] array that
> > defines which sysregs we sync to and from KVM.
>
> The register list is initialized with kvm_arm_init_cpreg_list() for KVM,
> which ignores those flags.
>
> target/arm/cpregs.h explicitly says: "registers marked ARM_CP_ALIAS will
> not be migrated but may have their state set by syncing of register
> state from KVM."
>
> ARM_CP_NO_RAW is still respected for KVM by write_cpustate_to_list() and
> write_list_to_cpustate().
>
> >
> > (We should arguably be consistent about our usage of the
> > NO_RAW flag between the pmintenclr and pmcntenclr registers.)
>
> I sent v2 to drop the flag. target/arm/cpregs.h suggests ARM_CP_NO_RAW
> is not a flag for these registers:
>  > Flag: Register has no underlying state and does not support raw access
>  > for state saving/loading; it will not be used for either migration or
>  > KVM state synchronization. Typically this is for "registers" which are
>  > actually used as instructions for cache maintenance and so on.
>
> These registers have underlying states and can support raw access.

No, the CLR registers don't have their own underlying state.
The underlying state is handled by the SET registers. NO_RAW
for the CLR registers is correct, because:
 * we don't want to migrate the state twice; the SET register
   of a CLR/SET pair will handle it
 * we don't want to try to write the state to KVM via the CLR
   register

-- PMM