On Fri, 14 Mar 2025 at 08:15, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2025/03/14 3:34, Peter Maydell wrote:
> > On Thu, 13 Mar 2025 at 07:16, Akihiko Odaki <akihiko.od...@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.od...@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
