[Qemu-devel] [RFC 0/2] update CMOS for single hand-created ISA-FDC

[Qemu-devel] [RFC 0/2] update CMOS for single hand-created ISA-FDC

[Laszlo Ersek]

This is for the other pc-q35-2.4 ISA-FDC problem reported by Jan.

Jan, can you give it a try pls?

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

Laszlo Ersek (2):
  hw/i386/pc: factor out pc_cmos_init_floppy()
  hw/i386/pc: reflect an explicitly created, sole FDC in the CMOS

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

-- 
1.8.3.1

[Qemu-devel] [RFC 1/2] 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>
---
 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 3f0d435..1ca0cdd 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] [RFC 2/2] hw/i386/pc: reflect an explicitly created, sole FDC 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 a single,
explicitly created ISA FDC device, if there has been no board default, and
reprogram the CMOS if found.

Cc: Jan Tomko <jtomko@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Jan Tomko <jtomko@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1ca0cdd..47a3082 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -333,8 +333,30 @@ static void pc_cmos_init_floppy(ISADevice *rtc_state, ISADevice *floppy)
 typedef struct pc_cmos_init_late_arg {
     ISADevice *rtc_state;
     BusState *idebus[2];
+    ISADevice *board_floppy;
 } 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;
+
+    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
+    if (fdc) {
+        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;
@@ -372,6 +394,29 @@ static void pc_cmos_init_late(void *opaque)
     }
     rtc_set_memory(s, 0x39, val);
 
+    /*
+     * If the board initialization code created no FDC, but exactly one FDC has
+     * been created since then explicitly, then we configure the CMOS registers
+     * now, in accordance with that one FDC.
+     */
+    if (arg->board_floppy == NULL) {
+        static const char * const paths[] = { "/peripheral",
+            "/peripheral-anon", NULL };
+        const char * const * path;
+        CheckFdcState state = { 0 };
+
+        for (path = paths; *path; ++path) {
+            Object *container;
+
+            container = container_get(qdev_get_machine(), *path);
+            object_child_foreach(container, check_fdc, &state);
+        }
+
+        if (state.floppy && !state.multiple) {
+            pc_cmos_init_floppy(s, state.floppy);
+        }
+    }
+
     qemu_unregister_reset(pc_cmos_init_late, opaque);
 }
 
@@ -447,6 +492,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     arg.rtc_state = s;
     arg.idebus[0] = idebus0;
     arg.idebus[1] = idebus1;
+    arg.board_floppy = floppy;
     qemu_register_reset(pc_cmos_init_late, &arg);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC 2/2] hw/i386/pc: reflect an explicitly created, sole FDC in the CMOS

[Markus Armbruster]

Wow, that was quick!

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 a single,
> explicitly created ISA FDC device, if there has been no board default, and
> reprogram the CMOS if found.

I don't think we want "single".  We want the isa-fdc at I/O address
0x3f0.

Test case:

    -M q35 -device isa-fdc,id=fdc1 -device isa-fdc,id=fdc2,iobase=0x370

should work, and CMOS should be set according to fdc1.

> Cc: Jan Tomko <jtomko@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Reported-by: Jan Tomko <jtomko@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/i386/pc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 1ca0cdd..47a3082 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -333,8 +333,30 @@ static void pc_cmos_init_floppy(ISADevice *rtc_state, ISADevice *floppy)
>  typedef struct pc_cmos_init_late_arg {
>      ISADevice *rtc_state;
>      BusState *idebus[2];
> +    ISADevice *board_floppy;
>  } 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;
> +
> +    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
> +    if (fdc) {
> +        if (state->floppy) {
> +            state->multiple = true;
> +        } else {
> +            state->floppy = ISA_DEVICE(obj);
> +        }
> +    }

Something like

    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
    if (fdc
        && object_property_get_int(obj, "iobase", &error_abort) == 0x3f0) {
        assert(!state->floppy);
        state->floppy = ISA_DEVICE(obj);
    }

The &error_abort is probably wrong, but I trust you get the general
idea.

The assertion is only valid if we actually prevent multiple ISA devices
claiming the same I/O port.  Checking... crap, multiple -device isa-fdc
are accepted.  No idea which one actually gets the port.

Two solutions:

* Find a way to determine whether the device is in possession of the
  port.  If it is, pick it and stop the search.  If not, continue the
  search.

  Alternative: always continue the search, assert that we pick at most
  one (because if we pick more, our "is in posession of the port" test
  is broken).

* Pick one that's easy to pick.  Might be the wrong one when there's
  more than one.  Feel free to print a warning then.  I'd happily accept
  such a stupid solution, as long as its stupidity is reasonably
  documented.

> +    return 0;
> +}
> +
>  static void pc_cmos_init_late(void *opaque)
>  {
>      pc_cmos_init_late_arg *arg = opaque;
> @@ -372,6 +394,29 @@ static void pc_cmos_init_late(void *opaque)
>      }
>      rtc_set_memory(s, 0x39, val);
>  
> +    /*
> +     * If the board initialization code created no FDC, but exactly one FDC has
> +     * been created since then explicitly, then we configure the CMOS registers
> +     * now, in accordance with that one FDC.
> +     */
> +    if (arg->board_floppy == NULL) {
> +        static const char * const paths[] = { "/peripheral",
> +            "/peripheral-anon", NULL };
> +        const char * const * path;
> +        CheckFdcState state = { 0 };
> +
> +        for (path = paths; *path; ++path) {
> +            Object *container;
> +
> +            container = container_get(qdev_get_machine(), *path);
> +            object_child_foreach(container, check_fdc, &state);
> +        }
> +
> +        if (state.floppy && !state.multiple) {
> +            pc_cmos_init_floppy(s, state.floppy);
> +        }
> +    }
> +
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }

This assumes that the onboard isa-fdc wins the fight over the I/O port.

Perhaps passing the onboard isa-fdc to check_fdc() would be cleaner.

> @@ -447,6 +492,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      arg.rtc_state = s;
>      arg.idebus[0] = idebus0;
>      arg.idebus[1] = idebus1;
> +    arg.board_floppy = floppy;
>      qemu_register_reset(pc_cmos_init_late, &arg);
>  }

Thanks for tackling this!

Re: [Qemu-devel] [RFC 0/2] update CMOS for single hand-created ISA-FDC

[Ján Tomko]

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

On Sat, Jun 20, 2015 at 04:43:46AM +0200, Laszlo Ersek wrote:
> This is for the other pc-q35-2.4 ISA-FDC problem reported by Jan.
> 
> Jan, can you give it a try pls?
> 

Works for me.

I tried the following command line with a Fedora 21 guest:
-device isa-fdc,driveA=drive-fdc0-0-0
-drive file=/var/lib/libvirt/images/floppy,if=none,id=drive-fdc0-0-0,format=raw

and I could see the floppy drive inside the guest.

Thanks!

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>
> 
> Laszlo Ersek (2):
>   hw/i386/pc: factor out pc_cmos_init_floppy()
>   hw/i386/pc: reflect an explicitly created, sole FDC in the CMOS
> 
>  hw/i386/pc.c | 113 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 84 insertions(+), 29 deletions(-)
> 
> -- 
> 1.8.3.1
> 

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