[Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Laszlo Ersek]

It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
Machine type.

Hence, let us not instantiate the FDC when all of the following apply:
- the machine type is pc-q35-2.4 or later,
- "-device isa-fdc" is not passed on the command line (nor in the config
  file),
- no "-drive if=floppy,..." is requested.

The first part of the code flow, updated by the patch, is as follows:

1. The "no_floppy = 1" machine class setting causes "default_floppy" in
   main() to become zero.

2. Hence default_drive() will not call drive_add() and drive_new() for
   IF_FLOPPY, index=0.

This alone would not suffice, because the pc_basic_device_init() function
creates the FDC regardless of actual floppy drives. Therefore,

3. pc_basic_device_init() should only create the FDC if that's desirable
   for the platform (= all PIIX types, and Q35 types earlier than 2.4),
   *or* the user requested at least one IF_FLOPPY drive.

4. If the FDC is not created, callers of pc_basic_device_init() --
   pc_q35_init() and pc_init1() -- will pass floppy=NULL to
   pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.

I verified the patch under the following scenarios, with "info qtree" and
with the MAP command of the UEFI shell (in OVMF):
- "pc" machine type -- no changes, FDC present,
- "pc-q35-2.3" machine type -- no changes, FDC present,
- "pc-q35" machine type with "-device isa-fdc" -- no changes, FDC present,
- "pc-q35" machine type with "-drive if=floppy,..." -- no changes, FDC
  present,
- "pc-q35" -- no FDC.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/i386/pc.h |  1 +
 hw/i386/pc.c         |  4 +++-
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     | 12 +++++++++---
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..55522b7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
 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 force_fdctrl,
                           ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 769eb25..b77d9b4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
 
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
+                          bool force_fdctrl,
                           ISADevice **floppy,
                           bool no_vmport,
                           uint32 hpet_irqs)
@@ -1489,8 +1490,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
 
     for(i = 0; i < MAX_FD; i++) {
         fd[i] = drive_get(IF_FLOPPY, 0, i);
+        force_fdctrl |= !!fd[i];
     }
-    *floppy = fdctrl_init_isa(isa_bus, fd);
+    *floppy = force_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
 }
 
 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 212e263..9f530c4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e67f2de..4d68340 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -60,6 +60,7 @@ static bool smbios_uuid_encoded = true;
  */
 static bool gigabyte_align = true;
 static bool has_reserved_memory = true;
+static bool force_fdctrl;
 
 /* PC hardware initialisation */
 static void pc_q35_init(MachineState *machine)
@@ -250,7 +251,7 @@ static void pc_q35_init(MachineState *machine)
     }
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
                          (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
@@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
 
 static void pc_compat_2_3(MachineState *machine)
 {
+    force_fdctrl = true;
 }
 
 static void pc_compat_2_2(MachineState *machine)
@@ -424,7 +426,8 @@ static void pc_q35_init_1_4(MachineState *machine)
 #define PC_Q35_2_4_MACHINE_OPTIONS                      \
     PC_Q35_MACHINE_OPTIONS,                             \
     .default_machine_opts = "firmware=bios-256k.bin",   \
-    .default_display = "std"
+    .default_display = "std",                           \
+    .no_floppy = 1
 
 static QEMUMachine pc_q35_machine_v2_4 = {
     PC_Q35_2_4_MACHINE_OPTIONS,
@@ -433,7 +436,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
     .init = pc_q35_init,
 };
 
-#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
+#define PC_Q35_2_3_MACHINE_OPTIONS                      \
+    PC_Q35_MACHINE_OPTIONS,                             \
+    .default_machine_opts = "firmware=bios-256k.bin",   \
+    .default_display = "std"
 
 static QEMUMachine pc_q35_machine_v2_3 = {
     PC_Q35_2_3_MACHINE_OPTIONS,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Paolo Bonzini]

On 27/05/2015 23:06, Laszlo Ersek wrote:
> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> Machine type.
> 
> Hence, let us not instantiate the FDC when all of the following apply:
> - the machine type is pc-q35-2.4 or later,
> - "-device isa-fdc" is not passed on the command line (nor in the config
>   file),
> - no "-drive if=floppy,..." is requested.
> 
> The first part of the code flow, updated by the patch, is as follows:
> 
> 1. The "no_floppy = 1" machine class setting causes "default_floppy" in
>    main() to become zero.
> 
> 2. Hence default_drive() will not call drive_add() and drive_new() for
>    IF_FLOPPY, index=0.
> 
> This alone would not suffice, because the pc_basic_device_init() function
> creates the FDC regardless of actual floppy drives. Therefore,
> 
> 3. pc_basic_device_init() should only create the FDC if that's desirable
>    for the platform (= all PIIX types, and Q35 types earlier than 2.4),
>    *or* the user requested at least one IF_FLOPPY drive.
> 
> 4. If the FDC is not created, callers of pc_basic_device_init() --
>    pc_q35_init() and pc_init1() -- will pass floppy=NULL to
>    pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.
> 
> I verified the patch under the following scenarios, with "info qtree" and
> with the MAP command of the UEFI shell (in OVMF):
> - "pc" machine type -- no changes, FDC present,
> - "pc-q35-2.3" machine type -- no changes, FDC present,
> - "pc-q35" machine type with "-device isa-fdc" -- no changes, FDC present,
> - "pc-q35" machine type with "-drive if=floppy,..." -- no changes, FDC
>   present,
> - "pc-q35" -- no FDC.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  1 +
>  hw/i386/pc.c         |  4 +++-
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 12 +++++++++---
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..55522b7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
>  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 force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 769eb25..b77d9b4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
>  
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
> +                          bool force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs)
> @@ -1489,8 +1490,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>  
>      for(i = 0; i < MAX_FD; i++) {
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
> +        force_fdctrl |= !!fd[i];
>      }
> -    *floppy = fdctrl_init_isa(isa_bus, fd);
> +    *floppy = force_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
>  }
>  
>  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 212e263..9f530c4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
>  
>      pc_nic_init(isa_bus, pci_bus);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e67f2de..4d68340 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -60,6 +60,7 @@ static bool smbios_uuid_encoded = true;
>   */
>  static bool gigabyte_align = true;
>  static bool has_reserved_memory = true;
> +static bool force_fdctrl;
>  
>  /* PC hardware initialisation */
>  static void pc_q35_init(MachineState *machine)
> @@ -250,7 +251,7 @@ static void pc_q35_init(MachineState *machine)
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
>  
>      /* connect pm stuff to lpc */
> @@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
>  
>  static void pc_compat_2_3(MachineState *machine)
>  {
> +    force_fdctrl = true;
>  }
>  
>  static void pc_compat_2_2(MachineState *machine)
> @@ -424,7 +426,8 @@ static void pc_q35_init_1_4(MachineState *machine)
>  #define PC_Q35_2_4_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
>      .default_machine_opts = "firmware=bios-256k.bin",   \
> -    .default_display = "std"
> +    .default_display = "std",                           \
> +    .no_floppy = 1
>  
>  static QEMUMachine pc_q35_machine_v2_4 = {
>      PC_Q35_2_4_MACHINE_OPTIONS,
> @@ -433,7 +436,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
>      .init = pc_q35_init,
>  };
>  
> -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> +#define PC_Q35_2_3_MACHINE_OPTIONS                      \
> +    PC_Q35_MACHINE_OPTIONS,                             \
> +    .default_machine_opts = "firmware=bios-256k.bin",   \
> +    .default_display = "std"
>  
>  static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> Machine type.

Surely Unnecessary Crap can still Kill Security.

> Hence, let us not instantiate the FDC when all of the following apply:
> - the machine type is pc-q35-2.4 or later,
> - "-device isa-fdc" is not passed on the command line (nor in the config
>   file),
> - no "-drive if=floppy,..." is requested.
>
> The first part of the code flow, updated by the patch, is as follows:
>
> 1. The "no_floppy = 1" machine class setting causes "default_floppy" in
>    main() to become zero.

Yup.  It's copied around some, but that's its only real effect.

> 2. Hence default_drive() will not call drive_add() and drive_new() for
>    IF_FLOPPY, index=0.

This is the backend created for the board to pick up.  If the board
doesn't pick it up, the backend remains unused, which is confusing.
Boards that don't pick up IF_FLOPPY backends should set no_floppy to
avoid the confusion.  Aside: precious few do.

In short: good.

> This alone would not suffice, because the pc_basic_device_init() function
> creates the FDC regardless of actual floppy drives. Therefore,
>
> 3. pc_basic_device_init() should only create the FDC if that's desirable
>    for the platform (= all PIIX types, and Q35 types earlier than 2.4),

This is the conservative choice.

Arguably, Q35 got versioned prematurely.  Dropping the FDC from Q35
unconditionally has been proposed, on the basis that it wasn't fit for
serious use in prior releases anyway.  I think if we decide we're fine
with changing the released Q35 machine types incompatibly, then we
should consider going all the way and simply drop them.

But let's not get review of this patch bogged down in a Q35 versioning
debate.  Whatever we want done there, we can do on top of this patch.

>    *or* the user requested at least one IF_FLOPPY drive.

This is the magic to keep -drive if=floppy just work with Q35.

On the one hand, we've come to hate command line magic.

On the other hand, this is exactly the kind of magic -drive does.  For
instance, PC boards create lsi53c895a SCSI HBAs for -drive if=scsi.
Your patch makes them create an isa-fdc for -drive if=floppy.

Magic or no magic, I don't care.  Whatever it takes to get the FDC out
of Q35.

If the magic should run into review trouble, putting it into its own
patch may help keeping the non-magic (and hopefully uncontroversial)
parts out of the debate.

> 4. If the FDC is not created, callers of pc_basic_device_init() --
>    pc_q35_init() and pc_init1() -- will pass floppy=NULL to
>    pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.
>
> I verified the patch under the following scenarios, with "info qtree" and
> with the MAP command of the UEFI shell (in OVMF):
> - "pc" machine type -- no changes, FDC present,
> - "pc-q35-2.3" machine type -- no changes, FDC present,
> - "pc-q35" machine type with "-device isa-fdc" -- no changes, FDC present,

Perhaps worth mentioning somewhere in the commit message: instead of the
arcane -global isa-fdc.driveA=... (which breaks down when you have more
than one isa-fdc), you simply do the regular -device isa-fdc,driveA=...

Discussed in a bit more details in
<http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg03121.html>.

> - "pc-q35" machine type with "-drive if=floppy,..." -- no changes, FDC
>   present,
> - "pc-q35" -- no FDC.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  1 +
>  hw/i386/pc.c         |  4 +++-
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 12 +++++++++---
>  4 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..55522b7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
>  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 force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs);

The name @force_fdctrl gave me pause: what are we forcing here?

I guess your rationale is something like "force creation of FDC even
when there are no if=floppy drives."  Hmm.

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 769eb25..b77d9b4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
>  
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
> +                          bool force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs)
> @@ -1489,8 +1490,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>  
>      for(i = 0; i < MAX_FD; i++) {
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
> +        force_fdctrl |= !!fd[i];
>      }
> -    *floppy = fdctrl_init_isa(isa_bus, fd);
> +    *floppy = force_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
>  }
>  
>  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 212e263..9f530c4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
>  
>      pc_nic_init(isa_bus, pci_bus);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e67f2de..4d68340 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -60,6 +60,7 @@ static bool smbios_uuid_encoded = true;
>   */
>  static bool gigabyte_align = true;
>  static bool has_reserved_memory = true;
> +static bool force_fdctrl;
>  
>  /* PC hardware initialisation */
>  static void pc_q35_init(MachineState *machine)
> @@ -250,7 +251,7 @@ static void pc_q35_init(MachineState *machine)
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
>  
>      /* connect pm stuff to lpc */

If we can get from MachineState *machine to its MachineClass, we could
use MachineClass member no_floppy, and not need force_fdctrl.  Probably
a wash.

> @@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
>  
>  static void pc_compat_2_3(MachineState *machine)
>  {
> +    force_fdctrl = true;
>  }
>  
>  static void pc_compat_2_2(MachineState *machine)
> @@ -424,7 +426,8 @@ static void pc_q35_init_1_4(MachineState *machine)
>  #define PC_Q35_2_4_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
>      .default_machine_opts = "firmware=bios-256k.bin",   \
> -    .default_display = "std"
> +    .default_display = "std",                           \
> +    .no_floppy = 1
>  
>  static QEMUMachine pc_q35_machine_v2_4 = {
>      PC_Q35_2_4_MACHINE_OPTIONS,
> @@ -433,7 +436,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
>      .init = pc_q35_init,
>  };
>  
> -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> +#define PC_Q35_2_3_MACHINE_OPTIONS                      \
> +    PC_Q35_MACHINE_OPTIONS,                             \
> +    .default_machine_opts = "firmware=bios-256k.bin",   \
> +    .default_display = "std"
>  
>  static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,

If you want to try ideas from my review, go ahead.  Even if not:

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

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Paolo Bonzini]

On 28/05/2015 10:39, Markus Armbruster wrote:
> > -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> > +    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
> >                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
> >  
> >      /* connect pm stuff to lpc */
> 
> If we can get from MachineState *machine to its MachineClass,

Sure you can, it's just MACHINE_GET_CLASS(machine).

Paolo

> we could
> use MachineClass member no_floppy, and not need force_fdctrl.  Probably
> a wash.

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Kevin Wolf]

Copying John Snow (FDC maintainer) and the qemu-block list.

Am 27.05.2015 um 23:06 hat Laszlo Ersek geschrieben:
> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> Machine type.
> 
> Hence, let us not instantiate the FDC when all of the following apply:
> - the machine type is pc-q35-2.4 or later,
> - "-device isa-fdc" is not passed on the command line (nor in the config
>   file),
> - no "-drive if=floppy,..." is requested.
> 
> The first part of the code flow, updated by the patch, is as follows:
> 
> 1. The "no_floppy = 1" machine class setting causes "default_floppy" in
>    main() to become zero.
> 
> 2. Hence default_drive() will not call drive_add() and drive_new() for
>    IF_FLOPPY, index=0.
> 
> This alone would not suffice, because the pc_basic_device_init() function
> creates the FDC regardless of actual floppy drives. Therefore,
> 
> 3. pc_basic_device_init() should only create the FDC if that's desirable
>    for the platform (= all PIIX types, and Q35 types earlier than 2.4),
>    *or* the user requested at least one IF_FLOPPY drive.
> 
> 4. If the FDC is not created, callers of pc_basic_device_init() --
>    pc_q35_init() and pc_init1() -- will pass floppy=NULL to
>    pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.
> 
> I verified the patch under the following scenarios, with "info qtree" and
> with the MAP command of the UEFI shell (in OVMF):
> - "pc" machine type -- no changes, FDC present,
> - "pc-q35-2.3" machine type -- no changes, FDC present,
> - "pc-q35" machine type with "-device isa-fdc" -- no changes, FDC present,
> - "pc-q35" machine type with "-drive if=floppy,..." -- no changes, FDC
>   present,
> - "pc-q35" -- no FDC.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  1 +
>  hw/i386/pc.c         |  4 +++-
>  hw/i386/pc_piix.c    |  2 +-
>  hw/i386/pc_q35.c     | 12 +++++++++---
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..55522b7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
>  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 force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 769eb25..b77d9b4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
>  
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
> +                          bool force_fdctrl,
>                            ISADevice **floppy,
>                            bool no_vmport,
>                            uint32 hpet_irqs)
> @@ -1489,8 +1490,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>  
>      for(i = 0; i < MAX_FD; i++) {
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
> +        force_fdctrl |= !!fd[i];
>      }
> -    *floppy = fdctrl_init_isa(isa_bus, fd);
> +    *floppy = force_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
>  }
>  
>  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 212e263..9f530c4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
>  
>      pc_nic_init(isa_bus, pci_bus);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index e67f2de..4d68340 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -60,6 +60,7 @@ static bool smbios_uuid_encoded = true;
>   */
>  static bool gigabyte_align = true;
>  static bool has_reserved_memory = true;
> +static bool force_fdctrl;
>  
>  /* PC hardware initialisation */
>  static void pc_q35_init(MachineState *machine)
> @@ -250,7 +251,7 @@ static void pc_q35_init(MachineState *machine)
>      }
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
>                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
>  
>      /* connect pm stuff to lpc */
> @@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
>  
>  static void pc_compat_2_3(MachineState *machine)
>  {
> +    force_fdctrl = true;
>  }
>  
>  static void pc_compat_2_2(MachineState *machine)
> @@ -424,7 +426,8 @@ static void pc_q35_init_1_4(MachineState *machine)
>  #define PC_Q35_2_4_MACHINE_OPTIONS                      \
>      PC_Q35_MACHINE_OPTIONS,                             \
>      .default_machine_opts = "firmware=bios-256k.bin",   \
> -    .default_display = "std"
> +    .default_display = "std",                           \
> +    .no_floppy = 1
>  
>  static QEMUMachine pc_q35_machine_v2_4 = {
>      PC_Q35_2_4_MACHINE_OPTIONS,
> @@ -433,7 +436,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
>      .init = pc_q35_init,
>  };
>  
> -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> +#define PC_Q35_2_3_MACHINE_OPTIONS                      \
> +    PC_Q35_MACHINE_OPTIONS,                             \
> +    .default_machine_opts = "firmware=bios-256k.bin",   \
> +    .default_display = "std"
>  
>  static QEMUMachine pc_q35_machine_v2_3 = {
>      PC_Q35_2_3_MACHINE_OPTIONS,
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 10:39:59AM +0200, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
> > It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> > Machine type.
> 
> Surely Unnecessary Crap can still Kill Security.
> 
> > Hence, let us not instantiate the FDC when all of the following apply:
> > - the machine type is pc-q35-2.4 or later,
> > - "-device isa-fdc" is not passed on the command line (nor in the config
> >   file),
> > - no "-drive if=floppy,..." is requested.
> >
> > The first part of the code flow, updated by the patch, is as follows:
> >
> > 1. The "no_floppy = 1" machine class setting causes "default_floppy" in
> >    main() to become zero.
> 
> Yup.  It's copied around some, but that's its only real effect.
> 
> > 2. Hence default_drive() will not call drive_add() and drive_new() for
> >    IF_FLOPPY, index=0.
> 
> This is the backend created for the board to pick up.  If the board
> doesn't pick it up, the backend remains unused, which is confusing.
> Boards that don't pick up IF_FLOPPY backends should set no_floppy to
> avoid the confusion.  Aside: precious few do.
> 
> In short: good.
> 
> > This alone would not suffice, because the pc_basic_device_init() function
> > creates the FDC regardless of actual floppy drives. Therefore,
> >
> > 3. pc_basic_device_init() should only create the FDC if that's desirable
> >    for the platform (= all PIIX types, and Q35 types earlier than 2.4),
> 
> This is the conservative choice.
> 
> Arguably, Q35 got versioned prematurely.  Dropping the FDC from Q35
> unconditionally has been proposed, on the basis that it wasn't fit for
> serious use in prior releases anyway.  I think if we decide we're fine
> with changing the released Q35 machine types incompatibly, then we
> should consider going all the way and simply drop them.
> 
> But let's not get review of this patch bogged down in a Q35 versioning
> debate.  Whatever we want done there, we can do on top of this patch.
> 
> >    *or* the user requested at least one IF_FLOPPY drive.
> 
> This is the magic to keep -drive if=floppy just work with Q35.
> 
> On the one hand, we've come to hate command line magic.
> 
> On the other hand, this is exactly the kind of magic -drive does.  For
> instance, PC boards create lsi53c895a SCSI HBAs for -drive if=scsi.
> Your patch makes them create an isa-fdc for -drive if=floppy.
> 
> Magic or no magic, I don't care.  Whatever it takes to get the FDC out
> of Q35.
> 
> If the magic should run into review trouble, putting it into its own
> patch may help keeping the non-magic (and hopefully uncontroversial)
> parts out of the debate.

I think it's a good idea to split, if you redo the patch anyway.


> > 4. If the FDC is not created, callers of pc_basic_device_init() --
> >    pc_q35_init() and pc_init1() -- will pass floppy=NULL to
> >    pc_cmos_init(). Luckily, pc_cmos_init() already handles that case.
> >
> > I verified the patch under the following scenarios, with "info qtree" and
> > with the MAP command of the UEFI shell (in OVMF):
> > - "pc" machine type -- no changes, FDC present,
> > - "pc-q35-2.3" machine type -- no changes, FDC present,
> > - "pc-q35" machine type with "-device isa-fdc" -- no changes, FDC present,
> 
> Perhaps worth mentioning somewhere in the commit message: instead of the
> arcane -global isa-fdc.driveA=... (which breaks down when you have more
> than one isa-fdc), you simply do the regular -device isa-fdc,driveA=...
> 
> Discussed in a bit more details in
> <http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg03121.html>.
> 
> > - "pc-q35" machine type with "-drive if=floppy,..." -- no changes, FDC
> >   present,
> > - "pc-q35" -- no FDC.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  include/hw/i386/pc.h |  1 +
> >  hw/i386/pc.c         |  4 +++-
> >  hw/i386/pc_piix.c    |  2 +-
> >  hw/i386/pc_q35.c     | 12 +++++++++---
> >  4 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 1b35168..55522b7 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -199,6 +199,7 @@ qemu_irq *pc_allocate_cpu_irq(void);
> >  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 force_fdctrl,
> >                            ISADevice **floppy,
> >                            bool no_vmport,
> >                            uint32 hpet_irqs);
> 
> The name @force_fdctrl gave me pause: what are we forcing here?
> 
> I guess your rationale is something like "force creation of FDC even
> when there are no if=floppy drives."  Hmm.

I kind of dislike this name too.

> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 769eb25..b77d9b4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1395,6 +1395,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
> >  
> >  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> >                            ISADevice **rtc_state,
> > +                          bool force_fdctrl,
> >                            ISADevice **floppy,
> >                            bool no_vmport,
> >                            uint32 hpet_irqs)
> > @@ -1489,8 +1490,9 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> >  
> >      for(i = 0; i < MAX_FD; i++) {
> >          fd[i] = drive_get(IF_FLOPPY, 0, i);
> > +        force_fdctrl |= !!fd[i];
> >      }
> > -    *floppy = fdctrl_init_isa(isa_bus, fd);
> > +    *floppy = force_fdctrl ? fdctrl_init_isa(isa_bus, fd) : NULL;
> >  }
> >  
> >  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 212e263..9f530c4 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -242,7 +242,7 @@ static void pc_init1(MachineState *machine,
> >      }
> >  
> >      /* init basic PC hardware */
> > -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> > +    pc_basic_device_init(isa_bus, gsi, &rtc_state, true, &floppy,
> >                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
> >  
> >      pc_nic_init(isa_bus, pci_bus);
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index e67f2de..4d68340 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -60,6 +60,7 @@ static bool smbios_uuid_encoded = true;
> >   */
> >  static bool gigabyte_align = true;
> >  static bool has_reserved_memory = true;
> > +static bool force_fdctrl;
> >  
> >  /* PC hardware initialisation */
> >  static void pc_q35_init(MachineState *machine)
> > @@ -250,7 +251,7 @@ static void pc_q35_init(MachineState *machine)
> >      }
> >  
> >      /* init basic PC hardware */
> > -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
> > +    pc_basic_device_init(isa_bus, gsi, &rtc_state, force_fdctrl, &floppy,
> >                           (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
> >  
> >      /* connect pm stuff to lpc */
> 
> If we can get from MachineState *machine to its MachineClass, we could
> use MachineClass member no_floppy, and not need force_fdctrl.  Probably
> a wash.
> 
> > @@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
> >  
> >  static void pc_compat_2_3(MachineState *machine)
> >  {
> > +    force_fdctrl = true;
> >  }
> >  
> >  static void pc_compat_2_2(MachineState *machine)
> > @@ -424,7 +426,8 @@ static void pc_q35_init_1_4(MachineState *machine)
> >  #define PC_Q35_2_4_MACHINE_OPTIONS                      \
> >      PC_Q35_MACHINE_OPTIONS,                             \
> >      .default_machine_opts = "firmware=bios-256k.bin",   \
> > -    .default_display = "std"
> > +    .default_display = "std",                           \
> > +    .no_floppy = 1
> >  
> >  static QEMUMachine pc_q35_machine_v2_4 = {
> >      PC_Q35_2_4_MACHINE_OPTIONS,
> > @@ -433,7 +436,10 @@ static QEMUMachine pc_q35_machine_v2_4 = {
> >      .init = pc_q35_init,
> >  };
> >  
> > -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS
> > +#define PC_Q35_2_3_MACHINE_OPTIONS                      \
> > +    PC_Q35_MACHINE_OPTIONS,                             \
> > +    .default_machine_opts = "firmware=bios-256k.bin",   \
> > +    .default_display = "std"
> >  
> >  static QEMUMachine pc_q35_machine_v2_3 = {
> >      PC_Q35_2_3_MACHINE_OPTIONS,
> 
> If you want to try ideas from my review, go ahead.  Even if not:
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Gabriel L. Somlo]

> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> Machine type.
> 
> Hence, let us not instantiate the FDC when all of the following apply:
> - the machine type is pc-q35-2.4 or later,
> - "-device isa-fdc" is not passed on the command line (nor in the config
>   file),
> - no "-drive if=floppy,..." is requested.

Don't forget the FDC0 entry in the ACPI DSDT !

Right now, FDC0's _STA method returns 0x0F or 0x00, depending on the
value of \_SB_.PCI0.ISA_.FDEN, which is hard-coded to 1 (so 0x0F
always, basically :) )

I remember tinkering with that a while ago (allowing for FDEN or the
actual value of FDC0._STA to be patched in at startup, depending on
no_floppy or force_Fdctrl. I also did that for the AppleSMC entry, but I
notice that now the entire SMC is patched in conditionally, from
hw/i386/acpi-build.c. Maybe patching in the whole FDC0 node in a
similar fashion would be appropriate here as well ?

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Gerd Hoffmann]

On Do, 2015-05-28 at 08:59 -0400, Gabriel L. Somlo wrote:
> > It is Very annoying to carry forward an outdatEd coNtroller with a mOdern
> > Machine type.
> > 
> > Hence, let us not instantiate the FDC when all of the following apply:
> > - the machine type is pc-q35-2.4 or later,
> > - "-device isa-fdc" is not passed on the command line (nor in the config
> >   file),
> > - no "-drive if=floppy,..." is requested.
> 
> Don't forget the FDC0 entry in the ACPI DSDT !

No worries, everything is fine here.

> Right now, FDC0's _STA method returns 0x0F or 0x00, depending on the
> value of \_SB_.PCI0.ISA_.FDEN,

Yes.

> which is hard-coded to 1 (so 0x0F
> always, basically :) )

Yes on piix4.

On q35 FDEN maps to the floppy drive enable bit of the ich9 lpc.  And
ich9_lpc_machine_ready() sets this (and other) enable bits.

/me double checks things.

Ok, in theory.  In practice there seems to be something wrong.  linux
kernel log on q35 (with floppy) yields this:

[root@fedora ~]# dmesg | grep pnp
pnp: PnP ACPI init
pnp 00:00: Plug and Play ACPI device, IDs PNP0b00 (active)
pnp 00:01: Plug and Play ACPI device, IDs PNP0303 (active)
pnp 00:02: Plug and Play ACPI device, IDs PNP0f13 (active)
pnp 00:03: Plug and Play ACPI device, IDs PNP0501 (active)
pnp: PnP ACPI: found 4 devices

Floppy (PNP0700) is not present.  Hmm ...

Seems be be this:

--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -494,7 +494,7 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
         /* lpt */
         pci_conf[0x82] |= 0x04;
     }
-    if (memory_region_present(io_as, 0x3f0)) {
+    if (memory_region_present(io_as, 0x3f2)) {
         /* floppy */
         pci_conf[0x82] |= 0x08;
     }

Oh, and this ...

$ virsh qemu-monitor-command fedora-org-q35.base --hmp "info mtree" | grep fdc
    00000000000003f1-00000000000003f5 (prio 0, RW): fdc
    00000000000003f7-00000000000003f7 (prio 0, RW): fdc

... looks fishy too.  Shouldn't that be 0x3f2-0x3f6 for the first range?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Paolo Bonzini]

On 28/05/2015 15:52, Gerd Hoffmann wrote:
> @@ -494,7 +494,7 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>          /* lpt */
>          pci_conf[0x82] |= 0x04;
>      }
> -    if (memory_region_present(io_as, 0x3f0)) {
> +    if (memory_region_present(io_as, 0x3f2)) {
>          /* floppy */
>          pci_conf[0x82] |= 0x08;
>      }
> 
> Oh, and this ...
> 
> $ virsh qemu-monitor-command fedora-org-q35.base --hmp "info mtree" | grep fdc
>     00000000000003f1-00000000000003f5 (prio 0, RW): fdc
>     00000000000003f7-00000000000003f7 (prio 0, RW): fdc
> 
> ... looks fishy too.  Shouldn't that be 0x3f2-0x3f6 for the first range?

I think 0x3f0-0x3f5 based on this:

hw/block/fdc.c:    FD_REG_SRA = 0x00,
hw/block/fdc.c:    FD_REG_SRB = 0x01,
hw/block/fdc.c:    FD_REG_DOR = 0x02,
hw/block/fdc.c:    FD_REG_TDR = 0x03,
hw/block/fdc.c:    FD_REG_MSR = 0x04,
hw/block/fdc.c:    FD_REG_DSR = 0x04,
hw/block/fdc.c:    FD_REG_FIFO = 0x05,
hw/block/fdc.c:    FD_REG_DIR = 0x07,
hw/block/fdc.c:    FD_REG_CCR = 0x07,

So:

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d8a8edd..c761291 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2186,7 +2186,7 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
 }
 
 static const MemoryRegionPortio fdc_portio_list[] = {
-    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
+    { 0, 6, 1, .read = fdctrl_read, .write = fdctrl_write },
     { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
     PORTIO_END_OF_LIST(),
 };

Paolo

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Gerd Hoffmann]

  Hi,

> > $ virsh qemu-monitor-command fedora-org-q35.base --hmp "info mtree" | grep fdc
> >     00000000000003f1-00000000000003f5 (prio 0, RW): fdc
> >     00000000000003f7-00000000000003f7 (prio 0, RW): fdc
> > 
> > ... looks fishy too.  Shouldn't that be 0x3f2-0x3f6 for the first range?
> 
> I think 0x3f0-0x3f5 based on this:
> 
> hw/block/fdc.c:    FD_REG_SRA = 0x00,
> hw/block/fdc.c:    FD_REG_SRB = 0x01,
> hw/block/fdc.c:    FD_REG_DOR = 0x02,
> hw/block/fdc.c:    FD_REG_TDR = 0x03,
> hw/block/fdc.c:    FD_REG_MSR = 0x04,
> hw/block/fdc.c:    FD_REG_DSR = 0x04,
> hw/block/fdc.c:    FD_REG_FIFO = 0x05,
> hw/block/fdc.c:    FD_REG_DIR = 0x07,
> hw/block/fdc.c:    FD_REG_CCR = 0x07,
> 
> So:
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index d8a8edd..c761291 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2186,7 +2186,7 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
>  }
>  
>  static const MemoryRegionPortio fdc_portio_list[] = {
> -    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
> +    { 0, 6, 1, .read = fdctrl_read, .write = fdctrl_write },
>      { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
>      PORTIO_END_OF_LIST(),
>  };

The io ranges for FDC0 in hw/i386/acpi-dsdt-isa.dsl are not consistent
with that though.  If 0x3f0 is correct (which seems to be the case from
a brief look) this needs to be fixed up.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Paolo Bonzini]

On 28/05/2015 16:56, Gerd Hoffmann wrote:
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index d8a8edd..c761291 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -2186,7 +2186,7 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
> >  }
> >  
> >  static const MemoryRegionPortio fdc_portio_list[] = {
> > -    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
> > +    { 0, 6, 1, .read = fdctrl_read, .write = fdctrl_write },
> >      { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
> >      PORTIO_END_OF_LIST(),
> >  };
>
> The io ranges for FDC0 in hw/i386/acpi-dsdt-isa.dsl are not consistent
> with that though.  If 0x3f0 is correct (which seems to be the case from
> a brief look) this needs to be fixed up.

Agreed.  Who draws the short straw? :)

Paolo

Re: [Qemu-devel] [PATCH] i386: drop FDC in pc-q35-2.4+ if neither

[Gerd Hoffmann]

On Do, 2015-05-28 at 17:20 +0200, Paolo Bonzini wrote:
> 
> On 28/05/2015 16:56, Gerd Hoffmann wrote:
> > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > > index d8a8edd..c761291 100644
> > > --- a/hw/block/fdc.c
> > > +++ b/hw/block/fdc.c
> > > @@ -2186,7 +2186,7 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
> > >  }
> > >  
> > >  static const MemoryRegionPortio fdc_portio_list[] = {
> > > -    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
> > > +    { 0, 6, 1, .read = fdctrl_read, .write = fdctrl_write },
> > >      { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
> > >      PORTIO_END_OF_LIST(),
> > >  };
> >
> > The io ranges for FDC0 in hw/i386/acpi-dsdt-isa.dsl are not consistent
> > with that though.  If 0x3f0 is correct (which seems to be the case from
> > a brief look) this needs to be fixed up.
> 
> Agreed.  Who draws the short straw? :)

====  <- mine is 4 ch^Winches ;)

cheers,
  Gerd

PS: patch sent.