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<pbonz...@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
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
