From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51486) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XolWU-0004SC-GY for qemu-devel@nongnu.org; Wed, 12 Nov 2014 22:56:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XolWT-0001Ie-2Q for qemu-devel@nongnu.org; Wed, 12 Nov 2014 22:56:42 -0500 Date: Thu, 13 Nov 2014 14:52:06 +1100 From: David Gibson Message-ID: <20141113035206.GH7291@voom.fritz.box> References: <20141105071019.26196.93729.stgit@aravindap> <20141105071315.26196.68104.stgit@aravindap> <20141111031635.GF15270@voom.redhat.com> <5461B04F.5080204@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="lrvsYIebpInmECXG" Content-Disposition: inline In-Reply-To: <5461B04F.5080204@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v3 4/4] target-ppc: Handle ibm, nmi-register RTAS call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aravinda Prasad Cc: qemu-ppc@nongnu.org, benh@au1.ibm.com, aik@au1.ibm.com, qemu-devel@nongnu.org, paulus@samba.org --lrvsYIebpInmECXG Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 11, 2014 at 12:14:31PM +0530, Aravinda Prasad wrote: > On Tuesday 11 November 2014 08:46 AM, David Gibson wrote: > > On Wed, Nov 05, 2014 at 12:43:15PM +0530, Aravinda Prasad wrote: [snip] > >> + . =3D 0x200 > >> + /* > >> + * Trampoline saves r3 in sprg2 and issues private hcall > >> + * to request qemu to build error log. QEMU builds the > >> + * error log, copies to rtas-blob and returns the address. > >> + * The initial 16 bytes in return adress consist of saved > >> + * srr0 and srr1 which we restore and pass on the actual error > >> + * log address to OS handled mcachine check notification > >> + * routine > >> + * > >> + * All the below instructions are copied to interrupt vector > >> + * 0x200 at the time of handling ibm,nmi-register rtas call. > >> + */ > >> + mtsprg 2,3 > >> + li 3,0 > >> + /* > >> + * ori r3,r3,KVMPPC_H_REPORT_MC_ERR. The KVMPPC_H_REPORT_MC_ERR > >> + * value is patched below > >> + */ > >> +1: ori 3,3,0 > >> + sc 1 /* Issue H_CALL */ > >> + cmpdi cr0,3,0 > >> + beq cr0,1b /* retry KVMPPC_H_REPORT_MC_ERR */ > >=20 > > Having to retry the hcall from here seems very awkward. This is a > > private hcall, so you can define it to do whatever retries are > > necessary internally (and I don't think your current implementation > > can fail anyway). >=20 > Retrying is required in the cases when multi-processors experience > machine check at or about the same time. As per PAPR, subsequent > processors should serialize and wait for the first processor to issue > the ibm,nmi-interlock call. The second processor retries if the first > processor which received a machine check is still reading the error log > and is yet to issue ibm,nmi-interlock call. Hmm.. ok. But I don't see any mechanism in the patches by which H_REPORT_MC_ERR will report failure if another CPU has an MC in progress. > Retrying cannot be done internally in h_report_mc_err hcall: only one > thread can succeed entering qemu upon parallel hcall and hence retrying > inside the hcall will not allow the ibm,nmi-interlock from first CPU to > succeed. It's possible, but would require some fiddling inside the h_call to unlock and wait for the other CPUs to finish, so yes, it might be more trouble than it's worth. >=20 > >=20 > >> + mtsprg 2,4 > >=20 > > Um.. doesn't this clobber the value of r3 you saved in SPRG2 just above. >=20 > The r3 saved in SPRG2 is moved to rtas area in the private hcall and > hence it is fine to clobber r3 here Ok, if you're going to do some magic register saving inside the HCALL, why not do the SRR[01] and CR restoration inside there as well. > >=20 > >> + ld 4,0(3) > >> + mtsrr0 4 /* Restore srr0 */ > >> + ld 4,8(3) > >> + mtsrr1 4 /* Restore srr1 */ > >> + ld 4,16(3) > >> + mtcrf 0,4 /* Restore cr */ > >=20 > > mtcrf? aren't you restoring the whole CR? >=20 > No. I am moving only cr0. The 0 in mtcrf 0,4 represents the cr field > mask that is replaced. Uh, yes it is. In which case a value of 0 means *no* condition register fields are transferred. >=20 > >=20 > >> + addi 3,3,24 > >> + mfsprg 4,2 > >> + /* > >> + * Branch to address registered by OS. The branch address is > >> + * patched in the ibm,nmi-register rtas call. > >> + */ > >> + ba 0x0 > >> + b . > >=20 > > The branch to self is pointless. Even if the instruction above is > > not patched, or patched incorrectly, it's a ba, so you're not likely > > to end up at the instruction underneath. >=20 > I added it to avoid speculative execution. Based on how it is used in > arch/powerpc/kernel/exceptions-64s.S Ah, I guess that makes sense, although surely any ba instruction would also have to inhibit speculative execution. > > Actually, what would probably make more sense would be to just have a > > "b ." *instead* of the ba, and have the qemu patching replace it with > > the correct ba instruction. That will limit the damage if it somehow > > gets executed without being patched. >=20 > good idea. Will do that. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --lrvsYIebpInmECXG Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUZCrmAAoJEGw4ysog2bOSj+MQAIKSAyvt0VjpsYSxzcCTr//T 0Upnin9LsWqVt41708X8Yh5fDRuXyFFy1QAYNW8D0hO04RWkMS/qWqRXdJWhRk3E srsiiQbnjtsjh5Pgcmcw0/Bhwr+DYkzVzXl+l9XdoAu9asK9xlg569qXsHZAAPlT PNTqV9Do0/bMank1WdKnLFkgy67+pDWDKGlXF4QRLkBR6H8mNYhzZjA189D0Yl8j rpg6VcIisIq9Dxi8CqSZDKUmecFlJ7zwMDWZgV0JLhYIrs3170WOwIYERmyu+gIz g463aQanC3Z1xtN3E2mxZbex7qDoFxLEwSa9qfMHiW/FkRx/DF1CAVQV2huExVVf ejAB5kvJ09qVPWONiWxWWFfXvOSZyEf9ubCGkkBEJ0eXzlgsHz8I6gxIj0UPGGu5 JSY1PqBRN27K+JVAwKGs7GcWGN2WLw/Q6qyfF0zpMePT+7G2DOuAsu8ORD8yhxcb AoUMdiMwujKphaQG/k3nI+ytlS7kbySv14I+FhctwsCEDkjWUxd7fL8CqBcHNtcT d56BwIW7i9hC72nglyZMQU8E2XrelkDjZp4gfh7vEhjBO6whn9qDCqXsmDGmOsqk /OP0nsv25t1eCSCceFo3Hrmf2gveJ7lUkNF0OmVrBMriEPxswFY4+Y/0DS2JXuW3 ngv8CVXUP8kQ5605ztAF =MqSE -----END PGP SIGNATURE----- --lrvsYIebpInmECXG--