From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoFTW-0000uC-E9 for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:02:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SoFTL-0007Ua-6J for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:02:09 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:53306) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoFTK-0007UF-SR for qemu-devel@nongnu.org; Mon, 09 Jul 2012 11:01:59 -0400 Received: by pbbro12 with SMTP id ro12so20936345pbb.4 for ; Mon, 09 Jul 2012 08:01:57 -0700 (PDT) Message-ID: <4FFAF261.5010804@codemonkey.ws> Date: Mon, 09 Jul 2012 10:01:53 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1341843388-5663-1-git-send-email-kwolf@redhat.com> <1341843388-5663-24-git-send-email-kwolf@redhat.com> In-Reply-To: <1341843388-5663-24-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 23/25] fdc: Move floppy geometry guessing back from block.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Markus Armbruster On 07/09/2012 09:16 AM, Kevin Wolf wrote: > From: Markus Armbruster > > Commit 5bbdbb46 moved it to block.c because "other geometry guessing > functions already reside in block.c". Device-specific functionality > should be kept in device code, not the block layer. Move it back. > > Disk geometry guessing is still in block.c. To be moved out in a > later patch series. > > Bonus: the floppy type used in pc_cmos_init() now obviously matches > the one in the FDrive. Before, we relied on > bdrv_get_floppy_geometry_hint() picking the same type both in > fd_revalidate() and in pc_cmos_init(). > > Signed-off-by: Markus Armbruster > Signed-off-by: Kevin Wolf > --- > block.c | 101 --------------------------------------------------- > block.h | 18 --------- > hw/fdc.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- > hw/fdc.h | 10 +++++- > hw/pc.c | 13 ++----- > 5 files changed, 124 insertions(+), 140 deletions(-) > > diff --git a/block.c b/block.c > index f2540b9..fa1789c 100644 > --- a/block.c > +++ b/block.c > @@ -2272,107 +2272,6 @@ void bdrv_set_io_limits(BlockDriverState *bs, > bs->io_limits_enabled = bdrv_io_limits_enabled(bs); > } > > -/* Recognize floppy formats */ > -typedef struct FDFormat { > - FDriveType drive; > - uint8_t last_sect; > - uint8_t max_track; > - uint8_t max_head; > - FDriveRate rate; > -} FDFormat; > - > -static const FDFormat fd_formats[] = { > - /* First entry is default format */ > - /* 1.44 MB 3"1/2 floppy disks */ > - { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, }, > - /* 2.88 MB 3"1/2 floppy disks */ > - { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, }, > - { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, }, > - /* 720 kB 3"1/2 floppy disks */ > - { FDRIVE_DRV_144, 9, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, }, > - /* 1.2 MB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, }, > - { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, > - /* 720 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 9, 80, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, }, > - /* 360 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 9, 40, 1, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 9, 40, 0, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, }, > - { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, }, > - /* 320 kB 5"1/4 floppy disks */ > - { FDRIVE_DRV_120, 8, 40, 1, FDRIVE_RATE_250K, }, > - { FDRIVE_DRV_120, 8, 40, 0, FDRIVE_RATE_250K, }, > - /* 360 kB must match 5"1/4 better than 3"1/2... */ > - { FDRIVE_DRV_144, 9, 80, 0, FDRIVE_RATE_250K, }, > - /* end */ > - { FDRIVE_DRV_NONE, -1, -1, 0, 0, }, > -}; > - > -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads, > - int *max_track, int *last_sect, > - FDriveType drive_in, FDriveType *drive, > - FDriveRate *rate) > -{ > - const FDFormat *parse; > - uint64_t nb_sectors, size; > - int i, first_match, match; > - > - bdrv_get_geometry(bs,&nb_sectors); > - match = -1; > - first_match = -1; > - for (i = 0; ; i++) { > - parse =&fd_formats[i]; > - if (parse->drive == FDRIVE_DRV_NONE) { > - break; > - } > - if (drive_in == parse->drive || > - drive_in == FDRIVE_DRV_NONE) { > - size = (parse->max_head + 1) * parse->max_track * > - parse->last_sect; > - if (nb_sectors == size) { > - match = i; > - break; > - } > - if (first_match == -1) { > - first_match = i; > - } > - } > - } > - if (match == -1) { > - if (first_match == -1) { > - match = 1; > - } else { > - match = first_match; > - } > - parse =&fd_formats[match]; > - } > - *nb_heads = parse->max_head + 1; > - *max_track = parse->max_track; > - *last_sect = parse->last_sect; > - *drive = parse->drive; > - *rate = parse->rate; > -} > - > int bdrv_get_translation_hint(BlockDriverState *bs) > { > return bs->translation; > diff --git a/block.h b/block.h > index 3af93c6..c1c7500 100644 > --- a/block.h > +++ b/block.h > @@ -267,24 +267,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs, > void bdrv_set_translation_hint(BlockDriverState *bs, int translation); > void bdrv_get_geometry_hint(BlockDriverState *bs, > int *pcyls, int *pheads, int *psecs); > -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; > - > -typedef enum FDriveRate { > - FDRIVE_RATE_500K = 0x00, /* 500 Kbps */ > - FDRIVE_RATE_300K = 0x01, /* 300 Kbps */ > - FDRIVE_RATE_250K = 0x02, /* 250 Kbps */ > - FDRIVE_RATE_1M = 0x03, /* 1 Mbps */ > -} FDriveRate; > - > -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads, > - int *max_track, int *last_sect, > - FDriveType drive_in, FDriveType *drive, > - FDriveRate *rate); > int bdrv_get_translation_hint(BlockDriverState *bs); > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > BlockErrorAction on_write_error); > diff --git a/hw/fdc.c b/hw/fdc.c > index edf0706..41191c7 100644 > --- a/hw/fdc.c > +++ b/hw/fdc.c > @@ -52,6 +52,113 @@ > /********************************************************/ > /* Floppy drive emulation */ > > +typedef enum FDriveRate { > + FDRIVE_RATE_500K = 0x00, /* 500 Kbps */ > + FDRIVE_RATE_300K = 0x01, /* 300 Kbps */ > + FDRIVE_RATE_250K = 0x02, /* 250 Kbps */ > + FDRIVE_RATE_1M = 0x03, /* 1 Mbps */ > +} FDriveRate; > + > +typedef struct FDFormat { > + FDriveType drive; > + uint8_t last_sect; > + uint8_t max_track; > + uint8_t max_head; > + FDriveRate rate; > +} FDFormat; > + > +static const FDFormat fd_formats[] = { > + /* First entry is default format */ > + /* 1.44 MB 3"1/2 floppy disks */ > + { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, }, > + /* 2.88 MB 3"1/2 floppy disks */ > + { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, }, > + { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, }, > + /* 720 kB 3"1/2 floppy disks */ > + { FDRIVE_DRV_144, 9, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, }, > + /* 1.2 MB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, }, > + { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, > + /* 720 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 9, 80, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, }, > + /* 360 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 9, 40, 1, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 9, 40, 0, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, }, > + { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, }, > + /* 320 kB 5"1/4 floppy disks */ > + { FDRIVE_DRV_120, 8, 40, 1, FDRIVE_RATE_250K, }, > + { FDRIVE_DRV_120, 8, 40, 0, FDRIVE_RATE_250K, }, > + /* 360 kB must match 5"1/4 better than 3"1/2... */ > + { FDRIVE_DRV_144, 9, 80, 0, FDRIVE_RATE_250K, }, > + /* end */ > + { FDRIVE_DRV_NONE, -1, -1, 0, 0, }, > +}; > + > +static void pick_geometry(BlockDriverState *bs, int *nb_heads, > + int *max_track, int *last_sect, > + FDriveType drive_in, FDriveType *drive, > + FDriveRate *rate) > +{ > + const FDFormat *parse; > + uint64_t nb_sectors, size; > + int i, first_match, match; > + > + bdrv_get_geometry(bs,&nb_sectors); > + match = -1; > + first_match = -1; > + for (i = 0; ; i++) { > + parse =&fd_formats[i]; > + if (parse->drive == FDRIVE_DRV_NONE) { > + break; > + } > + if (drive_in == parse->drive || > + drive_in == FDRIVE_DRV_NONE) { > + size = (parse->max_head + 1) * parse->max_track * > + parse->last_sect; > + if (nb_sectors == size) { > + match = i; > + break; > + } > + if (first_match == -1) { > + first_match = i; > + } > + } > + } > + if (match == -1) { > + if (first_match == -1) { > + match = 1; > + } else { > + match = first_match; > + } > + parse =&fd_formats[match]; > + } > + *nb_heads = parse->max_head + 1; > + *max_track = parse->max_track; > + *last_sect = parse->last_sect; > + *drive = parse->drive; > + *rate = parse->rate; > +} > + > #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv) > #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive)) > > @@ -187,8 +294,8 @@ static void fd_revalidate(FDrive *drv) > FLOPPY_DPRINTF("revalidate\n"); > if (drv->bs != NULL) { > ro = bdrv_is_read_only(drv->bs); > - bdrv_get_floppy_geometry_hint(drv->bs,&nb_heads,&max_track, > -&last_sect, drv->drive,&drive,&rate); > + pick_geometry(drv->bs,&nb_heads,&max_track, > +&last_sect, drv->drive,&drive,&rate); > if (!bdrv_is_inserted(drv->bs)) { > FLOPPY_DPRINTF("No disk in drive\n"); > } else { > @@ -2054,18 +2161,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev) > return fdctrl_init_common(fdctrl); > } > > -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev) > +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i) > { > - FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev); > - FDCtrl *fdctrl =&isa->state; > - int i; > + FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc); > > - for (i = 0; i< MAX_FD; i++) { > - bs[i] = fdctrl->drives[i].bs; > - } > + return isa->state.drives[i].drive; > } > > - > static const VMStateDescription vmstate_isa_fdc ={ > .name = "fdc", > .version_id = 2, > diff --git a/hw/fdc.h b/hw/fdc.h > index 1b32b17..b5c9f31 100644 > --- a/hw/fdc.h > +++ b/hw/fdc.h > @@ -6,11 +6,19 @@ > /* fdc.c */ > #define MAX_FD 2 > > +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; > + > ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds); > void fdctrl_init_sysbus(qemu_irq irq, int dma_chann, > target_phys_addr_t mmio_base, DriveInfo **fds); > void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base, > DriveInfo **fds, qemu_irq *fdc_tc); > -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev); > + > +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i); > > #endif > diff --git a/hw/pc.c b/hw/pc.c > index c7e9ab3..e5e7647 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, > ISADevice *floppy, BusState *idebus0, BusState *idebus1, > ISADevice *s) > { > - int val, nb, nb_heads, max_track, last_sect, i; > - FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE }; > - FDriveRate rate; > - BlockDriverState *fd[MAX_FD]; > + int val, nb, i; > + FDriveType fd_type[2]; This results in: CC i386-softmmu/hw/i386/../pc.o /home/anthony/git/qemu/hw/i386/../pc.c: In function ‘pc_cmos_init’: /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[1]’ may be used uninitialized in this function [-Werror=uninitialized] /home/anthony/git/qemu/hw/i386/../pc.c:339:16: error: ‘fd_type[0]’ may be used uninitialized in this function [-Werror=uninitialized] cc1: all warnings being treated as errors And GCC is right as: > static pc_cmos_init_late_arg arg; > > /* various important CMOS locations needed by PC/Bochs bios */ > @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, > > /* floppy type */ > if (floppy) { > - fdc_get_bs(fd, floppy); > for (i = 0; i< 2; i++) { > - if (fd[i]) { > - bdrv_get_floppy_geometry_hint(fd[i],&nb_heads,&max_track, > -&last_sect, FDRIVE_DRV_NONE, > -&fd_type[i],&rate); > - } > + fd_type[i] = isa_fdc_get_drive_type(floppy, i); > } > } > val = (cmos_get_fd_drive_type(fd_type[0])<< 4) | This is an unconditional use of fd_type[0]. If floppy == NULL, this is dereferencing an uninitialized value. I'm not sure why the explicit initialization was removed... Regards, Anthony Liguori