Re: [Qemu-devel] [PATCH] Do not emulate a floppy drive when -nodefaults

[John Snow <jsnow@redhat.com>]

On 05/13/2015 02:15 PM, Stefano Stabellini wrote:
> On Wed, 13 May 2015, Daniel P. Berrange wrote:
>> On Wed, May 13, 2015 at 06:29:46PM +0100, Stefano Stabellini wrote:
>>> Do not emulate a floppy drive if no drives are supposed to be present.
>>>
>>> This fixes the behavior of -nodefaults, that should remove the floppy
>>> drive (see docs/qdev-device-use.txt:Default Devices), but actually
>>> doesn't.
>>
>> Technically that doc is just refering to the disablement of the
>> primary floppy drive, as opposed to the floppy controller. The
>> floppy controller itself is really a built-in device that is
>> defined as part of the machine type, along with the various other
>> platform devices hanging off the PIIX and ISA brige.
> 
> I think you are right, this patch is a bit too harsh in fixing the
> problem: I just wanted to properly disable drive emulation, because from
> my tests the guest thinks that one drive is present even when is not.
> 

We should just be a little careful in explaining the difference between
the controller, the drives, and what circumstances things show up and to
whom.

Currently the FDC is always present and always has two drive objects
that are directly inlined to that device.

Now, based on whether or not this line fires in vl.c:
default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);

controls whether or not we find that drive in pc_basic_device_init as
you've found, which controls (ultimately) whether or not
fdctrl->drives[0].blk or fdctrl->drives[1].blk gets set.

If the blk pointer is set, even if you have no media inserted,
fd_revalidate (and pick_geometry) will *FORCIBLY PICK* a drive type for
this floppy, currently a 1.44MB type, (20 sect, 80 track, 1 head) with a
data rate of 500 Kbps. This is written to the rtc memory where seabios
later reads it to discover if you have a guest-visible floppy drive there.

Both QEMU and SeaBIOS confuse the concept of "A drive is present" with
"A disk is present" which leads to these kinds of confusing scenarios.

There's some work to do here, for sure.

Anyway, maybe this patch is inappropriate: Just because we have no
drives doesn't mean we don't have a controller.

I have seen some discussion recently about allowing users to
disable/remove the FDC altogether, though, and I think this is probably
the way to go.

> 
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index a8e6be1..c9f50b3 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1410,6 +1410,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>>      qemu_irq *cpu_exit_irq;
>>>      MemoryRegion *ioport80_io = g_new(MemoryRegion, 1);
>>>      MemoryRegion *ioportF0_io = g_new(MemoryRegion, 1);
>>> +    bool floppy_exist;
>>>  
>>>      memory_region_init_io(ioport80_io, NULL, &ioport80_io_ops, NULL, "ioport80", 1);
>>>      memory_region_add_subregion(isa_bus->address_space_io, 0x80, ioport80_io);
>>> @@ -1487,10 +1488,17 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>>      cpu_exit_irq = qemu_allocate_irqs(cpu_request_exit, NULL, 1);
>>>      DMA_init(0, cpu_exit_irq);
>>>  
>>> +    *floppy = NULL;
>>> +    floppy_exist = false;
>>>      for(i = 0; i < MAX_FD; i++) {
>>>          fd[i] = drive_get(IF_FLOPPY, 0, i);
>>> +        if (fd[i] != NULL) {
>>> +            floppy_exist = true;
>>> +        }
>>> +    }
>>> +    if (floppy_exist) {
>>> +        *floppy = fdctrl_init_isa(isa_bus, fd);
>>>      }
>>> -    *floppy = fdctrl_init_isa(isa_bus, fd);
>>>  }
>>
>> This results in a guest ABI change when updating QEMU, because the floppy
>> controller will disappear for existing guests. This is liable to break
>> guests upon migration.
>>
>> If we want to be able to disable the floppy controller itself, in addition
>> to the floppy drives, then we'd likely need to define a property against
>> the machine type to allow its existence to be toggled on/off. We'd then
>> need to at least make sure the existing machine types defaulted to on,
>> if we decided that new machine types should default to off.
>>
>> Libvirt would also need to gain the ability to represent the existance
>> of the floppy controller and allow it to be turned on / off explicitly.
> 
>