Re: [Qemu-devel] [PATCH 1/3] hd-geometry.c: Integrate HDIO_GETGEO in guessing

[Markus Armbruster]

Christian Borntraeger <address@hidden> writes:

> From: Einar Lueck <address@hidden>
>
> This patch extends the function guess_disk_lchs. If no geo could
> be derived from reading disk content, HDIO_GETGEO ioctl is issued.
> If this is not successful (e.g. image files) geometry is derived
> from the size of the disk (as before). To achieve this, the MSDOS
> partition table reading logic and the translation computation logic
> have been moved into a separate function guess_disk_msdosgeo. The
> HDIO_GETGEO logic is captured in a new function guess_disk_ioctlgeo.
> guess_disk_lchs then encapsulates both (the overall guessing logic).
> The new HDIO_GETGEO logic is required for two use cases:
> a) Support for geometries of Direct Attached Storage Disks (DASD)
> on s390x configured as backing of virtio block devices.
> b) Support for FCP attached SCSI disks that do not yet have a
> partition table. Without this patch, fdisk -l on the host would
> return different results then fdisk -l in the guest.
>
> Signed-off-by: Einar Lueck <address@hidden>
> Signed-off-by: Jens Freimann <address@hidden>
> Reviewed-by: Carsten Otte <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
>  hw/hd-geometry.c |  124 
> ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 87 insertions(+), 37 deletions(-)
>
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 1cdb9fb..4cf040d 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -33,6 +33,10 @@
>  #include "block.h"
>  #include "hw/block-common.h"
>  #include "trace.h"
> +#ifdef __linux__
> +#include <linux/fs.h>
> +#include <linux/hdreg.h>
> +#endif
>  
>  struct partition {
>          uint8_t boot_ind;           /* 0x80 - active */
> @@ -47,13 +51,65 @@ struct partition {
>          uint32_t nr_sects;          /* nr of sectors in partition */
>  } QEMU_PACKED;
>  
> +static void guess_chs_for_size(BlockDriverState *bs,
> +                uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs)
> +{
> +    uint64_t nb_sectors;
> +    int cylinders;
> +
> +    bdrv_get_geometry(bs, &nb_sectors);
> +
> +    cylinders = nb_sectors / (16 * 63);
> +    if (cylinders > 16383) {
> +        cylinders = 16383;
> +    } else if (cylinders < 2) {
> +        cylinders = 2;
> +    }
> +    *pcyls = cylinders;
> +    *pheads = 16;
> +    *psecs = 63;
> +}

Code motion, because you're reordering functions.  Best avoided, unless
it really helps readability.

> +
> +/* try to get geometry from disk via HDIO_GETGEO ioctl
> +   Return 0 if OK, -1 if ioctl does not work (e.g. image file) */
> +static inline int guess_disk_ioctlgeo(BlockDriverState *bs,
> +                                     uint32_t *pcylinders, uint32_t *pheads,
> +                                     uint32_t *psectors, int *ptranslation)
> +{
> +#ifdef __linux__
> +    struct hd_geometry geo;
> +
> +    if (bdrv_ioctl(bs, HDIO_GETGEO, &geo)) {
> +        return -1;
> +    }
> +
> +    /* HDIO_GETGEO may return success even though geo contains zeros
> +       (e.g. certain multipath setups) */
> +    if (!geo.heads || !geo.sectors || !geo.cylinders) {
> +        return -1;
> +    }
> +
> +    *pheads = geo.heads;
> +    *psectors = geo.sectors;
> +    *pcylinders = geo.cylinders;
> +    *ptranslation = BIOS_ATA_TRANSLATION_NONE;
> +    trace_hd_geometry_lchs_guess(bs, *pcylinders, *pheads, *psectors);

Abuse of existing trace point.  Please define a new one!

> +    return 0;
> +#else
> +    return -1;
> +#endif
> +}
> +
> +
>  /* 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)
> +static int guess_disk_msdosgeo(BlockDriverState *bs,
> +                               uint32_t *pcylinders, uint32_t *pheads,
> +                               uint32_t *psectors, int *ptranslation)

I don't like your new name.

There are two kinds of geometry in the DOS world:

* Physical: property of the disk, used for CHS addressing of disk
  sectors when communicating with the disk, say via ATA.  Obsolete in
  ATA, superseded by LBA.  This is the common meaning of geometry.

* Logical: something the PC BIOS makes up to make the best use of its
  ill-designed int 13h CHS addressing.  Used only by really old software
  like DOS.

The old function name makes it explicit that this function is about
*logical* CHS, unlike the other functions in this file.

I'd be fine with _dos_lchs, or _pc_lchs, or leaving it as _lchs.

>  {
>      uint8_t buf[BDRV_SECTOR_SIZE];
> -    int i, heads, sectors, cylinders;
> +    int i, translation;
> +    uint32_t heads, sectors, cylinders;
>      struct partition *p;
>      uint32_t nr_sects;
>      uint64_t nb_sectors;
> @@ -87,9 +143,26 @@ static int guess_disk_lchs(BlockDriverState *bs,
>              if (cylinders < 1 || cylinders > 16383) {
>                  continue;
>              }
> +
> +            if (heads > 16) {
> +                /* LCHS guess with heads > 16 means that a BIOS LBA
> +                   translation was active, so a standard physical disk
> +                   geometry is OK */
> +                guess_chs_for_size(bs, &cylinders, &heads, &sectors);
> +                translation = cylinders * heads <= 131072
> +                    ? BIOS_ATA_TRANSLATION_LARGE
> +                    : BIOS_ATA_TRANSLATION_LBA;
> +            } else {
> +                /* LCHS guess with heads <= 16: use as physical geometry */
> +                /* disable any translation to be in sync with
> +                   the logical geometry */
> +                translation = BIOS_ATA_TRANSLATION_NONE;
> +            }

You're undoing my work separation of guessing L-CHS from the DOS
partition table and choosing a translation.  Why?

>              *pheads = heads;
>              *psectors = sectors;
>              *pcylinders = cylinders;
> +            *ptranslation = translation;
> +
>              trace_hd_geometry_lchs_guess(bs, cylinders, heads, sectors);
>              return 0;
>          }
> @@ -97,51 +170,28 @@ static int guess_disk_lchs(BlockDriverState *bs,
>      return -1;
>  }
>  
> -static void guess_chs_for_size(BlockDriverState *bs,
> -                uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs)
> -{
> -    uint64_t nb_sectors;
> -    int cylinders;
> -
> -    bdrv_get_geometry(bs, &nb_sectors);
> -
> -    cylinders = nb_sectors / (16 * 63);
> -    if (cylinders > 16383) {
> -        cylinders = 16383;
> -    } else if (cylinders < 2) {
> -        cylinders = 2;
> +/* try to guess the disk logical geometry
> +   Return 0 if OK, -1 if could not guess */
> +static int guess_disk_lchs(BlockDriverState *bs,
> +                           uint32_t *pcylinders, uint32_t *pheads, 
> +                           uint32_t *psectors, int *ptranslation) {
> +    if (!guess_disk_msdosgeo(bs, pcylinders, pheads, psectors, 
> ptranslation)) {
> +        return 0;
>      }
> -    *pcyls = cylinders;
> -    *pheads = 16;
> -    *psecs = 63;
> +
> +    return guess_disk_ioctlgeo(bs, pcylinders, pheads, psectors, 
> ptranslation);

Are you sure HDIO_GETGEO returns *logical* geometry?

If not, the function name and comment lies.

>  }
>  
>  void hd_geometry_guess(BlockDriverState *bs,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans)
>  {
> -    int cylinders, heads, secs, translation;
> +    int translation;
>  
> -    if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 0) {
> +    if (guess_disk_lchs(bs, pcyls, pheads, psecs, &translation) < 0) {
>          /* no LCHS guess: use a standard physical disk geometry  */
>          guess_chs_for_size(bs, pcyls, pheads, psecs);
>          translation = hd_bios_chs_auto_trans(*pcyls, *pheads, *psecs);
> -    } else if (heads > 16) {
> -        /* LCHS guess with heads > 16 means that a BIOS LBA
> -           translation was active, so a standard physical disk
> -           geometry is OK */
> -        guess_chs_for_size(bs, pcyls, pheads, psecs);
> -        translation = *pcyls * *pheads <= 131072
> -            ? BIOS_ATA_TRANSLATION_LARGE
> -            : BIOS_ATA_TRANSLATION_LBA;
> -    } else {
> -        /* LCHS guess with heads <= 16: use as physical geometry */
> -        *pcyls = cylinders;
> -        *pheads = heads;
> -        *psecs = secs;
> -        /* disable any translation to be in sync with
> -           the logical geometry */
> -        translation = BIOS_ATA_TRANSLATION_NONE;
>      }
>      if (ptrans) {
>          *ptrans = translation;