[Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Laszlo Ersek]

This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
Jan. Addressing comments from Markus.

Jan, can you give it another try please? I realize this is getting old
pretty quick, so don't bother if you don't want to.

Cc: Jan Tomko <jtomko@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Thanks
Laszlo

Laszlo Ersek (3):
  hw/i386/pc: factor out pc_cmos_init_floppy()
  hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
  hw/i386/pc: don't carry FDC from pc_basic_device_init() to
    pc_cmos_init()

 include/hw/i386/pc.h |   3 +-
 hw/i386/pc.c         | 129 ++++++++++++++++++++++++++++++++++++++-------------
 hw/i386/pc_piix.c    |   5 +-
 hw/i386/pc_q35.c     |   5 +-
 4 files changed, 101 insertions(+), 41 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] hw/i386/pc: factor out pc_cmos_init_floppy()

[Laszlo Ersek]

Extract the pc_cmos_init_floppy() function from pc_cmos_init(). The
function sets two RTC registers: floppy drive types (0x10), overwriting
the earlier value in there), and REG_EQUIPMENT_BYTE (0x14), setting bits
in the prior value.

Cc: Jan Tomko <jtomko@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v1:
    - no changes relative to RFC

 hw/i386/pc.c | 67 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7072930..4a835be 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -294,6 +294,42 @@ static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
     set_boot_dev(opaque, boot_device, errp);
 }
 
+static void pc_cmos_init_floppy(ISADevice *rtc_state, ISADevice *floppy)
+{
+    int val, nb, i;
+    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
+
+    /* floppy type */
+    if (floppy) {
+        for (i = 0; i < 2; i++) {
+            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
+        }
+    }
+    val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
+        cmos_get_fd_drive_type(fd_type[1]);
+    rtc_set_memory(rtc_state, 0x10, val);
+
+    val = rtc_get_memory(rtc_state, REG_EQUIPMENT_BYTE);
+    nb = 0;
+    if (fd_type[0] < FDRIVE_DRV_NONE) {
+        nb++;
+    }
+    if (fd_type[1] < FDRIVE_DRV_NONE) {
+        nb++;
+    }
+    switch (nb) {
+    case 0:
+        break;
+    case 1:
+        val |= 0x01; /* 1 drive, ready for boot */
+        break;
+    case 2:
+        val |= 0x41; /* 2 drives, ready for boot */
+        break;
+    }
+    rtc_set_memory(rtc_state, REG_EQUIPMENT_BYTE, val);
+}
+
 typedef struct pc_cmos_init_late_arg {
     ISADevice *rtc_state;
     BusState *idebus[2];
@@ -344,8 +380,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   ISADevice *floppy, BusState *idebus0, BusState *idebus1,
                   ISADevice *s)
 {
-    int val, nb, i;
-    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
+    int val;
     static pc_cmos_init_late_arg arg;
     PCMachineState *pc_machine = PC_MACHINE(machine);
     Error *local_err = NULL;
@@ -402,37 +437,11 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
         exit(1);
     }
 
-    /* floppy type */
-    if (floppy) {
-        for (i = 0; i < 2; i++) {
-            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
-        }
-    }
-    val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
-        cmos_get_fd_drive_type(fd_type[1]);
-    rtc_set_memory(s, 0x10, val);
-
     val = 0;
-    nb = 0;
-    if (fd_type[0] < FDRIVE_DRV_NONE) {
-        nb++;
-    }
-    if (fd_type[1] < FDRIVE_DRV_NONE) {
-        nb++;
-    }
-    switch (nb) {
-    case 0:
-        break;
-    case 1:
-        val |= 0x01; /* 1 drive, ready for boot */
-        break;
-    case 2:
-        val |= 0x41; /* 2 drives, ready for boot */
-        break;
-    }
     val |= 0x02; /* FPU is there */
     val |= 0x04; /* PS/2 mouse installed */
     rtc_set_memory(s, REG_EQUIPMENT_BYTE, val);
+    pc_cmos_init_floppy(s, floppy);
 
     /* hard drives */
     arg.rtc_state = s;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS

[Laszlo Ersek]

With the pc-q35-2.4 machine type, if the user creates an ISA FDC manually:

  -device isa-fdc,driveA=drive-fdc0-0-0 \
  -drive file=...,if=none,id=drive-fdc0-0-0,format=raw

then the board-default FDC will be skipped, and only the explicitly
requested FDC will exist. qtree-wise, this is correct; however such an FDC
is currently not registered in the CMOS, because that code is only reached
for the board-default FDC.

The pc_cmos_init_late() one-shot reset handler -- one-shot because the
CMOS is not reprogrammed during warm reset -- should search for any ISA
FDC devices, created implicitly (by board code) or explicitly, and set the
CMOS accordingly to the ISA FDC(s) with iobase=0x3f0:

- if there is no such FDC, report both drives absent,
- if there is exactly one such FDC, report its drives in the CMOS,
- if there are more than one such FDCs, then pick one (it is not specified
  which one), and print a warning about the ambiguity.

Cc: Jan Tomko <jtomko@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Blue Swirl <blauwirbel@gmail.com>
Reported-by: Jan Tomko <jtomko@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v1:
    
    - Look for ISA FDC with iobase=0x3f0, not distinguishing the board
      default ISA FDC at all [Markus]. Handling the board-default ISA FDC
      uniformly requires scanning the "/unattached" container.
    
    - Checkpatch barfs at this patch (it doesn't recognize the compound
      literal (const char *[]) { ... }). I didn't care; this pattern is
      widely used in the tree. Just run
    
      git grep -F '*[])'
    
      CC'd Blue Swirl for this reason.

 hw/i386/pc.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4a835be..bdb2d2d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -335,6 +335,37 @@ typedef struct pc_cmos_init_late_arg {
     BusState *idebus[2];
 } pc_cmos_init_late_arg;
 
+typedef struct check_fdc_state {
+    ISADevice *floppy;
+    bool multiple;
+} CheckFdcState;
+
+static int check_fdc(Object *obj, void *opaque)
+{
+    CheckFdcState *state = opaque;
+    Object *fdc;
+    uint32_t iobase;
+    Error *local_err = NULL;
+
+    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
+    if (!fdc) {
+        return 0;
+    }
+
+    iobase = object_property_get_int(obj, "iobase", &local_err);
+    if (iobase != 0x3f0) {
+        error_free(local_err);
+        return 0;
+    }
+
+    if (state->floppy) {
+        state->multiple = true;
+    } else {
+        state->floppy = ISA_DEVICE(obj);
+    }
+    return 0;
+}
+
 static void pc_cmos_init_late(void *opaque)
 {
     pc_cmos_init_late_arg *arg = opaque;
@@ -343,6 +374,9 @@ static void pc_cmos_init_late(void *opaque)
     int8_t heads, sectors;
     int val;
     int i, trans;
+    const char **container_path;
+    Object *container;
+    CheckFdcState state = { 0 };
 
     val = 0;
     if (ide_get_geometry(arg->idebus[0], 0,
@@ -372,6 +406,26 @@ static void pc_cmos_init_late(void *opaque)
     }
     rtc_set_memory(s, 0x39, val);
 
+    /*
+     * Locate the FDC at IO address 0x3f0, and configure the CMOS registers
+     * accordingly.
+     */
+    container_path = (const char *[]) { "/unattached", "/peripheral",
+                                        "/peripheral-anon", NULL };
+    do {
+        container = container_get(qdev_get_machine(), *container_path);
+        object_child_foreach(container, check_fdc, &state);
+        ++container_path;
+    } while (*container_path);
+
+    if (state.multiple) {
+        error_report("warning: multiple floppy disk controllers with "
+                     "iobase=0x3f0 have been found;\n"
+                     "the one being picked for CMOS setup might not reflect "
+                     "your intent");
+    }
+    pc_cmos_init_floppy(s, state.floppy);
+
     qemu_unregister_reset(pc_cmos_init_late, opaque);
 }
 
@@ -441,9 +495,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     val |= 0x02; /* FPU is there */
     val |= 0x04; /* PS/2 mouse installed */
     rtc_set_memory(s, REG_EQUIPMENT_BYTE, val);
-    pc_cmos_init_floppy(s, floppy);
 
-    /* hard drives */
+    /* hard drives and FDC */
     arg.rtc_state = s;
     arg.idebus[0] = idebus0;
     arg.idebus[1] = idebus1;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] hw/i386/pc: don't carry FDC from pc_basic_device_init() to pc_cmos_init()

[Laszlo Ersek]

Thanks to the last patch, pc_cmos_init() doesn't need the (optional)
board-default FDC any longer as an input parameter. Update
pc_basic_device_init() not to hand it back to pc_init1() / pc_q35_init(),
and update the latter not to carry the FDC to pc_cmos_init(). This
simplifies the code.

pc_init1() | pc_q35_init()
  pc_basic_device_init()
  pc_cmos_init()

Cc: Jan Tomko <jtomko@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v1:
    - new in v1 (relative to RFC)

 include/hw/i386/pc.h | 3 +--
 hw/i386/pc.c         | 7 ++++---
 hw/i386/pc_piix.c    | 5 ++---
 hw/i386/pc_q35.c     | 5 ++---
 4 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 86c5651..c1ad48f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -198,13 +198,12 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           bool create_fdctrl,
-                          ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs);
 void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
 void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   const char *boot_device, MachineState *machine,
-                  ISADevice *floppy, BusState *ide0, BusState *ide1,
+                  BusState *ide0, BusState *ide1,
                   ISADevice *s);
 void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_pci_device_init(PCIBus *pci_bus);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bdb2d2d..0505c20 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -431,7 +431,7 @@ static void pc_cmos_init_late(void *opaque)
 
 void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   const char *boot_device, MachineState *machine,
-                  ISADevice *floppy, BusState *idebus0, BusState *idebus1,
+                  BusState *idebus0, BusState *idebus1,
                   ISADevice *s)
 {
     int val;
@@ -1464,7 +1464,6 @@ static const MemoryRegionOps ioportF0_io_ops = {
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           bool create_fdctrl,
-                          ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs)
 {
@@ -1560,7 +1559,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
         fd[i] = drive_get(IF_FLOPPY, 0, i);
         create_fdctrl |= !!fd[i];
     }
-    *floppy = create_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
+    if (create_fdctrl) {
+        fdctrl_init_isa(isa_bus, fd);
+    }
 }
 
 void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e142f75..3f534cf 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -94,7 +94,6 @@ static void pc_init1(MachineState *machine)
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
-    ISADevice *floppy;
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
@@ -241,7 +240,7 @@ static void pc_init1(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, true,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
@@ -273,7 +272,7 @@ static void pc_init1(MachineState *machine)
     }
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, machine->boot_order,
-                 machine, floppy, idebus[0], idebus[1], rtc_state);
+                 machine, idebus[0], idebus[1], rtc_state);
 
     if (pci_enabled && usb_enabled()) {
         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 082cd93..dcc3b7b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -73,7 +73,6 @@ static void pc_q35_init(MachineState *machine)
     PCIDevice *lpc;
     BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
-    ISADevice *floppy;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
     MemoryRegion *ram_memory;
@@ -249,7 +248,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, !mc->no_floppy, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, !mc->no_floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
@@ -278,7 +277,7 @@ static void pc_q35_init(MachineState *machine)
                       8, NULL, 0);
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, machine->boot_order,
-                 machine, floppy, idebus[0], idebus[1], rtc_state);
+                 machine, idebus[0], idebus[1], rtc_state);
 
     /* the rest devices to which pci devfn is automatically assigned */
     pc_vga_init(isa_bus, host_bus);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Ján Tomko]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Tue, Jun 23, 2015 at 06:58:41PM +0200, Laszlo Ersek wrote:
> This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
> Jan. Addressing comments from Markus.
> 
> Jan, can you give it another try please? I realize this is getting old
> pretty quick, so don't bother if you don't want to.
> 

This series also works for me.

Jan

> Cc: Jan Tomko <jtomko@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   hw/i386/pc: factor out pc_cmos_init_floppy()
>   hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
>   hw/i386/pc: don't carry FDC from pc_basic_device_init() to
>     pc_cmos_init()
> 
>  include/hw/i386/pc.h |   3 +-
>  hw/i386/pc.c         | 129 ++++++++++++++++++++++++++++++++++++++-------------
>  hw/i386/pc_piix.c    |   5 +-
>  hw/i386/pc_q35.c     |   5 +-
>  4 files changed, 101 insertions(+), 41 deletions(-)
> 
> -- 
> 1.8.3.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[John Snow]

On 06/23/2015 12:58 PM, Laszlo Ersek wrote:
> This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
> Jan. Addressing comments from Markus.
> 
> Jan, can you give it another try please? I realize this is getting old
> pretty quick, so don't bother if you don't want to.
> 
> Cc: Jan Tomko <jtomko@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (3):
>   hw/i386/pc: factor out pc_cmos_init_floppy()
>   hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
>   hw/i386/pc: don't carry FDC from pc_basic_device_init() to
>     pc_cmos_init()
> 
>  include/hw/i386/pc.h |   3 +-
>  hw/i386/pc.c         | 129 ++++++++++++++++++++++++++++++++++++++-------------
>  hw/i386/pc_piix.c    |   5 +-
>  hw/i386/pc_q35.c     |   5 +-
>  4 files changed, 101 insertions(+), 41 deletions(-)
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> With the pc-q35-2.4 machine type, if the user creates an ISA FDC manually:
>
>   -device isa-fdc,driveA=drive-fdc0-0-0 \
>   -drive file=...,if=none,id=drive-fdc0-0-0,format=raw
>
> then the board-default FDC will be skipped, and only the explicitly
> requested FDC will exist. qtree-wise, this is correct; however such an FDC
> is currently not registered in the CMOS, because that code is only reached
> for the board-default FDC.
>
> The pc_cmos_init_late() one-shot reset handler -- one-shot because the
> CMOS is not reprogrammed during warm reset -- should search for any ISA
> FDC devices, created implicitly (by board code) or explicitly, and set the
> CMOS accordingly to the ISA FDC(s) with iobase=0x3f0:
>
> - if there is no such FDC, report both drives absent,
> - if there is exactly one such FDC, report its drives in the CMOS,
> - if there are more than one such FDCs, then pick one (it is not specified
>   which one), and print a warning about the ambiguity.
>
> Cc: Jan Tomko <jtomko@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Reported-by: Jan Tomko <jtomko@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     v1:
>     
>     - Look for ISA FDC with iobase=0x3f0, not distinguishing the board
>       default ISA FDC at all [Markus]. Handling the board-default ISA FDC
>       uniformly requires scanning the "/unattached" container.
>     
>     - Checkpatch barfs at this patch (it doesn't recognize the compound
>       literal (const char *[]) { ... }). I didn't care; this pattern is
>       widely used in the tree. Just run
>     
>       git grep -F '*[])'
>     
>       CC'd Blue Swirl for this reason.
>
>  hw/i386/pc.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 4a835be..bdb2d2d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -335,6 +335,37 @@ typedef struct pc_cmos_init_late_arg {
>      BusState *idebus[2];
>  } pc_cmos_init_late_arg;
>  
> +typedef struct check_fdc_state {
> +    ISADevice *floppy;
> +    bool multiple;
> +} CheckFdcState;
> +
> +static int check_fdc(Object *obj, void *opaque)
> +{
> +    CheckFdcState *state = opaque;
> +    Object *fdc;
> +    uint32_t iobase;
> +    Error *local_err = NULL;
> +
> +    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
> +    if (!fdc) {
> +        return 0;
> +    }
> +
> +    iobase = object_property_get_int(obj, "iobase", &local_err);
> +    if (iobase != 0x3f0) {
> +        error_free(local_err);
> +        return 0;
> +    }

Works, because object_property_get_int() returns -1 on error, and
(uint32_t)-1 != 0x3f0.

Perhaps (local_err || iobase != 0x3f0) would be more obvious.

> +
> +    if (state->floppy) {
> +        state->multiple = true;
> +    } else {
> +        state->floppy = ISA_DEVICE(obj);
> +    }
> +    return 0;
> +}
> +
>  static void pc_cmos_init_late(void *opaque)
>  {
>      pc_cmos_init_late_arg *arg = opaque;
> @@ -343,6 +374,9 @@ static void pc_cmos_init_late(void *opaque)
>      int8_t heads, sectors;
>      int val;
>      int i, trans;
> +    const char **container_path;
> +    Object *container;
> +    CheckFdcState state = { 0 };
>  
>      val = 0;
>      if (ide_get_geometry(arg->idebus[0], 0,
> @@ -372,6 +406,26 @@ static void pc_cmos_init_late(void *opaque)
>      }
>      rtc_set_memory(s, 0x39, val);
>  
> +    /*
> +     * Locate the FDC at IO address 0x3f0, and configure the CMOS registers
> +     * accordingly.
> +     */
> +    container_path = (const char *[]) { "/unattached", "/peripheral",
> +                                        "/peripheral-anon", NULL };
> +    do {
> +        container = container_get(qdev_get_machine(), *container_path);
> +        object_child_foreach(container, check_fdc, &state);
> +        ++container_path;
> +    } while (*container_path);

I'd prefer a for loop, because it keeps the loop control in one place.
Stepping an index instead of the only pointer lets us use an
old-fashioned array initializer instead of a fancy compound literal.

    static const char *container_path[] = {
        "/unattached", "/peripheral", "/peripheral-anon"
    };

    for (i = 0; i < ARRAY_SIZE(container_path); i++) {
        container = container_get(qdev_get_machine(), container_path[i]);
        object_child_foreach(container, check_fdc, &state);
    }

Matter of taste.  I like old-fashioned and non-fancy :)

> +
> +    if (state.multiple) {
> +        error_report("warning: multiple floppy disk controllers with "
> +                     "iobase=0x3f0 have been found;\n"
> +                     "the one being picked for CMOS setup might not reflect "
> +                     "your intent");
> +    }
> +    pc_cmos_init_floppy(s, state.floppy);
> +
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }
>  
> @@ -441,9 +495,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      val |= 0x02; /* FPU is there */
>      val |= 0x04; /* PS/2 mouse installed */
>      rtc_set_memory(s, REG_EQUIPMENT_BYTE, val);
> -    pc_cmos_init_floppy(s, floppy);
>  
> -    /* hard drives */
> +    /* hard drives and FDC */
>      arg.rtc_state = s;
>      arg.idebus[0] = idebus0;
>      arg.idebus[1] = idebus1;

Re: [Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
> Jan. Addressing comments from Markus.
>
> Jan, can you give it another try please? I realize this is getting old
> pretty quick, so don't bother if you don't want to.
>
> Cc: Jan Tomko <jtomko@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
>   hw/i386/pc: factor out pc_cmos_init_floppy()
>   hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
>   hw/i386/pc: don't carry FDC from pc_basic_device_init() to
>     pc_cmos_init()
>
>  include/hw/i386/pc.h |   3 +-
>  hw/i386/pc.c         | 129 ++++++++++++++++++++++++++++++++++++++-------------
>  hw/i386/pc_piix.c    |   5 +-
>  hw/i386/pc_q35.c     |   5 +-
>  4 files changed, 101 insertions(+), 41 deletions(-)

Matter-of-taste comments on PATCH 3.  Regardless:

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] update CMOS for ISA-FDC with iobase=0x3f0

[Laszlo Ersek]

On 06/25/15 09:55, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> This is (again) for the other pc-q35-2.4 ISA-FDC problem reported by
>> Jan. Addressing comments from Markus.
>>
>> Jan, can you give it another try please? I realize this is getting old
>> pretty quick, so don't bother if you don't want to.
>>
>> Cc: Jan Tomko <jtomko@redhat.com>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (3):
>>   hw/i386/pc: factor out pc_cmos_init_floppy()
>>   hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS
>>   hw/i386/pc: don't carry FDC from pc_basic_device_init() to
>>     pc_cmos_init()
>>
>>  include/hw/i386/pc.h |   3 +-
>>  hw/i386/pc.c         | 129 ++++++++++++++++++++++++++++++++++++++-------------
>>  hw/i386/pc_piix.c    |   5 +-
>>  hw/i386/pc_q35.c     |   5 +-
>>  4 files changed, 101 insertions(+), 41 deletions(-)
> 
> Matter-of-taste comments on PATCH 3.  Regardless:
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks! I'll try to submit a new version soon, with John's and your R-b
added, in order to address your patch 3 remarks.

Thanks for your reviews.
Laszlo