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

[Michal Novotny <minovotn@redhat.com>]

On 06/14/2010 07:31 PM, Jan Kiszka wrote:
> 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<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.
>>      
> 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
>
>    
This is not my patch, it's Paolo's but I'm just telling you I can see it 
useful. If it's not used in the DPRINTF() it's being optimized by gcc 
anyway so not a big deal ;)

Michal

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