On 2025/03/17 20:53, Peter Maydell wrote:
On Mon, 17 Mar 2025 at 11:16, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
Supersedes: <20250314-clr-v2-1-7c7220c17...@daynix.com>
("[PATCH v2] target/arm: Define raw write for PMU CLR registers")
A normal write to PMCNTENCLR clears written bits so it is not
appropriate for writing a raw value. This kind of situation is usually
handled by setting a raw write function, but flag the register with
ARM_CP_NO_RAW instead to workaround a problem with KVM.
I'm not sure there's a "problem with KVM" here -- it implements the
write to PMCNTENCLR to have the semantics the architecture specifies.
The inline comment includes a reference:
> KVM applies bit operations that prevents writing back raw values
> since Linux 6.7:
>
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a45f41d754e0b37de4b7dc1fb3c6b7a1285882fc
KVM also has the same problem with PMINTENSET so add a comment to
note that. This register is already flagged with ARM_CP_NO_RAW.
What are we trying to achieve with this patchset?
You can't write to registers from gdb, and reading PMCNTENCLR
just gives you the same value as PMCNTENSET, so you might as
well read that, which we already expose.
That's also true for other registers marked with ARM_CP_ALIAS and
exposed to GDB so I followed those examples. It's up to user what
register to choose after all.
More generally, the gdb-stub access to system registers is
something we put in on a "this is a minimal amount of code
to provide something that more or less works" basis. If we
want it to work better (i.e. more comprehensively, possibly
with write support) then we should address that by looking at
the design of doing that more holistically, not by making
tweaks to address individual registers.
Specifically, I'm not really happy with adding explicit
ARM_CP_NO_GDB flags to everything we currently mark as
ARM_CP_NO_RAW. Either we stick with our existing
"minimal changes to produce something that works" approach,
in which case we deny gdb access to NO_RAW registers because
that's easy and most of them it won't work or doesn't make
sense. Or else we do the actual design work. That might turn
out to mean "we explicitly mark no-gdb registers rather than
overloading NO_RAW for this", or it might mean something else.
But we won't know until we actually do that design work.
(see also https://gitlab.com/qemu-project/qemu/-/issues/2760 )
I suspect that some registers with ARM_CP_NO_RAW cannot be determined as
GDB-visible or GDB-invisible based on the current ARMCPRegInfo fields,
and explicit marking with some flag is inevitable for them.
That said, I understand explicit markings with ARM_CP_NO_GDB may
interfere with the future design work, and I think I can still solve the
problem with PMCNTENCLR and similar registers without it. Below is a
detailed explanation:
The underlying problem here is that the current definitions of
ARM_CP_ALIAS and ARM_CP_NO_RAW are contradicting and incompatible.
Now let me describe characteristics of PMCNTENCLR with yes/no table:
1) It has a state - yes
2) It will handle migration and reset - no
3) The state can be synchronized with KVM - no
4) The state is shared with another register - yes
1) the presence of the state usually implies it is visible to GDB.
The documentation of ARM_CP_ALIAS says:
> Register is an alias view of some underlying state which is also
> visible via another register, and that the other register is handling
> migration and reset; registers marked ARM_CP_ALIAS will not be
> migrated but may have their state set by syncing of register state
> from KVM.
So ARM_CP_ALIAS implies:
1) It has a state - yes
2) It will handle migration and reset - no
3) The state can be synchronized with KVM - yes
4) The state is shared with another register - yes
1) is implied from 3) and 4); if there is no state, the state cannot be
synchronized and cannot be shared with another register.
The documentation of ARM_CP_NO_RAW says:
> 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.
So ARM_CP_NO_RAW implies:
1) It has a state - no
2) It will handle migration and reset - no
3) The state can be synchronized with KVM - no
4) The state is shared with another register - n/a (there is no state)
It also makes the register invisible from GDB since there is no
underlying state.
As you can see, ARM_CP_ALIAS and ARM_CP_NO_RAW have contradicting
semantics. A minimal change to make them compatible is to explicitly
define what (ARM_CP_ALIAS | ARM_CP_NO_RAW) implies:
1) It has a state - yes (it has a state shared with its alias)
2) It will handle migration and reset - no
3) The state can be synchronized with KVM - no (due to no raw access)
4) The state is shared with another register - yes
More concretely, the documentation of ARM_CP_NO_RAW can be changed as
follows:
"Register does not support raw access for state saving/loading; it will
not be used for either migration of KVM state synchronization. It also
implies reading the register does not expose a state and hides it from
GDB unless ARM_CP_ALIAS is set."
With this change, we can express the semantics of PMCNTENCLR without
extra explicit markings of ARM_CP_NO_GDB and any contradiction in code.
This explanation is long but the code change should be minimal.
What do you think?
Regards,
Akihiko Odaki
thanks
-- PMM
