From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51787 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PcOwn-0002Dh-V6 for qemu-devel@nongnu.org; Mon, 10 Jan 2011 16:06:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PcOwm-00068x-0v for qemu-devel@nongnu.org; Mon, 10 Jan 2011 16:06:37 -0500 Received: from fmmailgate03.web.de ([217.72.192.234]:43969) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PcOwl-000686-Hf for qemu-devel@nongnu.org; Mon, 10 Jan 2011 16:06:35 -0500 Message-ID: <4D2B74D8.4080309@web.de> Date: Mon, 10 Jan 2011 22:06:32 +0100 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 28/35] kvm: x86: Introduce kvmclock device to save/restore its state References: <4D2B6CB5.9050602@codemonkey.ws> In-Reply-To: <4D2B6CB5.9050602@codemonkey.ws> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig84F2FE8D7979C3306DFBC23A" Sender: jan.kiszka@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Anthony Liguori , Marcelo Tosatti , Glauber Costa , kvm@vger.kernel.org, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig84F2FE8D7979C3306DFBC23A Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Am 10.01.2011 21:31, Anthony Liguori wrote: > On 01/06/2011 11:56 AM, Marcelo Tosatti wrote: >> From: Jan Kiszka >> >> If kvmclock is used, which implies the kernel supports it, register a >> kvmclock device with the sysbus. Its main purpose is to save and resto= re >> the kernel state on migration, but this will also allow to visualize i= t >> one day. >> >> Signed-off-by: Jan Kiszka >> CC: Glauber Costa >> Signed-off-by: Marcelo Tosatti >> --- >> target-i386/kvm.c | 92 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 91 insertions(+), 1 deletions(-) >> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >> index 69b8234..47cb22b 100644 >> --- a/target-i386/kvm.c >> +++ b/target-i386/kvm.c >> @@ -29,6 +29,7 @@ >> #include "hw/apic.h" >> #include "ioport.h" >> #include "kvm_x86.h" >> +#include "hw/sysbus.h" >> >> #ifdef CONFIG_KVM_PARA >> #include >> @@ -309,6 +310,85 @@ void kvm_inject_x86_mce(CPUState *cenv, int bank,= >> uint64_t status, >> #endif >> } >> >> +#if defined(CONFIG_KVM_PARA)&& defined(KVM_CAP_ADJUST_CLOCK) >> +typedef struct KVMClockState { >> + SysBusDevice busdev; >> + uint64_t clock; >> + bool clock_valid; >> +} KVMClockState; >> + >> +static void kvmclock_pre_save(void *opaque) >> +{ >> + KVMClockState *s =3D opaque; >> + struct kvm_clock_data data; >> + int ret; >> + >> + if (s->clock_valid) { >> + return; >> + } >> + ret =3D kvm_vm_ioctl(KVM_GET_CLOCK,&data); >> + if (ret< 0) { >> + fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));= >> + data.clock =3D 0; >> + } >> + s->clock =3D data.clock; >> + /* >> + * If the VM is stopped, declare the clock state valid to avoid >> re-reading >> + * it on next vmsave (which would return a different value). Will= >> be reset >> + * when the VM is continued. >> + */ >> + s->clock_valid =3D !vm_running; >> +} >> + >> +static int kvmclock_post_load(void *opaque, int version_id) >> +{ >> + KVMClockState *s =3D opaque; >> + struct kvm_clock_data data; >> + >> + data.clock =3D s->clock; >> + data.flags =3D 0; >> + return kvm_vm_ioctl(KVM_SET_CLOCK,&data); >> +} >> + >> +static void kvmclock_vm_state_change(void *opaque, int running, int >> reason) >> +{ >> + KVMClockState *s =3D opaque; >> + >> + if (running) { >> + s->clock_valid =3D false; >> + } >> +} >> + >> +static int kvmclock_init(SysBusDevice *dev) >> +{ >> + KVMClockState *s =3D FROM_SYSBUS(KVMClockState, dev); >> + >> + qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s); >> + return 0; >> +} >> + >> +static const VMStateDescription kvmclock_vmsd=3D { >> + .name =3D "kvmclock", >> + .version_id =3D 1, >> + .minimum_version_id =3D 1, >> + .minimum_version_id_old =3D 1, >> + .pre_save =3D kvmclock_pre_save, >> + .post_load =3D kvmclock_post_load, >> + .fields =3D (VMStateField []) { >> + VMSTATE_UINT64(clock, KVMClockState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +static SysBusDeviceInfo kvmclock_info =3D { >> + .qdev.name =3D "kvmclock", >> + .qdev.size =3D sizeof(KVMClockState), >> + .qdev.vmsd =3D&kvmclock_vmsd, >> + .qdev.no_user =3D 1, >> + .init =3D kvmclock_init, >> +}; >> +#endif /* CONFIG_KVM_PARA&& KVM_CAP_ADJUST_CLOCK */ >> + >> int kvm_arch_init_vcpu(CPUState *env) >> { >> struct { >> @@ -335,7 +415,6 @@ int kvm_arch_init_vcpu(CPUState *env) >> env->cpuid_svm_features&=3D kvm_x86_get_supported_cpuid(0x800000= 0A, >> 0, R_EDX= ); >> >> - >> cpuid_i =3D 0; >> >> #ifdef CONFIG_KVM_PARA >> @@ -442,6 +521,13 @@ int kvm_arch_init_vcpu(CPUState *env) >> } >> #endif >> >> +#if defined(CONFIG_KVM_PARA)&& defined(KVM_CAP_ADJUST_CLOCK) >> + if (cpu_is_bsp(env)&& >> + (env->cpuid_kvm_features& (1ULL<< KVM_FEATURE_CLOCKSOURCE))= ) { >> + sysbus_create_simple("kvmclock", -1, NULL); >> + } >> +#endif >> + >> return kvm_vcpu_ioctl(env, KVM_SET_CPUID2,&cpuid_data); >> } >> >> @@ -531,6 +617,10 @@ int kvm_arch_init(int smp_cpus) >> int ret; >> struct utsname utsname; >> >> +#if defined(CONFIG_KVM_PARA)&& defined(KVM_CAP_ADJUST_CLOCK) >> + sysbus_register_withprop(&kvmclock_info); >> +#endif >> + >> ret =3D kvm_get_supported_msrs(); >> if (ret< 0) { >> return ret; >> =20 >=20 > There are a couple things wrong with this patch. It breaks > compatibility because it does not allow kvmclock to be created or > initiated in machines. Older machines didn't expose kvmclock but now > they do. It also makes it impossible to pass parameters to kvmclock in= > the future because the device creation is hidden deep in other code > paths. Device parameters should get passed as properties. Would already work today if we had any. > Calling any qdev creation function in anything but pc.c (or the > equivalent) should be a big red flag. >=20 > The solution is simple, introduce as kvm_has_clocksource(). Within the= > machine init, create the the kvm clock device after CPU creation wrappe= d > in a if (kvm_has_clocksource()) call. No problem with moving sysbus_create_simple to machine initialization, though. > kvmclock should be created with > kvm_state as a parameter and kvm_vm_ioctl() is passed the stored > reference. Taking a global reference to kvm_state in machine_init is > not a bad thing, obviously the machine initialization function needs > access to the kvm_state. This would also require changing sysbus interfaces for the sake of KVM's "abstraction". If this is the only way forward, I could look into this. Still, I do not see any benefit for the affected code. You then either need to "steal" a kvm_state reference from the first cpu or introduce a marvelous interface like kvm_get_state() to make this work from outside of the KVM core. Jan --------------enig84F2FE8D7979C3306DFBC23A Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAk0rdNgACgkQitSsb3rl5xR+2ACcDMokClUVTVyBGHh8oSTmJ+eO M9oAnRfwtfBhYWEcPIPKX6uzhaZoX6O1 =8Osz -----END PGP SIGNATURE----- --------------enig84F2FE8D7979C3306DFBC23A--