1. Patchew
  2. QEMU
  3. View series

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

Akihiko Odaki posted 1 patch 9 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250314-clr-v2-1-7c7220c177c9@daynix.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
target/arm/helper.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[PATCH v2] target/arm: Define raw write for PMU CLR registers
Posted by Akihiko Odaki 9 months, 1 week ago 
Raw writes to PMCNTENCLR and PMCNTENCLR_EL0 incorrectly used their
default write function, which clears written bits instead of writes the
raw value.

PMINTENCLR and PMINTENCLR_EL1 are similar registers, but they instead
had ARM_CP_NO_RAW. target/arm/cpregs.h suggests this flag usage is
inappropriate:
> 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.

PMINTENCLR and PMINTENCLR_EL1 have underlying states and can support
raw access for state saving/loading. Flagging a register with
ARM_CP_NO_RAW has a side effect that hides it from GDB.

Properly set raw write functions and drop the ARM_CP_NO_RAW flag from
PMINTENCLR and PMINTENCLR_EL1.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v2:
- Added raw write functions to PMCNTENCLR and PMINTENCLR.
- Dropped the ARM_CP_NO_RAW flag from PMINTENCLR and PMINTENCLR_EL1.
- Link to v1: https://lore.kernel.org/qemu-devel/20250313-clr-v1-1-2cc49df40fe9@daynix.com
---
 target/arm/helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f0ead22937bf..d05865983416 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1899,7 +1899,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmcnten),
       .accessfn = pmreg_access,
       .fgt = FGT_PMCNTEN,
-      .writefn = pmcntenclr_write,
+      .writefn = pmcntenclr_write, .raw_writefn = raw_write,
       .type = ARM_CP_ALIAS | ARM_CP_IO },
     { .name = "PMCNTENCLR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 2,
@@ -1907,7 +1907,7 @@ 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),
@@ -2024,16 +2024,16 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
       .access = PL1_RW, .accessfn = access_tpm,
       .fgt = FGT_PMINTEN,
-      .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-      .writefn = pmintenclr_write, },
+      .writefn = pmintenclr_write, .raw_writefn = raw_write },
     { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
       .access = PL1_RW, .accessfn = access_tpm,
       .fgt = FGT_PMINTEN,
-      .type = ARM_CP_ALIAS | ARM_CP_IO | ARM_CP_NO_RAW,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .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 v2] target/arm: Define raw write for PMU CLR registers
Posted by Peter Maydell 9 months, 1 week ago 
On Fri, 14 Mar 2025 at 08:13, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Raw writes to PMCNTENCLR and PMCNTENCLR_EL0 incorrectly used their
> default write function, which clears written bits instead of writes the
> raw value.
>
> PMINTENCLR and PMINTENCLR_EL1 are similar registers, but they instead
> had ARM_CP_NO_RAW. target/arm/cpregs.h suggests this flag usage is
> inappropriate:
> > 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.
>
> PMINTENCLR and PMINTENCLR_EL1 have underlying states and can support
> raw access for state saving/loading. Flagging a register with
> ARM_CP_NO_RAW has a side effect that hides it from GDB.

No, the CLR registers don't have their own underlying state:
all the state is handled by the SET registers, and it just
happens that you can read it via the CLR registers.

> Properly set raw write functions and drop the ARM_CP_NO_RAW flag from
> PMINTENCLR and PMINTENCLR_EL1.

I think the correct fix is to mark all the CLR registers as NO_RAW.

thanks
-- PMM
Re: [PATCH v2] target/arm: Define raw write for PMU CLR registers
Posted by Akihiko Odaki 9 months, 1 week ago 

On 2025/03/14 19:22, Peter Maydell wrote:
> On Fri, 14 Mar 2025 at 08:13, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> Raw writes to PMCNTENCLR and PMCNTENCLR_EL0 incorrectly used their
>> default write function, which clears written bits instead of writes the
>> raw value.
>>
>> PMINTENCLR and PMINTENCLR_EL1 are similar registers, but they instead
>> had ARM_CP_NO_RAW. target/arm/cpregs.h suggests this flag usage is
>> inappropriate:
>>> 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.
>>
>> PMINTENCLR and PMINTENCLR_EL1 have underlying states and can support
>> raw access for state saving/loading. Flagging a register with
>> ARM_CP_NO_RAW has a side effect that hides it from GDB.
> 
> No, the CLR registers don't have their own underlying state:
> all the state is handled by the SET registers, and it just
> happens that you can read it via the CLR registers.
> 
>> Properly set raw write functions and drop the ARM_CP_NO_RAW flag from
>> PMINTENCLR and PMINTENCLR_EL1.
> 
> I think the correct fix is to mark all the CLR registers as NO_RAW.

My understanding is that ARM_CP_ALIAS is ignored for KVM to avoid making 
an assumption what aliases KVM implement at cost of migrating one state 
multiple times. The CLR registers should also remain as possible 
sources/targets of raw values.

Regards,
Akihiko Odaki

> 
> thanks
> -- PMM
Re: [PATCH v2] target/arm: Define raw write for PMU CLR registers
Posted by Peter Maydell 9 months, 1 week ago 
On Fri, 14 Mar 2025 at 10:24, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
>
>
> On 2025/03/14 19:22, Peter Maydell wrote:
> > On Fri, 14 Mar 2025 at 08:13, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> Raw writes to PMCNTENCLR and PMCNTENCLR_EL0 incorrectly used their
> >> default write function, which clears written bits instead of writes the
> >> raw value.
> >>
> >> PMINTENCLR and PMINTENCLR_EL1 are similar registers, but they instead
> >> had ARM_CP_NO_RAW. target/arm/cpregs.h suggests this flag usage is
> >> inappropriate:
> >>> 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.
> >>
> >> PMINTENCLR and PMINTENCLR_EL1 have underlying states and can support
> >> raw access for state saving/loading. Flagging a register with
> >> ARM_CP_NO_RAW has a side effect that hides it from GDB.
> >
> > No, the CLR registers don't have their own underlying state:
> > all the state is handled by the SET registers, and it just
> > happens that you can read it via the CLR registers.
> >
> >> Properly set raw write functions and drop the ARM_CP_NO_RAW flag from
> >> PMINTENCLR and PMINTENCLR_EL1.
> >
> > I think the correct fix is to mark all the CLR registers as NO_RAW.
>
> My understanding is that ARM_CP_ALIAS is ignored for KVM to avoid making
> an assumption what aliases KVM implement at cost of migrating one state
> multiple times. The CLR registers should also remain as possible
> sources/targets of raw values.

Why? There's nothing you can do by doing a raw write to the CLR
register that you shouldn't have done by doing a raw write to
the SET register instead.

thanks
-- PMM
Re: [PATCH v2] target/arm: Define raw write for PMU CLR registers
Posted by Akihiko Odaki 9 months, 1 week ago 
On 2025/03/14 19:29, Peter Maydell wrote:
> On Fri, 14 Mar 2025 at 10:24, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>>
>>
>> On 2025/03/14 19:22, Peter Maydell wrote:
>>> On Fri, 14 Mar 2025 at 08:13, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> Raw writes to PMCNTENCLR and PMCNTENCLR_EL0 incorrectly used their
>>>> default write function, which clears written bits instead of writes the
>>>> raw value.
>>>>
>>>> PMINTENCLR and PMINTENCLR_EL1 are similar registers, but they instead
>>>> had ARM_CP_NO_RAW. target/arm/cpregs.h suggests this flag usage is
>>>> inappropriate:
>>>>> 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.
>>>>
>>>> PMINTENCLR and PMINTENCLR_EL1 have underlying states and can support
>>>> raw access for state saving/loading. Flagging a register with
>>>> ARM_CP_NO_RAW has a side effect that hides it from GDB.
>>>
>>> No, the CLR registers don't have their own underlying state:
>>> all the state is handled by the SET registers, and it just
>>> happens that you can read it via the CLR registers.
>>>
>>>> Properly set raw write functions and drop the ARM_CP_NO_RAW flag from
>>>> PMINTENCLR and PMINTENCLR_EL1.
>>>
>>> I think the correct fix is to mark all the CLR registers as NO_RAW.
>>
>> My understanding is that ARM_CP_ALIAS is ignored for KVM to avoid making
>> an assumption what aliases KVM implement at cost of migrating one state
>> multiple times. The CLR registers should also remain as possible
>> sources/targets of raw values.
> 
> Why? There's nothing you can do by doing a raw write to the CLR
> register that you shouldn't have done by doing a raw write to
> the SET register instead.

In theory, KVM may choose to expose only the CLR register and hide the 
SET register. There is no guarantee that both the CLR and SET are exposed.

Regards,
Akihiko Odaki

> 
> thanks
> -- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.