Re: [Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based

[Eric Blake]

On 06/21/2016 07:50 AM, Kevin Wolf wrote:
> Am 14.06.2016 um 23:30 hat Eric Blake geschrieben:
>> Sector-based limits are awkward to think about; in our on-going
>> quest to move to byte-based interfaces, convert max_transfer_length
>> and opt_transfer_length.  Rename them (dropping the _length suffix)
>> so that the compiler will help us catch the change in semantics
>> across any rebased code, and improve the documentation.  Use unsigned
>> values, so that we don't have to worry about negative values and
>> so that bit-twiddling is easier; however, we are still constrained
>> by 2^31 of signed int in most APIs.
>>
>> Signed-off-by: Eric Blake 
> 
>> @@ -1738,8 +1742,8 @@ static void iscsi_refresh_limits(BlockDriverState *bs, 
>> Error **errp)
>>  } else {
>>  bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
>>  }
>> -bs->bl.opt_transfer_length =
>> -sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
>> +assert(iscsilun->bl.opt_xfer_len < INT_MAX / iscsilun->block_size);
>> +bs->bl.opt_transfer = iscsilun->bl.opt_xfer_len * iscsilun->block_size;
>>  }
> 
> iscsilun->bl.opt_xfer_len comes directly from libiscsi, and presumably
> from the iscsi server, without being checked or sanitised. I don't think
> we can assert a specific range of values for it but must assume that it
> can be any uint32_t.
> 
> We can return an error for a device with a value that we don't like
> (even though using the maximum might be just fine), but crashing qemu is
> not an option.

I guess there's two possible problems: if the value is not a power of 2,
it affects how we want to use it (we probably ought to raise an error
there); and if it is oversized, we can just silently ignore the limit
(since we can't hit it).  I'll see what I can come up with for v3.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based

[Kevin Wolf]

Am 14.06.2016 um 23:30 hat Eric Blake geschrieben:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_transfer_length
> and opt_transfer_length.  Rename them (dropping the _length suffix)
> so that the compiler will help us catch the change in semantics
> across any rebased code, and improve the documentation.  Use unsigned
> values, so that we don't have to worry about negative values and
> so that bit-twiddling is easier; however, we are still constrained
> by 2^31 of signed int in most APIs.
> 
> Signed-off-by: Eric Blake 

> @@ -1738,8 +1742,8 @@ static void iscsi_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>  } else {
>  bs->bl.pwrite_zeroes_alignment = iscsilun->block_size;
>  }
> -bs->bl.opt_transfer_length =
> -sector_limits_lun2qemu(iscsilun->bl.opt_xfer_len, iscsilun);
> +assert(iscsilun->bl.opt_xfer_len < INT_MAX / iscsilun->block_size);
> +bs->bl.opt_transfer = iscsilun->bl.opt_xfer_len * iscsilun->block_size;
>  }

iscsilun->bl.opt_xfer_len comes directly from libiscsi, and presumably
from the iscsi server, without being checked or sanitised. I don't think
we can assert a specific range of values for it but must assume that it
can be any uint32_t.

We can return an error for a device with a value that we don't like
(even though using the maximum might be just fine), but crashing qemu is
not an option.

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index aacf132..f2bea85 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -752,7 +752,8 @@ static void raw_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>  if (S_ISBLK(st.st_mode)) {
>  int ret = hdev_get_max_transfer_length(s->fd);
>  if (ret >= 0) {
> -bs->bl.max_transfer_length = ret;
> +assert(ret <= BDRV_REQUEST_MAX_SECTORS);
> +bs->bl.max_transfer = ret << BDRV_SECTOR_BITS;
>  }
>  }
>  }

Same thing here.

Kevin

Re: [Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based

[Fam Zheng]

On Tue, 06/14 15:30, Eric Blake wrote:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_transfer_length
> and opt_transfer_length.  Rename them (dropping the _length suffix)
> so that the compiler will help us catch the change in semantics
> across any rebased code, and improve the documentation.  Use unsigned
> values, so that we don't have to worry about negative values and
> so that bit-twiddling is easier; however, we are still constrained
> by 2^31 of signed int in most APIs.
> 
> Signed-off-by: Eric Blake 

Looks good apart from the scsi-generic blocksize calculation, which is not an
issue of this patch.

Reviewed-by: Fam Zheng 

[Qemu-devel] [PATCH v2 14/17] block: Switch transfer length bounds to byte-based

[Eric Blake]

Sector-based limits are awkward to think about; in our on-going
quest to move to byte-based interfaces, convert max_transfer_length
and opt_transfer_length.  Rename them (dropping the _length suffix)
so that the compiler will help us catch the change in semantics
across any rebased code, and improve the documentation.  Use unsigned
values, so that we don't have to worry about negative values and
so that bit-twiddling is easier; however, we are still constrained
by 2^31 of signed int in most APIs.

Signed-off-by: Eric Blake 

---
v2: Hoist unrelated cleanups earlier, use name 'max_transfer' in more
places, tweak iscsi calculations
---
 include/block/block_int.h  | 11 +++
 include/sysemu/block-backend.h |  2 +-
 block/block-backend.c  | 10 +-
 block/io.c | 23 +++
 block/iscsi.c  | 20 
 block/nbd.c|  2 +-
 block/raw-posix.c  |  3 ++-
 hw/block/virtio-blk.c  |  9 +
 hw/scsi/scsi-generic.c | 11 ++-
 qemu-img.c |  8 
 10 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 16c43e2..2b45d57 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -338,11 +338,14 @@ typedef struct BlockLimits {
  * power of 2, and less than max_pwrite_zeroes if that is set */
 uint32_t pwrite_zeroes_alignment;

-/* optimal transfer length in sectors */
-int opt_transfer_length;
+/* optimal transfer length in bytes (must be power of 2, and
+ * multiple of bs->request_alignment), or 0 if no preferred size */
+uint32_t opt_transfer;

-/* maximal transfer length in sectors */
-int max_transfer_length;
+/* maximal transfer length in bytes (need not be power of 2, but
+ * should be multiple of opt_transfer), or 0 for no 32-bit limit.
+ * For now, anything larger than INT_MAX is clamped down. */
+uint32_t max_transfer;

 /* memory alignment so that no bounce buffer is needed */
 size_t min_mem_alignment;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..2469a1c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -170,7 +170,7 @@ bool blk_is_available(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
-int blk_get_max_transfer_length(BlockBackend *blk);
+uint32_t blk_get_max_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_try_blockalign(BlockBackend *blk, size_t size);
diff --git a/block/block-backend.c b/block/block-backend.c
index 1fb070b..e042544 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1303,16 +1303,16 @@ int blk_get_flags(BlockBackend *blk)
 }
 }

-/* Returns the maximum transfer length, in sectors; guaranteed nonzero */
-int blk_get_max_transfer_length(BlockBackend *blk)
+/* Returns the maximum transfer length, in bytes; guaranteed nonzero */
+uint32_t blk_get_max_transfer(BlockBackend *blk)
 {
 BlockDriverState *bs = blk_bs(blk);
-int max = 0;
+uint32_t max = 0;

 if (bs) {
-max = bs->bl.max_transfer_length;
+max = bs->bl.max_transfer;
 }
-return MIN_NON_ZERO(max, BDRV_REQUEST_MAX_SECTORS);
+return MIN_NON_ZERO(max, INT_MAX);
 }

 int blk_get_max_iov(BlockBackend *blk)
diff --git a/block/io.c b/block/io.c
index c425ce8..5e38ab4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -88,8 +88,8 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
 error_propagate(errp, local_err);
 return;
 }
-bs->bl.opt_transfer_length = bs->file->bs->bl.opt_transfer_length;
-bs->bl.max_transfer_length = bs->file->bs->bl.max_transfer_length;
+bs->bl.opt_transfer = bs->file->bs->bl.opt_transfer;
+bs->bl.max_transfer = bs->file->bs->bl.max_transfer;
 bs->bl.min_mem_alignment = bs->file->bs->bl.min_mem_alignment;
 bs->bl.opt_mem_alignment = bs->file->bs->bl.opt_mem_alignment;
 bs->bl.max_iov = bs->file->bs->bl.max_iov;
@@ -107,12 +107,10 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
**errp)
 error_propagate(errp, local_err);
 return;
 }
-bs->bl.opt_transfer_length =
-MAX(bs->bl.opt_transfer_length,
-bs->backing->bs->bl.opt_transfer_length);
-bs->bl.max_transfer_length =
-MIN_NON_ZERO(bs->bl.max_transfer_length,
- bs->backing->bs->bl.max_transfer_length);
+bs->bl.opt_transfer = MAX(bs->bl.opt_transfer,
+  bs->backing->bs->bl.opt_transfer);
+bs->bl.max_transfer =