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

[Jan Kiszka]

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

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