From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvVVR-00083X-Mo for qemu-devel@nongnu.org; Wed, 16 Jan 2013 11:06:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TvVVN-0001g7-3h for qemu-devel@nongnu.org; Wed, 16 Jan 2013 11:06:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:10420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TvVVM-0001fz-Rp for qemu-devel@nongnu.org; Wed, 16 Jan 2013 11:06:21 -0500 Date: Wed, 16 Jan 2013 14:05:33 -0200 From: Marcelo Tosatti Message-ID: <20130116160533.GA8541@amt.cnet> References: <1357831744-3950-1-git-send-email-jjherne@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1357831744-3950-1-git-send-email-jjherne@us.ibm.com> Subject: Re: [Qemu-devel] [PATCH 4/7 v2] KVM regsync: Add register bitmap parameter to do_kvm_cpu_synchronize_state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Jason J. Herne" Cc: aliguori@us.ibm.com, jan.kiszka@siemens.com, agraf@suse.de, qemu-devel@nongnu.org, borntraeger@de.ibm.com, R65777@freescale.com On Thu, Jan 10, 2013 at 10:29:04AM -0500, Jason J. Herne wrote: > From: "Jason J. Herne" > > 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 > Reviewed-by: Christian Borntraeger > --- > 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. 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).