Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix do_kvm_cpu_synchronize_state data integrity issue

[Alexander Graf]

On 04.01.2013, at 12:01, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:address@hidden
>> Sent: Friday, January 04, 2013 4:07 PM
>> To: Bhushan Bharat-R65777
>> Cc: Marcelo Tosatti; address@hidden; Christian Borntraeger; Anthony
>> Liguori; address@hidden qemu-devel
>> Subject: Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix
>> do_kvm_cpu_synchronize_state data integrity issue
>> 
>> 
>> On 04.01.2013, at 11:32, Bhushan Bharat-R65777 wrote:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>>>>> 
>>>>>>> Int timer_func(CPUxxState env)
>>>>>>> {
>>>>>>> Struct timer_regs;
>>>>>>> read_regs_type((env, &timer_regs,TIMER_REGS);  //update
>>>>>>> env->timer_registers  Write_regs_type(env, TIMER_REGS) }
>>>>>>> 
>>>>>>> This will get one type of register_types and can cause multiple
>>>>>>> IOCTL per
>>>>>> entry to QEMU.
>>>>>> 
>>>>>> This is still too explicit. How about
>>>>>> 
>>>>>> static inline void ppc_set_tsr(CPUState *env, target_ulong val) {
>>>>>>  env->kvm_sync_extra |= SYNC_TIMER_REGS;
>>>>>>  cpu_synchronize_registers(env);
>>>>>>  env->spr[SPR_TSR] = val;
>>>>> 
>>>>> You also want env->kvm_sync_dirty also, right?
>>>> 
>>>> Not quite, since SYNC_TIMER_REGS spans more than only TSR. So we need
>>>> to make sure we fetch the non-TSR register values before we can
>>>> declare TIMER_REGS as dirty.
>>> 
>>> Right , I actually want communicate mark dirty the TIMER_REGS only :)
>> 
>> Imagine TIMER_REGS includes TSR and TCR. Then
>> 
>> ppc_set_tsr();
>> 
>> should still work without a respective ppc_set_tcr() call. So we can't just 
>> mark
>> it dirty here. The only way we could optimize this bit would be by either
>> splitting the bitmap per register or by extending the setter functions to the
>> full scope of the sync:
> 
> Are you talking about GET all register of a type, say TIMER (TCR and TSR) on 
> first call to cpu_synchronization_function but SET will be only for dirty 
> register. Say if TSR is changed then setter will only communicate that TSR is 
> changed?
> 
> Or
> 
> (
> get multiple regs of a type , say TIMER (TCR and TSR), change one or more and 
> mark dirty if any one changed. Set all registers of a type, TIMER (TCR and 
> TSR) if dirty.
>  In this case the first call by ppc_set_tsr() to cpu_synchronization_function 
> will implicitly get all registers of TIMER (TCR and TSR) - means one or more 
> registers of timer can be updated by QEMU. This function will update only 
> env->TSR.
>  Later if ppc_set_tcr() is called, it will also call 
> cpu_synchronization_function but this will see that TIMER registers are 
> already available to QEMU, so it will not fetch again. This function will 
> update the TCR also.
>  Finally they all, TIMER (TCR and TSR) will be pushed to kernel.

Yes :)


Alex