| Message ID | 1518096522-31141-1-git-send-email-sarna@skytechnology.pl |
|---|---|
| State | New |
| Headers | show |
| Series | block: unify blocksize types | expand |
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) \ >
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
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?
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
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).
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) \
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(-)