From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49207) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9dqb-0004wL-QY for qemu-devel@nongnu.org; Thu, 17 Dec 2015 14:04:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9dqW-0002f7-JM for qemu-devel@nongnu.org; Thu, 17 Dec 2015 14:04:17 -0500 From: Markus Armbruster References: <1450304177-3935-1-git-send-email-jsnow@redhat.com> <1450304177-3935-4-git-send-email-jsnow@redhat.com> <87fuz11nqh.fsf@blackfin.pond.sub.org> <5672E9FD.9010204@redhat.com> <87poy5ueki.fsf@blackfin.pond.sub.org> <56730528.4090400@redhat.com> Date: Thu, 17 Dec 2015 20:04:02 +0100 In-Reply-To: <56730528.4090400@redhat.com> (John Snow's message of "Thu, 17 Dec 2015 13:55:36 -0500") Message-ID: <87h9jgrj6l.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org John Snow writes: > On 12/17/2015 01:15 PM, Markus Armbruster wrote: >> John Snow writes: >>=20 >>> On 12/17/2015 03:30 AM, Markus Armbruster wrote: >>>> John Snow 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* upd= ate >>>>> 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 informati= ve, >>>>> and will be rebuilt automatically via fd_revalidate on the new host. >>>>> >>>>> Signed-off-by: John Snow >>>>> --- >>>>> 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 =3D 0x00, /* 1.44 MB 3"5 drive */ >>>> FDRIVE_DRV_288 =3D 0x01, /* 2.88 MB 3"5 drive */ >>>> FDRIVE_DRV_120 =3D 0x02, /* 1.2 MB 5"25 drive */ >>>> FDRIVE_DRV_NONE =3D 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. >>=20 >> Could the following work? >>=20 >> @drive is the connected drive's type, if any. Can't change without a >> power cycle. >>=20 >> @flags, @last_sect, ... together describe the medium (a.k.a. disk), if >> any. @drive constrains the possible values. >>=20 >> *Except* we can have a special "Schr=C3=B6dinger'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. >>=20 >> [...] >>=20 > > 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.