From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52662 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OODYj-000492-Ci for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:35:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OODXm-0005Er-FM for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:33:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37574) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OODXm-0005Ej-89 for qemu-devel@nongnu.org; Mon, 14 Jun 2010 13:33:54 -0400 Message-ID: <4C16681C.9050000@redhat.com> Date: Mon, 14 Jun 2010 19:34:20 +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> <4C166370.2060607@redhat.com> <4C166780.20403@siemens.com> In-Reply-To: <4C166780.20403@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: Jan Kiszka Cc: Paolo Bonzini , "qemu-devel@nongnu.org" On 06/14/2010 07:31 PM, Jan Kiszka wrote: > Michal Novotny wrote: > >> 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 since if something goes wrong it could be easier >> to just put dest variable to DPRINTF() macro, like: >> >> DPRINTF("Data phase mismatch jump to %08x (== pmjad%d)\n", s->dsp, dest); >> >> rather than implementing it some other way. Now it could be easier to >> just know what the problem is - i.e. whether it's accessing the wrong >> register or now. >> > I don't mind. But if you have a use case for that separate variable, > then include it. No one can read your mind, and even less once this > patch is long merged. > > Jan > > This is not my patch, it's Paolo's but I'm just telling you I can see it useful. If it's not used in the DPRINTF() it's being optimized by gcc anyway so not a big deal ;) Michal -- Michal Novotny, RHCE Virtualization Team (xen userspace), Red Hat