* [Qemu-devel] [PATCH] block: unify blocksize types @ 2018-02-08 13:28 Piotr Sarna 2018-02-08 18:12 ` John Snow 2018-02-09 2:19 ` Fam Zheng 0 siblings, 2 replies; 6+ messages in thread From: Piotr Sarna @ 2018-02-08 13:28 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, famz, mreitz, Piotr Sarna 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: unify blocksize types 2018-02-08 13:28 [Qemu-devel] [PATCH] block: unify blocksize types Piotr Sarna @ 2018-02-08 18:12 ` John Snow 2018-02-09 2:19 ` Fam Zheng 1 sibling, 0 replies; 6+ messages in thread From: John Snow @ 2018-02-08 18:12 UTC (permalink / raw) To: Piotr Sarna, qemu-devel; +Cc: kwolf, famz, mreitz, Qemu-block 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: unify blocksize types 2018-02-08 13:28 [Qemu-devel] [PATCH] block: unify blocksize types Piotr Sarna 2018-02-08 18:12 ` John Snow @ 2018-02-09 2:19 ` Fam Zheng 2018-02-09 9:44 ` Piotr Sarna 1 sibling, 1 reply; 6+ messages in thread From: Fam Zheng @ 2018-02-09 2:19 UTC (permalink / raw) To: Piotr Sarna; +Cc: qemu-devel, kwolf, mreitz 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: unify blocksize types 2018-02-09 2:19 ` Fam Zheng @ 2018-02-09 9:44 ` Piotr Sarna 2018-02-09 15:11 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Piotr Sarna @ 2018-02-09 9:44 UTC (permalink / raw) To: Fam Zheng; +Cc: qemu-devel, kwolf, mreitz 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: unify blocksize types 2018-02-09 9:44 ` Piotr Sarna @ 2018-02-09 15:11 ` Kevin Wolf 2018-02-09 20:38 ` Eric Blake 0 siblings, 1 reply; 6+ messages in thread From: Kevin Wolf @ 2018-02-09 15:11 UTC (permalink / raw) To: Piotr Sarna; +Cc: Fam Zheng, qemu-devel, mreitz 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: unify blocksize types 2018-02-09 15:11 ` Kevin Wolf @ 2018-02-09 20:38 ` Eric Blake 0 siblings, 0 replies; 6+ messages in thread From: Eric Blake @ 2018-02-09 20:38 UTC (permalink / raw) To: Kevin Wolf, Piotr Sarna; +Cc: Fam Zheng, qemu-devel, mreitz 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-09 20:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-08 13:28 [Qemu-devel] [PATCH] block: unify blocksize types Piotr Sarna 2018-02-08 18:12 ` John Snow 2018-02-09 2:19 ` Fam Zheng 2018-02-09 9:44 ` Piotr Sarna 2018-02-09 15:11 ` Kevin Wolf 2018-02-09 20:38 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.