* Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:05 ` [Qemu-devel] " Jan Kiszka
@ 2010-06-14 17:10 ` Michal Novotny
2010-06-14 17:11 ` [Qemu-devel] [PATCH v2] " Paolo Bonzini
2010-06-14 17:14 ` [Qemu-devel] Re: [PATCH] " Michal Novotny
2 siblings, 0 replies; 9+ messages in thread
From: Michal Novotny @ 2010-06-14 17:10 UTC (permalink / raw)
To: qemu-devel
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 readable since something goes wrong it could
be easier to just put dest to DPRINTF() macro, like:
DPRINTF("Data phase mismatch jump to %08x (== pmjad%d)\n", s->dsp, dest);
rather than implementing it some other way.
Michal
--
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:05 ` [Qemu-devel] " Jan Kiszka
2010-06-14 17:10 ` Michal Novotny
@ 2010-06-14 17:11 ` Paolo Bonzini
2010-06-25 8:02 ` [Qemu-devel] " Paolo Bonzini
2010-06-29 21:11 ` [Qemu-devel] " Aurelien Jarno
2010-06-14 17:14 ` [Qemu-devel] Re: [PATCH] " Michal Novotny
2 siblings, 2 replies; 9+ messages in thread
From: Paolo Bonzini @ 2010-06-14 17:11 UTC (permalink / raw)
To: qemu-devel
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>
---
> Looks correct. But why not assigning s->pmjad[12] directly? Would
> improve readability IMO.
No particular reason, hence fine by me.
hw/lsi53c895a.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f5a91ba..9a37fed 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -490,10 +490,10 @@ 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;
+ if ((s->ccntl0 & LSI_CCNTL0_PMJCTL)) {
+ s->dsp = out ? s->pmjad1 : s->pmjad2;
} else {
- s->dsp = s->pmjad2;
+ s->dsp = (s->scntl2 & LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1);
}
DPRINTF("Data phase mismatch jump to %08x\n", s->dsp);
} else {
--
1.7.0.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] Re: [PATCH v2] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:11 ` [Qemu-devel] [PATCH v2] " Paolo Bonzini
@ 2010-06-25 8:02 ` Paolo Bonzini
2010-06-29 21:11 ` [Qemu-devel] " Aurelien Jarno
1 sibling, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2010-06-25 8:02 UTC (permalink / raw)
To: qemu-devel
On 06/14/2010 07:11 PM, 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>
> ---
> > Looks correct. But why not assigning s->pmjad[12] directly? Would
> > improve readability IMO.
>
> No particular reason, hence fine by me.
>
> hw/lsi53c895a.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index f5a91ba..9a37fed 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -490,10 +490,10 @@ 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;
> + if ((s->ccntl0& LSI_CCNTL0_PMJCTL)) {
> + s->dsp = out ? s->pmjad1 : s->pmjad2;
> } else {
> - s->dsp = s->pmjad2;
> + s->dsp = (s->scntl2& LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1);
> }
> DPRINTF("Data phase mismatch jump to %08x\n", s->dsp);
> } else {
PING
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v2] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:11 ` [Qemu-devel] [PATCH v2] " Paolo Bonzini
2010-06-25 8:02 ` [Qemu-devel] " Paolo Bonzini
@ 2010-06-29 21:11 ` Aurelien Jarno
1 sibling, 0 replies; 9+ messages in thread
From: Aurelien Jarno @ 2010-06-29 21:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Mon, Jun 14, 2010 at 07:11:54PM +0200, 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>
> ---
> > Looks correct. But why not assigning s->pmjad[12] directly? Would
> > improve readability IMO.
>
> No particular reason, hence fine by me.
Thanks, applied.
> hw/lsi53c895a.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index f5a91ba..9a37fed 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -490,10 +490,10 @@ 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;
> + if ((s->ccntl0 & LSI_CCNTL0_PMJCTL)) {
> + s->dsp = out ? s->pmjad1 : s->pmjad2;
> } else {
> - s->dsp = s->pmjad2;
> + s->dsp = (s->scntl2 & LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1);
> }
> DPRINTF("Data phase mismatch jump to %08x\n", s->dsp);
> } else {
> --
> 1.7.0.1
>
>
>
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:05 ` [Qemu-devel] " Jan Kiszka
2010-06-14 17:10 ` Michal Novotny
2010-06-14 17:11 ` [Qemu-devel] [PATCH v2] " Paolo Bonzini
@ 2010-06-14 17:14 ` Michal Novotny
2010-06-14 17:31 ` Jan Kiszka
2 siblings, 1 reply; 9+ messages in thread
From: Michal Novotny @ 2010-06-14 17:14 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Paolo Bonzini, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:14 ` [Qemu-devel] Re: [PATCH] " Michal Novotny
@ 2010-06-14 17:31 ` Jan Kiszka
2010-06-14 17:34 ` Michal Novotny
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2010-06-14 17:31 UTC (permalink / raw)
To: Michal Novotny; +Cc: Paolo Bonzini, qemu-devel@nongnu.org
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
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] Re: [PATCH] lsi53c895a: fix Phase Mismatch Jump
2010-06-14 17:31 ` Jan Kiszka
@ 2010-06-14 17:34 ` Michal Novotny
0 siblings, 0 replies; 9+ messages in thread
From: Michal Novotny @ 2010-06-14 17:34 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Paolo Bonzini, qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread