[V3,1/2] hd-geometry.c: Integrate HDIO_GETGEO in guessing for target-s390x

This patch extends the function hd_geometry_guess. It introduces a
target specific hook. The default implementation for this target
specific hook is empty, has therefore no effect and the existing logic
works as before. For target-s390x, the behaviour is chosen as follows:
If no geo could be guessed via guess_disk_lchs, a new function called
guess_disk_pchs is called. The latter utilizes HDIO_GET_GEO ioctl to ask
the underlying disk for geometry.
If this is not successful (e.g. image files) geometry is derived
from the size of the disk (as before).
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>
Reviewed-by: Carsten Otte <cotte@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/Makefile.objs |  6 ++++-
 hw/hd-geometry.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 2 deletions(-)

Einar Lueck <elelueck@linux.vnet.ibm.com> writes:

> This patch extends the function hd_geometry_guess. It introduces a
> target specific hook. The default implementation for this target
> specific hook is empty, has therefore no effect and the existing logic
> works as before. For target-s390x, the behaviour is chosen as follows:
> If no geo could be guessed via guess_disk_lchs, a new function called
> guess_disk_pchs is called. The latter utilizes HDIO_GET_GEO ioctl to ask
> the underlying disk for geometry.
> If this is not successful (e.g. image files) geometry is derived
> from the size of the disk (as before).
> 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>
> Reviewed-by: Carsten Otte <cotte@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/Makefile.objs |  6 ++++-
>  hw/hd-geometry.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 447e32a..7832d2c 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -178,7 +178,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 hd-geometry.o block-common.o
> +common-obj-y += scsi-disk.o cdrom.o block-common.o
>  common-obj-y += scsi-generic.o scsi-bus.o
>  common-obj-y += hid.o
>  common-obj-$(CONFIG_SSI) += ssi.o
> @@ -209,6 +209,10 @@ obj-$(CONFIG_VGA) += vga.o
>  obj-$(CONFIG_SOFTMMU) += device-hotplug.o
>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>  
> +# geometry is target/architecture dependent and therefore needs to be built
> +# for every target platform
> +obj-y += hd-geometry.o
> +
>  # Inter-VM PCI shared memory & VFIO PCI device assignment
>  ifeq ($(CONFIG_PCI), y)
>  obj-$(CONFIG_KVM) += ivshmem.o
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index c305143..98253d7 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -33,6 +33,10 @@
>  #include "block/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,6 +51,39 @@ 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);
> +
> +/* try to get geometry from disk via HDIO_GETGEO ioctl
> +   Return 0 if OK, -1 if ioctl does not work (e.g. image file) */
> +inline static int guess_disk_pchs(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;
> +    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,
> @@ -116,13 +153,43 @@ static void guess_chs_for_size(BlockDriverState *bs,
>      *psecs = 63;
>  }
>  
> +/* target specific geometry guessing hooks:
> + * return 0 if guess available, != 0 in any other case */
> +#ifdef TARGET_S390X
> +static inline int target_geometry_guess(BlockDriverState *bs,
> +                          uint32_t *pcyls, uint32_t *pheads,
> +                          uint32_t *psecs, int *ptranslation)
> +{
> +    int cyls, heads, secs;
> +    if (!guess_disk_lchs(bs, &cyls, &heads, &secs)) {

Suggest to test < 0 for clarity, like the existing caller does.

> +        *pcyls = cyls;
> +        *pheads = heads;
> +        *psecs = secs;
> +        *ptranslation = BIOS_ATA_TRANSLATION_NONE;
> +        return 0;
> +    } else {
> +        return guess_disk_pchs(bs, pcyls, pheads, psecs, ptranslation);
> +    }
> +}

I'd prefer

    if (guess_disk_lchs() < 0) {
        ...
        return 0;
    }
    return guess_disk_lchs();

over your "return else".  Matter of taste.

> +#else
> +static inline int target_geometry_guess(BlockDriverState *bs,
> +                          uint32_t *pcyls, uint32_t *pheads,
> +                          uint32_t *psecs, int *ptranslation)
> +{
> +    return -1;
> +}
> +#endif
> +
>  void hd_geometry_guess(BlockDriverState *bs,
>                         uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs,
>                         int *ptrans)
>  {
>      int cylinders, heads, secs, translation;
>  
> -    if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 0) {
> +    if (!target_geometry_guess(bs, pcyls, pheads, psecs, &translation)) {

Suggest to test < 0 for clarity.

> +        /* a target specific guess has highest priority */
> +        goto out;

Gratuitous goto: doing nothing here would take you to the exact same
place.  So let's do that.

> +    } else if (guess_disk_lchs(bs, &cylinders, &heads, &secs) < 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);
> @@ -143,6 +210,7 @@ void hd_geometry_guess(BlockDriverState *bs,
>             the logical geometry */
>          translation = BIOS_ATA_TRANSLATION_NONE;
>      }
> +out:
>      if (ptrans) {
>          *ptrans = translation;
>      }

Consider the following scenario on TARGET_S390X:

1. hd_geometry_guess() calls target_geometry_guess()

2. target_geometry_guess() calls guess_disk_lchs(), which fails

3. target_geometry_guess() calls guess_disk_pchs(), which fails

4. target_geometry_guess() fails.

5. hd_geometry_guess() calls guess_disk_lchs() again, and it fails
   again.

Since your target_geometry_guess() for s390x wants to do the
guess_disk_lchs() itself, what moveing the call of guess_disk_lchs()
there for all targets?