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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 15/08/2012 09:44, Jens Freimann ha scritto:
> From: Einar Lueck <elelueck@linux.vnet.ibm.com>
> 
> 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 <elelueck@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
>  hw/hd-geometry.c | 118 ++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 81 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 1cdb9fb..fc29075 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,59 @@ 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;
> +}
> +
> +/* 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;
> +    }
> +
> +    *pheads = geo.heads;
> +    *psectors = geo.sectors;
> +    *pcylinders = geo.cylinders;
> +    *ptranslation = BIOS_ATA_TRANSLATION_NONE;
> +    trace_hd_geometry_lchs_guess(bs, *pcylinders, *pheads, *psectors);
> +    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)
>  {
>      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 +137,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;
> +            }
>              *pheads = heads;
>              *psectors = sectors;
>              *pcylinders = cylinders;
> +            *ptranslation = translation;
> +
>              trace_hd_geometry_lchs_guess(bs, cylinders, heads, sectors);
>              return 0;
>          }
> @@ -97,51 +164,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);
>  }
>  
>  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;
> 

Looks good.

Paolo