From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60509 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P25Xp-0002UL-BC for qemu-devel@nongnu.org; Sat, 02 Oct 2010 13:06:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P25Xo-0008FM-1x for qemu-devel@nongnu.org; Sat, 02 Oct 2010 13:06:45 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:41287) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P25Xn-0008FF-SY for qemu-devel@nongnu.org; Sat, 02 Oct 2010 13:06:44 -0400 Received: by eyf18 with SMTP id 18so1868666eyf.4 for ; Sat, 02 Oct 2010 10:06:43 -0700 (PDT) Date: Sat, 2 Oct 2010 19:06:38 +0200 From: "Edgar E. Iglesias" Subject: Re: [Qemu-devel] Minor MMU fixes for PowerPC 40x emulation Message-ID: <20101002170638.GE10823@laped.lan> References: <4CA6C549.9090801@runbox.com> <6569D071-7989-4426-9E30-7B5E80C79027@suse.de> <4CA762B1.7060505@runbox.com> <0E0E4F59-0325-44C5-9330-EB48B8A8C46F@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0E0E4F59-0325-44C5-9330-EB48B8A8C46F@suse.de> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU Developers , John Clark On Sat, Oct 02, 2010 at 06:55:36PM +0200, Alexander Graf wrote: > > On 02.10.2010, at 18:49, John Clark wrote: > > >>> /* Check from TLB entry */ > >>> - /* XXX: there is a problem here or in the TLB fill code... */ > >>> + /* There is no longer a need to force PAGE_EXEC permission here */ > >>> + /* because of the tlb->attr fix in helper_4xx_tlbwe_lo() */ > >> > >> I guess that comment is superfluous, as readers several years from now don't care what was broken back in the day :). > > > > Yes, I suppose so :) > > > >>> @@ -3939,7 +3939,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry) > >>> tlb = &env->tlb[entry].tlbe; > >>> ret = tlb->EPN; > >>> if (tlb->prot & PAGE_VALID) > >>> - ret |= 0x400; > >>> + ret |= 0x40; /* V bit is 0x40, not 0x400 */ > >> > >> Ouch. Mind to make it a define? > > > > Sure, I was surprised that there wasn't a define for that when I found it. > > The ppc emulation code lacks a lot of defines. In fact, the same goes for x86 emulation too ;). But that doesn't mean we have to keep it that way! > > > > >>> size = booke_page_size_to_tlb(tlb->size); > >>> if (size < 0 || size > 0x7) > >>> size = 1; > >>> @@ -3948,7 +3948,7 @@ target_ulong helper_4xx_tlbre_lo (target_ulong entry) > >>> return ret; > >>> } > >>> > >>> -target_ulong helper_4xx_tlbre_hi (target_ulong entry) > >>> +target_ulong helper_4xx_tlbre_lo (target_ulong entry) > >> > >> Huh? > > > > To summarize, 'tlbre' has two forms: one to retrieve the high bits of > > a TLB entry (TLBHI), and one to retrieve the low bits (TLBLO) of a TLB > > entry. This code had the TLBLO form returning the bits corresponding > > to TLBHI and vice versa, hence the name change. You can verify this > > if you like with this IBM PowerPC 405 core user manual on page 362: > > Well the thing that strikes me as weird is mostly that you're changing a function name, but no callers to it. So is this function never used? Or was tlbre_lo defined before already and is now defined twice? Hi, Alex: I think you've missed the part of the patch that renames the _lo -> _hi. As John says, qemu had the hi/lo parts reversed when reading 4xx TLB regs. Except for the comments and the define, the patch looks good to me. John, please also add a Signed-off-by line. Cheers