Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field

[John Snow <jsnow@redhat.com>]

On 12/17/2015 03:30 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> This allows us to distinguish between the current disk type and the
>> current drive type. The drive is what's reported to CMOS, the disk is
>> whatever the pick_geometry function suspects has been inserted.
>>
>> The drive field maintains the exact same meaning as it did previously,
>> however pick_geometry/fd_revalidate will be refactored to *never* update
>> the drive field, considering it frozen in-place during an earlier
>> initialization call.
>>
>> Before this patch, pick_geometry/fd_revalidate could only change the
>> drive field when it was FDRIVE_DRV_NONE, which indicated that we had
>> not yet reported our drive type to CMOS. After we "pick one," even
>> though pick_geometry/fd_revalidate re-set drv->drive, it should always
>> be the same as the value going into these calls, so it is effectively
>> already static.
>>
>> As of this patch, disk and drive will always be the same, but that may
>> not be true by the end of this series.
>>
>> Disk does not need to be migrated because it is not user-visible state
>> nor is it currently used for any calculations. It is purely informative,
>> and will be rebuilt automatically via fd_revalidate on the new host.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 09bb63d..13fef23 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -133,7 +133,8 @@ typedef struct FDrive {
>    typedef struct FDrive {
>>      FDCtrl *fdctrl;
>>      BlockBackend *blk;
>>      /* Drive status */
>> -    FDriveType drive;
>> +    FDriveType drive;         /* CMOS drive type */
>> +    FDriveType disk;          /* Current disk type */
> 
> where
> 
>    typedef enum FDriveType {
>        FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
>        FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
>        FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
>        FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
>    } FDriveType;
> 
> FDriveType makes obvious sense as drive type.
> 

Sadly yes, because we have very thoroughly intermixed the concept of
media and drive, so DriveType still makes sense for the Diskette.

>>      uint8_t perpendicular;    /* 2.88 MB access mode    */
> 
> If I understand @perpendicular correctly, it's just an extra hoop a
> driver needs to jump through to actually access a 2.88M disk.
> 
>>      /* Position */
>>      uint8_t head;
>        uint8_t track;
>        uint8_t sect;
>        /* Media */
> 
> Shouldn't @disk go here?
> 

Oh, yes.

>        FDiskFlags flags;
>        uint8_t last_sect;        /* Nb sector per track    */
>        uint8_t max_track;        /* Nb of tracks           */
>        uint16_t bps;             /* Bytes per sector       */
>        uint8_t ro;               /* Is read-only           */
>        uint8_t media_changed;    /* Is media changed       */
>        uint8_t media_rate;       /* Data rate of medium    */
> 
>        bool media_inserted;      /* Is there a medium in the tray */
>    } FDrive;
> 
> A disk / medium is characterised by it's form factor, density /
> FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
> (ignoring a few details that don't interest us).  Obviously, the drive
> type restricts possible disk types.
> 
> What does @disk add over the media description we already have?
> 

It's mostly a semantic game to allow the pick_geometry function to
never, ever, ever write to the "drive" field -- it will operate
exclusively on the "disk" field. Callers can decide what to do with that
information.

The idea in the long haul is to make @drive a "write once or never" kind
of ordeal, and I introduced the new field to help enforce that.

It's purely sugar.

Maybe in the future if I work on the FDC some more it will become useful
for other purposes, but for now it's almost exclusively to inform the
'drive' type when drive is set to AUTO.

> Aside: @bps appears to be write-only, and the value written looks odd.
> 
>> @@ -157,6 +158,7 @@ static void fd_init(FDrive *drv)
>>      drv->drive = FDRIVE_DRV_NONE;
>>      drv->perpendicular = 0;
>>      /* Disk */
>> +    drv->disk = FDRIVE_DRV_NONE;
>>      drv->last_sect = 0;
>>      drv->max_track = 0;
>>  }
>> @@ -286,6 +288,7 @@ 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 : FDRIVE_DRV_NONE;
>>      drv->media_rate = parse->rate;
>>  
>>      if (drv->media_inserted) {

-- 
—js