| Message ID | 1276535514-19724-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/14/2010 07:11 PM, 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<pbonzini@redhat.com> > --- > > Looks correct. But why not assigning s->pmjad[12] directly? Would > > improve readability IMO. > > No particular reason, hence fine by me. > > hw/lsi53c895a.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c > index f5a91ba..9a37fed 100644 > --- a/hw/lsi53c895a.c > +++ b/hw/lsi53c895a.c > @@ -490,10 +490,10 @@ 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; > + if ((s->ccntl0& LSI_CCNTL0_PMJCTL)) { > + s->dsp = out ? s->pmjad1 : s->pmjad2; > } else { > - s->dsp = s->pmjad2; > + s->dsp = (s->scntl2& LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1); > } > DPRINTF("Data phase mismatch jump to %08x\n", s->dsp); > } else { PING Paolo
On Mon, Jun 14, 2010 at 07:11:54PM +0200, 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 <pbonzini@redhat.com> > --- > > Looks correct. But why not assigning s->pmjad[12] directly? Would > > improve readability IMO. > > No particular reason, hence fine by me. Thanks, applied. > hw/lsi53c895a.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c > index f5a91ba..9a37fed 100644 > --- a/hw/lsi53c895a.c > +++ b/hw/lsi53c895a.c > @@ -490,10 +490,10 @@ 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; > + if ((s->ccntl0 & LSI_CCNTL0_PMJCTL)) { > + s->dsp = out ? s->pmjad1 : s->pmjad2; > } else { > - s->dsp = s->pmjad2; > + s->dsp = (s->scntl2 & LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1); > } > DPRINTF("Data phase mismatch jump to %08x\n", s->dsp); > } else { > -- > 1.7.0.1 > > >
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c index f5a91ba..9a37fed 100644 --- a/hw/lsi53c895a.c +++ b/hw/lsi53c895a.c @@ -490,10 +490,10 @@ 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; + if ((s->ccntl0 & LSI_CCNTL0_PMJCTL)) { + s->dsp = out ? s->pmjad1 : s->pmjad2; } else { - s->dsp = s->pmjad2; + s->dsp = (s->scntl2 & LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1); } DPRINTF("Data phase mismatch jump to %08x\n", s->dsp); } else {
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 <pbonzini@redhat.com> --- > Looks correct. But why not assigning s->pmjad[12] directly? Would > improve readability IMO. No particular reason, hence fine by me. hw/lsi53c895a.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)