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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 12/17/2015 03:30 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.

Could the following work?

@drive is the connected drive's type, if any.  Can't change without a
power cycle.

@flags, @last_sect, ... together describe the medium (a.k.a. disk), if
any.  @drive constrains the possible values.

*Except* we can have a special "Schrödinger's drive", for backward
compatibility.  It's type is indeterminate until something looks at it.
Then its wave function collapses, and an ordinary drive emerges.

[...]