Re: [Qemu-devel] [PULL 07/13] fdc: add pick_drive

[John Snow]

On 01/27/2016 07:38 AM, Paolo Bonzini wrote:
> 
> 
> On 25/01/2016 20:41, John Snow wrote:
>> Split apart pick_geometry by creating a pick_drive routine that will only
>> ever called during device bring-up instead of relying on pick_geometry to
>> be used in both cases.
>>
>> With this change, the drive field is changed to be 'write once'. It is
>> not altered after the initialization routines exit.
>>
>> media_validated does not need to be migrated. The target VM
>> will just revalidate the media on post_load anyway.
>>
>> Reviewed-by: Eric Blake <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> Message-id: address@hidden
> 
> Sorry, this breaks the RHEL6.5 Linux installer CD.  It just hangs at
> floppy detection and finally panics.
> 
> Paolo
> 

Looks like the problem is that after this patch, drives with no inserted
medium no longer populate the diskette geometry fields.

This leads to a problem where Linux attempts to seek to the first sector
on an empty drive. When it reads back the status interrupt information,
it finds that the command has succeeded but the current
head/track/sector values are unchanged, so it tries again. (I think. The
linux floppy code is... well, it certainly _is_.)

fd_seek itself guards against out-of-bounds seeks, even though Hervé
Poussineau patched fdctrl_handle_seek explicitly to allow such
out-of-bound seeks.

The end result is that QEMU "lies" about having done the seek, and Linux
appears to get very, very confused.

It looks as if QEMU tries to keep the current track within a sane
boundary for integrity reasons, but the Linux FDC driver expects to be
able to seek an empty drive. In reality, QEMU must consider current
sector/track to be "untrusted" values, but it currently tries to enforce
them being valid, but there doesn't appear to be any precedent for
refusing/erroring out on a SEEK command.

(Unrelatedly, in trying to fix this, I tried to see what would happen if
on an invalid seek through FD_SEEK alone I set ABNTERM, and it leads the
Linux kernel through a route where it tries to issue
DRIVE_SPECIFICATION, which will also break QEMU/Linux, because
DRIVE_SPECIFICATION can be anywhere between 3-7 bytes, but our version
accepts statically exactly 6 bytes.)

((Even fixing the above, Linux still doesn't appear to take a rejected
SEEK very well, just infinitely resetting, seeking, recalibrating.))

Given the above, it looks like the quick, dumb, and easy way to fix this
for now without risking jostling esoteric hardware will be to just set
dummy geometries on boot for empty drives, like we used to.

Hervé, any thoughts?

--js

>> ---
>>  hw/block/fdc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 46 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index a8f0cf2..f8e070e 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -151,6 +151,7 @@ typedef struct FDrive {
>>      uint8_t media_rate;       /* Data rate of medium    */
>>  
>>      bool media_inserted;      /* Is there a medium in the tray */
>> +    bool media_validated;     /* Have we validated the media? */
>>  } FDrive;
>>  
>>  static void fd_init(FDrive *drv)
>> @@ -162,6 +163,8 @@ static void fd_init(FDrive *drv)
>>      drv->disk = FLOPPY_DRIVE_TYPE_NONE;
>>      drv->last_sect = 0;
>>      drv->max_track = 0;
>> +    drv->ro = true;
>> +    drv->media_changed = 1;
>>  }
>>  
>>  #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
>> @@ -244,13 +247,24 @@ static void fd_recalibrate(FDrive *drv)
>>      fd_seek(drv, 0, 0, 1, 1);
>>  }
>>  
>> -static void pick_geometry(FDrive *drv)
>> +/**
>> + * Determine geometry based on inserted diskette.
>> + * Will not operate on an empty drive.
>> + *
>> + * @return: 0 on success, -1 if the drive is empty.
>> + */
>> +static int pick_geometry(FDrive *drv)
>>  {
>>      BlockBackend *blk = drv->blk;
>>      const FDFormat *parse;
>>      uint64_t nb_sectors, size;
>>      int i, first_match, match;
>>  
>> +    /* We can only pick a geometry if we have a diskette. */
>> +    if (!drv->media_inserted) {
>> +        return -1;
>> +    }
>> +
>>      blk_get_geometry(blk, &nb_sectors);
>>      match = -1;
>>      first_match = -1;
>> @@ -290,31 +304,51 @@ static void pick_geometry(FDrive *drv)
>>      }
>>      drv->max_track = parse->max_track;
>>      drv->last_sect = parse->last_sect;
>> -    drv->drive = parse->drive;
>> -    drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
>> +    drv->disk = parse->drive;
>>      drv->media_rate = parse->rate;
>> +    return 0;
>> +}
>> +
>> +static void pick_drive_type(FDrive *drv)
>> +{
>> +    if (pick_geometry(drv) == 0) {
>> +        drv->drive = drv->disk;
>> +    } else {
>> +        /* Legacy behavior: default to 1.44MB floppy */
>> +        drv->drive = FLOPPY_DRIVE_TYPE_144;
>> +    }
>>  }
>>  
>>  /* Revalidate a disk drive after a disk change */
>>  static void fd_revalidate(FDrive *drv)
>>  {
>> +    int rc;
>> +
>>      FLOPPY_DPRINTF("revalidate\n");
>>      if (drv->blk != NULL) {
>>          drv->ro = blk_is_read_only(drv->blk);
>> -        pick_geometry(drv);
>>          if (!drv->media_inserted) {
>>              FLOPPY_DPRINTF("No disk in drive\n");
>> -        } else {
>> -            FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
>> -                           (drv->flags & FDISK_DBL_SIDES) ? 2 : 1,
>> -                           drv->max_track, drv->last_sect,
>> -                           drv->ro ? "ro" : "rw");
>> +            drv->disk = FLOPPY_DRIVE_TYPE_NONE;
>> +        } else if (!drv->media_validated) {
>> +            rc = pick_geometry(drv);
>> +            if (rc) {
>> +                FLOPPY_DPRINTF("Could not validate floppy drive media");
>> +            } else {
>> +                drv->media_validated = true;
>> +                FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
>> +                               (drv->flags & FDISK_DBL_SIDES) ? 2 : 1,
>> +                               drv->max_track, drv->last_sect,
>> +                               drv->ro ? "ro" : "rw");
>> +            }
>>          }
>>      } else {
>>          FLOPPY_DPRINTF("No drive connected\n");
>>          drv->last_sect = 0;
>>          drv->max_track = 0;
>>          drv->flags &= ~FDISK_DBL_SIDES;
>> +        drv->drive = FLOPPY_DRIVE_TYPE_NONE;
>> +        drv->disk = FLOPPY_DRIVE_TYPE_NONE;
>>      }
>>  }
>>  
>> @@ -2185,6 +2219,7 @@ static void fdctrl_change_cb(void *opaque, bool load)
>>      drive->media_inserted = load && drive->blk && 
>> blk_is_inserted(drive->blk);
>>  
>>      drive->media_changed = 1;
>> +    drive->media_validated = false;
>>      fd_revalidate(drive);
>>  }
>>  
>> @@ -2221,11 +2256,12 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, 
>> Error **errp)
>>          }
>>  
>>          fd_init(drive);
>> -        fdctrl_change_cb(drive, 0);
>>          if (drive->blk) {
>>              blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
>>              drive->media_inserted = blk_is_inserted(drive->blk);
>> +            pick_drive_type(drive);
>>          }
>> +        fd_revalidate(drive);
>>      }
>>  }
>>  
>>
>