Einar Lueck <elelu...@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 <elelu...@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jf...@linux.vnet.ibm.com>
> Reviewed-by: Carsten Otte <co...@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntrae...@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?
