Re: [PATCH] target/arm: Define raw write for PMU CLR registers
On Fri, 14 Mar 2025 at 08:15, Akihiko Odaki wrote:
>
> On 2025/03/14 3:34, Peter Maydell wrote:
> > On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki
> > 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
> >> ---
> >> 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
Re: [PATCH] target/arm: Define raw write for PMU CLR registers
On 2025/03/14 3:34, Peter Maydell wrote:
On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki 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
---
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
On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki 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
> ---
> 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
On 2025/03/13 19:13, Peter Maydell wrote: On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki 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 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
On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki 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 Nice catch. Did you find this because it caused a visible problem/bug, or was it just code inspection? thanks -- PMM
