Re: [Qemu-devel] [PATCH] ide: Enable byte&word access to DMA address register

[Jan Kiszka]

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 <address@hidden>
>> ---
>>  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;