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

[John Snow]

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

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

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.

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