Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
Ekaterina Tumanova writes: > On 11/28/2014 01:35 PM, Stefan Hajnoczi wrote: >> On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote: >>> On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: 3. Why does s390 need to customize hd_geometry_guess()? >>> Since hd_geometry_guess contains semantics of x86-specific LBA translation, >>> we have to modify it not to get in the way of z >>> architecture >> >> If the behavior is x86-specific, it should be made x86-specific. z, >> ppc, sparc, arm, etc should share the non-x86-specific code path. It's >> weird to single out z here, this seems like the wrong way around. >> >> Is the x86-specific behavior a problem in practice? No one seemed to >> mind for the other targets. >> > > on s390 arch this adds 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. Isn't it like this for all targets? If we can't guess LCHS from MSDOS partition table guess geometry on size translation is NONE for very small disk, else LBA Else if LCHS guess has heads > 16 BIOS LBA translation must be active guess geometry on size translation is LARGE for sufficiently small disk, else LBA Else use LCHS guess translation is NONE Before writing a partition table, the default geometry is based on size. Afterwards, we it may be based on the partition table instead. Yes, writing an MS-DOS partition table can change the default geometry. Horrible misfeature, independent of target. Every time I try to kill a horrible misfeature, Kevin yells at me ;) Mitigating factor 1: no change when the partition table looks like LBA is active. Should be the common case nowadays. Mitigating factor 2: most guest software is blissfully unaware of geometry. > If you guys don't like the way patch goes, I can exclude it from patch > series and we can discuss it later. > > But I thought that since it's related to the hard drive geometry, > and since we change hd_geometry_guess in this patchset anyway, why not > get rid of this problem as well. I think we should discuss what we actually want, then let you implement it. Perhaps the stupidest solution that could possibly work is the state after your PATCH 5: If backend has a geometry (currently only DASD has; may change) // Incompatible change! use backend geometry translation is NONE Else if we can't guess LCHS from MSDOS partition table guess geometry on size translation is NONE for very small disk, else LBA Else if LCHS guess has heads > 16 BIOS LBA translation must be active guess geometry on size translation is LARGE for sufficiently small disk, else LBA Else use LCHS guess translation is NONE Note that translation is relevant only for a PC machine's IDE disks. Everything else ignores it.
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
On 11/28/2014 01:35 PM, Stefan Hajnoczi wrote: On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote: On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: 3. Why does s390 need to customize hd_geometry_guess()? Since hd_geometry_guess contains semantics of x86-specific LBA translation, we have to modify it not to get in the way of z architecture If the behavior is x86-specific, it should be made x86-specific. z, ppc, sparc, arm, etc should share the non-x86-specific code path. It's weird to single out z here, this seems like the wrong way around. Is the x86-specific behavior a problem in practice? No one seemed to mind for the other targets. on s390 arch this adds 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. If you guys don't like the way patch goes, I can exclude it from patch series and we can discuss it later. But I thought that since it's related to the hard drive geometry, and since we change hd_geometry_guess in this patchset anyway, why not get rid of this problem as well. Kate.
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
Ekaterina Tumanova writes: > On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: [...] >> 3. Why does s390 need to customize hd_geometry_guess()? >> > Since hd_geometry_guess contains semantics of x86-specific LBA > translation, we have to modify it not to get in the way of z > architecture I already explained this in my review, but it's buried among other stuff, so let me repeat it here: The only caller that passes non-null ptrans to hd_geometry_guess() is ide_dev_initfn(), and the only user of the value that gets stored there is pc_cmos_init_late(). We store a highly PC-specific value there. That's okay, it's used only by PC machines. The whole concept "BIOS translation" makes no sense for other machines. Heck, it makes barely sense even on PCs.
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote: > On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: > >On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: > >4. Please use scripts/checkpatch.pl to check coding style. > > > I did :) You are right, checkpatch.pl doesn't complain. That's weird, I thought there were a few instances that don't follow the QEMU coding style. Either I was wrong or checkpatch.pl is incomplete. Stefan pgp_ZYDPqDTK1.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
On Wed, Nov 26, 2014 at 01:16:48PM +0300, Ekaterina Tumanova wrote: > On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: > >On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: > >3. Why does s390 need to customize hd_geometry_guess()? > > > Since hd_geometry_guess contains semantics of x86-specific LBA translation, > we have to modify it not to get in the way of z > architecture If the behavior is x86-specific, it should be made x86-specific. z, ppc, sparc, arm, etc should share the non-x86-specific code path. It's weird to single out z here, this seems like the wrong way around. Is the x86-specific behavior a problem in practice? No one seemed to mind for the other targets. pgpT85luBvEdh.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
On 11/25/2014 04:01 PM, Stefan Hajnoczi wrote: On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: Hi folks, I'm sorry for the recent spam. I messed up during code submission last time. So please ignore any previous notes you received from me and answer only to this thread. This is the rework of the geometry+blocksize patch, which was recently discussed here: http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html Markus suggested that we only detect blocksize and geometry for DASDs. According to this agreement new version contains DASD special casing. The driver methods are implemented only for "host_device" and inner hdev_xxx functions check if the backing storage is a DASD by means of BIODASDINFO2 ioctl. Original patchset can be found here: http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html This is description is mainly a changelog. Links to previous email threads are useful for additional info but please include a self-contained description of the series and the rationale behind it. will include into the next version Comments: 1. This series overrides the logical_block_size and physical_block_size options for raw images on DASD devices. Users expect their command-line options to be honored, so the options should not be overriden if they have been given on the command-line. will fix that 2. Only virtio_blk is modified, this is inconsistent. All emulated storage controllers using BlockConf have the same block size probing behavior. I will add blkconf_blocksizes call to other BlockConf users. 3. Why does s390 need to customize hd_geometry_guess()? Since hd_geometry_guess contains semantics of x86-specific LBA translation, we have to modify it not to get in the way of z architecture 4. Please use scripts/checkpatch.pl to check coding style. I did :) Thanks a lot, Kate.
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
On Wed, Nov 19, 2014 at 11:17:50AM +0100, Ekaterina Tumanova wrote: > Hi folks, > > I'm sorry for the recent spam. I messed up during code submission last time. > So please ignore any previous notes you received from me and answer only to > this thread. > > This is the rework of the geometry+blocksize patch, which was > recently discussed here: > http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html > > Markus suggested that we only detect blocksize and geometry for DASDs. > > According to this agreement new version contains DASD special casing. > The driver methods are implemented only for "host_device" and inner hdev_xxx > functions check if the backing storage is a DASD by means of > BIODASDINFO2 ioctl. > > Original patchset can be found here: > http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html This is description is mainly a changelog. Links to previous email threads are useful for additional info but please include a self-contained description of the series and the rationale behind it. Comments: 1. This series overrides the logical_block_size and physical_block_size options for raw images on DASD devices. Users expect their command-line options to be honored, so the options should not be overriden if they have been given on the command-line. 2. Only virtio_blk is modified, this is inconsistent. All emulated storage controllers using BlockConf have the same block size probing behavior. 3. Why does s390 need to customize hd_geometry_guess()? 4. Please use scripts/checkpatch.pl to check coding style. pgpwVAPAj1gSd.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: > Hi folks, > > I'm sorry for the recent spam. I messed up during code submission last time. > So please ignore any previous notes you received from me and answer only to > this thread. > > This is the rework of the geometry+blocksize patch, which was > recently discussed here: > http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html > > Markus suggested that we only detect blocksize and geometry for DASDs. > > According to this agreement new version contains DASD special casing. > The driver methods are implemented only for "host_device" and inner hdev_xxx > functions check if the backing storage is a DASD by means of > BIODASDINFO2 ioctl. > > Original patchset can be found here: > http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html I have done some review on the patches itself, but the overall direction if this is the way to do for the block layer has to come from Kevin, Stefan or Markus. Christian
Re: [Qemu-devel] [PATCH v2 0/6] Geometry and blocksize support for backing devices
Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova:
> Hi folks,
>
> I'm sorry for the recent spam. I messed up during code submission last time.
> So please ignore any previous notes you received from me and answer only to
> this thread.
>
> This is the rework of the geometry+blocksize patch, which was
> recently discussed here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01148.html
>
> Markus suggested that we only detect blocksize and geometry for DASDs.
>
> According to this agreement new version contains DASD special casing.
> The driver methods are implemented only for "host_device" and inner hdev_xxx
> functions check if the backing storage is a DASD by means of
> BIODASDINFO2 ioctl.
>
> Original patchset can be found here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03791.html
>
> Ekaterina Tumanova (6):
> geometry: add bdrv functions for geometry and blocksize
> geometry: Detect blocksize via ioctls in separate static functions
> geometry: Add driver methods to probe blocksizes and geometry
> geometry: Add block-backend wrappers for geometry probing
> geometry: Call backend function to detect geometry and blocksize
> geometry: Target specific hook for s390x in geometry guessing
>
> block.c| 26 +
> block/block-backend.c | 10
> block/raw-posix.c | 123
> ++---
> block/raw_bsd.c| 12
> hw/block/Makefile.objs | 6 +-
> hw/block/block.c | 11
> hw/block/hd-geometry.c | 43 --
> hw/block/virtio-blk.c | 1 +
> include/block/block.h | 20 +++
> include/block/block_int.h | 3 +
> include/hw/block/block.h | 1 +
> include/sysemu/block-backend.h | 2 +
> 12 files changed, 234 insertions(+), 24 deletions(-)
>
I can confirm that it makes dasd devices on s390 work (partition detection is
fine, so geometry/sector size must be as well)
This patch set needs to be fixed for i386, though:
/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c: In function
'hd_geometry_guess':
/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c:159:5: error: pointer targets
in passing argument 2 of 'guess_disk_lchs' differ in signedness
[-Werror=pointer-sign]
if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) {
^
/home/cborntra/REPOS/qemu/hw/block/hd-geometry.c:52:12: note: expected
'uint32_t *' but argument is of type 'int *'
static int guess_disk_lchs(BlockBackend *blk, uint32_t *pcylinders,
