[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field |
Date: |
Thu, 17 Dec 2015 20:04:02 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
[Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option, John Snow, 2015/12/16
[Qemu-devel] [PATCH v3 08/11] fdc: add physical disk sizes, John Snow, 2015/12/16
[Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry, John Snow, 2015/12/16
[Qemu-devel] [PATCH v3 09/11] fdc: rework pick_geometry, John Snow, 2015/12/16
[Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject, John Snow, 2015/12/16