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

[John Snow]

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]. But it doesn't really matter, 1.44MB is 1.44MB as far as
CMOS cares.

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

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

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

>>>> @@ -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?
--js