[PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Paolo Bonzini]

As per $SUBJECT, one less global and one more syntactic sugar property.
Like -no-fd-bootchk, this could be deprecated but it can be done
separately---and would slightly worsen documentation, so I'm leaving
it aside for now.

Paolo

Paolo Bonzini (2):
  ide: collapse parameters to ide_init_drive
  ide, vl: turn -win2k-hack into a property on IDE devices

 include/hw/ide/internal.h |  8 +++-----
 include/sysemu/sysemu.h   |  1 -
 hw/ide/core.c             | 43 ++++++++++++++++++---------------------
 hw/ide/qdev.c             |  6 ++----
 system/globals.c          |  1 -
 system/vl.c               |  2 +-
 qemu-options.hx           |  3 ++-
 7 files changed, 28 insertions(+), 36 deletions(-)

-- 
2.43.2

[PATCH 1/2] ide: collapse parameters to ide_init_drive

[Paolo Bonzini]

All calls to ide_init_drive comes from ide_dev_initfn.  Just pass down the
IDEDevice (IDEState is kinda obsolete and should be merged into IDEDevice).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/ide/internal.h |  6 +-----
 hw/ide/core.c             | 40 ++++++++++++++++++---------------------
 hw/ide/qdev.c             |  5 +----
 3 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 3bdcc75597d..0ad0d59d578 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -614,11 +614,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model,
-                   uint64_t wwn,
-                   uint32_t cylinders, uint32_t heads, uint32_t secs,
-                   int chs_trans, Error **errp);
+int ide_init_drive(IDEState *s, IDEDevice *dev, IDEDriveKind kind, Error **errp);
 void ide_exit(IDEState *s);
 void ide_bus_init_output_irq(IDEBus *bus, qemu_irq irq_out);
 int ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9c4a8129028..3c42d72ac25 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2589,24 +2589,20 @@ static const BlockDevOps ide_hd_block_ops = {
     .resize_cb = ide_resize_cb,
 };
 
-int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model,
-                   uint64_t wwn,
-                   uint32_t cylinders, uint32_t heads, uint32_t secs,
-                   int chs_trans, Error **errp)
+int ide_init_drive(IDEState *s, IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
     uint64_t nb_sectors;
 
-    s->blk = blk;
+    s->blk = dev->conf.blk;
     s->drive_kind = kind;
 
-    blk_get_geometry(blk, &nb_sectors);
-    s->cylinders = cylinders;
-    s->heads = s->drive_heads = heads;
-    s->sectors = s->drive_sectors = secs;
-    s->chs_trans = chs_trans;
+    blk_get_geometry(s->blk, &nb_sectors);
+    s->cylinders = dev->conf.cyls;
+    s->heads = s->drive_heads = dev->conf.heads;
+    s->sectors = s->drive_sectors = dev->conf.secs;
+    s->chs_trans = dev->chs_trans;
     s->nb_sectors = nb_sectors;
-    s->wwn = wwn;
+    s->wwn = dev->wwn;
     /* The SMART values should be preserved across power cycles
        but they aren't.  */
     s->smart_enabled = 1;
@@ -2614,26 +2610,26 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
     s->smart_errors = 0;
     s->smart_selftest_count = 0;
     if (kind == IDE_CD) {
-        blk_set_dev_ops(blk, &ide_cd_block_ops, s);
+        blk_set_dev_ops(s->blk, &ide_cd_block_ops, s);
     } else {
         if (!blk_is_inserted(s->blk)) {
             error_setg(errp, "Device needs media, but drive is empty");
             return -1;
         }
-        if (!blk_is_writable(blk)) {
+        if (!blk_is_writable(s->blk)) {
             error_setg(errp, "Can't use a read-only drive");
             return -1;
         }
-        blk_set_dev_ops(blk, &ide_hd_block_ops, s);
+        blk_set_dev_ops(s->blk, &ide_hd_block_ops, s);
     }
-    if (serial) {
-        pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
+    if (dev->serial) {
+        pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), dev->serial);
     } else {
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
                  "QM%05d", s->drive_serial);
     }
-    if (model) {
-        pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
+    if (dev->model) {
+        pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), dev->model);
     } else {
         switch (kind) {
         case IDE_CD:
@@ -2648,14 +2644,14 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
         }
     }
 
-    if (version) {
-        pstrcpy(s->version, sizeof(s->version), version);
+    if (dev->version) {
+        pstrcpy(s->version, sizeof(s->version), dev->version);
     } else {
         pstrcpy(s->version, sizeof(s->version), qemu_hw_version());
     }
 
     ide_reset(s);
-    blk_iostatus_enable(blk);
+    blk_iostatus_enable(s->blk);
     return 0;
 }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 1b3b4da01df..9244c5a0fdb 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -208,10 +208,7 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         return;
     }
 
-    if (ide_init_drive(s, dev->conf.blk, kind,
-                       dev->version, dev->serial, dev->model, dev->wwn,
-                       dev->conf.cyls, dev->conf.heads, dev->conf.secs,
-                       dev->chs_trans, errp) < 0) {
+    if (ide_init_drive(s, dev, kind, errp) < 0) {
         return;
     }
 
-- 
2.43.2

[PATCH 2/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/ide/internal.h | 2 ++
 include/sysemu/sysemu.h   | 1 -
 hw/ide/core.c             | 3 ++-
 hw/ide/qdev.c             | 1 +
 system/globals.c          | 1 -
 system/vl.c               | 2 +-
 qemu-options.hx           | 3 ++-
 7 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 0ad0d59d578..b743170d097 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -384,6 +384,7 @@ struct IDEState {
     int drive_serial;
     char drive_serial_str[21];
     char drive_model_str[41];
+    bool win2k_install_hack;
     uint64_t wwn;
     /* ide regs */
     uint8_t feature;
@@ -527,6 +528,7 @@ struct IDEDevice {
      * 0xffff        - reserved
      */
     uint16_t rotation_rate;
+    bool win2k_install_hack;
 };
 
 /* These are used for the error_status field of IDEBus */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 73a37949c24..eb1dc1e4eda 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -41,7 +41,6 @@ extern int graphic_height;
 extern int graphic_depth;
 extern int display_opengl;
 extern const char *keyboard_layout;
-extern int win2k_install_hack;
 extern int graphic_rotate;
 extern int old_param;
 extern uint8_t *boot_splash_filedata;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3c42d72ac25..3f8c0ede2a1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1059,7 +1059,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
                            ide_sector_write);
     }
 
-    if (win2k_install_hack && ((++s->irq_count % 16) == 0)) {
+    if (s->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
@@ -2597,6 +2597,7 @@ int ide_init_drive(IDEState *s, IDEDevice *dev, IDEDriveKind kind, Error **errp)
     s->drive_kind = kind;
 
     blk_get_geometry(s->blk, &nb_sectors);
+    s->win2k_install_hack = dev->win2k_install_hack;
     s->cylinders = dev->conf.cyls;
     s->heads = s->drive_heads = dev->conf.heads;
     s->sectors = s->drive_sectors = dev->conf.secs;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 9244c5a0fdb..61dba354302 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -41,6 +41,7 @@ static void idebus_unrealize(BusState *qdev);
 
 static Property ide_props[] = {
     DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
+    DEFINE_PROP_BOOL("win2k-install-hack", IDEDevice, win2k_install_hack, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/system/globals.c b/system/globals.c
index 5d0046ba105..e3535842010 100644
--- a/system/globals.c
+++ b/system/globals.c
@@ -40,7 +40,6 @@ int autostart = 1;
 int vga_interface_type = VGA_NONE;
 bool vga_interface_created;
 Chardev *parallel_hds[MAX_PARALLEL_PORTS];
-int win2k_install_hack;
 int graphic_rotate;
 QEMUOptionRom option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
diff --git a/system/vl.c b/system/vl.c
index 98bf0c386b4..e480afd7a00 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3265,7 +3265,7 @@ void qemu_init(int argc, char **argv)
                 pid_file = optarg;
                 break;
             case QEMU_OPTION_win2k_hack:
-                win2k_install_hack = 1;
+                object_register_sugar_prop("ide-device", "win2k-install-hack", "true", true);
                 break;
             case QEMU_OPTION_acpitable:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("acpi"),
diff --git a/qemu-options.hx b/qemu-options.hx
index 1136642c21d..9a47385c157 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2641,7 +2641,8 @@ SRST
 ``-win2k-hack``
     Use it when installing Windows 2000 to avoid a disk full bug. After
     Windows 2000 is installed, you no longer need this option (this
-    option slows down the IDE transfers).
+    option slows down the IDE transfers).  Synonym of ``-global
+    ide-device.win2k-install-hack=on``.
 ERST
 
 DEF("no-fd-bootchk", 0, QEMU_OPTION_no_fd_bootchk,
-- 
2.43.2

Re: [PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Philippe Mathieu-Daudé]

On 26/2/24 09:29, Paolo Bonzini wrote:
> As per $SUBJECT, one less global and one more syntactic sugar property.
> Like -no-fd-bootchk, this could be deprecated but it can be done
> separately---and would slightly worsen documentation, so I'm leaving
> it aside for now.
> 
> Paolo
> 
> Paolo Bonzini (2):
>    ide: collapse parameters to ide_init_drive
>    ide, vl: turn -win2k-hack into a property on IDE devices

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

(This series will be queued in 24h)

Re: [PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Philippe Mathieu-Daudé]

On 26/2/24 09:45, Philippe Mathieu-Daudé wrote:
> On 26/2/24 09:29, Paolo Bonzini wrote:
>> As per $SUBJECT, one less global and one more syntactic sugar property.
>> Like -no-fd-bootchk, this could be deprecated but it can be done
>> separately---and would slightly worsen documentation, so I'm leaving
>> it aside for now.
>>
>> Paolo
>>
>> Paolo Bonzini (2):
>>    ide: collapse parameters to ide_init_drive
>>    ide, vl: turn -win2k-hack into a property on IDE devices
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> (This series will be queued in 24h)

Need a rebase :/

Re: [PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Paolo Bonzini]

On Mon, Feb 26, 2024 at 9:50 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
> On 26/2/24 09:45, Philippe Mathieu-Daudé wrote:
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> > (This series will be queued in 24h)
>
> Need a rebase :/

The include/hw/ide/internal.h hunks simply have to move to
include/hw/ide/ide-dev.h. I'll send a pull request myself, thanks for
the review!

Paolo

Re: [PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Michael Tokarev]

26.02.2024 11:29, Paolo Bonzini wrote:
> As per $SUBJECT, one less global and one more syntactic sugar property.
> Like -no-fd-bootchk, this could be deprecated but it can be done
> separately---and would slightly worsen documentation, so I'm leaving
> it aside for now.

At this time, I don't think touching these options is a good thing.

On one hand, it is all over the internet, suggested when some old
OS doesn't work.

On another hand, these aren't used often these days.  This particular
option isn't necessary with windows 2000, at least with two different
kits of it which I have locally.  So finding the thing when you actually
need it (if it gets moved elsewhere) will be difficult.

On yet another, the code to handle this stuff is small enough and does
not require much to maintain, -- at least for now, maybe in future it
will be more difficult.

Thanks,

/mjt

Re: [PATCH 0/2] ide, vl: turn -win2k-hack into a property on IDE devices

[Paolo Bonzini]

On Mon, Feb 26, 2024 at 10:24 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
> On another hand, these aren't used often these days.  This particular
> option isn't necessary with windows 2000, at least with two different
> kits of it which I have locally.  So finding the thing when you actually
> need it (if it gets moved elsewhere) will be difficult.
>
> On yet another, the code to handle this stuff is small enough and does
> not require much to maintain, -- at least for now, maybe in future it
> will be more difficult.

Yes, I agree. The purpose of these patches is to keep the
implementation of the options separate from the implementation of the
workaround, and that makes things even simpler.

Paolo