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

[John Snow <jsnow@redhat.com>]

On 12/17/2015 01:15 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> 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.
> 
> 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.
> 
> [...]
> 

That is essentially what's going on now.
By the end of this series, the drive type is initialized to 120, 144,
288, auto or none. (default auto.)

Three of those are static and then never change. None reverts you back
to having "no drive" permanently. Auto is the Schrodinger's Drive type.

During initialization, we collapse the waveform via pick_drive. After
this series, there is no code that runs post-init that sets the drive
type anywhere.

Though you are arguing that I could do it all without @disk, by
investigating other metadata. This is true, but I thought it was nice to
have it cached. Not strictly necessary but I just felt like it was the
right thing to do to save it.

--js