From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54966 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OODPs-00051j-EZ for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:25:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OODAs-0001TK-Kc for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:10:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45483) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OODAs-0001Sz-Cj for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:10:14 -0400 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5EHAAMa027738 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 14 Jun 2010 13:10:11 -0400 Received: from mig-laptop.brq.redhat.com (mig-laptop.brq.redhat.com [10.34.28.133]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5EHA9DX017869 for ; Mon, 14 Jun 2010 13:10:10 -0400 Message-ID: <4C16628D.7070904@redhat.com> Date: Mon, 14 Jun 2010 19:10:37 +0200 From: Michal Novotny MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump References: <1276533689-16293-1-git-send-email-pbonzini@redhat.com> <4C166149.2010909@siemens.com> In-Reply-To: <4C166149.2010909@siemens.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On 06/14/2010 07:05 PM, Jan Kiszka wrote: > Paolo Bonzini wrote: > >> lsi_bad_phase has a bug in the choice of pmjad1/pmjad2. This does >> not matter with Linux guests because it uses just one routine for >> both, but it breaks Windows 64-bit guests. This is the text >> from the spec: >> >> "[The PMJCTL] bit controls which decision mechanism is used >> when jumping on phase mismatch. When this bit is cleared the >> LSI53C895A will use Phase Mismatch Jump Address 1 (PMJAD1) when >> the WSR bit is cleared and Phase Mismatch Jump Address 2 (PMJAD2) >> when the WSR bit is set. When this bit is set the LSI53C895A will >> use jump address one (PMJAD1) on data out (data out, command, >> message out) transfers and jump address two (PMJAD2) on data in >> (data in, status, message in) transfers." >> >> Which means: >> >> CCNTL0.PMJCTL >> 0 SCNTL2.WSR = 0 PMJAD1 >> 0 SCNTL2.WSR = 1 PMJAD2 >> 1 out PMJAD1 >> 1 in PMJAD2 >> >> In qemu, what you get instead is: >> >> CCNTL0.PMJCTL >> 0 out PMJAD1 >> 0 in PMJAD2<<<<< >> 1 out PMJAD1 >> 1 in PMJAD1<<<<< >> >> Considering that qemu always has SCNTL2.WSR cleared, the two marked cases >> (corresponding to phase mismatch on input) are always jumping to the >> wrong PMJAD register. The patch implements the correct semantics. >> >> Signed-off-by: Paolo Bonzini >> --- >> hw/lsi53c895a.c | 12 +++++++++--- >> 1 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c >> index f5a91ba..00df2bd 100644 >> --- a/hw/lsi53c895a.c >> +++ b/hw/lsi53c895a.c >> @@ -490,11 +490,14 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase) >> { >> /* Trigger a phase mismatch. */ >> if (s->ccntl0& LSI_CCNTL0_ENPMJ) { >> - if ((s->ccntl0& LSI_CCNTL0_PMJCTL) || out) { >> - s->dsp = s->pmjad1; >> + int dest; >> + if ((s->ccntl0& LSI_CCNTL0_PMJCTL)) { >> + dest = out ? 1 : 2; >> } else { >> - s->dsp = s->pmjad2; >> + dest = (s->scntl2& LSI_SCNTL2_WSR ? 2 : 1); >> } >> + >> + s->dsp = (dest == 1) ? s->pmjad1 : s->pmjad2; >> DPRINTF("Data phase mismatch jump to %08x\n", s->dsp); >> } else { >> DPRINTF("Phase mismatch interrupt\n"); >> > Looks correct. But why not assigning s->pmjad[12] directly? Would > improve readability IMO. > > Jan > > Jan, I think this is better readable since something goes wrong it could be easier to just put dest to DPRINTF() macro, like: DPRINTF("Data phase mismatch jump to %08x (== pmjad%d)\n", s->dsp, dest); rather than implementing it some other way. Michal -- Michal Novotny, RHCE Virtualization Team (xen userspace), Red Hat