Re: [Qemu-devel] [PATCH V3 1/2] hd-geometry.c: Integrate HDIO_GETGEO in guessing for target-s390x

[Markus Armbruster]

Einar Lueck <address@hidden> 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 <address@hidden>
> Signed-off-by: Jens Freimann <address@hidden>
> Reviewed-by: Carsten Otte <address@hidden>
> Signed-off-by: Christian Borntraeger <address@hidden>
> ---
>  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?