| Message ID | 1357923660-35202-2-git-send-email-elelueck@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Einar Lueck <elelueck@linux.vnet.ibm.com> writes: > This patch extends the function hd_geometry_guess. 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. I'm afraid this could mess up existing, working disks. Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds. The old code arbitrarily picks a "standard" physical geometry then. Your code picks the one returned by HDIO_GETGEO. Unless the two geometries happen to lead to a compatible address translation, any guest that actually uses the translation now sees different disk contents, with predictably bad results. Can this happen? If no, why not?
On 01/14/2013 02:23 PM, Markus Armbruster wrote: > Einar Lueck <elelueck@linux.vnet.ibm.com> writes: > >> This patch extends the function hd_geometry_guess. 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. > > I'm afraid this could mess up existing, working disks. > > Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds. > > The old code arbitrarily picks a "standard" physical geometry then. > > Your code picks the one returned by HDIO_GETGEO. > > Unless the two geometries happen to lead to a compatible address > translation, any guest that actually uses the translation now sees > different disk contents, with predictably bad results. > > Can this happen? If no, why not? > You are right. There may be such cases. Thus, I see two options: a) making this code portion platform specific (s390x) b) introduction of a a command line option to force reconsideration of HDIO_GETGEO My impression is, that the value of b) is limited to s390x and therefore a) is the way to go. Regards, Einar.
Einar Lueck <elelueck@linux.vnet.ibm.com> writes: > On 01/14/2013 02:23 PM, Markus Armbruster wrote: >> Einar Lueck <elelueck@linux.vnet.ibm.com> writes: >> >>> This patch extends the function hd_geometry_guess. 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. >> >> I'm afraid this could mess up existing, working disks. >> >> Consider a disk where guess_disk_lchs() fails and HDIO_GETGEO succeeds. >> >> The old code arbitrarily picks a "standard" physical geometry then. >> >> Your code picks the one returned by HDIO_GETGEO. >> >> Unless the two geometries happen to lead to a compatible address >> translation, any guest that actually uses the translation now sees >> different disk contents, with predictably bad results. >> >> Can this happen? If no, why not? >> > > You are right. There may be such cases. Thus, I see two options: > a) making this code portion platform specific (s390x) > b) introduction of a a command line option to force reconsideration of > HDIO_GETGEO > My impression is, that the value of b) is limited to s390x and > therefore a) is the way to go. No objection from me.
diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c index c305143..d35e25f 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) */ +static inline 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, @@ -123,9 +160,11 @@ void hd_geometry_guess(BlockDriverState *bs, int cylinders, heads, secs, translation; 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); + if (guess_disk_pchs(bs, pcyls, pheads, psecs, &translation) < 0) { + /* no LCHS and no PCHS 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