From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59297 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OFkyX-00036v-P9 for qemu-devel@nongnu.org; Sat, 22 May 2010 05:26:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OFkyV-0001eY-LD for qemu-devel@nongnu.org; Sat, 22 May 2010 05:26:33 -0400 Received: from mail-pw0-f45.google.com ([209.85.160.45]:51844) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OFkyV-0001eO-Cx for qemu-devel@nongnu.org; Sat, 22 May 2010 05:26:31 -0400 Received: by pwj8 with SMTP id 8so807101pwj.4 for ; Sat, 22 May 2010 02:26:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1274478829-10840-1-git-send-email-atar4qemu@gmail.com> From: Blue Swirl Date: Sat, 22 May 2010 09:26:10 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 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: Artyom Tarasenko Cc: qemu-devel@nongnu.org On Sat, May 22, 2010 at 8:32 AM, Artyom Tarasenko wrote: > 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 >>> --- >>> =C2=A0hw/sparc32_dma.c | =C2=A0 11 +++++++---- >>> =C2=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 @@ >>> =C2=A0#define DMA_DRAIN_FIFO 0x40 >>> =C2=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... 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. >> >>> + >>> =C2=A0typedef struct DMAState DMAState; >>> >>> =C2=A0struct DMAState { >>> @@ -187,7 +190,7 @@ static void dma_mem_writel(void *opaque, target_phy= s_addr_t addr, uint32_t val) >>> =C2=A0 =C2=A0 switch (saddr) { >>> =C2=A0 =C2=A0 case 0: >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (val & DMA_INTREN) { >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (val & DMA_INTR) { >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=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: =C2=A0th= e > real hardware would have some latency too. > > Unless you have a test case which I broke... > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 DPRINTF("Raise = IRQ\n"); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_irq_raise(= s->irq); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >>> @@ -204,16 +207,16 @@ static void dma_mem_writel(void *opaque, target_p= hys_addr_t addr, uint32_t val) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val &=3D ~DMA_DRAIN_FIFO; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else if (val =3D=3D 0) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 val =3D DMA_DRAIN_FIFO; >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0val &=3D 0x0fffffff; >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0val &=3D ~DMA_CSR_RO_MASK; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 val |=3D DMA_VER; >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->dmaregs[0] =3D (s->dmaregs[0] & DMA_CSR= _RO_MASK) | val; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 case 1: >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->dmaregs[0] |=3D DMA_LOADED; >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> >> A comment about fall through should be added. > > ok. > >>> =C2=A0 =C2=A0 default: >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->dmaregs[saddr] =3D val; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 } >>> - =C2=A0 =C2=A0s->dmaregs[saddr] =3D val; >>> =C2=A0} >>> >>> =C2=A0static CPUReadMemoryFunc * const dma_mem_read[3] =3D { >>> -- >>> 1.6.2.5 >>> >>> >> > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ >