[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Blue Swirl]

On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> On a real hardware changing read-only bits has no effect
> Use a mask common for SCSI and Ethernet registers. The crucial
> bit is DMA_INTR, because setting or clearing it may produce
> spurious interrupts.
>
> This patch allows booting Solaris 2.3

Great!

> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  hw/sparc32_dma.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
> index 3ceb851..d54e165 100644
> --- a/hw/sparc32_dma.c
> +++ b/hw/sparc32_dma.c
> @@ -62,6 +62,9 @@
>  #define DMA_DRAIN_FIFO 0x40
>  #define DMA_RESET 0x80
>
> +/* XXX SCSI and ethernet should have different read-only bit masks */
> +#define DMA_CSR_RO_MASK 0xfe000007

I'm preparing (again) some generic DMA patches, it looks like I have
to make Lance and ESP DMA controllers separate. Your patch highlights
yet another problem with the current shared design. This part of the
patch is fine.

> +
>  typedef struct DMAState DMAState;
>
>  struct DMAState {
> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>     switch (saddr) {
>     case 0:
>         if (val & DMA_INTREN) {
> -            if (val & DMA_INTR) {
> +            if (s->dmaregs[0] & DMA_INTR) {

Doesn't this change the way irqs are raised so that a pending irq is
only generated on the write access _after_ the access that enables the
irq. Currently we check for pending irqs immediately when the irq is
enabled.

>                 DPRINTF("Raise IRQ\n");
>                 qemu_irq_raise(s->irq);
>             }
> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>             val &= ~DMA_DRAIN_FIFO;
>         } else if (val == 0)
>             val = DMA_DRAIN_FIFO;
> -        val &= 0x0fffffff;
> +        val &= ~DMA_CSR_RO_MASK;
>         val |= DMA_VER;
> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>         break;
>     case 1:
>         s->dmaregs[0] |= DMA_LOADED;
> -        break;

A comment about fall through should be added.

>     default:
> +        s->dmaregs[saddr] = val;
>         break;
>     }
> -    s->dmaregs[saddr] = val;
>  }
>
>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
> --
> 1.6.2.5
>
>

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Artyom Tarasenko]

2010/5/22 Blue Swirl <blauwirbel@gmail.com>:
> On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> On a real hardware changing read-only bits has no effect
>> Use a mask common for SCSI and Ethernet registers. The crucial
>> bit is DMA_INTR, because setting or clearing it may produce
>> spurious interrupts.
>>
>> This patch allows booting Solaris 2.3
>
> Great!
>
>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>> ---
>>  hw/sparc32_dma.c |   11 +++++++----
>>  1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
>> index 3ceb851..d54e165 100644
>> --- a/hw/sparc32_dma.c
>> +++ b/hw/sparc32_dma.c
>> @@ -62,6 +62,9 @@
>>  #define DMA_DRAIN_FIFO 0x40
>>  #define DMA_RESET 0x80
>>
>> +/* XXX SCSI and ethernet should have different read-only bit masks */
>> +#define DMA_CSR_RO_MASK 0xfe000007
>
> I'm preparing (again) some generic DMA patches, it looks like I have
> to make Lance and ESP DMA controllers separate.

Good idea! They are too different. And also if we remember that there
is a parallel port dma too...
Are you splitting them just to improve the design, or are you adding
some features too?
A Test CSR register in the Lance would be great: it would allow
network boot with OBP (which is the default when it's used with qemu)
and hence automated Solaris boot tests.

> Your patch highlights
> yet another problem with the current shared design. This part of the
> patch is fine.
>
>> +
>>  typedef struct DMAState DMAState;
>>
>>  struct DMAState {
>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>     switch (saddr) {
>>     case 0:
>>         if (val & DMA_INTREN) {
>> -            if (val & DMA_INTR) {
>> +            if (s->dmaregs[0] & DMA_INTR) {
>
> Doesn't this change the way irqs are raised so that a pending irq is
> only generated on the write access _after_ the access that enables the
> irq. Currently we check for pending irqs immediately when the irq is
> enabled.

No, we still check for _pending_ irqs immediately, but don't allow
making a spurious interrupt by writing 1 to the DMA_INTR bit.

And frankly speaking I don't think timing can be a problem here:  the
real hardware would have some latency too.

Unless you have a test case which I broke...

>>                 DPRINTF("Raise IRQ\n");
>>                 qemu_irq_raise(s->irq);
>>             }
>> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>             val &= ~DMA_DRAIN_FIFO;
>>         } else if (val == 0)
>>             val = DMA_DRAIN_FIFO;
>> -        val &= 0x0fffffff;
>> +        val &= ~DMA_CSR_RO_MASK;
>>         val |= DMA_VER;
>> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>>         break;
>>     case 1:
>>         s->dmaregs[0] |= DMA_LOADED;
>> -        break;
>
> A comment about fall through should be added.

ok.

>>     default:
>> +        s->dmaregs[saddr] = val;
>>         break;
>>     }
>> -    s->dmaregs[saddr] = val;
>>  }
>>
>>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
>> --
>> 1.6.2.5
>>
>>
>



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Qemu-devel] [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Artyom Tarasenko]

On a real hardware changing read-only bits has no effect
Use a mask common for SCSI and Ethernet registers. The crucial
bit is DMA_INTR, because setting or clearing it may produce
spurious interrupts.

This patch allows booting Solaris 2.3
---
 hw/sparc32_dma.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
index 3ceb851..b521707 100644
--- a/hw/sparc32_dma.c
+++ b/hw/sparc32_dma.c
@@ -62,6 +62,9 @@
 #define DMA_DRAIN_FIFO 0x40
 #define DMA_RESET 0x80
 
+/* XXX SCSI and ethernet should have different read-only bit masks */
+#define DMA_CSR_RO_MASK 0xfe000007
+
 typedef struct DMAState DMAState;
 
 struct DMAState {
@@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
     switch (saddr) {
     case 0:
         if (val & DMA_INTREN) {
-            if (val & DMA_INTR) {
+            if (s->dmaregs[0] & DMA_INTR) {
                 DPRINTF("Raise IRQ\n");
                 qemu_irq_raise(s->irq);
             }
@@ -204,16 +207,17 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
             val &= ~DMA_DRAIN_FIFO;
         } else if (val == 0)
             val = DMA_DRAIN_FIFO;
-        val &= 0x0fffffff;
+        val &= ~DMA_CSR_RO_MASK;
         val |= DMA_VER;
+        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
         break;
     case 1:
         s->dmaregs[0] |= DMA_LOADED;
-        break;
+        /* fall through */
     default:
+        s->dmaregs[saddr] = val;
         break;
     }
-    s->dmaregs[saddr] = val;
 }
 
 static CPUReadMemoryFunc * const dma_mem_read[3] = {
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Blue Swirl]

Thanks, applied. You forgot SoB-line, I copied it from the previous version.

On Sat, May 22, 2010 at 8:38 AM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> On a real hardware changing read-only bits has no effect
> Use a mask common for SCSI and Ethernet registers. The crucial
> bit is DMA_INTR, because setting or clearing it may produce
> spurious interrupts.
>
> This patch allows booting Solaris 2.3
> ---
>  hw/sparc32_dma.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
> index 3ceb851..b521707 100644
> --- a/hw/sparc32_dma.c
> +++ b/hw/sparc32_dma.c
> @@ -62,6 +62,9 @@
>  #define DMA_DRAIN_FIFO 0x40
>  #define DMA_RESET 0x80
>
> +/* XXX SCSI and ethernet should have different read-only bit masks */
> +#define DMA_CSR_RO_MASK 0xfe000007
> +
>  typedef struct DMAState DMAState;
>
>  struct DMAState {
> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>     switch (saddr) {
>     case 0:
>         if (val & DMA_INTREN) {
> -            if (val & DMA_INTR) {
> +            if (s->dmaregs[0] & DMA_INTR) {
>                 DPRINTF("Raise IRQ\n");
>                 qemu_irq_raise(s->irq);
>             }
> @@ -204,16 +207,17 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>             val &= ~DMA_DRAIN_FIFO;
>         } else if (val == 0)
>             val = DMA_DRAIN_FIFO;
> -        val &= 0x0fffffff;
> +        val &= ~DMA_CSR_RO_MASK;
>         val |= DMA_VER;
> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>         break;
>     case 1:
>         s->dmaregs[0] |= DMA_LOADED;
> -        break;
> +        /* fall through */
>     default:
> +        s->dmaregs[saddr] = val;
>         break;
>     }
> -    s->dmaregs[saddr] = val;
>  }
>
>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
> --
> 1.6.2.5
>
>

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Blue Swirl]

On Sat, May 22, 2010 at 8:32 AM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2010/5/22 Blue Swirl <blauwirbel@gmail.com>:
>> On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>> On a real hardware changing read-only bits has no effect
>>> Use a mask common for SCSI and Ethernet registers. The crucial
>>> bit is DMA_INTR, because setting or clearing it may produce
>>> spurious interrupts.
>>>
>>> This patch allows booting Solaris 2.3
>>
>> Great!
>>
>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>>> ---
>>>  hw/sparc32_dma.c |   11 +++++++----
>>>  1 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
>>> index 3ceb851..d54e165 100644
>>> --- a/hw/sparc32_dma.c
>>> +++ b/hw/sparc32_dma.c
>>> @@ -62,6 +62,9 @@
>>>  #define DMA_DRAIN_FIFO 0x40
>>>  #define DMA_RESET 0x80
>>>
>>> +/* XXX SCSI and ethernet should have different read-only bit masks */
>>> +#define DMA_CSR_RO_MASK 0xfe000007
>>
>> I'm preparing (again) some generic DMA patches, it looks like I have
>> to make Lance and ESP DMA controllers separate.
>
> Good idea! They are too different. And also if we remember that there
> is a parallel port dma too...

Also cs4231.

> Are you splitting them just to improve the design, or are you adding
> some features too?

No new features, it's just needed by the overall design.

> A Test CSR register in the Lance would be great: it would allow
> network boot with OBP (which is the default when it's used with qemu)
> and hence automated Solaris boot tests.
>
>> Your patch highlights
>> yet another problem with the current shared design. This part of the
>> patch is fine.
>>
>>> +
>>>  typedef struct DMAState DMAState;
>>>
>>>  struct DMAState {
>>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>>     switch (saddr) {
>>>     case 0:
>>>         if (val & DMA_INTREN) {
>>> -            if (val & DMA_INTR) {
>>> +            if (s->dmaregs[0] & DMA_INTR) {
>>
>> Doesn't this change the way irqs are raised so that a pending irq is
>> only generated on the write access _after_ the access that enables the
>> irq. Currently we check for pending irqs immediately when the irq is
>> enabled.
>
> No, we still check for _pending_ irqs immediately, but don't allow
> making a spurious interrupt by writing 1 to the DMA_INTR bit.
>
> And frankly speaking I don't think timing can be a problem here:  the
> real hardware would have some latency too.
>
> Unless you have a test case which I broke...
>
>>>                 DPRINTF("Raise IRQ\n");
>>>                 qemu_irq_raise(s->irq);
>>>             }
>>> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>>             val &= ~DMA_DRAIN_FIFO;
>>>         } else if (val == 0)
>>>             val = DMA_DRAIN_FIFO;
>>> -        val &= 0x0fffffff;
>>> +        val &= ~DMA_CSR_RO_MASK;
>>>         val |= DMA_VER;
>>> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>>>         break;
>>>     case 1:
>>>         s->dmaregs[0] |= DMA_LOADED;
>>> -        break;
>>
>> A comment about fall through should be added.
>
> ok.
>
>>>     default:
>>> +        s->dmaregs[saddr] = val;
>>>         break;
>>>     }
>>> -    s->dmaregs[saddr] = val;
>>>  }
>>>
>>>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
>>> --
>>> 1.6.2.5
>>>
>>>
>>
>
>
>
> --
> Regards,
> Artyom Tarasenko
>
> solaris/sparc under qemu blog: http://tyom.blogspot.com/
>

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Artyom Tarasenko]

2010/5/22 Blue Swirl <blauwirbel@gmail.com>:
> Thanks, applied. You forgot SoB-line, I copied it from the previous version.

Sorry. Btw, is there a way to tell 'format-patch' to always include it?
Can't find it in the git docs. Otherwise I'll define an alias so I won't
need to remember about the '-s' switch.

> On Sat, May 22, 2010 at 8:38 AM, Artyom Tarasenko
> <atar4qemu@googlemail.com> wrote:
>> On a real hardware changing read-only bits has no effect
>> Use a mask common for SCSI and Ethernet registers. The crucial
>> bit is DMA_INTR, because setting or clearing it may produce
>> spurious interrupts.
>>
>> This patch allows booting Solaris 2.3
>> ---
>>  hw/sparc32_dma.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
>> index 3ceb851..b521707 100644
>> --- a/hw/sparc32_dma.c
>> +++ b/hw/sparc32_dma.c
>> @@ -62,6 +62,9 @@
>>  #define DMA_DRAIN_FIFO 0x40
>>  #define DMA_RESET 0x80
>>
>> +/* XXX SCSI and ethernet should have different read-only bit masks */
>> +#define DMA_CSR_RO_MASK 0xfe000007
>> +
>>  typedef struct DMAState DMAState;
>>
>>  struct DMAState {
>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>     switch (saddr) {
>>     case 0:
>>         if (val & DMA_INTREN) {
>> -            if (val & DMA_INTR) {
>> +            if (s->dmaregs[0] & DMA_INTR) {
>>                 DPRINTF("Raise IRQ\n");
>>                 qemu_irq_raise(s->irq);
>>             }
>> @@ -204,16 +207,17 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>             val &= ~DMA_DRAIN_FIFO;
>>         } else if (val == 0)
>>             val = DMA_DRAIN_FIFO;
>> -        val &= 0x0fffffff;
>> +        val &= ~DMA_CSR_RO_MASK;
>>         val |= DMA_VER;
>> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>>         break;
>>     case 1:
>>         s->dmaregs[0] |= DMA_LOADED;
>> -        break;
>> +        /* fall through */
>>     default:
>> +        s->dmaregs[saddr] = val;
>>         break;
>>     }
>> -    s->dmaregs[saddr] = val;
>>  }
>>
>>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
>> --
>> 1.6.2.5
>>
>>
>



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers

[Blue Swirl]

On Sat, May 22, 2010 at 9:29 AM, Artyom Tarasenko
<atar4qemu@googlemail.com> wrote:
> 2010/5/22 Blue Swirl <blauwirbel@gmail.com>:
>> Thanks, applied. You forgot SoB-line, I copied it from the previous version.
>
> Sorry. Btw, is there a way to tell 'format-patch' to always include it?
> Can't find it in the git docs. Otherwise I'll define an alias so I won't
> need to remember about the '-s' switch.

[format]
        thread = true
        signoff = true

Though format.thread (adds reference headers to the messages, so
patches reference the cover letter) does not seem to work with git
1.6.2.4.

>> On Sat, May 22, 2010 at 8:38 AM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>> On a real hardware changing read-only bits has no effect
>>> Use a mask common for SCSI and Ethernet registers. The crucial
>>> bit is DMA_INTR, because setting or clearing it may produce
>>> spurious interrupts.
>>>
>>> This patch allows booting Solaris 2.3
>>> ---
>>>  hw/sparc32_dma.c |   12 ++++++++----
>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/sparc32_dma.c b/hw/sparc32_dma.c
>>> index 3ceb851..b521707 100644
>>> --- a/hw/sparc32_dma.c
>>> +++ b/hw/sparc32_dma.c
>>> @@ -62,6 +62,9 @@
>>>  #define DMA_DRAIN_FIFO 0x40
>>>  #define DMA_RESET 0x80
>>>
>>> +/* XXX SCSI and ethernet should have different read-only bit masks */
>>> +#define DMA_CSR_RO_MASK 0xfe000007
>>> +
>>>  typedef struct DMAState DMAState;
>>>
>>>  struct DMAState {
>>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>>     switch (saddr) {
>>>     case 0:
>>>         if (val & DMA_INTREN) {
>>> -            if (val & DMA_INTR) {
>>> +            if (s->dmaregs[0] & DMA_INTR) {
>>>                 DPRINTF("Raise IRQ\n");
>>>                 qemu_irq_raise(s->irq);
>>>             }
>>> @@ -204,16 +207,17 @@ static void dma_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>>>             val &= ~DMA_DRAIN_FIFO;
>>>         } else if (val == 0)
>>>             val = DMA_DRAIN_FIFO;
>>> -        val &= 0x0fffffff;
>>> +        val &= ~DMA_CSR_RO_MASK;
>>>         val |= DMA_VER;
>>> +        s->dmaregs[0] = (s->dmaregs[0] & DMA_CSR_RO_MASK) | val;
>>>         break;
>>>     case 1:
>>>         s->dmaregs[0] |= DMA_LOADED;
>>> -        break;
>>> +        /* fall through */
>>>     default:
>>> +        s->dmaregs[saddr] = val;
>>>         break;
>>>     }
>>> -    s->dmaregs[saddr] = val;
>>>  }
>>>
>>>  static CPUReadMemoryFunc * const dma_mem_read[3] = {
>>> --
>>> 1.6.2.5
>>>
>>>
>>
>
>
>
> --
> Regards,
> Artyom Tarasenko
>
> solaris/sparc under qemu blog: http://tyom.blogspot.com/
>