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

[Aurelien Jarno <aurelien@aurel32.net>]

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
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net