Re: [Qemu-devel] [PATCH v2] target-arm: kvm: Differentiate registers based on write-back levels

[Claudio Fontana]

On 17.07.2015 17:39, Christoffer Dall wrote:
> On Fri, Jul 17, 2015 at 03:29:56PM +0100, Peter Maydell wrote:
>> On 16 July 2015 at 12:34, Christoffer Dall <address@hidden> wrote:
>>> Some registers like the CNTVCT register should only be written to the
>>> kernel as part of machine initialization or on vmload operations, but
>>> never during runtime, as this can potentially make time go backwards or
>>> create inconsistent time observations between VCPUs.
>>>
>>> Introduce a list of registers that should not be written back at runtime
>>> and check this list on syncing the register state to the KVM state.
>>
>> Thanks. I think this should go into QEMU 2.4, given that it fixes
>> a bug with time misbehaving in guests. Are you happy that it's
>> received enough testing? (I have given 32-bit KVM a spin but have
>> no convenient 64-bit box to test with, and besides, I didn't notice the
>> bug in the first place :-))
> 
> I tested this on both Juno and Mustang, with a simple loop kernel
> booting and doing hackbench test, but if we want to be on the extra
> careful side, perhaps Alex can run it through his migration test setup?
> I don't think that's necessary though.
> 
>>
>>> Signed-off-by: Christoffer Dall <address@hidden>
>>> ---
>>> Changes since RFC:
>>>  - Move cpreg_level to kvm_arm_cpreg_level and into kvm32.c and kvm64.c
>>>  - Changed struct name and declare as static const
>>
>> I have a couple of minor comments on the comments below, and you
>> forgot to update the stub version of write_list_to_kvmstate().
>> I can just fix these up as I put it into target-arm.next, though.
>> Fixed up version at:
>>
>>  https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next
>>
>> If interested parties could test that by end-of-Monday that
>> would be nice (since rc2 is scheduled for Tuesday).
>>
>>>  dtc                  |  2 +-
>>>  target-arm/kvm.c     |  6 +++++-
>>>  target-arm/kvm32.c   | 30 +++++++++++++++++++++++++++++-
>>>  target-arm/kvm64.c   | 30 +++++++++++++++++++++++++++++-
>>>  target-arm/kvm_arm.h | 12 +++++++++++-
>>>  target-arm/machine.c |  2 +-
>>>  6 files changed, 76 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/dtc b/dtc
>>> index 65cc4d2..bc895d6 160000
>>> --- a/dtc
>>> +++ b/dtc
>>> @@ -1 +1 @@
>>> -Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
>>> +Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde
>>
>> Stray submodule change :-)
>>
> 
> Damn, keeps happening to me.  ok, I'll stop using git commit -a for qemu
> changes.
> 
>>> diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
>>> index d7e7d68..6769815 100644
>>> --- a/target-arm/kvm32.c
>>> +++ b/target-arm/kvm32.c
>>> @@ -153,6 +153,34 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
>>>      }
>>>  }
>>>
>>> +typedef struct CPRegStateLevel {
>>> +    uint64_t regidx;
>>> +    int level;
>>> +} CPRegStateLevel;
>>> +
>>> +/* All coprocessor registers not listed in the following table are assumed 
>>> to
>>> + * be of the level KVM_PUT_RUNTIME_STATE, a register should be written less
>>
>> ". If a register".
>>
>>> + * often, you must add it to this table with a state of either
>>> + * KVM_PUT_RESET_STATE or KVM_PUT_FULL_STATE.
>>> + */
>>> +static const CPRegStateLevel non_runtime_cpregs[] = {
>>> +    { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE },
>>> +};
>>
>>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>>> index ac34f51..d59f41c 100644
>>> --- a/target-arm/kvm64.c
>>> +++ b/target-arm/kvm64.c
>>> @@ -139,6 +139,34 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
>>>      }
>>>  }
>>>
>>> +typedef struct CPRegStateLevel {
>>> +    uint64_t regidx;
>>> +    int level;
>>> +} CPRegStateLevel;
>>> +
>>> +/* All system not listed in the following table are assumed to be of the 
>>> level
>>
>> "system registers"
>>
>>> + * KVM_PUT_RUNTIME_STATE, a register should be written less often, you must
>>
>> ". If a register"
>>
>>> + * add it to this table with a state of either KVM_PUT_RESET_STATE or
>>> + * KVM_PUT_FULL_STATE.
>>> + */
>>> +static const CPRegStateLevel non_runtime_cpregs[] = {
>>> +    { KVM_REG_ARM_TIMER_CNT, KVM_PUT_FULL_STATE },
>>> +};
>>
>>> --- a/target-arm/kvm_arm.h
>>> +++ b/target-arm/kvm_arm.h
>>
>>> @@ -83,7 +93,7 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx);
>>>  * Note that we do not stop early on failure -- we will attempt
>>>   * writing all registers in the list.
>>>  */
>>> -bool write_list_to_kvmstate(ARMCPU *cpu);
>>> +bool write_list_to_kvmstate(ARMCPU *cpu, int level);
>>
>> You forgot to update the stub function in target-arm/kvm-stub.c,
>> so this breaks compilation on non-ARM hosts.
>>
> whoops, who cares about non-ARM hosts anyway.
> 
> Thanks for fixing these up, the fixed up version looks good.
> -Christoffer
> 

Hi,

I can if you want check if this patch actually fixes the problem without the 
KVM workaround.
Is this the version I am supposed to test, or should I wait for the next respin?

Thanks,

Claudio



-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München