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

[Bhushan Bharat-R65777]

> -----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.
)

Thanks
-Bharat

> 
> static inline void ppc_set_timer_regs(CPUState *env, target_ulong tsr,
> target_ulong tcr) {
>     ...
> }
> 
> 
> Alex
>