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

[Michal Novotny <minovotn@redhat.com>]

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

Michal


-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat