From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41396 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OFk8Y-0003L0-DR for qemu-devel@nongnu.org; Sat, 22 May 2010 04:32:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OFk8V-00027q-D2 for qemu-devel@nongnu.org; Sat, 22 May 2010 04:32:49 -0400 Received: from mail-ww0-f45.google.com ([74.125.82.45]:62086) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OFk8V-00027f-8n for qemu-devel@nongnu.org; Sat, 22 May 2010 04:32:47 -0400 Received: by wwb39 with SMTP id 39so700683wwb.4 for ; Sat, 22 May 2010 01:32:46 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1274478829-10840-1-git-send-email-atar4qemu@gmail.com> From: Artyom Tarasenko Date: Sat, 22 May 2010 10:32:26 +0200 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH] sparc32 protect read-only bits in DMA CSR registers List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel@nongnu.org 2010/5/22 Blue Swirl : > On Fri, May 21, 2010 at 9:53 PM, Artyom Tarasenko > 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 >> --- >> =A0hw/sparc32_dma.c | =A0 11 +++++++---- >> =A01 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 @@ >> =A0#define DMA_DRAIN_FIFO 0x40 >> =A0#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. > >> + >> =A0typedef struct DMAState DMAState; >> >> =A0struct DMAState { >> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phys= _addr_t addr, uint32_t val) >> =A0 =A0 switch (saddr) { >> =A0 =A0 case 0: >> =A0 =A0 =A0 =A0 if (val & DMA_INTREN) { >> - =A0 =A0 =A0 =A0 =A0 =A0if (val & DMA_INTR) { >> + =A0 =A0 =A0 =A0 =A0 =A0if (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... >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DPRINTF("Raise IRQ\n"); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 qemu_irq_raise(s->irq); >> =A0 =A0 =A0 =A0 =A0 =A0 } >> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_ph= ys_addr_t addr, uint32_t val) >> =A0 =A0 =A0 =A0 =A0 =A0 val &=3D ~DMA_DRAIN_FIFO; >> =A0 =A0 =A0 =A0 } else if (val =3D=3D 0) >> =A0 =A0 =A0 =A0 =A0 =A0 val =3D DMA_DRAIN_FIFO; >> - =A0 =A0 =A0 =A0val &=3D 0x0fffffff; >> + =A0 =A0 =A0 =A0val &=3D ~DMA_CSR_RO_MASK; >> =A0 =A0 =A0 =A0 val |=3D DMA_VER; >> + =A0 =A0 =A0 =A0s->dmaregs[0] =3D (s->dmaregs[0] & DMA_CSR_RO_MASK) | v= al; >> =A0 =A0 =A0 =A0 break; >> =A0 =A0 case 1: >> =A0 =A0 =A0 =A0 s->dmaregs[0] |=3D DMA_LOADED; >> - =A0 =A0 =A0 =A0break; > > A comment about fall through should be added. ok. >> =A0 =A0 default: >> + =A0 =A0 =A0 =A0s->dmaregs[saddr] =3D val; >> =A0 =A0 =A0 =A0 break; >> =A0 =A0 } >> - =A0 =A0s->dmaregs[saddr] =3D val; >> =A0} >> >> =A0static CPUReadMemoryFunc * const dma_mem_read[3] =3D { >> -- >> 1.6.2.5 >> >> > --=20 Regards, Artyom Tarasenko solaris/sparc under qemu blog: http://tyom.blogspot.com/