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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 07/04/2015 02:47 AM, Markus Armbruster wrote:
>> 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.
>> 
>
> It's this bit here (prior to this RFC):
>
>     if (match == -1) {
>         if (first_match == -1) {
>             match = 1;
>
> If we found absolutely nothing, we choose the "first" entry, literally
> &fd_formats[1].

Sneaky!

>                 But it doesn't really matter, 1.44MB is 1.44MB as far as
> CMOS cares.

Okay.

>>> 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.
>> 
>
> Partly why I wanted to preserve the "Pick a drive type based on the
> media" approach to maximize compatibility where possible. I may well end
> up hurting some compatibility in some places when you boot without a
> diskette, but MSDOS, Windows 7, 8, and Fedora all seemed happy, so I was
> happy.
>
>>> 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.
>> 
>
> You can change diskettes, though. The code confuses drive types with
> diskette types. The static data table we have describes both.
>
> Initial validation controls what nibble we put in CMOS. secondary
> validations actually only change the CHS and rotation rate variables. So
> the first validation controls /drive/ type and secondary validations
> control /diskette/ type.
>
> The way it works now is that once you choose your *drive* type, you
> cannot change the *diskette*, but yes in the real world you can put a
> 1.44MB into a 2.88MB capable drive (I think? The emulator seems happy
> with it...!)

I figure the following could make the most sense:

* Initial validation: if the user didn't ask for a specific drive type,
  pick one.

* Any validation, including initial: pick one of the diskette types this
  drive type can do.

>>>> 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.
>> 
>
> Yup.
>
>>> 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.
>> 
>
> Are you suggesting we ditch the diskette heuristic for picking drive
> type altogether in new machine types?
>
> I wouldn't mind, per se, since it's more mindful and explicit, but I
> think I have the capacity to break a lot of user scripts with that.

Current behavior: pick drive type and diskette type to fit image.

New default behavior: drive type is fixed, pick diskette type to fit
image.  Breaks only when

* no diskette type fits this image, or

* the one we pick presents different medium contents to the guest than
  we got before, or

* the guest chokes on the different disk or diskette type even though
  the actual contents is the same.

Whether it breaks "a lot of user scripts" depends on what default drive
type we pick, and on what images users use.  I suspect 1.44MiB is most
common.  I could be wrong.

>> If you want the old behavior with a new machine type then, you get to
>> specify type=magic or something.
>> 
>> Matter of UI taste.
>> 
>
> Hm.

If you feel the time for getting our floppy user interface right has
long passed, you have a point.  Use your judgement.

>>>>> @@ -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[].
>> 
>
> OK, you mean "Surely if MAX_FD is ever not two, then we will also remove
> e.g. driveB" -- I was thinking there was some kind of magic.
>
> Anyway, yes. I'll replace the weird logic with just an assertion. If
> anyone does change MAX_FD they'll get a "helpful reminder" that this
> function might also need to change.
>
> In summary, your recommendations are:
> - Try using 'typeA' and 'typeB' to force drive type instead of
> defaultA/defaultB to set preferences for the default magic type
> - Use a type of 'magic' to perform autodetection if that behavior is
> desired.
>
> Yeah?

Yeah, and

- Pick the default for typeA and typeB you think is best.