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