1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v7 00/12] More fully implement ARM PMUv3
  4. View patch

[Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations

Aaron Lindsay posted 12 patches 7 years ago
There is a newer version of this series
[Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations
Posted by Aaron Lindsay 7 years ago 
Because of the PMU's design, many register accesses have side effects
which are inter-related, meaning that the normal method of saving CP
registers can result in inconsistent state. These side-effects are
largely handled in pmu_op_start/finish functions which can be called
before and after the state is saved/restored. By doing this and adding
raw read/write functions for the affected registers, we avoid
migration-related inconsistencies.

Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
---
 target/arm/helper.c  |  6 ++++--
 target/arm/machine.c | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 281bcff1da..5deff3d11f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1450,11 +1450,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 0,
       .access = PL0_RW, .accessfn = pmreg_access_ccntr,
       .type = ARM_CP_IO,
-      .readfn = pmccntr_read, .writefn = pmccntr_write, },
+      .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
+      .readfn = pmccntr_read, .writefn = pmccntr_write,
+      .raw_readfn = raw_read, .raw_writefn = raw_write, },
 #endif
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
-      .writefn = pmccfiltr_write,
+      .writefn = pmccfiltr_write, .raw_writefn = raw_write,
       .access = PL0_RW, .accessfn = pmreg_access,
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 239fe4e84d..6d14b08e0c 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -604,6 +604,8 @@ static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
+    pmu_op_start(&cpu->env);
+
     if (kvm_enabled()) {
         if (!write_kvmstate_to_list(cpu)) {
             /* This should never fail */
@@ -625,6 +627,20 @@ static int cpu_pre_save(void *opaque)
     return 0;
 }
 
+static int cpu_post_save(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    pmu_op_finish(&cpu->env);
+    return 0;
+}
+
+static int cpu_pre_load(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    pmu_op_start(&cpu->env);
+    return 0;
+}
+
 static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
@@ -672,6 +688,8 @@ static int cpu_post_load(void *opaque, int version_id)
     hw_breakpoint_update_all(cpu);
     hw_watchpoint_update_all(cpu);
 
+    pmu_op_finish(&cpu->env);
+
     return 0;
 }
 
@@ -680,6 +698,8 @@ const VMStateDescription vmstate_arm_cpu = {
     .version_id = 22,
     .minimum_version_id = 22,
     .pre_save = cpu_pre_save,
+    .post_save = cpu_post_save,
+    .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
-- 
2.19.1
Re: [Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations
Posted by Peter Maydell 6 years, 11 months ago 
On 5 November 2018 at 18:51, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> Because of the PMU's design, many register accesses have side effects
> which are inter-related, meaning that the normal method of saving CP
> registers can result in inconsistent state. These side-effects are
> largely handled in pmu_op_start/finish functions which can be called
> before and after the state is saved/restored. By doing this and adding
> raw read/write functions for the affected registers, we avoid
> migration-related inconsistencies.
>
> Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>

> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -604,6 +604,8 @@ static int cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
>
> +    pmu_op_start(&cpu->env);
> +
>      if (kvm_enabled()) {
>          if (!write_kvmstate_to_list(cpu)) {
>              /* This should never fail */
> @@ -625,6 +627,20 @@ static int cpu_pre_save(void *opaque)
>      return 0;
>  }
>
> +static int cpu_post_save(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    pmu_op_finish(&cpu->env);
> +    return 0;
> +}
> +
> +static int cpu_pre_load(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    pmu_op_start(&cpu->env);
> +    return 0;
> +}
> +
>  static int cpu_post_load(void *opaque, int version_id)
>  {
>      ARMCPU *cpu = opaque;
> @@ -672,6 +688,8 @@ static int cpu_post_load(void *opaque, int version_id)
>      hw_breakpoint_update_all(cpu);
>      hw_watchpoint_update_all(cpu);
>
> +    pmu_op_finish(&cpu->env);
> +
>      return 0;
>  }

This will end up calling pmu_op_start() and pmu_op_finish()
even if the guest is running KVM and we're not using the
TCG code. Is that what you intended?

thanks
-- PMM
Re: [Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations
Posted by Aaron Lindsay 6 years, 11 months ago 
On Nov 16 14:53, Peter Maydell wrote:
> On 5 November 2018 at 18:51, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> > Because of the PMU's design, many register accesses have side effects
> > which are inter-related, meaning that the normal method of saving CP
> > registers can result in inconsistent state. These side-effects are
> > largely handled in pmu_op_start/finish functions which can be called
> > before and after the state is saved/restored. By doing this and adding
> > raw read/write functions for the affected registers, we avoid
> > migration-related inconsistencies.
> >
> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> 
> > --- a/target/arm/machine.c
> > +++ b/target/arm/machine.c
> > @@ -604,6 +604,8 @@ static int cpu_pre_save(void *opaque)
> >  {
> >      ARMCPU *cpu = opaque;
> >
> > +    pmu_op_start(&cpu->env);
> > +
> >      if (kvm_enabled()) {
> >          if (!write_kvmstate_to_list(cpu)) {
> >              /* This should never fail */
> > @@ -625,6 +627,20 @@ static int cpu_pre_save(void *opaque)
> >      return 0;
> >  }
> >
> > +static int cpu_post_save(void *opaque)
> > +{
> > +    ARMCPU *cpu = opaque;
> > +    pmu_op_finish(&cpu->env);
> > +    return 0;
> > +}
> > +
> > +static int cpu_pre_load(void *opaque)
> > +{
> > +    ARMCPU *cpu = opaque;
> > +    pmu_op_start(&cpu->env);
> > +    return 0;
> > +}
> > +
> >  static int cpu_post_load(void *opaque, int version_id)
> >  {
> >      ARMCPU *cpu = opaque;
> > @@ -672,6 +688,8 @@ static int cpu_post_load(void *opaque, int version_id)
> >      hw_breakpoint_update_all(cpu);
> >      hw_watchpoint_update_all(cpu);
> >
> > +    pmu_op_finish(&cpu->env);
> > +
> >      return 0;
> >  }
> 
> This will end up calling pmu_op_start() and pmu_op_finish()
> even if the guest is running KVM and we're not using the
> TCG code. Is that what you intended?

The counters are still stored in their 'difference' format for KVM, so I
think this still makes sense. Or is there something I'm missing about
how this will interact with KVM? I'm much more familiar with TCG.

-Aaron
Re: [Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations
Posted by Peter Maydell 6 years, 11 months ago 
On 16 November 2018 at 16:09, Aaron Lindsay
<aaron@os.amperecomputing.com> wrote:
> On Nov 16 14:53, Peter Maydell wrote:
>> On 5 November 2018 at 18:51, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>> > Because of the PMU's design, many register accesses have side effects
>> > which are inter-related, meaning that the normal method of saving CP
>> > registers can result in inconsistent state. These side-effects are
>> > largely handled in pmu_op_start/finish functions which can be called
>> > before and after the state is saved/restored. By doing this and adding
>> > raw read/write functions for the affected registers, we avoid
>> > migration-related inconsistencies.
>> >
>> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
>>
>> > --- a/target/arm/machine.c
>> > +++ b/target/arm/machine.c
>> > @@ -604,6 +604,8 @@ static int cpu_pre_save(void *opaque)
>> >  {
>> >      ARMCPU *cpu = opaque;
>> >
>> > +    pmu_op_start(&cpu->env);
>> > +
>> >      if (kvm_enabled()) {
>> >          if (!write_kvmstate_to_list(cpu)) {
>> >              /* This should never fail */
>> > @@ -625,6 +627,20 @@ static int cpu_pre_save(void *opaque)
>> >      return 0;
>> >  }
>> >
>> > +static int cpu_post_save(void *opaque)
>> > +{
>> > +    ARMCPU *cpu = opaque;
>> > +    pmu_op_finish(&cpu->env);
>> > +    return 0;
>> > +}
>> > +
>> > +static int cpu_pre_load(void *opaque)
>> > +{
>> > +    ARMCPU *cpu = opaque;
>> > +    pmu_op_start(&cpu->env);
>> > +    return 0;
>> > +}
>> > +
>> >  static int cpu_post_load(void *opaque, int version_id)
>> >  {
>> >      ARMCPU *cpu = opaque;
>> > @@ -672,6 +688,8 @@ static int cpu_post_load(void *opaque, int version_id)
>> >      hw_breakpoint_update_all(cpu);
>> >      hw_watchpoint_update_all(cpu);
>> >
>> > +    pmu_op_finish(&cpu->env);
>> > +
>> >      return 0;
>> >  }
>>
>> This will end up calling pmu_op_start() and pmu_op_finish()
>> even if the guest is running KVM and we're not using the
>> TCG code. Is that what you intended?
>
> The counters are still stored in their 'difference' format for KVM, so I
> think this still makes sense. Or is there something I'm missing about
> how this will interact with KVM? I'm much more familiar with TCG.

For KVM the counter values are stored in the kernel, until the
write_kvmstate_to_list() function (which is performed after your
pmu_op_start() call in cpu_pre_save()) writes them from the
kernel into the cpreg_vmstate array. Similarly on load they
go straight from the vmstate array into the kernel registers.
It's not clear to me what the pmu_op_start()/finish() calls
are intended to do in the KVM case, and they look at fields
in the env->cp15 struct which will not have valid values at
this point.

thanks
-- PMM
Re: [Qemu-devel] [PATCH v7 03/12] target/arm: Swap PMU values before/after migrations
Posted by Aaron Lindsay 6 years, 11 months ago 
On Nov 16 16:44, Peter Maydell wrote:
> On 16 November 2018 at 16:09, Aaron Lindsay
> <aaron@os.amperecomputing.com> wrote:
> > On Nov 16 14:53, Peter Maydell wrote:
> >> On 5 November 2018 at 18:51, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> >> > Because of the PMU's design, many register accesses have side effects
> >> > which are inter-related, meaning that the normal method of saving CP
> >> > registers can result in inconsistent state. These side-effects are
> >> > largely handled in pmu_op_start/finish functions which can be called
> >> > before and after the state is saved/restored. By doing this and adding
> >> > raw read/write functions for the affected registers, we avoid
> >> > migration-related inconsistencies.
> >> >
> >> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> >>
> >> > --- a/target/arm/machine.c
> >> > +++ b/target/arm/machine.c
> >> > @@ -604,6 +604,8 @@ static int cpu_pre_save(void *opaque)
> >> >  {
> >> >      ARMCPU *cpu = opaque;
> >> >
> >> > +    pmu_op_start(&cpu->env);
> >> > +
> >> >      if (kvm_enabled()) {
> >> >          if (!write_kvmstate_to_list(cpu)) {
> >> >              /* This should never fail */
> >> > @@ -625,6 +627,20 @@ static int cpu_pre_save(void *opaque)
> >> >      return 0;
> >> >  }
> >> >
> >> > +static int cpu_post_save(void *opaque)
> >> > +{
> >> > +    ARMCPU *cpu = opaque;
> >> > +    pmu_op_finish(&cpu->env);
> >> > +    return 0;
> >> > +}
> >> > +
> >> > +static int cpu_pre_load(void *opaque)
> >> > +{
> >> > +    ARMCPU *cpu = opaque;
> >> > +    pmu_op_start(&cpu->env);
> >> > +    return 0;
> >> > +}
> >> > +
> >> >  static int cpu_post_load(void *opaque, int version_id)
> >> >  {
> >> >      ARMCPU *cpu = opaque;
> >> > @@ -672,6 +688,8 @@ static int cpu_post_load(void *opaque, int version_id)
> >> >      hw_breakpoint_update_all(cpu);
> >> >      hw_watchpoint_update_all(cpu);
> >> >
> >> > +    pmu_op_finish(&cpu->env);
> >> > +
> >> >      return 0;
> >> >  }
> >>
> >> This will end up calling pmu_op_start() and pmu_op_finish()
> >> even if the guest is running KVM and we're not using the
> >> TCG code. Is that what you intended?
> >
> > The counters are still stored in their 'difference' format for KVM, so I
> > think this still makes sense. Or is there something I'm missing about
> > how this will interact with KVM? I'm much more familiar with TCG.
> 
> For KVM the counter values are stored in the kernel, until the
> write_kvmstate_to_list() function (which is performed after your
> pmu_op_start() call in cpu_pre_save()) writes them from the
> kernel into the cpreg_vmstate array. Similarly on load they
> go straight from the vmstate array into the kernel registers.
> It's not clear to me what the pmu_op_start()/finish() calls
> are intended to do in the KVM case, and they look at fields
> in the env->cp15 struct which will not have valid values at
> this point.

Oh, I had a fundamental misunderstanding and expected the registers
would be in env->cp15 when this was called, even for KVM.

After looking into this more, it seems like my pmu_op_* calls here
should be contained in `if (!kvm_enabled())` blocks. It doesn't seem
like anything bad would happen otherwise, but it also isn't necessary or
helpful to make those calls.

I'll make this change for v8 if you don't have further feedback.

-Aaron

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.