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 <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.

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 <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.

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 <address@hidden>