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

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 12/17/2015 01:15 PM, Markus Armbruster wrote:
>> 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.
>> 
>> [...]
>> 
>
> That is essentially what's going on now.

Quite possible, but the two FDriveType confuse me :)

> 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.

I'm not saying it isn't the right thing, only that it's locally
unobvious to me.  Perhaps it gets obvious later on.  Perhaps a few more
words in comments or the commit message could help.