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

[Paolo Bonzini]

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

> ---
>  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);
>      }
>  }
>  
>