From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54889) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXWWh-0006WR-IL for qemu-devel@nongnu.org; Thu, 24 May 2012 07:48:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXWWf-0005M4-J3 for qemu-devel@nongnu.org; Thu, 24 May 2012 07:48:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30021) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXWWf-0005Li-Ae for qemu-devel@nongnu.org; Thu, 24 May 2012 07:48:17 -0400 Message-ID: <4FBE1FF7.7050305@redhat.com> Date: Thu, 24 May 2012 13:48:07 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1337858575-37612-1-git-send-email-borntraeger@de.ibm.com> <1337858575-37612-2-git-send-email-borntraeger@de.ibm.com> In-Reply-To: <1337858575-37612-2-git-send-email-borntraeger@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1.1 1/1] Fix geometry sector calculation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger Cc: Alexander Graf , Stefan Weinhuber , Christoph Hellwig , Markus Armbruster , qemu-devel Il 24/05/2012 13:22, Christian Borntraeger ha scritto: > Currently the sector value for the geometry is masked, even if the > user usesa command line parameter that explicitely gives a number. > This breaks dasd devices on s390. A dasd device can have > a physical block size of 4096 (== same for logical block size) > and a typcial geometry of 15 heads and 12 sectors per cyl. > The ibm partition detection relies on a correct geometry > reported by the device. Unfortunately the current code changes > 12 to 8. This would be necessary if the total size is > not a multiple of logical sector size, but for dasd this > is not the case. > > This patch checks the device size and only applies sector > mask if necessary. Rereading the code, I have no idea what the masking is for. Perhaps we can even remove it. However, your patch makes sense, it is safe, and it would be nice to apply it even for 1.1. Reviewed-by: Paolo Bonzini Paolo > Signed-off-by: Christian Borntraeger > CC: Christoph Hellwig > --- > hw/virtio-blk.c | 17 ++++++++++++++++- > 1 files changed, 16 insertions(+), 1 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index f9e1896..41c5bae 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -489,7 +489,22 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size); > stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size); > blkcfg.heads = heads; > - blkcfg.sectors = secs & ~s->sector_mask; > + /* > + * We must ensure that the block device capacity is a multiple of > + * the logical block size. If that is not the case, lets use > + * sector_mask to adopt the geometry to have a correct picture. > + * For those devices where the capacity is ok for the given geometry > + * we dont touch the sector value of the geometry, since some devices > + * (like s390 dasd) need a specific value. Here the capacity is already > + * cyls*heads*secs*blz_size and the sector value is not block size > + * divided by 512 - instead it is the amount of blk_size blocks > + * per track (cylinder). > + */ > + if (bdrv_getlength(s->bs) / heads / secs % blk_size) { > + blkcfg.sectors = secs & ~s->sector_mask; > + } else { > + blkcfg.sectors = secs; > + } > blkcfg.size_max = 0; > blkcfg.physical_block_exp = get_physical_block_exp(s->conf); > blkcfg.alignment_offset = 0;