Patchwork [v2] Write cmos hd data for ide drives using -device parm

login
register
mail settings
Submitter Bruce Rogers
Date April 13, 2010, 8:41 p.m.
Message ID <4BC482850200004800092E82@sinclair.provo.novell.com>
Download mbox | patch
Permalink /patch/50085/
State New
Headers show

Comments

Bruce Rogers - April 13, 2010, 8:41 p.m.
When specifying ide devices using -device, the cmos information 
which the bios depends on is not written. This patch generalizes 
the cmos hd data setting for the existing code path and adds the 
ability to call that code on a per machine, per ide drive basis. 

This is important for older guests, such as Windows XP, and Windows 
Server 2003. 

I needed to add an id to the IDEBus structure since there wasn't 
a way to identify whether an ide bus was primary or secondary in 
the lower level code. 

v2:Fixed same issue for isapc machine type (previous patch addressed
pc machine types only). Additionally, the bus naming needed to be fixed
in the isa case to be able to reference the secondary ide bus in -device
nomenclature.

Signed-off-by: Bruce Rogers <brogers@novell.com> 

Bruce 

 hw/ide/internal.h |    3 +-
 hw/ide/isa.c      |    8 +++++-
 hw/ide/piix.c     |    6 +++-
 hw/ide/qdev.c     |   13 +++++++---
 hw/pc.c           |   68 ++++++++++++++++++++++++++++++++----------------------
 sysemu.h          |    1 
 vl.c              |    6 ++++
 7 files changed, 71 insertions(+), 34 deletions(-)
Gerd Hoffmann - April 14, 2010, 7:24 a.m.
Hi,

> When specifying ide devices using -device, the cmos information
> which the bios depends on is not written. This patch generalizes
> the cmos hd data setting for the existing code path and adds the
> ability to call that code on a per machine, per ide drive basis.

Hmm.

With -device and thus alot of device setup happening after 
QEMUMachine->init() I think it would be very useful to have 
QEMUMachine->init_late().  There are a bunch of machine specific jobs 
which need to be done after all devices have been plugged into the machine.

Setting up cmos is one of them.

i440fx_init_memory_mappings() could be moved there, which would allow to 
add vga cards via -device.

And I'm pretty sure there are more use cases.  fw_cfg maybe?

Comments?

cheers,
   Gerd
Bruce Rogers - April 19, 2010, 8:04 p.m.
>>> On 4/14/2010 at 01:24 AM, Gerd Hoffmann <kraxel@redhat.com> wrote: 
> Hi,
> 
>> When specifying ide devices using -device, the cmos information
>> which the bios depends on is not written. This patch generalizes
>> the cmos hd data setting for the existing code path and adds the
>> ability to call that code on a per machine, per ide drive basis.
> 
> Hmm.
> 
> With -device and thus alot of device setup happening after 
> QEMUMachine->init() I think it would be very useful to have 
> QEMUMachine->init_late().  There are a bunch of machine specific jobs 
> which need to be done after all devices have been plugged into the machine.
> 
> Setting up cmos is one of them.
> 
> i440fx_init_memory_mappings() could be moved there, which would allow to 
> add vga cards via -device.
> 
> And I'm pretty sure there are more use cases.  fw_cfg maybe?
> 
> Comments?
> 
> cheers,
>    Gerd

Not much traffic on this thread ;-)
I can see the usefulness of an init_late() to generalize post device setup issues.
I assume then that you didn't have any other issues with my patch, other than general code structure concerns?

Bruce
Gerd Hoffmann - April 20, 2010, 7:28 a.m.
Hi,

> Not much traffic on this thread ;-)

Indeed ;)

> I can see the usefulness of an init_late() to generalize post device setup issues.
> I assume then that you didn't have any other issues with my patch, other than general code structure concerns?

Yes, that is the major one.  I think it is much saner to just have a 
init_late() and collect everything there instead of creating a new hook 
each time you figure you need one.

I think this also allows to make the ide changes less intrusive as all 
the cmos setup logic stays local to pc.c.  pc.c can simply keep a 
pointer to the DeviceState structs of the ide interface(s) created in 
pc_init1().  pc_init_late() then can check which drives are plugged in 
and update cmos accordingly.

cheers,
   Gerd
Anthony Liguori - May 3, 2010, 6:24 p.m.
On 04/20/2010 02:28 AM, Gerd Hoffmann wrote:
>   Hi,
>
>> Not much traffic on this thread ;-)
>
> Indeed ;)
>
>> I can see the usefulness of an init_late() to generalize post device 
>> setup issues.
>> I assume then that you didn't have any other issues with my patch, 
>> other than general code structure concerns?
>
> Yes, that is the major one.  I think it is much saner to just have a 
> init_late() and collect everything there instead of creating a new 
> hook each time you figure you need one.
>
> I think this also allows to make the ide changes less intrusive as all 
> the cmos setup logic stays local to pc.c.  pc.c can simply keep a 
> pointer to the DeviceState structs of the ide interface(s) created in 
> pc_init1().  pc_init_late() then can check which drives are plugged in 
> and update cmos accordingly.

I'd personally prefer to see some sort of registration mechanism.  
Something notifier based.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>
>

Patch

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 027029e..4ea62bd 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -450,6 +450,7 @@  struct IDEBus {
     IDEState ifs[2];
     uint8_t unit;
     uint8_t cmd;
+    uint8_t id;
     qemu_irq irq;
 };
 
@@ -565,7 +566,7 @@  void ide_init2(IDEBus *bus, DriveInfo *hd0, DriveInfo *hd1,
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
 
 /* hw/ide/qdev.c */
-void ide_bus_new(IDEBus *idebus, DeviceState *dev);
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, const char *name);
 IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
 
 #endif /* HW_IDE_INTERNAL_H */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index dff7c79..ac3ee3b 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -67,7 +67,13 @@  static int isa_ide_initfn(ISADevice *dev)
 {
     ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
 
-    ide_bus_new(&s->bus, &s->dev.qdev);
+    if (s->iobase == 0x1f0) {
+        ide_bus_new(&s->bus, &s->dev.qdev, "ide.0");
+        s->bus.id = 0;
+    } else {
+        ide_bus_new(&s->bus, &s->dev.qdev, "ide.1");
+        s->bus.id = 1;
+    }
     ide_init_ioport(&s->bus, s->iobase, s->iobase2);
     isa_init_irq(dev, &s->irq, s->isairq);
     ide_init2(&s->bus, NULL, NULL, s->irq);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 295a93d..377fda8 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -131,11 +131,13 @@  static int pci_piix_ide_initfn(PCIIDEState *d)
 
     vmstate_register(0, &vmstate_ide_pci, d);
 
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
+    ide_bus_new(&d->bus[0], &d->dev.qdev, NULL);
+    ide_bus_new(&d->bus[1], &d->dev.qdev, NULL);
     ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
     ide_init_ioport(&d->bus[1], 0x170, 0x376);
 
+    d->bus[0].id = 0;
+    d->bus[1].id = 1;
     ide_init2(&d->bus[0], NULL, NULL, isa_reserve_irq(14));
     ide_init2(&d->bus[1], NULL, NULL, isa_reserve_irq(15));
     return 0;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b18693d..c868b8e 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -29,9 +29,9 @@  static struct BusInfo ide_bus_info = {
     .size  = sizeof(IDEBus),
 };
 
-void ide_bus_new(IDEBus *idebus, DeviceState *dev)
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, const char *name)
 {
-    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
+    qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, name);
 }
 
 static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
@@ -39,6 +39,7 @@  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
     IDEDevice *dev = DO_UPCAST(IDEDevice, qdev, qdev);
     IDEDeviceInfo *info = DO_UPCAST(IDEDeviceInfo, qdev, base);
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
+    int i;
 
     if (!dev->conf.dinfo) {
         fprintf(stderr, "%s: no drive specified\n", qdev->info->name);
@@ -65,7 +66,13 @@  static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
     default:
         goto err;
     }
-    return info->init(dev);
+    dev->dinfo->bus = bus->id;
+    dev->dinfo->unit = dev->unit;
+    i = info->init(dev);
+    if (i >= 0) {
+        machine_ide_finalize(dev->dinfo);
+    }
+    return i;
 
 err:
     return -1;
diff --git a/hw/pc.c b/hw/pc.c
index 69e597f..29f47d4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -215,6 +215,44 @@  static void cmos_init_hd(int type_ofs, int info_ofs, BlockDriverState *hd)
     rtc_set_memory(s, info_ofs + 8, sectors);
 }
 
+static void set_cmos_hd_data(DriveInfo *dinfo)
+{
+    static int hd_type = 0;
+    static int hd_trans = 0;
+    RTCState *s = rtc_state;
+    int cylinders, heads, sectors, translation;
+
+    if (dinfo->bus == 0) {
+        if (dinfo->unit == 0) {
+            hd_type |= 0xf0;
+            cmos_init_hd(0x19, 0x1b, dinfo->bdrv);
+        } else {
+            hd_type |= 0x0f;
+            cmos_init_hd(0x1a, 0x24, dinfo->bdrv);
+        }
+        rtc_set_memory(s, 0x12, hd_type);
+    }
+    /* NOTE: bdrv_get_geometry_hint() returns the physical
+        geometry.  It is always such that: 1 <= sects <= 63, 1
+        <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
+        geometry can be different if a translation is done. */
+    translation = bdrv_get_translation_hint(dinfo->bdrv);
+    if (translation == BIOS_ATA_TRANSLATION_AUTO) {
+        bdrv_get_geometry_hint(dinfo->bdrv, &cylinders, &heads, &sectors);
+        if (cylinders <= 1024 && heads <= 16 && sectors <= 63) {
+            /* No translation. */
+            translation = 0;
+        } else {
+            /* LBA translation. */
+            translation = 1;
+        }
+    } else {
+        translation--;
+    }
+    hd_trans |= translation << ((dinfo->bus * 4) + (dinfo->unit * 2));
+    rtc_set_memory(s, 0x39, hd_trans);
+}
+
 /* convert boot_device letter to something recognizable by the bios */
 static int boot_device2nibble(char boot_device)
 {
@@ -338,37 +376,11 @@  static void cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 
     /* hard drives */
 
-    rtc_set_memory(s, 0x12, (hd_table[0] ? 0xf0 : 0) | (hd_table[1] ? 0x0f : 0));
-    if (hd_table[0])
-        cmos_init_hd(0x19, 0x1b, hd_table[0]->bdrv);
-    if (hd_table[1])
-        cmos_init_hd(0x1a, 0x24, hd_table[1]->bdrv);
-
-    val = 0;
     for (i = 0; i < 4; i++) {
         if (hd_table[i]) {
-            int cylinders, heads, sectors, translation;
-            /* NOTE: bdrv_get_geometry_hint() returns the physical
-                geometry.  It is always such that: 1 <= sects <= 63, 1
-                <= heads <= 16, 1 <= cylinders <= 16383. The BIOS
-                geometry can be different if a translation is done. */
-            translation = bdrv_get_translation_hint(hd_table[i]->bdrv);
-            if (translation == BIOS_ATA_TRANSLATION_AUTO) {
-                bdrv_get_geometry_hint(hd_table[i]->bdrv, &cylinders, &heads, &sectors);
-                if (cylinders <= 1024 && heads <= 16 && sectors <= 63) {
-                    /* No translation. */
-                    translation = 0;
-                } else {
-                    /* LBA translation. */
-                    translation = 1;
-                }
-            } else {
-                translation--;
-            }
-            val |= translation << (i * 2);
+            set_cmos_hd_data(hd_table[i]);
         }
     }
-    rtc_set_memory(s, 0x39, val);
 }
 
 void ioport_set_a20(int enable)
@@ -820,6 +832,8 @@  static void pc_init1(ram_addr_t ram_size,
     DriveInfo *fd[MAX_FD];
     void *fw_cfg;
 
+    machine_ide_finalize = set_cmos_hd_data;
+
     if (ram_size >= 0xe0000000 ) {
         above_4g_mem_size = ram_size - 0xe0000000;
         below_4g_mem_size = 0xe0000000;
diff --git a/sysemu.h b/sysemu.h
index d0effa0..564d477 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -242,5 +242,6 @@  void usb_info(Monitor *mon);
 void rtc_change_mon_event(struct tm *tm);
 
 void register_devices(void);
+extern void (*machine_ide_finalize)(DriveInfo *dinfo);
 
 #endif
diff --git a/vl.c b/vl.c
index 4fb55b8..f92bfeb 100644
--- a/vl.c
+++ b/vl.c
@@ -228,6 +228,7 @@  int ctrl_grab = 0;
 unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 int boot_menu;
+void (*machine_ide_finalize)(DriveInfo *dinfo);
 
 int nb_numa_nodes;
 uint64_t node_mem[MAX_NODES];
@@ -269,6 +270,10 @@  static struct {
     { .driver = "vmware-svga",          .flag = &default_vga       },
 };
 
+static void default_ide_finalize(DriveInfo *dinfo)
+{
+}
+
 static int default_driver_check(QemuOpts *opts, void *opaque)
 {
     const char *driver = qemu_opt_get(opts, "driver");
@@ -2615,6 +2620,7 @@  int main(int argc, char **argv, char **envp)
 
     module_call_init(MODULE_INIT_MACHINE);
     machine = find_default_machine();
+    machine_ide_finalize = default_ide_finalize;
     cpu_model = NULL;
     initrd_filename = NULL;
     ram_size = 0;