On Thu, May 28, 2015 at 10:39:59AM +0200, Markus Armbruster wrote: > Laszlo Ersek <ler...@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 <arm...@redhat.com> > > Cc: Markus Armbruster <arm...@redhat.com> > > Cc: Paolo Bonzini <pbonz...@redhat.com> > > Cc: "Michael S. Tsirkin" <m...@redhat.com> > > Signed-off-by: Laszlo Ersek <ler...@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 <arm...@redhat.com>
