Re: [Qemu-devel] [PATCH 08/32] hd-geometry: Move disk geometry guessing back from block.c

[Blue Swirl]

On Fri, Jun 29, 2012 at 3:34 PM, Markus Armbruster <address@hidden> wrote:
> Commit f3d54fc4 factored it out of hw/ide.c for reuse.  Sensible,
> except it was put into block.c.  Device-specific functionality should
> be kept in device code, not the block layer.  Move it to
> hw/hd-geometry.c, and make stylistic changes required to keep
> checkpatch.pl happy.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block.c          |  121 ------------------------------------------
>  block.h          |    1 -
>  blockdev.h       |    5 ++
>  hw/Makefile.objs |    2 +-
>  hw/hd-geometry.c |  153 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ide/core.c    |    2 +-
>  hw/scsi-disk.c   |    4 +-
>  hw/virtio-blk.c  |    2 +-
>  8 files changed, 163 insertions(+), 127 deletions(-)
>  create mode 100644 hw/hd-geometry.c
>
> diff --git a/block.c b/block.c
> index 9c538f1..d021fd0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2103,127 +2103,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t 
> *nb_sectors_ptr)
>     *nb_sectors_ptr = length;
>  }
>
> -struct partition {
> -        uint8_t boot_ind;           /* 0x80 - active */
> -        uint8_t head;               /* starting head */
> -        uint8_t sector;             /* starting sector */
> -        uint8_t cyl;                /* starting cylinder */
> -        uint8_t sys_ind;            /* What partition type */
> -        uint8_t end_head;           /* end head */
> -        uint8_t end_sector;         /* end sector */
> -        uint8_t end_cyl;            /* end cylinder */
> -        uint32_t start_sect;        /* starting sector counting from 0 */
> -        uint32_t nr_sects;          /* nr of sectors in partition */
> -} QEMU_PACKED;
> -
> -/* try to guess the disk logical geometry from the MSDOS partition table. 
> Return 0 if OK, -1 if could not guess */
> -static int guess_disk_lchs(BlockDriverState *bs,
> -                           int *pcylinders, int *pheads, int *psectors)
> -{
> -    uint8_t buf[BDRV_SECTOR_SIZE];
> -    int i, heads, sectors, cylinders;
> -    struct partition *p;
> -    uint32_t nr_sects;
> -    uint64_t nb_sectors;
> -
> -    bdrv_get_geometry(bs, &nb_sectors);
> -
> -    /**
> -     * The function will be invoked during startup not only in sync I/O mode,
> -     * but also in async I/O mode. So the I/O throttling function has to
> -     * be disabled temporarily here, not permanently.
> -     */
> -    if (bdrv_read_unthrottled(bs, 0, buf, 1) < 0) {
> -        return -1;
> -    }
> -    /* test msdos magic */
> -    if (buf[510] != 0x55 || buf[511] != 0xaa)
> -        return -1;
> -    for(i = 0; i < 4; i++) {
> -        p = ((struct partition *)(buf + 0x1be)) + i;
> -        nr_sects = le32_to_cpu(p->nr_sects);
> -        if (nr_sects && p->end_head) {
> -            /* We make the assumption that the partition terminates on
> -               a cylinder boundary */
> -            heads = p->end_head + 1;
> -            sectors = p->end_sector & 63;
> -            if (sectors == 0)
> -                continue;
> -            cylinders = nb_sectors / (heads * sectors);
> -            if (cylinders < 1 || cylinders > 16383)
> -                continue;
> -            *pheads = heads;
> -            *psectors = sectors;
> -            *pcylinders = cylinders;
> -#if 0
> -            printf("guessed geometry: LCHS=%d %d %d\n",
> -                   cylinders, heads, sectors);
> -#endif
> -            return 0;
> -        }
> -    }
> -    return -1;
> -}
> -
> -void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int 
> *psecs)
> -{
> -    int translation, lba_detected = 0;
> -    int cylinders, heads, secs;
> -    uint64_t nb_sectors;
> -
> -    /* if a geometry hint is available, use it */
> -    bdrv_get_geometry(bs, &nb_sectors);
> -    bdrv_get_geometry_hint(bs, &cylinders, &heads, &secs);
> -    translation = bdrv_get_translation_hint(bs);
> -    if (cylinders != 0) {
> -        *pcyls = cylinders;
> -        *pheads = heads;
> -        *psecs = secs;
> -    } else {
> -        if (guess_disk_lchs(bs, &cylinders, &heads, &secs) == 0) {
> -            if (heads > 16) {
> -                /* if heads > 16, it means that a BIOS LBA
> -                   translation was active, so the default
> -                   hardware geometry is OK */
> -                lba_detected = 1;
> -                goto default_geometry;
> -            } else {
> -                *pcyls = cylinders;
> -                *pheads = heads;
> -                *psecs = secs;
> -                /* disable any translation to be in sync with
> -                   the logical geometry */
> -                if (translation == BIOS_ATA_TRANSLATION_AUTO) {
> -                    bdrv_set_translation_hint(bs,
> -                                              BIOS_ATA_TRANSLATION_NONE);
> -                }
> -            }
> -        } else {
> -        default_geometry:
> -            /* if no geometry, use a standard physical disk geometry */
> -            cylinders = nb_sectors / (16 * 63);
> -
> -            if (cylinders > 16383)
> -                cylinders = 16383;
> -            else if (cylinders < 2)
> -                cylinders = 2;
> -            *pcyls = cylinders;
> -            *pheads = 16;
> -            *psecs = 63;
> -            if ((lba_detected == 1) && (translation == 
> BIOS_ATA_TRANSLATION_AUTO)) {
> -                if ((*pcyls * *pheads) <= 131072) {
> -                    bdrv_set_translation_hint(bs,
> -                                              BIOS_ATA_TRANSLATION_LARGE);
> -                } else {
> -                    bdrv_set_translation_hint(bs,
> -                                              BIOS_ATA_TRANSLATION_LBA);
> -                }
> -            }
> -        }
> -        bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs);
> -    }
> -}
> -
>  void bdrv_set_geometry_hint(BlockDriverState *bs,
>                             int cyls, int heads, int secs)
>  {
> diff --git a/block.h b/block.h
> index 1c769ad..052d0ce 100644
> --- a/block.h
> +++ b/block.h
> @@ -177,7 +177,6 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset);
>  int64_t bdrv_getlength(BlockDriverState *bs);
>  int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
>  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> -void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int 
> *psecs);
>  int bdrv_commit(BlockDriverState *bs);
>  int bdrv_commit_all(void);
>  int bdrv_change_backing_file(BlockDriverState *bs,
> diff --git a/blockdev.h b/blockdev.h
> index 260e16b..7b05945 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -62,4 +62,9 @@ void qmp_change_blockdev(const char *device, const char 
> *filename,
>                          bool has_format, const char *format, Error **errp);
>  void do_commit(Monitor *mon, const QDict *qdict);
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +
> +/* Hard disk geometry */
> +void hd_geometry_guess(BlockDriverState *bs,
> +                       int *pcyls, int *pheads, int *psecs);

I'd move this to a separate header under hw/.

> +
>  #endif
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 3d77259..5bf1d76 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -137,7 +137,7 @@ common-obj-$(CONFIG_MAX111X) += max111x.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
>  common-obj-y += i2c.o smbus.o smbus_eeprom.o
>  common-obj-y += eeprom93xx.o
> -common-obj-y += scsi-disk.o cdrom.o
> +common-obj-y += scsi-disk.o cdrom.o hd-geometry.o
>  common-obj-y += scsi-generic.o scsi-bus.o
>  common-obj-y += hid.o
>  common-obj-$(CONFIG_SSI) += ssi.o
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> new file mode 100644
> index 0000000..9b22e3f
> --- /dev/null
> +++ b/hw/hd-geometry.c
> @@ -0,0 +1,153 @@
> +/*
> + * Hard disk geometry utilities
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "blockdev.h"
> +
> +struct partition {

Partition

> +        uint8_t boot_ind;           /* 0x80 - active */
> +        uint8_t head;               /* starting head */
> +        uint8_t sector;             /* starting sector */
> +        uint8_t cyl;                /* starting cylinder */
> +        uint8_t sys_ind;            /* What partition type */
> +        uint8_t end_head;           /* end head */
> +        uint8_t end_sector;         /* end sector */
> +        uint8_t end_cyl;            /* end cylinder */
> +        uint32_t start_sect;        /* starting sector counting from 0 */
> +        uint32_t nr_sects;          /* nr of sectors in partition */
> +} QEMU_PACKED;
> +
> +/* try to guess the disk logical geometry from the MSDOS partition table.
> +   Return 0 if OK, -1 if could not guess */
> +static int guess_disk_lchs(BlockDriverState *bs,
> +                           int *pcylinders, int *pheads, int *psectors)
> +{
> +    uint8_t buf[BDRV_SECTOR_SIZE];
> +    int i, heads, sectors, cylinders;
> +    struct partition *p;
> +    uint32_t nr_sects;
> +    uint64_t nb_sectors;
> +
> +    bdrv_get_geometry(bs, &nb_sectors);
> +
> +    /**
> +     * The function will be invoked during startup not only in sync I/O mode,
> +     * but also in async I/O mode. So the I/O throttling function has to
> +     * be disabled temporarily here, not permanently.
> +     */
> +    if (bdrv_read_unthrottled(bs, 0, buf, 1) < 0) {
> +        return -1;
> +    }
> +    /* test msdos magic */
> +    if (buf[510] != 0x55 || buf[511] != 0xaa) {
> +        return -1;
> +    }
> +    for (i = 0; i < 4; i++) {
> +        p = ((struct partition *)(buf + 0x1be)) + i;
> +        nr_sects = le32_to_cpu(p->nr_sects);
> +        if (nr_sects && p->end_head) {
> +            /* We make the assumption that the partition terminates on
> +               a cylinder boundary */
> +            heads = p->end_head + 1;
> +            sectors = p->end_sector & 63;
> +            if (sectors == 0) {
> +                continue;
> +            }
> +            cylinders = nb_sectors / (heads * sectors);
> +            if (cylinders < 1 || cylinders > 16383) {
> +                continue;
> +            }
> +            *pheads = heads;
> +            *psectors = sectors;
> +            *pcylinders = cylinders;
> +#if 0
> +            printf("guessed geometry: LCHS=%d %d %d\n",
> +                   cylinders, heads, sectors);
> +#endif
> +            return 0;
> +        }
> +    }
> +    return -1;
> +}
> +
> +void hd_geometry_guess(BlockDriverState *bs,
> +                       int *pcyls, int *pheads, int *psecs)
> +{
> +    int translation, lba_detected = 0;
> +    int cylinders, heads, secs;
> +    uint64_t nb_sectors;
> +
> +    /* if a geometry hint is available, use it */
> +    bdrv_get_geometry(bs, &nb_sectors);
> +    bdrv_get_geometry_hint(bs, &cylinders, &heads, &secs);
> +    translation = bdrv_get_translation_hint(bs);
> +    if (cylinders != 0) {
> +        *pcyls = cylinders;
> +        *pheads = heads;
> +        *psecs = secs;
> +    } else {
> +        if (guess_disk_lchs(bs, &cylinders, &heads, &secs) == 0) {
> +            if (heads > 16) {
> +                /* if heads > 16, it means that a BIOS LBA
> +                   translation was active, so the default
> +                   hardware geometry is OK */
> +                lba_detected = 1;
> +                goto default_geometry;
> +            } else {
> +                *pcyls = cylinders;
> +                *pheads = heads;
> +                *psecs = secs;
> +                /* disable any translation to be in sync with
> +                   the logical geometry */
> +                if (translation == BIOS_ATA_TRANSLATION_AUTO) {
> +                    bdrv_set_translation_hint(bs,
> +                                              BIOS_ATA_TRANSLATION_NONE);
> +                }
> +            }
> +        } else {
> +        default_geometry:
> +            /* if no geometry, use a standard physical disk geometry */
> +            cylinders = nb_sectors / (16 * 63);
> +
> +            if (cylinders > 16383) {
> +                cylinders = 16383;
> +            } else if (cylinders < 2) {
> +                cylinders = 2;
> +            }
> +            *pcyls = cylinders;
> +            *pheads = 16;
> +            *psecs = 63;
> +            if ((lba_detected == 1)
> +                && (translation == BIOS_ATA_TRANSLATION_AUTO)) {
> +                if ((*pcyls * *pheads) <= 131072) {
> +                    bdrv_set_translation_hint(bs,
> +                                              BIOS_ATA_TRANSLATION_LARGE);
> +                } else {
> +                    bdrv_set_translation_hint(bs,
> +                                              BIOS_ATA_TRANSLATION_LBA);
> +                }
> +            }
> +        }
> +        bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs);
> +    }
> +}
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 71d4d77..00ee280 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1933,7 +1933,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, 
> IDEDriveKind kind,
>     s->drive_kind = kind;
>
>     bdrv_get_geometry(bs, &nb_sectors);
> -    bdrv_guess_geometry(bs, &cylinders, &heads, &secs);
> +    hd_geometry_guess(bs, &cylinders, &heads, &secs);
>     if (cylinders < 1 || cylinders > 16383) {
>         error_report("cyls must be between 1 and 16383");
>         return -1;
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index ae25194..47e1246 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -948,7 +948,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
> uint8_t **p_outbuf,
>             break;
>         }
>         /* if a geometry hint is available, use it */
> -        bdrv_guess_geometry(bdrv, &cylinders, &heads, &secs);
> +        hd_geometry_guess(bdrv, &cylinders, &heads, &secs);
>         p[2] = (cylinders >> 16) & 0xff;
>         p[3] = (cylinders >> 8) & 0xff;
>         p[4] = cylinders & 0xff;
> @@ -982,7 +982,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
> uint8_t **p_outbuf,
>         p[2] = 5000 >> 8;
>         p[3] = 5000 & 0xff;
>         /* if a geometry hint is available, use it */
> -        bdrv_guess_geometry(bdrv, &cylinders, &heads, &secs);
> +        hd_geometry_guess(bdrv, &cylinders, &heads, &secs);
>         p[4] = heads & 0xff;
>         p[5] = secs & 0xff;
>         p[6] = s->qdev.blocksize >> 8;
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index fe07746..c976749 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -622,7 +622,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, 
> VirtIOBlkConf *blk)
>     s->blk = blk;
>     s->rq = NULL;
>     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
> -    bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
> +    hd_geometry_guess(s->bs, &cylinders, &heads, &secs);
>
>     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>
> --
> 1.7.6.5
>
>