From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9bu8-0002hG-20 for qemu-devel@nongnu.org; Thu, 17 Dec 2015 11:59:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9bu6-0002GS-SF for qemu-devel@nongnu.org; Thu, 17 Dec 2015 11:59:47 -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> From: John Snow Message-ID: <5672E9FD.9010204@redhat.com> Date: Thu, 17 Dec 2015 11:59:41 -0500 MIME-Version: 1.0 In-Reply-To: <87fuz11nqh.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 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 03:30 AM, Markus Armbruster wrote: > John Snow writes: >=20 >> 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* upda= te >> 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 informativ= e, >> 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 */ >=20 > where >=20 > 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; >=20 > FDriveType makes obvious sense as drive type. >=20 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 */ >=20 > If I understand @perpendicular correctly, it's just an extra hoop a > driver needs to jump through to actually access a 2.88M disk. >=20 >> /* Position */ >> uint8_t head; > uint8_t track; > uint8_t sect; > /* Media */ >=20 > Shouldn't @disk go here? >=20 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 */ >=20 > bool media_inserted; /* Is there a medium in the tray */ > } FDrive; >=20 > 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. >=20 > What does @disk add over the media description we already have? >=20 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. > Aside: @bps appears to be write-only, and the value written looks odd. >=20 >> @@ -157,6 +158,7 @@ static void fd_init(FDrive *drv) >> drv->drive =3D FDRIVE_DRV_NONE; >> drv->perpendicular =3D 0; >> /* Disk */ >> + drv->disk =3D FDRIVE_DRV_NONE; >> drv->last_sect =3D 0; >> drv->max_track =3D 0; >> } >> @@ -286,6 +288,7 @@ static void pick_geometry(FDrive *drv) >> drv->max_track =3D parse->max_track; >> drv->last_sect =3D parse->last_sect; >> drv->drive =3D parse->drive; >> + drv->disk =3D drv->media_inserted ? parse->drive : FDRIVE_DRV_NON= E; >> drv->media_rate =3D parse->rate; >> =20 >> if (drv->media_inserted) { --=20 =97js