From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Jt9Kx-0007uG-GZ for qemu-devel@nongnu.org; Mon, 05 May 2008 18:39:11 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Jt9Kw-0007tx-Vn for qemu-devel@nongnu.org; Mon, 05 May 2008 18:39:11 -0400 Received: from [199.232.76.173] (port=53693 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Jt9Kw-0007tr-M1 for qemu-devel@nongnu.org; Mon, 05 May 2008 18:39:10 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Jt9Kw-0006AB-8v for qemu-devel@nongnu.org; Mon, 05 May 2008 18:39:10 -0400 Received: from smtp05.web.de (fmsmtp05.dlan.cinetic.de [172.20.4.166]) by fmmailgate01.web.de (Postfix) with ESMTP id 79F73DDEA4F8 for ; Tue, 6 May 2008 00:39:09 +0200 (CEST) Received: from [88.65.243.112] (helo=[192.168.1.198]) by smtp05.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1Jt9Kv-0003Az-00 for qemu-devel@nongnu.org; Tue, 06 May 2008 00:39:09 +0200 Message-ID: <481F8C8C.60602@web.de> Date: Tue, 06 May 2008 00:39:08 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] ide: Enable byte&word access to DMA address register References: <481EC357.9030401@siemens.com> <20080505212004.GA25810@volta.aurel32.net> In-Reply-To: <20080505212004.GA25810@volta.aurel32.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: jan.kiszka@web.de Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Aurelien Jarno wrote: > On Mon, May 05, 2008 at 10:20:39AM +0200, Jan Kiszka wrote: >> According to the specs, also byte- and word-wise access to the busmaster >> DMA address register is allowed. Patch below fixes the IDE emulation >> in this regard (avoiding to touch the existing common case of 32-bit >> access) and makes our guest happy. >> >> Signed-off-by: Jan Kiszka >> --- >> hw/ide.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> Index: b/hw/ide.c >> =================================================================== >> --- a/hw/ide.c >> +++ b/hw/ide.c >> @@ -2838,6 +2838,29 @@ static void bmdma_writeb(void *opaque, u >> } >> } >> >> +static uint32_t bmdma_addr_readb(void *opaque, uint32_t addr) >> +{ >> + BMDMAState *bm = opaque; >> + uint32_t val; >> + val = (bm->addr >> ((addr & 3) * 8)) & 0xff; >> +#ifdef DEBUG_IDE >> + printf("%s: 0x%08x\n", __func__, val); >> +#endif >> + return val; >> +} >> + >> +static void bmdma_addr_writeb(void *opaque, uint32_t addr, uint32_t val) >> +{ >> + BMDMAState *bm = opaque; >> + int shift = (addr & 3) * 8; >> +#ifdef DEBUG_IDE >> + printf("%s: 0x%08x\n", __func__, val); >> +#endif >> + bm->addr &= ~(0xFF << shift); >> + bm->addr |= (val & 0xfc) << shift; > > Are you sure it is correct? If you want to make sure the 2 lowest bits > are 0, it should be instead: > > bm->addr |= ((val & 0xFF) << shift) & ~3; Oh, oh, oh. Of course. I just wonder why my colleague was able to work with it despite of this bug. Looks like we have been lucky. Find corrected patch below. > >> + bm->cur_addr = bm->addr; >> +} >> + >> static uint32_t bmdma_addr_readl(void *opaque, uint32_t addr) >> { >> BMDMAState *bm = opaque; >> @@ -2876,6 +2899,8 @@ static void bmdma_map(PCIDevice *pci_dev >> register_ioport_write(addr + 1, 3, 1, bmdma_writeb, bm); >> register_ioport_read(addr, 4, 1, bmdma_readb, bm); >> >> + register_ioport_write(addr + 4, 4, 1, bmdma_addr_writeb, bm); >> + register_ioport_read(addr + 4, 4, 1, bmdma_addr_readb, bm); >> register_ioport_write(addr + 4, 4, 4, bmdma_addr_writel, bm); >> register_ioport_read(addr + 4, 4, 4, bmdma_addr_readl, bm); >> addr += 8; >> > > Otherwise, looks ok. Are word accesses supported? If yes it may be nice > to implement bmdma_addr_writew and bmdma_addr_readw at the same time. Yes, word access is supported, the default handler will simply call twice into our byte handler. Thanks! Jan --- hw/ide.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) Index: b/hw/ide.c =================================================================== --- a/hw/ide.c +++ b/hw/ide.c @@ -2838,6 +2838,29 @@ static void bmdma_writeb(void *opaque, u } } +static uint32_t bmdma_addr_readb(void *opaque, uint32_t addr) +{ + BMDMAState *bm = opaque; + uint32_t val; + val = (bm->addr >> ((addr & 3) * 8)) & 0xff; +#ifdef DEBUG_IDE + printf("%s: 0x%08x\n", __func__, val); +#endif + return val; +} + +static void bmdma_addr_writeb(void *opaque, uint32_t addr, uint32_t val) +{ + BMDMAState *bm = opaque; + int shift = (addr & 3) * 8; +#ifdef DEBUG_IDE + printf("%s: 0x%08x\n", __func__, val); +#endif + bm->addr &= ~(0xFF << shift); + bm->addr |= ((val & 0xFF) << shift) & ~3; + bm->cur_addr = bm->addr; +} + static uint32_t bmdma_addr_readl(void *opaque, uint32_t addr) { BMDMAState *bm = opaque; @@ -2876,6 +2899,8 @@ static void bmdma_map(PCIDevice *pci_dev register_ioport_write(addr + 1, 3, 1, bmdma_writeb, bm); register_ioport_read(addr, 4, 1, bmdma_readb, bm); + register_ioport_write(addr + 4, 4, 1, bmdma_addr_writeb, bm); + register_ioport_read(addr + 4, 4, 1, bmdma_addr_readb, bm); register_ioport_write(addr + 4, 4, 4, bmdma_addr_writel, bm); register_ioport_read(addr + 4, 4, 4, bmdma_addr_readl, bm); addr += 8;