| Message ID | 1335448165-26174-2-git-send-email-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
Il 26/04/2012 15:49, Christian Borntraeger ha scritto: > Currently the sector value for the geometry is masked based on > the sector size. This works fine for with 512 physical sector size, > but it fails for 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 for a physical block size / > device size that is not dividable by 4096) but for dasd this > is not the case. > > This tries to fix the block layer by handling both cases > dasd and non-dasd by taking the physical block size into > account. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > CC: Christoph Hellwig <hch@lst.de> > --- > hw/virtio-blk.c | 12 +++++++++++- > 1 files changed, 11 insertions(+), 1 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 49990f8..5258533 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -483,6 +483,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) > uint64_t capacity; > int cylinders, heads, secs; > int blk_size = s->conf->logical_block_size; > + int pblk_size = s->conf->physical_block_size; > > bdrv_get_geometry(s->bs, &capacity); > bdrv_get_geometry_hint(s->bs, &cylinders, &heads, &secs); > @@ -494,7 +495,16 @@ 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 round down the block device size to a multiple of the logical > + * block size. The geometry value for sectors is based on the physical > + * sector size and NOT on the Linux default block size of 512. For > + * example a dasd device usually has 12 sectors per track and each > + * sector has 4096 bytes (the whole cylinder == 48k). The geometry > + * is specified as secs=12. Thus we cannot use sector_mask, instead > + * we have to use some special case. > + */ > + blkcfg.sectors = secs & ~(blk_size / pblk_size - 1); I'm not sure here what you mean. Usually blk_size >= pblk_size on non-s390 systems, so this is completely different from the previous code, which is effectively blkcfg.sectors = secs & ~(blk_size / 512 - 1); I wonder if s390 gives a different meaning to logical vs. physical sector sizes, compared to what virtio expects (which is what SCSI says, basically). Physical block sizes on non-s390 systems are really just useful as an alignment hint, they do not affect correctness. Paolo > blkcfg.size_max = 0; > blkcfg.physical_block_exp = get_physical_block_exp(s->conf); > blkcfg.alignment_offset = 0;
>> + blkcfg.sectors = secs & ~(blk_size / pblk_size - 1); > > I'm not sure here what you mean. Usually blk_size >= pblk_size on > non-s390 systems, so this is completely different from the previous > code, which is effectively I was trying to prevent the masking of the sector number. the first version of the patch simply did blkcfg.sectors = secs; but this broke setups that really need that masking. > > blkcfg.sectors = secs & ~(blk_size / 512 - 1); > > I wonder if s390 gives a different meaning to logical vs. physical > sector sizes, compared to what virtio expects (which is what SCSI says, > basically). Physical block sizes on non-s390 systems are really just > useful as an alignment hint, they do not affect correctness. Maybe that really points to the problem that we are trying to solve here. For a dasd device, there is usually a 4096 byte block size and on the host these 4096 arereported via getss and getpbsz. The geometry reported by the device driver is usually 15 head and 12 sectors per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k). What I want to achieve is that the guest view is identical to the host view for cyls, heads, secs, and all block sizes. Christian
Il 02/05/2012 12:18, Christian Borntraeger ha scritto: > Maybe that really points to the problem that we are trying to solve here. > For a dasd device, there is usually a 4096 byte block size and on the host > these 4096 arereported via getss and getpbsz. > The geometry reported by the device driver is usually 15 head and 12 sectors > per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k). > > What I want to achieve is that the guest view is identical to the host view > for cyls, heads, secs, and all block sizes. I think what you want is _not_ to have the same view as the host. What you want is simply to have a default that is consistent with what is common on actual s390 disks. Perhaps what we want here is to move the guessing of cyls/head/sector count from -drive to -device, so that virtio-blk-s390 can apply a different default (cyls=auto, head=15, sector=12, and also lbsz=pbsz=4096). Markus, have you ever thought about that? I'm a bit torn because these are media parameters rather than device parameters, on the other hand the same applies to lbsz/pbsz at least. Paolo
On 02/05/12 12:25, Paolo Bonzini wrote: > Il 02/05/2012 12:18, Christian Borntraeger ha scritto: >> Maybe that really points to the problem that we are trying to solve here. >> For a dasd device, there is usually a 4096 byte block size and on the host >> these 4096 arereported via getss and getpbsz. >> The geometry reported by the device driver is usually 15 head and 12 sectors >> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k). >> >> What I want to achieve is that the guest view is identical to the host view >> for cyls, heads, secs, and all block sizes. > > I think what you want is _not_ to have the same view as the host. What > you want is simply to have a default that is consistent with what is > common on actual s390 disks. Let me put it in another way: I want to have these values to match the _device_ that we are passing to the guest because several tools and the partition detection code for a compatible disk format (those that can be accessed by z/OS) needs those values to work properly. That of course means that the guest view is identical to the host view because both views describe a real property of the hardware. IOW the geometry for dasd devices is not an artifical number, it has some real meaning that has a influence on the data structures on the disk. Thing is, the easiest way of getting the hardware property is to query the host. Does that make the situation a bit clearer? Christian
Il 02/05/2012 12:50, Christian Borntraeger ha scritto: > On 02/05/12 12:25, Paolo Bonzini wrote: >> Il 02/05/2012 12:18, Christian Borntraeger ha scritto: >>> Maybe that really points to the problem that we are trying to solve here. >>> For a dasd device, there is usually a 4096 byte block size and on the host >>> these 4096 arereported via getss and getpbsz. >>> The geometry reported by the device driver is usually 15 head and 12 sectors >>> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k). >>> >>> What I want to achieve is that the guest view is identical to the host view >>> for cyls, heads, secs, and all block sizes. >> >> I think what you want is _not_ to have the same view as the host. What >> you want is simply to have a default that is consistent with what is >> common on actual s390 disks. > > Let me put it in another way: > > I want to have these values to match the _device_ that we are passing to the guest > because several tools and the partition detection code for a compatible disk format > (those that can be accessed by z/OS) needs those values to work properly. Ah, you never pass part of a disk to a guest and part of the same disk to another? > IOW the geometry for dasd devices is not an artifical number, it has some real meaning > that has a influence on the data structures on the disk. Yes, I understood this. Paolo
On 05/02/2012 12:50 PM, Christian Borntraeger wrote: > On 02/05/12 12:25, Paolo Bonzini wrote: >> Il 02/05/2012 12:18, Christian Borntraeger ha scritto: >>> Maybe that really points to the problem that we are trying to solve here. >>> For a dasd device, there is usually a 4096 byte block size and on the host >>> these 4096 arereported via getss and getpbsz. >>> The geometry reported by the device driver is usually 15 head and 12 sectors >>> per track, but actually means 12 sectors of 4096 bytes size (a track ~ 48k). >>> >>> What I want to achieve is that the guest view is identical to the host view >>> for cyls, heads, secs, and all block sizes. >> I think what you want is _not_ to have the same view as the host. What >> you want is simply to have a default that is consistent with what is >> common on actual s390 disks. > Let me put it in another way: > > I want to have these values to match the _device_ that we are passing to the guest > because several tools and the partition detection code for a compatible disk format > (those that can be accessed by z/OS) needs those values to work properly. > > That of course means that the guest view is identical to the host view because both > views describe a real property of the hardware. > > IOW the geometry for dasd devices is not an artifical number, it has some real meaning > that has a influence on the data structures on the disk. > > Thing is, the easiest way of getting the hardware property is to query the host. > > Does that make the situation a bit clearer? Another thing to consider is that there are 2 generic types of disks: * SCSI disks * DASD disks Both can be accessed using virtio-blk-s390. The former have the same semantics on geometry as what we're used to. They use normal MBRs and essentially the geometry doesn't mean too much these days anymore ;). However, if possible, I would like to diverge these as little as possible from x86 virtio disks. For DASD disks, the geometry is important, as its disk label is usually not MBR, but something s390 specific. That one is different depending on the geometry. So here the geometry is very important. The geometry on the same disk can also be different, depending on how it's formatted. So it's not quite as easy to reconstruct it from the imformation we already have. IIUC if we have the logical block size and the information that it's a DASD disk, we should be able to guess the geometry though. Alex
Il 02/05/2012 13:07, Alexander Graf ha scritto: > Both can be accessed using virtio-blk-s390. The former have the same > semantics on geometry as what we're used to. They use normal MBRs and > essentially the geometry doesn't mean too much these days anymore ;). > However, if possible, I would like to diverge these as little as > possible from x86 virtio disks. If anything, the geometry should be guessed like we do on x86. > For DASD disks, the geometry is important, as its disk label is usually > not MBR, but something s390 specific. Can we use this to guess the geometry like we do on x86? Paolo
On 05/02/2012 01:09 PM, Paolo Bonzini wrote: > Il 02/05/2012 13:07, Alexander Graf ha scritto: >> Both can be accessed using virtio-blk-s390. The former have the same >> semantics on geometry as what we're used to. They use normal MBRs and >> essentially the geometry doesn't mean too much these days anymore ;). >> However, if possible, I would like to diverge these as little as >> possible from x86 virtio disks. > If anything, the geometry should be guessed like we do on x86. > >> For DASD disks, the geometry is important, as its disk label is usually >> not MBR, but something s390 specific. > Can we use this to guess the geometry like we do on x86? Yes, but what do you do with a blank disk? :) Alex
Il 02/05/2012 13:10, Alexander Graf ha scritto: >>> For DASD disks, the geometry is important, as its disk label is usually >>> not MBR, but something s390 specific. >> Can we use this to guess the geometry like we do on x86? > > Yes, but what do you do with a blank disk? :) Right. :) Paolo
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 49990f8..5258533 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -483,6 +483,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) uint64_t capacity; int cylinders, heads, secs; int blk_size = s->conf->logical_block_size; + int pblk_size = s->conf->physical_block_size; bdrv_get_geometry(s->bs, &capacity); bdrv_get_geometry_hint(s->bs, &cylinders, &heads, &secs); @@ -494,7 +495,16 @@ 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 round down the block device size to a multiple of the logical + * block size. The geometry value for sectors is based on the physical + * sector size and NOT on the Linux default block size of 512. For + * example a dasd device usually has 12 sectors per track and each + * sector has 4096 bytes (the whole cylinder == 48k). The geometry + * is specified as secs=12. Thus we cannot use sector_mask, instead + * we have to use some special case. + */ + blkcfg.sectors = secs & ~(blk_size / pblk_size - 1); blkcfg.size_max = 0; blkcfg.physical_block_exp = get_physical_block_exp(s->conf); blkcfg.alignment_offset = 0;
Currently the sector value for the geometry is masked based on the sector size. This works fine for with 512 physical sector size, but it fails for 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 for a physical block size / device size that is not dividable by 4096) but for dasd this is not the case. This tries to fix the block layer by handling both cases dasd and non-dasd by taking the physical block size into account. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> CC: Christoph Hellwig <hch@lst.de> --- hw/virtio-blk.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)