Skip to contentSkip to conversation

qemu: IDE/ATAPI emulation reliability fixes

PATCH In review Carlo Marcelo Arenas Belon 2 participants 9 messages Nov 28, 2007 – Nov 30, 2007 1 message not in the archive KVM

lore
Share & tools
lore
#0 lore raw

cover qemu: IDE/ATAPI emulation reliability fixes

The following patch series implements fixes to the IDE/ATAPI emulation in qemu which are already committed upstream or proposed for inclusion; including 2 serious regressions that result in availability and data loss problems when using OpenSolaris or FreeBSD guests (at least) by the first 2:

Patch 1/5 : fixes GET_CONFIGURATION to allow OpenSolaris cdrom access Patch 2/5 : fixes IDE sector write operations resulting in data lost Patch 3/5 : enables ACPI interrupts
Patch 4/5 : uses DVD-ROM as model name for INQUIRY and IDENTIFY DEVICE Patch 5/5 : removes obsolete triton chip support

They had been tested in Gentoo Linux 2007.0 amd64 using an Intel Core 2 6320 host and several different versions of FreeBSD, OpenBSD, NetBSD, Linux, OpenSolaris and Windows 2000 guests

Carlo

-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper from Novell. From the desktop to the data center, Linux is going mainstream. Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4

#1 lore raw

PATCH 1/5 qemu: GET_CONFIGURATION fixes for MMC-6 DVD-ROM implementation

The following patch complements "Partial IDE DVD emulation", added in ide.c revision 1.66 and that was generating the following timeouts for OpenSolaris guests when trying to access the ATAPI cdrom (during installation for example):

  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1);
          timeout: abort request, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: abort device, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: reset target, target=0 lun=0
  WARNING: /pci@0,0/pci-ide@1,1/ide@1 (ata1):
          timeout: reset bus, target=0 lun=0

The changes required to the "GET CONFIGURATION" implementation are :

* recognize and honor "Allocation Length" command parameter
* only set "current profile" for the response if a profile is current (either CD or DVD loaded)
* calculate "data length" including all headers
* refactor code and add comments to help document references to all implemented standards (ATAPI-4, SPC-3 and MMC-6)

---
 qemu/hw/ide.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)
qemu/hw/ide.c +19 -8
index 329d053..29fc7a9 100644
@@ -1648,6 +1648,7 @@ static void ide_atapi_cmd(IDEState *s)
         break;
     case GPCMD_GET_CONFIGURATION:
         {
+            uint32_t len;
             int64_t total_sectors;
 
             /* only feature 0 is supported */
@@ -1656,17 +1657,27 @@ static void ide_atapi_cmd(IDEState *s)
                                     ASC_INV_FIELD_IN_CMD_PACKET);
                 break;
             }
-            memset(buf, 0, 32);
+            max_len = ube16_to_cpu(packet + 7);
             bdrv_get_geometry(s->bs, &total_sectors);
-            buf[3] = 16;
-            buf[7] = total_sectors <= 1433600 ? 0x08 : 0x10; /* current profile */
-            buf[10] = 0x10 | 0x1;
-            buf[11] = 0x08; /* size of profile list */
+            memset(buf, 0, 32);
+            if (total_sectors) {
+                if (total_sectors > 1433600) {
+                    buf[7] = 0x10; /* DVD-ROM */
+                } else {
+                    buf[7] = 0x08; /* CD-ROM */
+                }
+            } else {
+                buf[7] = 0x00; /* no current profile */
+            }
+            buf[10] = 0x02 | 0x01; /* persistent and current */
+            buf[11] = 0x08; /* size of profile list = 4 bytes per profile */
             buf[13] = 0x10; /* DVD-ROM profile */
-            buf[14] = buf[7] == 0x10; /* (in)active */
+            buf[14] = buf[13] == buf[7]; /* (in)active */
             buf[17] = 0x08; /* CD-ROM profile */
-            buf[18] = buf[7] == 0x08; /* (in)active */
-            ide_atapi_cmd_reply(s, 32, 32);
+            buf[18] = buf[17] == buf[7]; /* (in)active */
+            len = 8 + 4 + buf[11]; /* headers + size of profile list */
+            cpu_to_ube32(buf, len - 4); /* data length */
+            ide_atapi_cmd_reply(s, len, max_len);
             break;
         }
     default:
Signature · 1 line
1.5.2.5
#2 lore raw

PATCH 2/5 qemu: revert ide sector write operation not to use AIO

In reply to Carlo Marcelo Arenas Belon

This patch revert "Last AIO Patch" as requested by Fabrice, it is incomplete and breaks other (non-Windows) systems.

originally from qemu CVS :

cvs -q diff -r1.69 -r1.70 hw/ide.c

Data corruption resulting in kernel panics and unbootable images is experienced frequently with FreeBSD and OpenSolaris guests unless committed

---
 qemu/hw/ide.c |   56 ++++++++++++++++++--------------------------------------
 1 files changed, 18 insertions(+), 38 deletions(-)
qemu/hw/ide.c +18 -38
index 29fc7a9..a8d4339 100644
@@ -358,7 +358,7 @@ typedef struct IDEState {
     uint8_t *data_ptr;
     uint8_t *data_end;
     uint8_t io_buffer[MAX_MULT_SECTORS*512 + 4];
-    QEMUTimer *sector_write_timer; /* only used for win2k instal hack */
+    QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
     /* CF-ATA extended error */
     uint8_t ext_error;
@@ -865,44 +865,10 @@ static void ide_sector_write_timer_cb(void *opaque)
     ide_set_irq(s);
 }
 
-static void ide_sector_write_aio_cb(void *opaque, int ret)
-{
-    BMDMAState *bm = opaque;
-    IDEState *s = bm->ide_if;
-
-#ifdef TARGET_I386
-    if (win2k_install_hack && ((++s->irq_count % 16) == 0)) {
-	/* It seems there is a bug in the Windows 2000 installer HDD
-	   IDE driver which fills the disk with empty logs when the
-	   IDE write IRQ comes too early. This hack tries to correct
-	   that at the expense of slower write performances. Use this
-	   option _only_ to install Windows 2000. You must disable it
-	   for normal use. */
-	qemu_mod_timer(s->sector_write_timer,
-		       qemu_get_clock(vm_clock) + (ticks_per_sec / 1000));
-    } else
-#endif
-    {
-	ide_set_irq(s);
-    }
-    bm->aiocb = NULL;
-}
-
 static void ide_sector_write(IDEState *s)
 {
-    BMDMAState *bm;
     int64_t sector_num;
-    int n, n1;
-
-    s->io_buffer_index = 0;
-    s->io_buffer_size = 0;
-    bm = s->bmdma;
-    if(bm == NULL) {
-	bm = qemu_mallocz(sizeof(BMDMAState));
-	s->bmdma = bm;
-    }
-    bm->ide_if = s;
-    bm->dma_cb = ide_sector_write_aio_cb;
+    int ret, n, n1;
 
     s->status = READY_STAT | SEEK_STAT;
     sector_num = ide_get_sector(s);
@@ -912,6 +878,7 @@ static void ide_sector_write(IDEState *s)
     n = s->nsector;
     if (n > s->req_nb_sectors)
         n = s->req_nb_sectors;
+    ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
     s->nsector -= n;
     if (s->nsector == 0) {
         /* no more sectors to write */
@@ -924,8 +891,21 @@ static void ide_sector_write(IDEState *s)
     }
     ide_set_sector(s, sector_num + n);
 
-    bm->aiocb = bdrv_aio_write(s->bs, sector_num, s->io_buffer, n,
-			       ide_sector_write_aio_cb, bm);
+#ifdef TARGET_I386
+    if (win2k_install_hack && ((++s->irq_count % 16) == 0)) {
+        /* It seems there is a bug in the Windows 2000 installer HDD
+           IDE driver which fills the disk with empty logs when the
+           IDE write IRQ comes too early. This hack tries to correct
+           that at the expense of slower write performances. Use this
+           option _only_ to install Windows 2000. You must disable it
+           for normal use. */
+        qemu_mod_timer(s->sector_write_timer, 
+                       qemu_get_clock(vm_clock) + (ticks_per_sec / 1000));
+    } else 
+#endif
+    {
+        ide_set_irq(s);
+    }
 }
 
 /* XXX: handle errors */
Signature · 8 lines
1.5.2.5


-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4
#3 lore raw

PATCH 3/5 qemu: piix_pci enable ACPI interrupts

In reply to Carlo Marcelo Arenas Belon

This patch enable ACPI interrupts, by Igor Lvovsky

Originally from qemu CVS :

cvs -q diff -r1.11 -r1.12 hw/piix_pci.c

---
 qemu/hw/piix_pci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
qemu/hw/piix_pci.c +1 -0
index 8c00f0d..3c04e3a 100644
@@ -208,6 +208,7 @@ static void piix3_set_irq(qemu_irq *pic, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
 
+    piix3_dev->config[0x60 + irq_num] &= ~0x80;   // enable bit
     pci_irq_levels[irq_num] = level;
 
     /* now we change the pic irq level according to the piix irq mappings */
Signature · 8 lines
1.5.2.5


-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4
#4 lore raw

PATCH 4/5 qemu: ide INQUIRY and IDENTIFY DEVICE report DVD-ROM model

In reply to Carlo Marcelo Arenas Belon

This patch complements "Partial IDE DVD emulation" which was added in ide.c revision 1.66 so that the CD-ROM identifies itself as a DVD-ROM to "INQUIRY" and "IDENTIFY DEVICE" commands

---
 qemu/hw/ide.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
qemu/hw/ide.c +2 -2
index a8d4339..a97d519 100644
@@ -533,7 +533,7 @@ static void ide_atapi_identify(IDEState *s)
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((uint8_t *)(p + 23), QEMU_VERSION, 8); /* firmware version */
-    padstr((uint8_t *)(p + 27), "QEMU CD-ROM", 40); /* model */
+    padstr((uint8_t *)(p + 27), "QEMU DVD-ROM", 40); /* model */
     put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
 #ifdef USE_DMA_CDROM
     put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
@@ -1622,7 +1622,7 @@ static void ide_atapi_cmd(IDEState *s)
         buf[6] = 0; /* reserved */
         buf[7] = 0; /* reserved */
         padstr8(buf + 8, 8, "QEMU");
-        padstr8(buf + 16, 16, "QEMU CD-ROM");
+        padstr8(buf + 16, 16, "QEMU DVD-ROM");
         padstr8(buf + 32, 4, QEMU_VERSION);
         ide_atapi_cmd_reply(s, 36, max_len);
         break;
Signature · 1 line
1.5.2.5
#5 lore raw

PATCH 5/5 qemu: piix_pci: remove 82371FB support

In reply to Carlo Marcelo Arenas Belon

This patch removes support for 82371FB Step A1 (AKA triton) chips as they are superseded by 82371SB and are currently dead code as all other references were removed already from ide.c since revision 1.57

---
 qemu/hw/piix_pci.c |   25 -------------------------
 1 files changed, 0 insertions(+), 25 deletions(-)
qemu/hw/piix_pci.c +0 -25
index 3c04e3a..f40dad9 100644
@@ -311,31 +311,6 @@ static int piix_load(QEMUFile* f, void *opaque, int version_id)
     return pci_device_load(d, f);
 }
 
-int piix_init(PCIBus *bus, int devfn)
-{
-    PCIDevice *d;
-    uint8_t *pci_conf;
-
-    d = pci_register_device(bus, "PIIX", sizeof(PCIDevice),
-                                    devfn, NULL, NULL);
-    register_savevm("PIIX", 0, 2, piix_save, piix_load, d);
-
-    piix3_dev = d;
-    pci_conf = d->config;
-
-    pci_conf[0x00] = 0x86; // Intel
-    pci_conf[0x01] = 0x80;
-    pci_conf[0x02] = 0x2E; // 82371FB PIIX PCI-to-ISA bridge
-    pci_conf[0x03] = 0x12;
-    pci_conf[0x08] = 0x02; // Step A1
-    pci_conf[0x0a] = 0x01; // class_sub = PCI_ISA
-    pci_conf[0x0b] = 0x06; // class_base = PCI_bridge
-    pci_conf[0x0e] = 0x80; // header_type = PCI_multifunction, generic
-
-    piix3_reset(d);
-    return d->devfn;
-}
-
 int piix3_init(PCIBus *bus, int devfn)
 {
     PCIDevice *d;
Signature · 1 line
1.5.2.5
#6 lore raw
In reply to Carlo Marcelo Arenas Belon
Quoted 11 lines · Carlo Marcelo Arenas Belon

The following patch series implements fixes to the IDE/ATAPI emulation in qemu which are already committed upstream or proposed for inclusion; including 2 serious regressions that result in availability and data loss problems when using OpenSolaris or FreeBSD guests (at least) by the first 2:

Patch 1/5 : fixes GET_CONFIGURATION to allow OpenSolaris cdrom access Patch 2/5 : fixes IDE sector write operations resulting in data lost Patch 3/5 : enables ACPI interrupts
Patch 4/5 : uses DVD-ROM as model name for INQUIRY and IDENTIFY DEVICE Patch 5/5 : removes obsolete triton chip support

Quoted 5 lines

They had been tested in Gentoo Linux 2007.0 amd64 using an Intel Core 2 6320 host and several different versions of FreeBSD, OpenBSD, NetBSD, Linux, OpenSolaris and Windows 2000 guests

Thanks; the last time I tried to merge qemu I had several regressions. I'll try again soon once these are merged (or merge them myself if qemu-devel takes too long).

Signature · 8 lines
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4

A message that is not in the archive (sent off-list or not yet received): 474FB8AB.70601-atKUWr5tajBWk0Htik3J/[email protected]

#8 lore raw
Quoted 12 lines · Avi Kivity
Quoted 11 lines · Carlo Marcelo Arenas Belon

The following patch series implements fixes to the IDE/ATAPI emulation in qemu which are already committed upstream or proposed for inclusion; including 2 serious regressions that result in availability and data loss problems when using OpenSolaris or FreeBSD guests (at least) by the first 2:

Patch 1/5 : fixes GET_CONFIGURATION to allow OpenSolaris cdrom access Patch 2/5 : fixes IDE sector write operations resulting in data lost Patch 3/5 : enables ACPI interrupts
Patch 4/5 : uses DVD-ROM as model name for INQUIRY and IDENTIFY DEVICE Patch 5/5 : removes obsolete triton chip support

Quoted 5 lines
Quoted 3 lines

They had been tested in Gentoo Linux 2007.0 amd64 using an Intel Core 2 6320 host and several different versions of FreeBSD, OpenBSD, NetBSD, Linux, OpenSolaris and Windows 2000 guests

Thanks; the last time I tried to merge qemu I had several regressions.

some of the patches I cherry picked from qemu cvs are part of the last import in the qemu-vendor-drops branch but since they had not been yet merged (as you mention) I though it was better to cherry pick them from now to expedite this patch.

Quoted 2 lines

I'll try again soon once these are merged (or merge them myself if qemu-devel takes too long).

for qemu is probably not that much of a priority as they already have the patches, most of them committed and the bugs are only in their development tree which they don't release anyway, for kvm it is IMHO different since the bugs are on released code with patch 2 (which ate 3 of my FreeBSD guests with unrecoverable images which wouldn't even have a valid partition table) being around since the qemu import of Oct 1 and therefore part of kvm-45 and all releases up to the date.

I understand though why you wouldn't like to divert from qemu, and I'll do my best to try to get qemu to commit those patches so that any future conflicts are resolved automatically, but as I said before, qemu just had different priorities.

Carlo

-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper from Novell. From the desktop to the data center, Linux is going mainstream. Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4

#9 lore raw
Quoted 8 lines · Carlo Marcelo Arenas Belon

for qemu is probably not that much of a priority as they already have the patches, most of them committed and the bugs are only in their development tree which they don't release anyway, for kvm it is IMHO different since the bugs are on released code with patch 2 (which ate 3 of my FreeBSD guests with unrecoverable images which wouldn't even have a valid partition table) being around since the qemu import of Oct 1 and therefore part of kvm-45 and all releases up to the date.

Quoted 6 lines

I understand though why you wouldn't like to divert from qemu, and I'll do my best to try to get qemu to commit those patches so that any future conflicts are resolved automatically, but as I said before, qemu just had different priorities.

You make some good points; I'll respin the merge and this time bisect the problems and revert the offending patches. I'll add these patches as well if qemu hasn't merged it by that time.

Signature · 8 lines
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
SF.Net email is sponsored by: The Future of Linux Business White Paper
from Novell.  From the desktop to the data center, Linux is going
mainstream.  Let it simplify your IT future.
http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4