1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] block: unify blocksize types

Piotr Sarna posted 1 patch 6 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1518096522-31141-1-git-send-email-sarna@skytechnology.pl
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppc passed
Test ppcbe passed
Test s390x passed
There is a newer version of this series
include/hw/block/block.h     | 4 ++--
include/hw/qdev-properties.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
[Qemu-devel] [PATCH] block: unify blocksize types
Posted by Piotr Sarna 6 years, 2 months ago 
BlockSizes structure used in block size probing has uint32_t types
for logical and physical sizes. These fields are wrongfully assigned
to uint16_t in BlockConf, which results, among other errors,
in assigning 0 instead of 65536 (which will be the case in at least
future LizardFS block device driver among other things).

This commit makes BlockConf's physical_block_size and logical_block_size
fields uint32_t to avoid inconsistencies.

Signed-off-by: Piotr Sarna <sarna@skytechnology.pl>
---
 include/hw/block/block.h     | 4 ++--
 include/hw/qdev-properties.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 64b9298..c9e6e27 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -17,8 +17,8 @@
 
 typedef struct BlockConf {
     BlockBackend *blk;
-    uint16_t physical_block_size;
-    uint16_t logical_block_size;
+    uint32_t physical_block_size;
+    uint32_t logical_block_size;
     uint16_t min_io_size;
     uint32_t opt_io_size;
     int32_t bootindex;
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 1d61a35..c68d7bf 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
 #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
 #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
-    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
+    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
 #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
 #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
-- 
2.7.4
Re: [Qemu-devel] [PATCH] block: unify blocksize types
Posted by John Snow 6 years, 2 months ago 
CC qemu-block

On 02/08/2018 08:28 AM, Piotr Sarna wrote:
> BlockSizes structure used in block size probing has uint32_t types
> for logical and physical sizes. These fields are wrongfully assigned
> to uint16_t in BlockConf, which results, among other errors,
> in assigning 0 instead of 65536 (which will be the case in at least
> future LizardFS block device driver among other things).
> 
> This commit makes BlockConf's physical_block_size and logical_block_size
> fields uint32_t to avoid inconsistencies.
> 
> Signed-off-by: Piotr Sarna <sarna@skytechnology.pl>
> ---
>  include/hw/block/block.h     | 4 ++--
>  include/hw/qdev-properties.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 64b9298..c9e6e27 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -17,8 +17,8 @@
>  
>  typedef struct BlockConf {
>      BlockBackend *blk;
> -    uint16_t physical_block_size;
> -    uint16_t logical_block_size;
> +    uint32_t physical_block_size;
> +    uint32_t logical_block_size;
>      uint16_t min_io_size;
>      uint32_t opt_io_size;
>      int32_t bootindex;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 1d61a35..c68d7bf 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>  #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
> 

-- 
—js
Re: [Qemu-devel] [PATCH] block: unify blocksize types
Posted by Fam Zheng 6 years, 2 months ago 
On Thu, 02/08 14:28, Piotr Sarna wrote:
> BlockSizes structure used in block size probing has uint32_t types
> for logical and physical sizes. These fields are wrongfully assigned
> to uint16_t in BlockConf, which results, among other errors,
> in assigning 0 instead of 65536 (which will be the case in at least
> future LizardFS block device driver among other things).
> > This commit makes BlockConf's physical_block_size and logical_block_size > fields uint32_t to avoid inconsistencies.
> 
> Signed-off-by: Piotr Sarna <sarna@skytechnology.pl>
> ---
>  include/hw/block/block.h     | 4 ++--
>  include/hw/qdev-properties.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 64b9298..c9e6e27 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -17,8 +17,8 @@
>  
>  typedef struct BlockConf {
>      BlockBackend *blk;
> -    uint16_t physical_block_size;
> -    uint16_t logical_block_size;
> +    uint32_t physical_block_size;
> +    uint32_t logical_block_size;
>      uint16_t min_io_size;
>      uint32_t opt_io_size;
>      int32_t bootindex;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 1d61a35..c68d7bf 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>  #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>  #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \

Do you need to update qdev_prop_blocksize and set_blocksize as well?

    const PropertyInfo qdev_prop_blocksize = {
        .name  = "uint16",
        .description = "A power of two between 512 and 32768",
        .get   = get_uint16,
        .set   = set_blocksize,
        .set_default_value = set_default_value_uint,
    };

Fam
Re: [Qemu-devel] [PATCH] block: unify blocksize types
Posted by Piotr Sarna 6 years, 2 months ago 
On 09.02.2018 03:19, Fam Zheng wrote:
> On Thu, 02/08 14:28, Piotr Sarna wrote:
>> BlockSizes structure used in block size probing has uint32_t types
>> for logical and physical sizes. These fields are wrongfully assigned
>> to uint16_t in BlockConf, which results, among other errors,
>> in assigning 0 instead of 65536 (which will be the case in at least
>> future LizardFS block device driver among other things).
>>> This commit makes BlockConf's physical_block_size and logical_block_size > fields uint32_t to avoid inconsistencies.
>> Signed-off-by: Piotr Sarna <sarna@skytechnology.pl>
>> ---
>>   include/hw/block/block.h     | 4 ++--
>>   include/hw/qdev-properties.h | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>> index 64b9298..c9e6e27 100644
>> --- a/include/hw/block/block.h
>> +++ b/include/hw/block/block.h
>> @@ -17,8 +17,8 @@
>>   
>>   typedef struct BlockConf {
>>       BlockBackend *blk;
>> -    uint16_t physical_block_size;
>> -    uint16_t logical_block_size;
>> +    uint32_t physical_block_size;
>> +    uint32_t logical_block_size;
>>       uint16_t min_io_size;
>>       uint32_t opt_io_size;
>>       int32_t bootindex;
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 1d61a35..c68d7bf 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
>>   #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
>>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
>>   #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
>> -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
>> +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
>>   #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
>>       DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
>>   #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
> Do you need to update qdev_prop_blocksize and set_blocksize as well?
>
>      const PropertyInfo qdev_prop_blocksize = {
>          .name  = "uint16",
>          .description = "A power of two between 512 and 32768",
>          .get   = get_uint16,
>          .set   = set_blocksize,
>          .set_default_value = set_default_value_uint,
>      };
>
> Fam
Yes, I do, thanks - I'll prepare patch v2 today. Also, I haven't found 
any hidden dependencies on blocksize being <= 32768, so I assume 
changing the new max value to 2^31 is safe. Could somebody more familiar 
with qemu code confirm(or invalidate) my assumption?
Re: [Qemu-devel] [PATCH] block: unify blocksize types
Posted by Kevin Wolf 6 years, 2 months ago 
Am 09.02.2018 um 10:44 hat Piotr Sarna geschrieben:
> On 09.02.2018 03:19, Fam Zheng wrote:
> > On Thu, 02/08 14:28, Piotr Sarna wrote:
> > > BlockSizes structure used in block size probing has uint32_t types
> > > for logical and physical sizes. These fields are wrongfully assigned
> > > to uint16_t in BlockConf, which results, among other errors,
> > > in assigning 0 instead of 65536 (which will be the case in at least
> > > future LizardFS block device driver among other things).
> > > > This commit makes BlockConf's physical_block_size and logical_block_size > fields uint32_t to avoid inconsistencies.
> > > Signed-off-by: Piotr Sarna <sarna@skytechnology.pl>
> > > ---
> > >   include/hw/block/block.h     | 4 ++--
> > >   include/hw/qdev-properties.h | 2 +-
> > >   2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > > index 64b9298..c9e6e27 100644
> > > --- a/include/hw/block/block.h
> > > +++ b/include/hw/block/block.h
> > > @@ -17,8 +17,8 @@
> > >   typedef struct BlockConf {
> > >       BlockBackend *blk;
> > > -    uint16_t physical_block_size;
> > > -    uint16_t logical_block_size;
> > > +    uint32_t physical_block_size;
> > > +    uint32_t logical_block_size;
> > >       uint16_t min_io_size;
> > >       uint32_t opt_io_size;
> > >       int32_t bootindex;
> > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > > index 1d61a35..c68d7bf 100644
> > > --- a/include/hw/qdev-properties.h
> > > +++ b/include/hw/qdev-properties.h
> > > @@ -210,7 +210,7 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
> > >   #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
> > >       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
> > >   #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
> > > -    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
> > > +    DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint32_t)
> > >   #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> > >       DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> > >   #define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
> > Do you need to update qdev_prop_blocksize and set_blocksize as well?
> > 
> >      const PropertyInfo qdev_prop_blocksize = {
> >          .name  = "uint16",
> >          .description = "A power of two between 512 and 32768",
> >          .get   = get_uint16,
> >          .set   = set_blocksize,
> >          .set_default_value = set_default_value_uint,
> >      };
> > 
> > Fam
> Yes, I do, thanks - I'll prepare patch v2 today. Also, I haven't found any
> hidden dependencies on blocksize being <= 32768, so I assume changing the
> new max value to 2^31 is safe. Could somebody more familiar with qemu code
> confirm(or invalidate) my assumption?

The IDE code has the following line in the emulation of the IDENTIFY
DEVICE command:

    put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));

That is, the result of get_physical_block_exp() is just blindly ORed to
the word. The IDE spec says that bits 0-3 contain the exponent. Four
bits mean a maximum of 15 (and 2^15 = 32768). After that, we don't
actually expose a larger block size, but start modifying reserved bits.

I haven't checked other device models, but I wouldn't rule out that they
make similar assumptions.

Kevin
Re: [Qemu-devel] [PATCH] block: unify blocksize types
Posted by Eric Blake 6 years, 2 months ago 
On 02/09/2018 09:11 AM, Kevin Wolf wrote:

>> Yes, I do, thanks - I'll prepare patch v2 today. Also, I haven't found any
>> hidden dependencies on blocksize being <= 32768, so I assume changing the
>> new max value to 2^31 is safe. Could somebody more familiar with qemu code
>> confirm(or invalidate) my assumption?
> 
> The IDE code has the following line in the emulation of the IDENTIFY
> DEVICE command:
> 
>      put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
> 
> That is, the result of get_physical_block_exp() is just blindly ORed to
> the word. The IDE spec says that bits 0-3 contain the exponent. Four
> bits mean a maximum of 15 (and 2^15 = 32768). After that, we don't
> actually expose a larger block size, but start modifying reserved bits.
> 
> I haven't checked other device models, but I wouldn't rule out that they
> make similar assumptions.

NBD documents that:

The minimum block size represents the smallest addressable length and 
alignment within the export, although writing to an area that small may 
require the server to use a less-efficient read-modify-write action. If 
advertised, this value MUST be a power of 2, MUST NOT be larger than 
2^16 (65,536), and MAY be as small as 1 for an export backed by a 
regular file, although the values of 2^9 (512) or 2^12 (4,096) are more 
typical for an export backed by a block device. If a server advertises a 
minimum block size, the advertised export size SHOULD be an integer 
multiple of that block size, since otherwise, the client would be unable 
to access the final few bytes of the export.

We probably need to get that enlarged to a bigger minimum block size, if 
there really are devices that require more than a 64k minimum access 
size.  But you do NOT want 2^31 as a permitted block size; that implies 
that any action smaller than the block size will be performed as a 
read-modify-write; and reading 2G just to modify a subset then write 
back 2G is painfully slow.  1M might be a much more reasonable maximum 
block size (if 64k is indeed too small in practice for existing hardware).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.