Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 07/03/2015 09:34 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>> 
>>> Respect the default drive type as proffered via the CLI.
>>>
>>> This patch overloads the "drive out" parameter of pick_geometry
>>> to be used as a "default hint" which is offered by fd_revalidate.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 1023a01..87fd770 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int 
>>> *nb_heads,
>>>      /* Pick a default drive type if there's no media inserted AND we have
>>>       * not yet announced our drive type to the CMOS. */
>>>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>>> -        parse = &fd_formats[0];
>>> +        for (i = 0; ; i++) {
>>> +            parse = &fd_formats[i];
>>> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
>>> +                (parse->drive == *drive)) {
>>> +                break;
>>> +            }
>>> +        }
>>>          goto out;
>>>      }
>>>  
>> 
>> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
>> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
>> 
>
> My commit message is actually wrong, the code actually picks format #1,

Now I'm confused: &fd_formats[0] looks like format #0 to me.

> but that's only useful for choosing the drive type while there's no
> floppy. As soon as one is inserted it will re-validate to the closest
> 1.44MB type.
>
>> Afterwards: pick first one matching the value of
>> get_default_drive_type(), i.e. the value of the new property.
>> 
>> So the property is really a default type, which applies only when we
>> start without a medium in the drive.
>> 
>> Is that what we want?
>> 
>
> It is a "minimal" change that just allows you to configure what it
> defaults back to. It's a 'weak' setting.
>
> Was that a good call? hmm...
>
>> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
>> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
>> I've forced a funny medium in before startup.
>> 
>
> Ah, here it is. I can just add "typeA" and "typeB" properties instead of
> defaultA/B, and force the drive into that role.
>
>> The obvious way to do drive types is selecting one with a property,
>> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
>> 
>
> Hm?
>
> Not sure I follow, but the goal here is to use the 2.88MB type, because
> it can accept both kinds of diskettes, so it has the greatest
> compatibility for floppy images (including the virtio driver diskette
> which is a 2.88MB type.)

The 2.88 type may well be a more useful default, because it takes a
strict superset of media.  On the other hand, it puts a different value
into the CMOS floppy byte, which could conceivably confuse guests that
haven't been updated for this kind of high-tech gadget.  I'm not telling
you what default to pick, only what the tradeoffs are.

> The problem is the 1.44MB drive type and the current inflexibility of
> the revalidate+pick_geometry algorithm that doesn't allow you to change
> from 1.44 to 2.88 or vice versa.

Well, you can't change physical drives from 1.44 to 2.88 or vice versa,
either.

>> To preserve backward compatibility, we need a way to say "pick one for
>> the medium, else pick standard, and we need to make it the default.  At
>> least for old machine types.
>> 
>> Opinions?
>> 
>
> Machines prior to (let's say 2.5 here) should use something close to the
> old behavior: "Choose 1.44MB if no diskette, pick the best match (by
> sector count) otherwise."

If you want the new, saner behavior with and old machine type, pick the
appropriate type, e.g. say type=288.

> New machines apply nearly the same behavior, except they opt for 2.88MB
> as the default.
>
> New properties allow users to override the default-selection behavior
> and/or just force a drive type.

Possible alternative for new machines: default to 2.88 type, done.

If you want the old behavior with a new machine type then, you get to
specify type=magic or something.

Matter of UI taste.

>>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>>      fd_seek(drv, 0, 0, 1, 1);
>>>  }
>>>  
>>> +static FDriveType get_default_drive_type(FDrive *drv);
>>> +
>>>  /* Revalidate a disk drive after a disk change */
>>>  static void fd_revalidate(FDrive *drv)
>>>  {
>>>      int nb_heads, max_track, last_sect, ro;
>>> -    FDriveType drive;
>>> +    FDriveType drive = get_default_drive_type(drv);
>>>      FDriveRate rate;
>>>  
>>>      FLOPPY_DPRINTF("revalidate\n");
>>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>>      int32_t bootindexB;
>>>  } FDCtrlISABus;
>>>  
>>> +static FDriveType get_default_drive_type(FDrive *drv)
>>> +{
>>> +    FDCtrl *fdctrl = drv->fdctrl;
>>> +
>>> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>>> +        return fdctrl->defaultA;
>>> +    }
>>> +
>>> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>>> +        return fdctrl->defaultB;
>>> +    }
>>> +
>>> +    return FDRIVE_DEFAULT;
>>> +}
>>> +
>>>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>>  {
>>>      FDCtrl *fdctrl = opaque;
>> 
>> Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
>> properties don't exist, and fdctrl->drives[i] still has its initial
>> value FDRIVE_DEFAULT, doesn't it?
>> 
>
> Why would the properties not exist?

Before I answer your question: currently, MAX_FD is always 2, so the
question is moot.  The #ifdeffery in fdc.c suggests the code is also
prepared for MAX_FD 4, but no other values.

Now your question.  MAX_FD limits the number of drives the controller
supports.  Surely the controller's QOM/qdev interface shouldn't expose
more drives than you can actually connect.

"isa-fdc" and "sysbus-fdc" support two, "driveA" and "driveB", mapping
to state.drives[0..1].  "SUNW,fdtwo" supports one "drive", mapping to
state.drives[0].

Note that state.drives[] has MAX_FD elements.  If we changed MAX_FD to
one, we'd have to drop "isa-fdc"'s and "sysbus-fdc"'s "driveB", or else
they'd overrun state.drives[].