[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: |
Bhushan Bharat-R65777 |
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 17:00:52 +0000 |
> -----Original Message-----
> From: Marcelo Tosatti [mailto:address@hidden
> Sent: Wednesday, January 16, 2013 9:36 PM
> To: Jason J. Herne
> Cc: address@hidden; address@hidden; address@hidden; qemu-
> address@hidden; Bhushan Bharat-R65777; address@hidden
> Subject: Re: [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to
> do_kvm_cpu_synchronize_state
>
> On Thu, Jan 10, 2013 at 10:29:04AM -0500, Jason J. Herne wrote:
> > From: "Jason J. Herne" <address@hidden>
> >
> > do_kvm_cpu_synchronize_state is called via run_on_cpu, so we can only
> > pass a single argument. Create SyncStateArgs struct for this purpose
> > and add register bitmap data member to it.
> >
> > Signed-off-by: Jason J. Herne <address@hidden>
> > Reviewed-by: Christian Borntraeger <address@hidden>
> > ---
> > include/sysemu/kvm.h | 6 ++++++
> > kvm-all.c | 27 +++++++++++++++++----------
> > 2 files changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
> > e0738ba..193d1f4 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -223,6 +223,12 @@ int kvm_check_extension(KVMState *s, unsigned int
> > extension);
> >
> > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> > uint32_t index, int reg);
> > +
> > +struct kvm_cpu_syncstate_args {
> > + CPUState *cpu;
> > + int regmap;
> > +};
> > +
> > void kvm_cpu_synchronize_state(CPUArchState *env); void
> > kvm_cpu_synchronize_post_reset(CPUArchState *env); void
> > kvm_cpu_synchronize_post_init(CPUArchState *env); diff --git
> > a/kvm-all.c b/kvm-all.c index 1aa61bb..77ab72a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -231,7 +231,7 @@ int kvm_init_vcpu(CPUArchState *env)
> >
> > cpu->kvm_fd = ret;
> > cpu->kvm_state = s;
> > - cpu->kvm_vcpu_dirty = true;
> > + cpu->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
> >
> > mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
> > if (mmap_size < 0) {
> > @@ -1491,20 +1491,27 @@ void kvm_flush_coalesced_mmio_buffer(void)
> >
> > static void do_kvm_cpu_synchronize_state(void *arg) {
> > - CPUState *cpu = arg;
> > + struct kvm_cpu_syncstate_args *args = arg;
> >
> > - if (!cpu->kvm_vcpu_dirty) {
> > - kvm_arch_get_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > - cpu->kvm_vcpu_dirty = true;
> > + /* Do not sync regs that are already dirty */
> > + int regs_to_get = args->regmap & ~args->cpu->kvm_vcpu_dirty;
> > +
> > + if (regs_to_get) {
> > + kvm_arch_get_registers(args->cpu, regs_to_get);
> > + args->cpu->kvm_vcpu_dirty |= regs_to_get;
> > }
> > }
> >
> > void kvm_cpu_synchronize_state(CPUArchState *env) {
> > CPUState *cpu = ENV_GET_CPU(env);
> > + struct kvm_cpu_syncstate_args args;
> > +
> > + args.cpu = cpu;
> > + args.regmap = KVM_REGSYNC_FULL_STATE;
> >
> > - if (!cpu->kvm_vcpu_dirty) {
> > - run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> > + if (args.regmap & ~cpu->kvm_vcpu_dirty) {
> > + run_on_cpu(cpu, do_kvm_cpu_synchronize_state, &args);
> > }
> > }
> >
> > @@ -1513,7 +1520,7 @@ void kvm_cpu_synchronize_post_reset(CPUArchState *env)
> > CPUState *cpu = ENV_GET_CPU(env);
> >
> > kvm_arch_put_registers(cpu, KVM_REGSYNC_RESET_STATE);
> > - cpu->kvm_vcpu_dirty = false;
> > + cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RESET_STATE;
> > }
> >
> > void kvm_cpu_synchronize_post_init(CPUArchState *env) @@ -1521,7
> > +1528,7 @@ void kvm_cpu_synchronize_post_init(CPUArchState *env)
> > CPUState *cpu = ENV_GET_CPU(env);
> >
> > kvm_arch_put_registers(cpu, KVM_REGSYNC_FULL_STATE);
> > - cpu->kvm_vcpu_dirty = false;
> > + cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_FULL_STATE;
> > }
> >
> > int kvm_cpu_exec(CPUArchState *env)
> > @@ -1540,7 +1547,7 @@ int kvm_cpu_exec(CPUArchState *env)
> > do {
> > if (cpu->kvm_vcpu_dirty) {
> > kvm_arch_put_registers(cpu, KVM_REGSYNC_RUNTIME_STATE);
> > - cpu->kvm_vcpu_dirty = false;
> > + cpu->kvm_vcpu_dirty &= ~KVM_REGSYNC_RUNTIME_STATE;
> > }
>
> 1) This implies a vcpu can enter guest mode with kvm_vcpu_dirty non-zero.
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.
>
> 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.
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.
Thanks
-Bharat
- [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Jason J. Herne, 2013/01/10
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Blue Swirl, 2013/01/12
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Bhushan Bharat-R65777, 2013/01/14
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Marcelo Tosatti, 2013/01/16
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state,
Bhushan Bharat-R65777 <=
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Marcelo Tosatti, 2013/01/16
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Alexander Graf, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Scott Wood, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Peter Maydell, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Alexander Graf, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Scott Wood, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Alexander Graf, 2013/01/24
- Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Marcelo Tosatti, 2013/01/16
Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Marcelo Tosatti, 2013/01/16
Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state, Christian Borntraeger, 2013/01/16