qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump


From: Michal Novotny
Subject: Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump
Date: Mon, 14 Jun 2010 19:34:20 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-3.fc13 Thunderbird/3.0.4

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<address@hidden>
---
   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<address@hidden>, RHCE
Virtualization Team (xen userspace), Red Hat




reply via email to

[Prev in Thread] Current Thread [Next in Thread]