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

[Paolo Bonzini <pbonzini@redhat.com>]

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>