qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap paramet


From: Marcelo Tosatti
Subject: Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state
Date: Wed, 16 Jan 2013 15:23:36 -0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Wed, Jan 16, 2013 at 05:00:52PM +0000, Bhushan Bharat-R65777 wrote:
> I think above code should be:
>             kvm_arch_put_registers(cpu, cpu->kvm_vcpu_dirty);
>             cpu->kvm_vcpu_dirty = false;
> 
> so vcpu will not enter guest state with dirty registers in qemu.

Not so clear - currently PUT_FULL/PUT_RESET are performed on
pre-defined points.

> > Unrelated:
> > 
> > 2) Also, what is the reason for specifying sets of registers in 
> > arch-specific
> > code? Is that because it allows PPC to fix their sync-timer register 
> > problem?
> > 
> > When you are writing generic code, what does it mean to use
> > 'KVM_REGSYNC_{RUNTIME,RESET,FULL}_STATE' ?
> > Answer: it depends on the architecture.
> > 
> > 3) On x86, kvm_arch_get_registers(GET_FULL) must not imply
> > kvm_arch_put_registers(PUT_FULL).
> > 
> > The S/390 problem, from
> > http://lists.nongnu.org/archive/html/qemu-devel/2012-11/msg02213.html:
> > 
> > ">>> The kvm register sync needs to happen in the kvm register sync
> > >>> function :)
> > >> That would eliminate the whole purpose of sync regs and forces us to
> > >> have an expensive ioctl on lots of exits (again). I would prefer to
> > >> sync the registers that we never need in qemu just here.
> > >
> > > That's why the register sync has different stages.
> > 
> > Not the get_register. Which is called on every synchronize_state. Which 
> > happen
> > quite often on s390."
> > 
> > But wait: on these S/390 codepaths, you do GET_REGS already, via
> > cpu_synchronize_state.
> > 
> > So on S/390
> > 
> > - cpu_synchronize_state(env)
> > - read any register from env
> > 
> > Is not valid? This is what generic code assumes.
> > 
> > 
> > Bhushan Bharat, the PPC problem, can you describe it clearly: from what i
> > understood, an in-kernel register cannot be read/written back because that
> > register value can change in the meantime. When is it necessary to write it
> > back? (there is a similar problem with TSC on x86, which is "fixed" by only
> > writing TSC on FULL_STATE arch_put_registers).
> 
> There are two things:
> 
> First-)
> For timer related changes on PowerPC, some registers needed to be changed 
> from QEMU, so we have to get the registers via KVM_GET_SREGS and then set 
> those registers back to KVM via KVM_SET_SREGS. cpu_synchronize_state() will 
> get registers but kvm_arch_put_registers() works on level based mechanism and 
> does not provide a good way of setting a register-set. So we wrote a separate 
> function that will push these registers back to KVM and this also uses 
> KVM_SET_SREGS ioctl. This solves what is needed for PPC.

Can you describe the problem in detail? You must sync a particular
timer register only on special conditions, not during normal
cpu_synchronize_state() runs?

What register is that and why it cannot be synced normally? When is it
necessary to sync it?

> Second-)
> Currently kvm_arch_get_registers() is not optimized in two sense; one, it 
> always get all registers from KVM; two, in kvm_arch_get_registers() it copies 
> all registers to env->. This patch-set handles the second issue of 
> optimization, copy only the requested registers to env-> in 
> kvm_arch_get_registers(), plus when kvm_arch_put_registers() is called then 
> it copies only the modified registers for KVM_SET_SREGS.
> 
> This optimization is looking good to me and allows sync of registers via one 
> common kvm_arch_get/set_registers() and no separate function definition for 
> setting is needed for timer related changes.

The problem with S390 is not clear to me (besides
cpu_synchronize_state must sync all state as mentioned).

> 
> Thanks
> -Bharat
>  



reply via email to

[Prev in Thread] Current Thread [Next in Thread]