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

[Markus Armbruster]

Laszlo Ersek <address@hidden> 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 <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  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 <address@hidden>