[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR regis
From: |
Artyom Tarasenko |
Subject: |
[Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers |
Date: |
Sat, 22 May 2010 10:32:26 +0200 |
2010/5/22 Blue Swirl <address@hidden>:
> On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko
> <address@hidden> 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 <address@hidden>
>> ---
>> 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/