From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46117) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9diO-0001CJ-IO for qemu-devel@nongnu.org; Thu, 17 Dec 2015 13:55:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9diN-000077-8M for qemu-devel@nongnu.org; Thu, 17 Dec 2015 13:55:48 -0500 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> From: John Snow Message-ID: <56730528.4090400@redhat.com> Date: Thu, 17 Dec 2015 13:55:36 -0500 MIME-Version: 1.0 In-Reply-To: <87poy5ueki.fsf@blackfin.pond.sub.org> 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: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org 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 i= s >>>> whatever the pick_geometry function suspects has been inserted. >>>> >>>> The drive field maintains the exact same meaning as it did previousl= y, >>>> however pick_geometry/fd_revalidate will be refactored to *never* up= date >>>> 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 alwa= ys >>>> be the same as the value going into these calls, so it is effectivel= y >>>> already static. >>>> >>>> As of this patch, disk and drive will always be the same, but that m= ay >>>> not be true by the end of this series. >>>> >>>> Disk does not need to be migrated because it is not user-visible sta= te >>>> nor is it currently used for any calculations. It is purely informat= ive, >>>> 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 driv= e >>> 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 th= at >> information. >> >> The idea in the long haul is to make @drive a "write once or never" ki= nd >> 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 usef= ul >> 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. 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