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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux