| Message ID | 20250317-raw-v1-0-09e2dfff0e90@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/arm: Flag PMCNTENCLR with ARM_CP_NO_RAW | expand |
On Mon, 17 Mar 2025 at 11:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > Supersedes: <20250314-clr-v2-1-7c7220c177c9@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. > 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. 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 ) thanks -- PMM
On 2025/03/17 20:53, Peter Maydell wrote: > On Mon, 17 Mar 2025 at 11:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> Supersedes: <20250314-clr-v2-1-7c7220c177c9@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
On Mon, 17 Mar 2025 at 11:53, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 17 Mar 2025 at 11:16, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > Supersedes: <20250314-clr-v2-1-7c7220c177c9@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. I checked with the KVM developers on this one, and you are correct that this is a KVM issue, so I was wrong here; sorry about that. There is a fix queued: https://web.git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=next&id=f2aeb7bbd5745fbcf7f0769e29a184e24924b9a9 which will restore the "just writes the value" semantics. This is apparently the intended semantics for KVM_SET_ONE_REG for all cases: the write is supposed to do "sets register to the specified value", not "behaves as if the guest vCPU had done the register write insn, with attendant side effects". thanks -- PMM
Supersedes: <20250314-clr-v2-1-7c7220c177c9@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. 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. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- Akihiko Odaki (4): target/arm: Explicitly set ARM_CP_NO_GDB target/arm: Do not imply ARM_CP_NO_GDB target/arm: Expose PMINTENCLR to GDB target/arm: Flag PMCNTENCLR with ARM_CP_NO_RAW target/arm/cpregs.h | 8 +- hw/intc/arm_gicv3_cpuif.c | 100 +++++++++--------- hw/intc/arm_gicv3_kvm.c | 2 +- target/arm/debug_helper.c | 2 +- target/arm/gdbstub.c | 2 +- target/arm/helper.c | 229 +++++++++++++++++++++++----------------- target/arm/tcg/tlb-insns.c | 256 +++++++++++++++++++++++++-------------------- 7 files changed, 336 insertions(+), 263 deletions(-) --- base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 change-id: 20250314-raw-198885f67b73 Best regards,