[Qemu-devel] Re: Bug in Sparc64/IDE Code

[Juan Quintela]

Igor Kovalenko <address@hidden> wrote:
> On Sat, Dec 12, 2009 at 3:18 PM, Igor Kovalenko
> <address@hidden> wrote:
>> On Sat, Dec 12, 2009 at 1:12 PM, Blue Swirl <address@hidden> wrote:
>>> On Fri, Dec 11, 2009 at 10:16 PM, Nick Couchman <address@hidden> wrote:
>>>> In working to try to get Sparc64 system emulation developed, we seem to 
>>>> have run into an issue with the IDE code in Qemu.  The OpenBIOS folks have 
>>>> been working quite a few issues with the OpenBIOS code that need to be 
>>>> resolved in order to boot 64-bit Solaris kernels correctly, but the most 
>>>> recent issue indicates that the IDE code for the Sparc64 emulator is 
>>>> reading from and writing to the wrong memory locations.  The end result is 
>>>> the following output when trying to boot off an ISO image in Qemu:
>>>
>>>> bmdma_cmd_writeb: 0x00000054
>>>> bmdma: writeb 0x701 : 0xd7
>>>> bmdma: writeb 0x702 : 0x79
>>>> bmdma: writeb 0x703 : 0xfe
>>>> bmdma_addr_writew: 0x0000ddef
>>>> bmdma_addr_writew: 0x0000b12b
>>>> bmdma_cmd_writeb: 0x000000da
>>>> bmdma: writeb 0x709 : 0x95
>>>> Segmentation fault
>>>
>>> I can't reproduce this with milaX 0.3.1, QEMU git HEAD and OpenBIOS
>>> svn r644. The bug could be that the BMDMA address may need BE to LE
>>> conversion, or OpenBIOS could just clobber BMDMA registers with
>>> garbage (the DMA address candidates 0xddefb12b and 0xb12bddef do not
>>> look valid).
>>>
>>> Another possibility is that the PCI host bridge should have an IOMMU
>>> which is not implemented yet, but I doubt we are at that stage.
>>>
>>> Could you run QEMU in a GDB session and send the backtrace from the 
>>> segfault?
>>>
>>
>> There seems to be an issue with pci_from_bm cast: bm->unit is not
>> assigned anywhere
>> in the code so it is zero for second unit, and pci_from_bm returns
>> wrong address.
>> Crash happens writing to address mapped for second unit.
>
> This appears to be a regression in cmd646. After removal of pci_dev from
> BMDMAState structure we cannot do much to work around this issue.
>
> The problem here is that we cannot rely on bm->unit value since it is getting
> changed while dma operations are in progress, f.e. it is set to -1 on
> dma cancel.
> Thus we cannot get to pci_dev from BMDMAState passed to i/o read/write
> callbacks.
>
> Juan, can you please take a look at this issue?

   I don't have a sparc setup, but could you try this patch?  It also fixes
   the test for bm.

>From 6d2dfb38804fdf869b59f75768c8376951f81bb5 Mon Sep 17 00:00:00 2001
From: Juan Quintela <address@hidden>
Date: Sun, 13 Dec 2009 20:03:30 +0100
Subject: [PATCH] cmd646: pass pci_dev as it needs it

Instead of doing tricks to get the pci_dev, just pass it in the 1st
place.  Patch is a bit longer that reverting the pci_dev field, but it
states more clearly (IMHO) what we are doing.

It also fixes the bm test, now that you told me that ->unit is not
always valid.

Could you test and tell me the result?  Thanks in advance.

Later, Juan.

Signed-off-by: Juan Quintela <address@hidden>
---
 hw/ide/cmd646.c |   64 ++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 3b7c606..f5edc14 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -68,19 +68,9 @@ static void ide_map(PCIDevice *pci_dev, int region_num,
     }
 }

-static PCIIDEState *pci_from_bm(BMDMAState *bm)
+static uint32_t bmdma_readb_common(PCIIDEState *pci_dev, BMDMAState *bm,
+                                   uint32_t addr)
 {
-    if (bm->unit == 0) {
-        return container_of(bm, PCIIDEState, bmdma[0]);
-    } else {
-        return container_of(bm, PCIIDEState, bmdma[1]);
-    }
-}
-
-static uint32_t bmdma_readb(void *opaque, uint32_t addr)
-{
-    BMDMAState *bm = opaque;
-    PCIIDEState *pci_dev = pci_from_bm(bm);
     uint32_t val;

     switch(addr & 3) {
@@ -94,7 +84,7 @@ static uint32_t bmdma_readb(void *opaque, uint32_t addr)
         val = bm->status;
         break;
     case 3:
-        if (bm->unit == 0) {
+        if (bm == &pci_dev->bmdma[0]) {
             val = pci_dev->dev.config[UDIDETCR0];
         } else {
             val = pci_dev->dev.config[UDIDETCR1];
@@ -110,10 +100,25 @@ static uint32_t bmdma_readb(void *opaque, uint32_t addr)
     return val;
 }

-static void bmdma_writeb(void *opaque, uint32_t addr, uint32_t val)
+static uint32_t bmdma_readb_0(void *opaque, uint32_t addr)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[0];
+
+    return bmdma_readb_common(pci_dev, bm, addr);
+}
+
+static uint32_t bmdma_readb_1(void *opaque, uint32_t addr)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[1];
+
+    return bmdma_readb_common(pci_dev, bm, addr);
+}
+
+static void bmdma_writeb_common(PCIIDEState *pci_dev, BMDMAState *bm,
+                                uint32_t addr, uint32_t val)
 {
-    BMDMAState *bm = opaque;
-    PCIIDEState *pci_dev = pci_from_bm(bm);
 #ifdef DEBUG_IDE
     printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
 #endif
@@ -127,7 +132,7 @@ static void bmdma_writeb(void *opaque, uint32_t addr, 
uint32_t val)
         bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 
0x06);
         break;
     case 3:
-        if (bm->unit == 0)
+        if (bm == &pci_dev->bmdma[0])
             pci_dev->dev.config[UDIDETCR0] = val;
         else
             pci_dev->dev.config[UDIDETCR1] = val;
@@ -135,6 +140,22 @@ static void bmdma_writeb(void *opaque, uint32_t addr, 
uint32_t val)
     }
 }

+static void bmdma_writeb_0(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[0];
+
+    bmdma_writeb_common(pci_dev, bm, addr, val);
+}
+
+static void bmdma_writeb_1(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[1];
+
+    bmdma_writeb_common(pci_dev, bm, addr, val);
+}
+
 static void bmdma_map(PCIDevice *pci_dev, int region_num,
                     pcibus_t addr, pcibus_t size, int type)
 {
@@ -149,8 +170,13 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,

         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);

-        register_ioport_write(addr + 1, 3, 1, bmdma_writeb, bm);
-        register_ioport_read(addr, 4, 1, bmdma_readb, bm);
+        if (i == 0) {
+            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_0, d);
+            register_ioport_read(addr, 4, 1, bmdma_readb_0, d);
+        } else {
+            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_1, d);
+            register_ioport_read(addr, 4, 1, bmdma_readb_1, d);
+        }

         register_ioport_write(addr + 4, 4, 1, bmdma_addr_writeb, bm);
         register_ioport_read(addr + 4, 4, 1, bmdma_addr_readb, bm);
-- 
1.6.5.2