| Message ID | 1335448165-26174-3-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 26.04.2012, at 15:49, Christian Borntraeger wrote: > From: Einar Lueck <elelueck@de.ibm.com> > > This patch uses ioctl HDIO_GETGEO to guess geometry of a disk in > case nothing is specified explicitly. > > Signed-off-by: Einar Lueck <elelueck@de.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > block.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/block.c b/block.c > index fe74ddd..8af4d19 100644 > --- a/block.c > +++ b/block.c > @@ -32,6 +32,10 @@ > #include "qmp-commands.h" > #include "qemu-timer.h" > > +#ifdef __linux__ > +#include <linux/hdreg.h> > +#endif > + > #ifdef CONFIG_BSD > #include <sys/types.h> > #include <sys/stat.h> > @@ -2054,6 +2058,7 @@ void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *pse > int translation, lba_detected = 0; > int cylinders, heads, secs; > uint64_t nb_sectors; > + struct hd_geometry geo; This one certainly belongs in the block below, otherwise you get an unused variable warning (== error) on other OSs. Alex > > /* if a geometry hint is available, use it */ > bdrv_get_geometry(bs, &nb_sectors); > @@ -2063,6 +2068,13 @@ void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *pse > *pcyls = cylinders; > *pheads = heads; > *psecs = secs; > +#ifdef __linux__ > + } else if (bdrv_ioctl(bs, HDIO_GETGEO, &geo) == 0) { > + *pcyls = geo.cylinders; > + *pheads = geo.heads; > + *psecs = geo.sectors; > + bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs); > +#endif > } else { > if (guess_disk_lchs(bs, &cylinders, &heads, &secs) == 0) { > if (heads > 16) { > -- > 1.7.0.1 >
Il 26/04/2012 15:49, Christian Borntraeger ha scritto: > +#ifdef __linux__ > + } else if (bdrv_ioctl(bs, HDIO_GETGEO, &geo) == 0) { > + *pcyls = geo.cylinders; > + *pheads = geo.heads; > + *psecs = geo.sectors; > + bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs); > +#endif Perhaps you could instead move guess_disk_lchs to target-specific code, adding add this code to the s390-specific implementation and under #ifdef __s390__. For x86 it doesn't make much sense, because a disk's geometry most likely will be a wrong guess for the geometry that a guest (for guests that care at all about geometries). Paolo
On 27/04/12 18:12, Paolo Bonzini wrote: > Il 26/04/2012 15:49, Christian Borntraeger ha scritto: >> +#ifdef __linux__ >> + } else if (bdrv_ioctl(bs, HDIO_GETGEO, &geo) == 0) { >> + *pcyls = geo.cylinders; >> + *pheads = geo.heads; >> + *psecs = geo.sectors; >> + bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs); >> +#endif > > Perhaps you could instead move guess_disk_lchs to target-specific code, > adding add this code to the s390-specific implementation and under > #ifdef __s390__. For x86 it doesn't make much sense, because a disk's > geometry most likely will be a wrong guess for the geometry that a guest > (for guests that care at all about geometries). Fine with me. We care about the geometry only for dasd devices, Even for FCP-based SCSI devices on s390 the geometry is not relevant. So moving that part to s390 specific code might make sense if nobody else needs that. Is that the case? Alex, would that be ok for you? Christian
On 05/02/2012 12:27 PM, Christian Borntraeger wrote: > On 27/04/12 18:12, Paolo Bonzini wrote: >> Il 26/04/2012 15:49, Christian Borntraeger ha scritto: >>> +#ifdef __linux__ >>> + } else if (bdrv_ioctl(bs, HDIO_GETGEO,&geo) == 0) { >>> + *pcyls = geo.cylinders; >>> + *pheads = geo.heads; >>> + *psecs = geo.sectors; >>> + bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs); >>> +#endif >> Perhaps you could instead move guess_disk_lchs to target-specific code, >> adding add this code to the s390-specific implementation and under >> #ifdef __s390__. For x86 it doesn't make much sense, because a disk's >> geometry most likely will be a wrong guess for the geometry that a guest >> (for guests that care at all about geometries). > Fine with me. We care about the geometry only for dasd devices, Even for FCP-based > SCSI devices on s390 the geometry is not relevant. So moving that part to > s390 specific code might make sense if nobody else needs that. > Is that the case? > Alex, would that be ok for you? As hinted in my other mail, I think the way to go would be to give a hint to the geometry code that we're running on a DASD disk. Then we can * Ask the host device if it can give us its geometry, if so, use it * Guess depending on the logical block size and everyone should be happy :). I would really like to have as little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse, as it means we won't be able to execise that code path on other architectures. Alex
> and everyone should be happy :). I would really like to have as > little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse, > as it means we won't be able to execise that code path on other > architectures. True, but how do you exercise that code path with DASD geometry on !__s390__? Paolo
On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >> and everyone should be happy :). I would really like to have as >> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is even worse, >> as it means we won't be able to execise that code path on other >> architectures. > True, but how do you exercise that code path with DASD geometry > on !__s390__? If we make things a flag for the guessing code, it should work just as well with image files, right? Alex
> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: > >> and everyone should be happy :). I would really like to have as > >> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is > >> even worse, > >> as it means we won't be able to execise that code path on other > >> architectures. > > True, but how do you exercise that code path with DASD geometry > > on !__s390__? > > If we make things a flag for the guessing code, it should work just > as well with image files, right? Only when they're not blank. :) I was only thinking of #ifdef __s390__ for the call to HDIO_GETGEO. Paolo
> As hinted in my other mail, I think the way to go would be to give a hint to the geometry code that we're running on a DASD disk..
Just as an idea if we are going that path,
we might use the BIODASDINFO2 or DASDAPIVER ioctls in qemu to detect if that is a dasd.
Christian
On 05/02/2012 01:38 PM, Paolo Bonzini wrote: >> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >>>> and everyone should be happy :). I would really like to have as >>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is >>>> even worse, >>>> as it means we won't be able to execise that code path on other >>>> architectures. >>> True, but how do you exercise that code path with DASD geometry >>> on !__s390__? >> If we make things a flag for the guessing code, it should work just >> as well with image files, right? > Only when they're not blank. :) I was only thinking of #ifdef __s390__ > for the call to HDIO_GETGEO. Well, if guessing is a function guess_size(disk_size, block_size) then we would be able to do the same on an image file. Christian, would that work? Alex
On 02/05/12 14:54, Alexander Graf wrote: > On 05/02/2012 01:38 PM, Paolo Bonzini wrote: >>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >>>>> and everyone should be happy :). I would really like to have as >>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is >>>>> even worse, >>>>> as it means we won't be able to execise that code path on other >>>>> architectures. >>>> True, but how do you exercise that code path with DASD geometry >>>> on !__s390__? >>> If we make things a flag for the guessing code, it should work just >>> as well with image files, right? >> Only when they're not blank. :) I was only thinking of #ifdef __s390__ >> for the call to HDIO_GETGEO. > > Well, if guessing is a function > > guess_size(disk_size, block_size) > > then we would be able to do the same on an image file. Christian, would that work? I think that the geometry values can not always be guessed correctly based on block_size and disk_size. Stefan, can you clarify that? If we cannot reliably guess the geometry based on blocksize and size, I still think that we should use the host values, e.g. after checking that BIODASDINFO2 returns successfully. Christian
On 02.05.2012, at 16:27, Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 02/05/12 14:54, Alexander Graf wrote: >> On 05/02/2012 01:38 PM, Paolo Bonzini wrote: >>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >>>>>> and everyone should be happy :). I would really like to have as >>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is >>>>>> even worse, >>>>>> as it means we won't be able to execise that code path on other >>>>>> architectures. >>>>> True, but how do you exercise that code path with DASD geometry >>>>> on !__s390__? >>>> If we make things a flag for the guessing code, it should work just >>>> as well with image files, right? >>> Only when they're not blank. :) I was only thinking of #ifdef __s390__ >>> for the call to HDIO_GETGEO. >> >> Well, if guessing is a function >> >> guess_size(disk_size, block_size) >> >> then we would be able to do the same on an image file. Christian, would that work? > > I think that the geometry values can not always be guessed correctly based on > block_size and disk_size. > > Stefan, can you clarify that? > > If we cannot reliably guess the geometry based on blocksize and size, I still think > that we should use the host values, e.g. after checking that BIODASDINFO2 returns > successfully. Yeah, but only if it's always possible to force a specific geometry through the command line - otherwise reproducability suffers. Alex
On 2012-05-02 16:27, Christian Borntraeger wrote: > On 02/05/12 14:54, Alexander Graf wrote: >> On 05/02/2012 01:38 PM, Paolo Bonzini wrote: >>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >>>>>> and everyone should be happy :). I would really like to have as >>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is >>>>>> even worse, >>>>>> as it means we won't be able to execise that code path on other >>>>>> architectures. >>>>> True, but how do you exercise that code path with DASD geometry >>>>> on !__s390__? >>>> If we make things a flag for the guessing code, it should work just >>>> as well with image files, right? >>> Only when they're not blank. :) I was only thinking of #ifdef __s390__ >>> for the call to HDIO_GETGEO. >> >> Well, if guessing is a function >> >> guess_size(disk_size, block_size) >> >> then we would be able to do the same on an image file. Christian, would that work? > > I think that the geometry values can not always be guessed correctly based on > block_size and disk_size. > > Stefan, can you clarify that? > > If we cannot reliably guess the geometry based on blocksize and size, I still think > that we should use the host values, e.g. after checking that BIODASDINFO2 returns > successfully. If we know the device type (e.g. 3390) and the block_size, then we can compute the number of blocks per track. The number of tracks per cylinder is a given (15) and the number of cylinders can be computed from these numbers and the disk_size. How do we get the device type? I think we could get away with restricting ECKD DASDs to type 3390, but even then, how would we distinguish an ECKD DASD from anything else, e.g. a SCSI device? We could simply attempt the above cylinder calculation for every device and if we get a result without a remainder we just assume that we have a DASD. This could lead to false positives, but maybe that is acceptable? Stefan Weinhuber
On 02.05.2012, at 17:57, Stefan Weinhuber <wein@linux.vnet.ibm.com> wrote: > On 2012-05-02 16:27, Christian Borntraeger wrote: >> On 02/05/12 14:54, Alexander Graf wrote: >>> On 05/02/2012 01:38 PM, Paolo Bonzini wrote: >>>>> On 05/02/2012 01:26 PM, Paolo Bonzini wrote: >>>>>>> and everyone should be happy :). I would really like to have as >>>>>>> little #ifdef TARGET_S390 code in QEMU. And #ifdef __s390__ is >>>>>>> even worse, >>>>>>> as it means we won't be able to execise that code path on other >>>>>>> architectures. >>>>>> True, but how do you exercise that code path with DASD geometry >>>>>> on !__s390__? >>>>> If we make things a flag for the guessing code, it should work just >>>>> as well with image files, right? >>>> Only when they're not blank. :) I was only thinking of #ifdef __s390__ >>>> for the call to HDIO_GETGEO. >>> >>> Well, if guessing is a function >>> >>> guess_size(disk_size, block_size) >>> >>> then we would be able to do the same on an image file. Christian, would that work? >> >> I think that the geometry values can not always be guessed correctly based on >> block_size and disk_size. >> >> Stefan, can you clarify that? >> >> If we cannot reliably guess the geometry based on blocksize and size, I still think >> that we should use the host values, e.g. after checking that BIODASDINFO2 returns >> successfully. > > If we know the device type (e.g. 3390) and the block_size, then we can compute the number of blocks per track. The number of tracks per cylinder is a given (15) and the number of cylinders can be computed from these numbers and the disk_size. > > How do we get the device type? I think we could get away with restricting ECKD DASDs to type 3390, but even then, how would we distinguish an ECKD DASD from anything else, e.g. a SCSI device? > > We could simply attempt the above cylinder calculation for every device and if we get a result without a remainder we just assume that we have a DASD. This could lead to false positives, but maybe that is acceptable? We could add a parameter to the disk configuration like type=dasd (default would be type=auto) which tells the geometry detection code to assume a 3390. If type == auto, we try a dasd ioctl and if that works use type=dasd. That way you could easily create a dump of the disk and get it working with a simple type=dasd. Later we could even add dasd disk label detection code which defaults type=auto to dasd if it finds one. That way disk images should be as easy to use as real dasd devices :). Alex
>>> Well, if guessing is a function >>> >>> guess_size(disk_size, block_size) >>> >>> then we would be able to do the same on an image file. Christian, would that work? >> >> I think that the geometry values can not always be guessed correctly based on >> block_size and disk_size. >> >> Stefan, can you clarify that? >> >> If we cannot reliably guess the geometry based on blocksize and size, I still think >> that we should use the host values, e.g. after checking that BIODASDINFO2 returns >> successfully. > > Yeah, but only if it's always possible to force a specific geometry through the command line - otherwise reproducability suffers. That is already possible, with the exception of the sector part of the geometry (see the other patch :-) ) Christian
diff --git a/block.c b/block.c index fe74ddd..8af4d19 100644 --- a/block.c +++ b/block.c @@ -32,6 +32,10 @@ #include "qmp-commands.h" #include "qemu-timer.h" +#ifdef __linux__ +#include <linux/hdreg.h> +#endif + #ifdef CONFIG_BSD #include <sys/types.h> #include <sys/stat.h> @@ -2054,6 +2058,7 @@ void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *pse int translation, lba_detected = 0; int cylinders, heads, secs; uint64_t nb_sectors; + struct hd_geometry geo; /* if a geometry hint is available, use it */ bdrv_get_geometry(bs, &nb_sectors); @@ -2063,6 +2068,13 @@ void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *pse *pcyls = cylinders; *pheads = heads; *psecs = secs; +#ifdef __linux__ + } else if (bdrv_ioctl(bs, HDIO_GETGEO, &geo) == 0) { + *pcyls = geo.cylinders; + *pheads = geo.heads; + *psecs = geo.sectors; + bdrv_set_geometry_hint(bs, *pcyls, *pheads, *psecs); +#endif } else { if (guess_disk_lchs(bs, &cylinders, &heads, &secs) == 0) { if (heads > 16) {