From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53557 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pe8wA-0008Kj-AQ for qemu-devel@nongnu.org; Sat, 15 Jan 2011 11:25:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pe8w8-0000Yx-Hf for qemu-devel@nongnu.org; Sat, 15 Jan 2011 11:25:10 -0500 Received: from fmmailgate02.web.de ([217.72.192.227]:33004) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pe8w8-0000Yj-06 for qemu-devel@nongnu.org; Sat, 15 Jan 2011 11:25:08 -0500 Message-ID: <4D31CA58.6010905@web.de> Date: Sat, 15 Jan 2011 17:24:56 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <497ea69043289b0421a8265f32e9b1a80a3c9481.1294336601.git.mtosatti@redhat.com> <4D2A03A8.9080600@web.de> In-Reply-To: <4D2A03A8.9080600@web.de> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigC27BF8D2DD41C1470F8E056C" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [PATCH 04/35] Add "broadcast" option for mce command List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti , Jin Dongming Cc: Anthony Liguori , Hidetoshi Seto , qemu-devel@nongnu.org, kvm@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigC27BF8D2DD41C1470F8E056C Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Am 09.01.2011 19:51, Jan Kiszka wrote: > Am 06.01.2011 18:56, Marcelo Tosatti wrote: >> From: Jin Dongming >> >> When the following test case is injected with mce command, maybe user = could not >> get the expected result. >> DATA >> command cpu bank status mcg_status addr = misc >> (qemu) mce 1 1 0xbd00000000000000 0x05 0x1234 = 0x8c >> >> Expected Result >> panic type: "Fatal Machine check" >> >> That is because each mce command can only inject the given cpu and cou= ld not >> inject mce interrupt to other cpus. So user will get the following res= ult: >> panic type: "Fatal machine check on current CPU" >> >> "broadcast" option is used for injecting dummy data into other cpus. I= njecting >> mce with this option the expected result could be gotten. >> >> Usage: >> Broadcast[on] >> command broadcast cpu bank status mcg_status a= ddr misc >> (qemu) mce -b 1 1 0xbd00000000000000 0x05 0= x1234 0x8c >> >> Broadcast[off] >> command cpu bank status mcg_status addr misc= >> (qemu) mce 1 1 0xbd00000000000000 0x05 0x1234 0x8c= >> >> Signed-off-by: Jin Dongming >> Signed-off-by: Marcelo Tosatti >> --- >> cpu-all.h | 3 ++- >> hmp-commands.hx | 6 +++--- >> monitor.c | 7 +++++-- >> target-i386/helper.c | 20 ++++++++++++++++++-- >> target-i386/kvm.c | 16 ++++++++++++---- >> target-i386/kvm_x86.h | 5 ++++- >> 6 files changed, 44 insertions(+), 13 deletions(-) >> >> diff --git a/cpu-all.h b/cpu-all.h >> index 30ae17d..4ce4e83 100644 >> --- a/cpu-all.h >> +++ b/cpu-all.h >> @@ -964,6 +964,7 @@ int cpu_memory_rw_debug(CPUState *env, target_ulon= g addr, >> uint8_t *buf, int len, int is_write); >> =20 >> void cpu_inject_x86_mce(CPUState *cenv, int bank, uint64_t status, >> - uint64_t mcg_status, uint64_t addr, uint64_t = misc); >> + uint64_t mcg_status, uint64_t addr, uint64_t = misc, >> + int broadcast); >> =20 >> #endif /* CPU_ALL_H */ >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index df134f8..c82fb10 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1091,9 +1091,9 @@ ETEXI >> =20 >> { >> .name =3D "mce", >> - .args_type =3D "cpu_index:i,bank:i,status:l,mcg_status:l,add= r:l,misc:l", >> - .params =3D "cpu bank status mcgstatus addr misc", >> - .help =3D "inject a MCE on the given CPU", >> + .args_type =3D "broadcast:-b,cpu_index:i,bank:i,status:l,mcg= _status:l,addr:l,misc:l", >> + .params =3D "[-b] cpu bank status mcgstatus addr misc", >> + .help =3D "inject a MCE on the given CPU [and broadcast= to other CPUs with -b option]", >> .mhandler.cmd =3D do_inject_mce, >> }, >> =20 >> diff --git a/monitor.c b/monitor.c >> index f258000..f4f624b 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -2671,12 +2671,15 @@ static void do_inject_mce(Monitor *mon, const = QDict *qdict) >> uint64_t mcg_status =3D qdict_get_int(qdict, "mcg_status"); >> uint64_t addr =3D qdict_get_int(qdict, "addr"); >> uint64_t misc =3D qdict_get_int(qdict, "misc"); >> + int broadcast =3D qdict_get_try_bool(qdict, "broadcast", 0); >> =20 >> - for (cenv =3D first_cpu; cenv !=3D NULL; cenv =3D cenv->next_cpu)= >> + for (cenv =3D first_cpu; cenv !=3D NULL; cenv =3D cenv->next_cpu)= { >> if (cenv->cpu_index =3D=3D cpu_index && cenv->mcg_cap) { >> - cpu_inject_x86_mce(cenv, bank, status, mcg_status, addr, = misc); >> + cpu_inject_x86_mce(cenv, bank, status, mcg_status, addr, = misc, >> + broadcast); >> break; >> } >> + } >> } >> #endif >> =20 >> diff --git a/target-i386/helper.c b/target-i386/helper.c >> index 2c94130..2cfb4a4 100644 >> --- a/target-i386/helper.c >> +++ b/target-i386/helper.c >> @@ -1069,18 +1069,34 @@ static void qemu_inject_x86_mce(CPUState *cenv= , int bank, uint64_t status, >> } >> =20 >> void cpu_inject_x86_mce(CPUState *cenv, int bank, uint64_t status, >> - uint64_t mcg_status, uint64_t addr, uint64_t = misc) >> + uint64_t mcg_status, uint64_t addr, uint64_t = misc, >> + int broadcast) >> { >> unsigned bank_num =3D cenv->mcg_cap & 0xff; >> + CPUState *env; >> + int flag =3D 0; >> =20 >> if (bank >=3D bank_num || !(status & MCI_STATUS_VAL)) { >> return; >> } >> =20 >> if (kvm_enabled()) { >> - kvm_inject_x86_mce(cenv, bank, status, mcg_status, addr, misc= , 0); >> + if (broadcast) { >> + flag |=3D MCE_BROADCAST; >> + } >> + >> + kvm_inject_x86_mce(cenv, bank, status, mcg_status, addr, misc= , flag); >> } else { >> qemu_inject_x86_mce(cenv, bank, status, mcg_status, addr, mis= c); >> + if (broadcast) { >> + for (env =3D first_cpu; env !=3D NULL; env =3D env->next_= cpu) { >> + if (cenv =3D=3D env) { >> + continue; >> + } >> + >> + qemu_inject_x86_mce(env, 1, 0xa000000000000000, 0, 0,= 0); >=20 > Constant lacks "ULL". Can probably be fixed up on commit. Actually, the right fix is MCI_STATUS_VAL | MCI_STATUS_UC instead of the magic number. Still, there is an inconsistency: kvm_mce_broadcast_rest injects mcg_state =3D MCG_STATUS_MCIP | MCG_STATUS_RIPV, the above code sets it t= o 0. I presume the KVM code is correct, isn't it? This demonstrates one problem of the MCE code base: It contains too much redundancy between TCG and KVM paths. And as the KVM part is also not yet well integrated with VCPU state writeback, e.g. VCPU events, we have some races over there. The good news is that we can drop half of the KVM MCE bits when reusing qemu_inject_x86_mce for setting up the event injection. How to proceed? Fix up those nits, merge the patches, and then rework MCE on top (I started like this already)? Or rather do the rework on top of current qemu? I'm willing to drive this, but I would also welcome if we could distribute the effort. Jan --------------enigC27BF8D2DD41C1470F8E056C 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/ iEYEARECAAYFAk0xymAACgkQitSsb3rl5xQzxgCg5xWZupnJIQYexp0hZ2kjLcJx 6oQAoNNMLTghCUJS3VGfIa7g5rrzEPmY =VusP -----END PGP SIGNATURE----- --------------enigC27BF8D2DD41C1470F8E056C--