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

[Igor Kovalenko]

On Sun, Dec 13, 2009 at 10:06 PM, Juan Quintela <address@hidden> wrote:
> 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.

Looks good, but runtime aborts in register_ioport_read.

You cannot install different opaque for read and write of the same i/o address.
Seems like every other device has the same driver for reading and writing,
but in cmd646 it calls out to ide/pci.c code for bmdma_cmd_writeb write
method, whereas it reads with own bmdma_readb_0 method.

Probably bmdma_writeb_* should call out to bmdma_cmd_writeb for address 0
and whole 4 byte is to be mapped to bmdma_writeb_*

I tested the following fix on top of yours patch with my previous workaround
reverted. Both my workaround and these two combined show the same
qemu.log trace.

commit 26c618af44c91a806d88044d468733b86028e352
Author: Igor V. Kovalenko <address@hidden>
Date:   Mon Dec 14 00:05:10 2009 +0300

    cmd646 fix abort due to changed opaque pointer for ioport read

    Signed-off-by: Igor V. Kovalenko <address@hidden>

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 9d60590..07fcf4d 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -123,6 +123,9 @@ static void bmdma_writeb_common(PCIIDEState
*pci_dev, BMDMAState *bm,
     printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
 #endif
     switch(addr & 3) {
+    case 0:
+        bmdma_cmd_writeb(bm, addr, val);
+        break;
     case 1:
         pci_dev->dev.config[MRDMODE] =
             (pci_dev->dev.config[MRDMODE] & ~0x30) | (val & 0x30);
@@ -168,13 +171,11 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
         bm->bus = d->bus+i;
         qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);

-        register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
-
         if (i == 0) {
-            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_0, d);
+            register_ioport_write(addr, 4, 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_write(addr, 4, 1, bmdma_writeb_1, d);
             register_ioport_read(addr, 4, 1, bmdma_readb_1, d);
         }



-- 
Kind regards,
Igor V. Kovalenko