From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH -v2] Add savevm/loadvm support for MCE Date: Tue, 02 Mar 2010 11:20:00 +0100 Message-ID: <4B8CE650.6030803@web.de> References: <1267508413.1640.88.camel@yhuang-dev.sh.intel.com> <4B8CC7B1.4050609@web.de> <1267518988.1640.122.camel@yhuang-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigE8DF6B140A0D4190C7D6E67C" Cc: Avi Kivity , Anthony Liguori , Andi Kleen , "kvm@vger.kernel.org" To: Huang Ying Return-path: Received: from fmmailgate01.web.de ([217.72.192.221]:59600 "EHLO fmmailgate01.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752253Ab0CBKUZ (ORCPT ); Tue, 2 Mar 2010 05:20:25 -0500 In-Reply-To: <1267518988.1640.122.camel@yhuang-dev.sh.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigE8DF6B140A0D4190C7D6E67C Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Huang Ying wrote: > On Tue, 2010-03-02 at 16:09 +0800, Jan Kiszka wrote: >> Huang Ying wrote: >>> MCE registers are saved/load into/from CPUState in >>> kvm_arch_save/load_regs. To simulate the MCG_STATUS clearing upon >>> reset, MSR_MCG_STATUS is set to 0 for KVM_PUT_RESET_STATE. >>> >>> v2: >>> >>> - Rebased on new CPU registers save/load framework. >> Yep, much closer. :) >> >>> Signed-off-by: Huang Ying >>> --- >>> qemu-kvm-x86.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 43 insertions(+) >>> >>> --- a/qemu-kvm-x86.c >>> +++ b/qemu-kvm-x86.c >>> @@ -979,11 +979,33 @@ void kvm_arch_load_regs(CPUState *env, i >>> set_msr_entry(&msrs[n++], MSR_KVM_SYSTEM_TIME, env->system_t= ime_msr); >>> set_msr_entry(&msrs[n++], MSR_KVM_WALL_CLOCK, env->wall_cloc= k_msr); >>> } >>> +#ifdef KVM_CAP_MCE >>> + if (env->mcg_cap && level =3D=3D KVM_PUT_RESET_STATE) { >>> + /* >>> + * MCG_STATUS should reset to 0 after reset, while other MCE= >>> + * registers should be unchanged >>> + */ >>> + set_msr_entry(&msrs[n++], MSR_MCG_STATUS, 0); >> For the sake of consistency, just write mcg_status here (it's properly= >> updated in cpu_reset). >=20 > OK. >=20 >>> + } >>> +#endif >>> =20 >>> rc =3D kvm_set_msrs(env, msrs, n); >>> if (rc =3D=3D -1) >>> perror("kvm_set_msrs FAILED"); >>> =20 >>> +#ifdef KVM_CAP_MCE >>> + if (env->mcg_cap && level =3D=3D KVM_PUT_FULL_STATE) { >>> + n =3D 0; >>> + set_msr_entry(&msrs[n++], MSR_MCG_STATUS, env->mcg_status); >>> + set_msr_entry(&msrs[n++], MSR_MCG_CTL, env->mcg_ctl); >> You can move this block up, reusing the kvm_set_msrs above. But... >> >>> + for (i =3D 0; i < (env->mcg_cap & 0xff); i++) >> ...this requires some care. We have space for writing up to 100 >> registers in our msrs array. You may have to extend it unless this >> number is much smaller in reality. >=20 > The default number of MCE banks is 10, this means 42 entries. So I thin= k > it is safer to use another kvm_set_msrs. And the stack space is limited= > too. It's as safe as assuming 100 entries is enough - with or without the base entries (which are only 12 if I can count correctly). Unless you can exclude that the number of banks will make things blow up, add a check. And I don't see a need for two runs yet. >=20 >>> + set_msr_entry(&msrs[n++], MSR_MC0_CTL + i, env->mce_bank= s[i]); >>> + rc =3D kvm_set_msrs(env, msrs, n); >>> + if (rc =3D=3D -1) >>> + perror("kvm_set_msrs FAILED"); >>> + } >>> +#endif >>> + >>> if (level >=3D KVM_PUT_RESET_STATE) { >>> kvm_arch_load_mpstate(env); >>> kvm_load_lapic(env); >>> @@ -1155,6 +1177,27 @@ void kvm_arch_save_regs(CPUState *env) >>> return; >>> } >>> } >>> + >>> +#ifdef KVM_CAP_MCE >>> + if (env->mcg_cap) { >> No need to check for msg_cap, the kernel will ignore unknown MSRs. >=20 > And some error message will be printed in kernel log. Is it OK? Nope, good point. >=20 >>> + msrs[0].index =3D MSR_MCG_STATUS; >>> + msrs[1].index =3D MSR_MCG_CTL; >>> + n =3D (env->mcg_cap & 0xff) * 4; >>> + for (i =3D 0; i < n; i++) >> Same are above, we may run out of array space. >> >>> + msrs[2 + i].index =3D MSR_MC0_CTL + i; >>> + >>> + rc =3D kvm_get_msrs(env, msrs, n + 2); >>> + if (rc =3D=3D -1) >>> + perror("kvm_get_msrs FAILED"); >>> + else { >>> + env->mcg_status =3D msrs[0].data; >>> + env->mcg_ctl =3D msrs[1].data; >>> + for (i =3D 0; i < n; i++) >>> + env->mce_banks[i] =3D msrs[2 + i].data; >>> + } >> Please split this block into setup and MSR transfer, and then merge it= >> into the existing MSR readout to avoid calling kvm_get_msrs twice. >=20 > I will do that if the stack space is not an issue. >=20 As I said, the space issue need to be address independent of the question one vs. two runs. When there are really KBytes required, go for qemu_malloc. Jan --------------enigE8DF6B140A0D4190C7D6E67C 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.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkuM5lkACgkQitSsb3rl5xRH/gCgomROqDceFVDbp2b+EtGZmLZJ +NgAoOrjcOfqmpZUNMmCgEo9eiBN1VMX =vMTl -----END PGP SIGNATURE----- --------------enigE8DF6B140A0D4190C7D6E67C--