From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJC2-0004mJ-TQ for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:54:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XuJBt-0004wv-9f for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:54:30 -0500 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:60780) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJBt-0004wh-1m for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:54:21 -0500 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Nov 2014 10:54:20 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 8239A1B0804B for ; Fri, 28 Nov 2014 10:54:32 +0000 (GMT) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sASAsGcn16646372 for ; Fri, 28 Nov 2014 10:54:16 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sASAsGA9017093 for ; Fri, 28 Nov 2014 03:54:16 -0700 Message-ID: <54785457.6040609@linux.vnet.ibm.com> Date: Fri, 28 Nov 2014 13:54:15 +0300 From: Ekaterina Tumanova MIME-Version: 1.0 References: <1416392276-10408-1-git-send-email-tumanova@linux.vnet.ibm.com> <1416392276-10408-6-git-send-email-tumanova@linux.vnet.ibm.com> <87d287bpp5.fsf@blackfin.pond.sub.org> In-Reply-To: <87d287bpp5.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 5/6] geometry: Call backend function to detect geometry and blocksize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, dahi@linux.vnet.ibm.com, Public KVM Mailing List , mihajlov@linux.vnet.ibm.com, borntraeger@de.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com On 11/28/2014 01:10 PM, Markus Armbruster wrote: > Ekaterina Tumanova writes: > >> hd_geometry_guess function autodetects the drive geometry. This patch >> adds a block backend call, that probes the backing device geometry. >> If the inner driver method is implemented and succeeds (currently only DASDs), >> the blkconf_geometry will pass-through the backing device geometry. >> >> Signed-off-by: Ekaterina Tumanova >> --- >> hw/block/block.c | 11 +++++++++++ >> hw/block/hd-geometry.c | 9 +++++++++ >> hw/block/virtio-blk.c | 1 + >> include/hw/block/block.h | 1 + >> 4 files changed, 22 insertions(+) >> >> diff --git a/hw/block/block.c b/hw/block/block.c >> index a625773..f1d29bc 100644 >> --- a/hw/block/block.c >> +++ b/hw/block/block.c >> @@ -25,6 +25,17 @@ void blkconf_serial(BlockConf *conf, char **serial) >> } >> } >> >> +void blkconf_blocksizes(BlockConf *conf) >> +{ >> + BlockBackend *blk = conf->blk; >> + struct ProbeBlockSize blocksize = blk_probe_blocksizes(blk); >> + >> + if (blocksize.rc == 0) { >> + conf->physical_block_size = blocksize.size.phys; >> + conf->logical_block_size = blocksize.size.log; >> + } >> +} >> + > > Uh, doesn't this overwrite the user's geometry? > > The existing blkconf_ functions do nothing when the user supplied the > configuration. In other words, they only provide defaults. Why should > this one be different? > this will be fixed in the next version >> void blkconf_geometry(BlockConf *conf, int *ptrans, >> unsigned cyls_max, unsigned heads_max, unsigned secs_max, >> Error **errp) >> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c >> index 6fcf74d..4972114 100644 >> --- a/hw/block/hd-geometry.c >> +++ b/hw/block/hd-geometry.c >> @@ -121,6 +121,14 @@ void hd_geometry_guess(BlockBackend *blk, >> int *ptrans) >> { >> int cylinders, heads, secs, translation; >> + struct ProbeGeometry geometry = blk_probe_geometry(blk); >> + >> + if (geometry.rc == 0) { >> + *pcyls = geometry.geo.cylinders; >> + *psecs = geometry.geo.sectors; >> + *pheads = geometry.geo.heads; >> + goto done; >> + } >> > > hd_geometry_guess() is called by blkconf_geometry() to conjure up a > default geometry when the user didn't pick one. Fairly elaborate > guesswork. blkconf_geometry() runs for IDE, SCSI and virtio-blk only. > > Your patch makes it pick the backend's geometry as reported by > blk_probe_geometry() before anything else. > > This is an incompatible change for backends where blk_probe_geometry() > succeeds. Currently, it succeeds only for DASD, and there you *want* > the incompatible change. > > My point is: if we rely on blk_probe_geometry() failing except for DASD, > then we should call it something like blk_dasd_geometry() to make that > obvious. > This function itself is not DASD specific. It calls the inner method for "host_device" driver, which currently only succeeds for DASDs. But in future someone can define methods for other drivers or even modify the host_device driver. > The commit message needs to spell out the incompatible change. > >> if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) { >> /* no LCHS guess: use a standard physical disk geometry */ >> @@ -143,6 +151,7 @@ void hd_geometry_guess(BlockBackend *blk, >> the logical geometry */ >> translation = BIOS_ATA_TRANSLATION_NONE; >> } >> +done: >> if (ptrans) { >> *ptrans = translation; >> } >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index b19b102..6f01565 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) >> error_propagate(errp, err); >> return; >> } >> + blkconf_blocksizes(&conf->conf); >> >> virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, >> sizeof(struct virtio_blk_config)); > > Why only virtio-blk, and not the other device models sporting > configurable block sizes (nvme, IDE, SCSI, usb-storage)? > > This is an incompatible change for backends where blk_probe_blocksizes() > succeeds and returns something other than (512, 512). Currently, it > succeeds only for DASD. Is that what we want? > > The commit message needs to spell out the incompatible change. > > Perhaps this patch should be split up into one for geometry and one for > block sizes. > >> diff --git a/include/hw/block/block.h b/include/hw/block/block.h >> index 0d0ce9a..856bf75 100644 >> --- a/include/hw/block/block.h >> +++ b/include/hw/block/block.h >> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial); >> void blkconf_geometry(BlockConf *conf, int *trans, >> unsigned cyls_max, unsigned heads_max, unsigned secs_max, >> Error **errp); >> +void blkconf_blocksizes(BlockConf *conf); >> >> /* Hard disk geometry */ >