Re: [PATCH] virtio-blk: avoid using ioeventfd state in irqfd conditional
On Mon, 22 Jan 2024 at 12:27, Stefan Hajnoczi wrote:
>
> Requests that complete in an IOThread use irqfd to notify the guest
> while requests that complete in the main loop thread use the traditional
> qdev irq code path. The reason for this conditional is that the irq code
> path requires the BQL:
>
> if (s->ioeventfd_started && !s->ioeventfd_disabled) {
> virtio_notify_irqfd(vdev, req->vq);
> } else {
> virtio_notify(vdev, req->vq);
> }
>
> There is a corner case where the conditional invokes the irq code path
> instead of the irqfd code path:
>
> static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
> {
> ...
> /*
>* Set ->ioeventfd_started to false before draining so that host
> notifiers
>* are not detached/attached anymore.
>*/
> s->ioeventfd_started = false;
>
> /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
> blk_drain(s->conf.conf.blk);
>
> During blk_drain() the conditional produces the wrong result because
> ioeventfd_started is false.
>
> Use qemu_in_iothread() instead of checking the ioeventfd state.
>
> Buglink: https://issues.redhat.com/browse/RHEL-15394
> Signed-off-by: Stefan Hajnoczi
> ---
Ping?
> Based-on:
> https://repo.or.cz/qemu/kevin.git/shortlog/c14962c3ea6f0998d028142ed14affcb9dfccf28
>
> Stable backport notes: dataplane_started is being renamed to
> ioeventfd_started in the next block pull request. This patch can be
> safely applied to -stable although the variable name has changed and
> git-am will complain.
>
> hw/block/virtio-blk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 227d83569f..287c31ee3c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -64,7 +64,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req,
> unsigned char status)
> iov_discard_undo(>inhdr_undo);
> iov_discard_undo(>outhdr_undo);
> virtqueue_push(req->vq, >elem, req->in_len);
> -if (s->ioeventfd_started && !s->ioeventfd_disabled) {
> +if (qemu_in_iothread()) {
> virtio_notify_irqfd(vdev, req->vq);
> } else {
> virtio_notify(vdev, req->vq);
> --
> 2.43.0
>
>
[PULL 2/5] block/blkio: Make s->mem_region_alignment be 64 bits
From: "Richard W.M. Jones"
With GCC 14 the code failed to compile on i686 (and was wrong for any
version of GCC):
../block/blkio.c: In function ‘blkio_file_open’:
../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
incompatible pointer type [-Wincompatible-pointer-types]
857 |>mem_region_alignment);
|^~~~
||
|size_t * {aka unsigned int *}
In file included from ../block/blkio.c:12:
/usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
*value);
| ~~^
Signed-off-by: Richard W.M. Jones
Message-id: 20240130122006.2977938-1-rjo...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
block/blkio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blkio.c b/block/blkio.c
index 0a0a6c0f5f..bc2f21784c 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -68,7 +68,7 @@ typedef struct {
CoQueue bounce_available;
/* The value of the "mem-region-alignment" property */
-size_t mem_region_alignment;
+uint64_t mem_region_alignment;
/* Can we skip adding/deleting blkio_mem_regions? */
bool needs_mem_regions;
--
2.43.0
[PULL 4/5] hw/core/qdev.c: add qdev_get_human_name()
From: Manos Pitsidianakis
Add a simple method to return some kind of human readable identifier for
use in error messages.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Manos Pitsidianakis
Message-id:
8b566bfced98ae44be1fcc1f8e7215f0c3393aa1.1706598705.git.manos.pitsidiana...@linaro.org
Signed-off-by: Stefan Hajnoczi
---
include/hw/qdev-core.h | 14 ++
hw/core/qdev.c | 8
2 files changed, 22 insertions(+)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..66338f479f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,20 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..c68d0f7c51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,14 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+g_assert(dev != NULL);
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
--
2.43.0
[PULL 1/5] block/io_uring: improve error message when init fails
From: Fiona Ebner
The man page for io_uring_queue_init states:
> io_uring_queue_init(3) returns 0 on success and -errno on failure.
and the man page for io_uring_setup (which is one of the functions
where the return value of io_uring_queue_init() can come from) states:
> On error, a negative error code is returned. The caller should not
> rely on errno variable.
Tested using 'sysctl kernel.io_uring_disabled=2'. Output before this
change:
> failed to init linux io_uring ring
Output after this change:
> failed to init linux io_uring ring: Operation not permitted
Signed-off-by: Fiona Ebner
Signed-off-by: Stefan Hajnoczi
Message-ID: <20240123135044.204985-1-f.eb...@proxmox.com>
---
block/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/io_uring.c b/block/io_uring.c
index d77ae55745..d11b2051ab 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -432,7 +432,7 @@ LuringState *luring_init(Error **errp)
rc = io_uring_queue_init(MAX_ENTRIES, ring, 0);
if (rc < 0) {
-error_setg_errno(errp, errno, "failed to init linux io_uring ring");
+error_setg_errno(errp, -rc, "failed to init linux io_uring ring");
g_free(s);
return NULL;
}
--
2.43.0
[PULL 3/5] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()
The following expression is incorrect because blk_pread_nonzeroes()
deals in units of bytes, not sectors:
bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS)
^^^
BDRV_REQUEST_MAX_BYTES is the appropriate constant.
Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image")
Cc: Xiang Zheng
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20240130002712.257815-1-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
hw/block/block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ff503002aa 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
BlockDriverState *bs = blk_bs(blk);
for (;;) {
-bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
+bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES);
if (bytes <= 0) {
return 0;
}
--
2.43.0
Re: [PATCH v3 0/2] hw/block/block.c: improve confusing error
On Tue, Jan 30, 2024 at 09:30:30AM +0200, Manos Pitsidianakis wrote: > In cases where a device tries to read more bytes than the block device > contains with the blk_check_size_and_read_all() function, the error is > vague: "device requires X bytes, block backend provides Y bytes". > > This patch changes the errors of this function to include the block > backend name, the device id and device type name where appropriate. > > Version 3: > - Changed phrasing "%s device with id='%s'" to "%s device '%s'" since > second parameter might be either device id or device path. > (thanks Stefan Hajnoczi ) > > Version 2: > - Assert dev is not NULL on qdev_get_human_name > (thanks Phil Mathieu-Daudé ) > > Manos Pitsidianakis (2): > hw/core/qdev.c: add qdev_get_human_name() > hw/block/block.c: improve confusing blk_check_size_and_read_all() > error > > include/hw/block/block.h | 4 ++-- > include/hw/qdev-core.h | 14 ++ > hw/block/block.c | 25 +++-- > hw/block/m25p80.c| 3 ++- > hw/block/pflash_cfi01.c | 4 ++-- > hw/block/pflash_cfi02.c | 2 +- > hw/core/qdev.c | 8 > 7 files changed, 44 insertions(+), 16 deletions(-) > > Range-diff against v2: > 1: 5fb5879708 ! 1: 8b566bfced hw/core/qdev.c: add qdev_get_human_name() > @@ Commit message > Add a simple method to return some kind of human readable identifier > for > use in error messages. > > +Reviewed-by: Stefan Hajnoczi > Signed-off-by: Manos Pitsidianakis > > ## include/hw/qdev-core.h ## > 2: 8e7eb17fbd ! 2: 7260eadff2 hw/block/block.c: improve confusing > blk_check_size_and_read_all() error > @@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, > hwaddr size, > - "block backend provides %" PRIu64 " bytes", > - size, blk_len); > +dev_id = qdev_get_human_name(dev); > -+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu > ++error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu > + " bytes, %s block backend provides %" PRIu64 " > bytes", > + object_get_typename(OBJECT(dev)), dev_id, size, > + blk_name(blk), blk_len); > @@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, > void *buf, > -error_setg_errno(errp, -ret, "can't read block backend"); > +dev_id = qdev_get_human_name(dev); > +error_setg_errno(errp, -ret, "can't read %s block backend" > -+ "for %s device with id='%s'", > ++ " for %s device '%s'", > + blk_name(blk), > object_get_typename(OBJECT(dev)), > + dev_id); > return false; > > base-commit: 11be70677c70fdccd452a3233653949b79e97908 > -- > γαῖα πυρί μιχθήτω > Thanks, applied to my block tree: https://gitlab.com/stefanha/qemu/commits/block Stefan signature.asc Description: PGP signature
[PULL 5/5] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
From: Manos Pitsidianakis
In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
Message-id:
7260eadff22c08457740117c1bb7bd2b4353acb9.1706598705.git.manos.pitsidiana...@linaro.org
Signed-off-by: Stefan Hajnoczi
---
include/hw/block/block.h | 4 ++--
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 15fff66435..de3946a5f1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -88,8 +88,8 @@ static inline unsigned int get_physical_block_exp(BlockConf
*conf)
/* Backend access helpers */
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp);
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp);
/* Configuration helpers */
diff --git a/hw/block/block.c b/hw/block/block.c
index ff503002aa..3ceca7dce6 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
* BDRV_REQUEST_MAX_BYTES.
* On success, return true.
* On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
* This function not intended for actual block devices, which read on
* demand. It's for things like memory devices that (ab)use a block
* backend to provide persistence.
*/
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp)
{
int64_t blk_len;
int ret;
+g_autofree char *dev_id = NULL;
blk_len = blk_getlength(blk);
if (blk_len < 0) {
error_setg_errno(errp, -blk_len,
- "can't get size of block backend");
+ "can't get size of %s block backend", blk_name(blk));
return false;
}
if (blk_len != size) {
-error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
- "block backend provides %" PRIu64 " bytes",
- size, blk_len);
+dev_id = qdev_get_human_name(dev);
+error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
+ " bytes, %s block backend provides %" PRIu64 " bytes",
+ object_get_typename(OBJECT(dev)), dev_id, size,
+ blk_name(blk), blk_len);
return false;
}
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
*buf, hwaddr size,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
-error_setg_errno(errp, -ret, "can't read block backend");
+dev_id = qdev_get_human_name(dev);
+error_setg_errno(errp, -ret, "can't read %s block backend"
+ " for %s device '%s'",
+ blk_name(blk), object_get_typename(OBJECT(dev)),
+ dev_id);
return false;
}
return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+ s->storage, s->size, errp)) {
return;
}
} else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
- errp)) {
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+ total_len, errp)) {
vmstate_unregister_ram(>mem, DEVICE(pfl));
return;
}
diff --git
[PULL 0/5] Block patches
The following changes since commit 11be70677c70fdccd452a3233653949b79e97908: Merge tag 'pull-vfio-20240129' of https://github.com/legoater/qemu into staging (2024-01-29 10:53:56 +) are available in the Git repository at: https://gitlab.com/stefanha/qemu.git tags/block-pull-request for you to fetch changes up to 954b33daee83fe79293fd81c2f7371db48e7d6bd: hw/block/block.c: improve confusing blk_check_size_and_read_all() error (2024-01-30 16:19:00 -0500) Pull request Fiona Ebner (1): block/io_uring: improve error message when init fails Manos Pitsidianakis (2): hw/core/qdev.c: add qdev_get_human_name() hw/block/block.c: improve confusing blk_check_size_and_read_all() error Richard W.M. Jones (1): block/blkio: Make s->mem_region_alignment be 64 bits Stefan Hajnoczi (1): pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes() include/hw/block/block.h | 4 ++-- include/hw/qdev-core.h | 14 ++ block/blkio.c| 2 +- block/io_uring.c | 2 +- hw/block/block.c | 27 --- hw/block/m25p80.c| 3 ++- hw/block/pflash_cfi01.c | 4 ++-- hw/block/pflash_cfi02.c | 2 +- hw/core/qdev.c | 8 9 files changed, 47 insertions(+), 19 deletions(-) -- 2.43.0
Re: [PATCH] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()
On Mon, Jan 29, 2024 at 07:27:12PM -0500, Stefan Hajnoczi wrote:
> The following expression is incorrect because blk_pread_nonzeroes()
> deals in units of bytes, not sectors:
>
> bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS)
> ^^^
>
> BDRV_REQUEST_MAX_BYTES is the appropriate constant.
>
> Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image")
> Cc: Xiang Zheng
> Signed-off-by: Stefan Hajnoczi
> ---
> hw/block/block.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v2] block/blkio: Make s->mem_region_alignment be 64 bits
On Tue, Jan 30, 2024 at 12:20:01PM +, Richard W.M. Jones wrote:
> With GCC 14 the code failed to compile on i686 (and was wrong for any
> version of GCC):
>
> ../block/blkio.c: In function ‘blkio_file_open’:
> ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
> incompatible pointer type [-Wincompatible-pointer-types]
> 857 |>mem_region_alignment);
> |^~~~
> ||
> |size_t * {aka unsigned int *}
> In file included from ../block/blkio.c:12:
> /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
>49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> *value);
> |
> ~~^
>
> Signed-off-by: Richard W.M. Jones
> ---
> block/blkio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block
Stefan
signature.asc
Description: PGP signature
Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit
On Tue, Jan 30, 2024 at 12:19:37PM +, Richard W.M. Jones wrote:
> On Tue, Jan 30, 2024 at 01:04:46PM +0100, Kevin Wolf wrote:
> > Am 30.01.2024 um 11:30 hat Richard W.M. Jones geschrieben:
> > > On Tue, Jan 30, 2024 at 09:51:59AM +0100, Kevin Wolf wrote:
> > > > Am 29.01.2024 um 19:53 hat Richard W.M. Jones geschrieben:
> > > > > With GCC 14 the code failed to compile on i686 (and was wrong for any
> > > > > version of GCC):
> > > > >
> > > > > ../block/blkio.c: In function ‘blkio_file_open’:
> > > > > ../block/blkio.c:857:28: error: passing argument 3 of
> > > > > ‘blkio_get_uint64’ from incompatible pointer type
> > > > > [-Wincompatible-pointer-types]
> > > > > 857 |>mem_region_alignment);
> > > > > |^~~~
> > > > > ||
> > > > > |size_t * {aka unsigned int *}
> > > > > In file included from ../block/blkio.c:12:
> > > > > /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long
> > > > > long unsigned int *’} but argument is of type ‘size_t *’ {aka
> > > > > ‘unsigned int *’}
> > > > >49 | int blkio_get_uint64(struct blkio *b, const char *name,
> > > > > uint64_t *value);
> > > > > |
> > > > > ~~^
> > > > >
> > > > > Signed-off-by: Richard W.M. Jones
> > > >
> > > > Why not simply make BDRVBlkioState.mem_region_alignment a uint64_t
> > > > instead of keeping it size_t and doing an additional conversion with
> > > > a check that requires an #if (probably to avoid a warning on 64 bit
> > > > hosts because the condition is never true)?
> > >
> > > The smaller change (attached) does work on i686, but this worries me a
> > > little (although it doesn't give any error or warning):
> > >
> > > if (((uintptr_t)host | size) % s->mem_region_alignment) {
> > > error_setg(errp, "unaligned buf %p with size %zu", host, size);
> > > return BMRR_FAIL;
> > > }
> >
> > I don't see the problem? The calculation will now be done in 64 bits
> > even on a 32 bit host, but that seems fine to me. Is there a trap I'm
> > missing?
>
> I guess not. Stefan, any comments on whether we need to worry about
> huge mem-region-alignment? I'll post the updated patch as a new
> message in a second.
An alignment of 32 or more bits is not required in any scenario that I'm
aware of.
Stefan
signature.asc
Description: PGP signature
[PATCH v2] block/blkio: Make s->mem_region_alignment be 64 bits
With GCC 14 the code failed to compile on i686 (and was wrong for any
version of GCC):
../block/blkio.c: In function ‘blkio_file_open’:
../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
incompatible pointer type [-Wincompatible-pointer-types]
857 |>mem_region_alignment);
|^~~~
||
|size_t * {aka unsigned int *}
In file included from ../block/blkio.c:12:
/usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
*value);
| ~~^
Signed-off-by: Richard W.M. Jones
---
block/blkio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blkio.c b/block/blkio.c
index 0a0a6c0f5fd..bc2f21784c7 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -68,7 +68,7 @@ typedef struct {
CoQueue bounce_available;
/* The value of the "mem-region-alignment" property */
-size_t mem_region_alignment;
+uint64_t mem_region_alignment;
/* Can we skip adding/deleting blkio_mem_regions? */
bool needs_mem_regions;
--
2.43.0
Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit
On Tue, Jan 30, 2024 at 01:04:46PM +0100, Kevin Wolf wrote:
> Am 30.01.2024 um 11:30 hat Richard W.M. Jones geschrieben:
> > On Tue, Jan 30, 2024 at 09:51:59AM +0100, Kevin Wolf wrote:
> > > Am 29.01.2024 um 19:53 hat Richard W.M. Jones geschrieben:
> > > > With GCC 14 the code failed to compile on i686 (and was wrong for any
> > > > version of GCC):
> > > >
> > > > ../block/blkio.c: In function ‘blkio_file_open’:
> > > > ../block/blkio.c:857:28: error: passing argument 3 of
> > > > ‘blkio_get_uint64’ from incompatible pointer type
> > > > [-Wincompatible-pointer-types]
> > > > 857 |>mem_region_alignment);
> > > > |^~~~
> > > > ||
> > > > |size_t * {aka unsigned int *}
> > > > In file included from ../block/blkio.c:12:
> > > > /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> > > > unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int
> > > > *’}
> > > >49 | int blkio_get_uint64(struct blkio *b, const char *name,
> > > > uint64_t *value);
> > > > |
> > > > ~~^
> > > >
> > > > Signed-off-by: Richard W.M. Jones
> > >
> > > Why not simply make BDRVBlkioState.mem_region_alignment a uint64_t
> > > instead of keeping it size_t and doing an additional conversion with
> > > a check that requires an #if (probably to avoid a warning on 64 bit
> > > hosts because the condition is never true)?
> >
> > The smaller change (attached) does work on i686, but this worries me a
> > little (although it doesn't give any error or warning):
> >
> > if (((uintptr_t)host | size) % s->mem_region_alignment) {
> > error_setg(errp, "unaligned buf %p with size %zu", host, size);
> > return BMRR_FAIL;
> > }
>
> I don't see the problem? The calculation will now be done in 64 bits
> even on a 32 bit host, but that seems fine to me. Is there a trap I'm
> missing?
I guess not. Stefan, any comments on whether we need to worry about
huge mem-region-alignment? I'll post the updated patch as a new
message in a second.
Rich.
> Kevin
>
> > From 500f3a81652dcefa79a4864c1f3fa6747c16952e Mon Sep 17 00:00:00 2001
> > From: "Richard W.M. Jones"
> > Date: Mon, 29 Jan 2024 18:20:46 +
> > Subject: [PATCH] block/blkio: Make s->mem_region_alignment be 64 bits
> > MIME-Version: 1.0
> > Content-Type: text/plain; charset=UTF-8
> > Content-Transfer-Encoding: 8bit
> >
> > With GCC 14 the code failed to compile on i686 (and was wrong for any
> > version of GCC):
> >
> > ../block/blkio.c: In function ‘blkio_file_open’:
> > ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’
> > from incompatible pointer type [-Wincompatible-pointer-types]
> > 857 |>mem_region_alignment);
> > |^~~~
> > ||
> > |size_t * {aka unsigned int *}
> > In file included from ../block/blkio.c:12:
> > /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> > unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
> >49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> > *value);
> > |
> > ~~^
> >
> > Signed-off-by: Richard W.M. Jones
> > ---
> > block/blkio.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/blkio.c b/block/blkio.c
> > index 0a0a6c0f5fd..bc2f21784c7 100644
> > --- a/block/blkio.c
> > +++ b/block/blkio.c
> > @@ -68,7 +68,7 @@ typedef struct {
> > CoQueue bounce_available;
> >
> > /* The value of the "mem-region-alignment" property */
> > -size_t mem_region_alignment;
> > +uint64_t mem_region_alignment;
> >
> > /* Can we skip adding/deleting blkio_mem_regions? */
> > bool needs_mem_regions;
> > --
> > 2.43.0
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit
Am 30.01.2024 um 11:30 hat Richard W.M. Jones geschrieben:
> On Tue, Jan 30, 2024 at 09:51:59AM +0100, Kevin Wolf wrote:
> > Am 29.01.2024 um 19:53 hat Richard W.M. Jones geschrieben:
> > > With GCC 14 the code failed to compile on i686 (and was wrong for any
> > > version of GCC):
> > >
> > > ../block/blkio.c: In function ‘blkio_file_open’:
> > > ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’
> > > from incompatible pointer type [-Wincompatible-pointer-types]
> > > 857 |>mem_region_alignment);
> > > |^~~~
> > > ||
> > > |size_t * {aka unsigned int *}
> > > In file included from ../block/blkio.c:12:
> > > /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> > > unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
> > >49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> > > *value);
> > > |
> > > ~~^
> > >
> > > Signed-off-by: Richard W.M. Jones
> >
> > Why not simply make BDRVBlkioState.mem_region_alignment a uint64_t
> > instead of keeping it size_t and doing an additional conversion with
> > a check that requires an #if (probably to avoid a warning on 64 bit
> > hosts because the condition is never true)?
>
> The smaller change (attached) does work on i686, but this worries me a
> little (although it doesn't give any error or warning):
>
> if (((uintptr_t)host | size) % s->mem_region_alignment) {
> error_setg(errp, "unaligned buf %p with size %zu", host, size);
> return BMRR_FAIL;
> }
I don't see the problem? The calculation will now be done in 64 bits
even on a 32 bit host, but that seems fine to me. Is there a trap I'm
missing?
Kevin
> From 500f3a81652dcefa79a4864c1f3fa6747c16952e Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones"
> Date: Mon, 29 Jan 2024 18:20:46 +
> Subject: [PATCH] block/blkio: Make s->mem_region_alignment be 64 bits
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> With GCC 14 the code failed to compile on i686 (and was wrong for any
> version of GCC):
>
> ../block/blkio.c: In function ‘blkio_file_open’:
> ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
> incompatible pointer type [-Wincompatible-pointer-types]
> 857 |>mem_region_alignment);
> |^~~~
> ||
> |size_t * {aka unsigned int *}
> In file included from ../block/blkio.c:12:
> /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
>49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> *value);
> |
> ~~^
>
> Signed-off-by: Richard W.M. Jones
> ---
> block/blkio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/blkio.c b/block/blkio.c
> index 0a0a6c0f5fd..bc2f21784c7 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -68,7 +68,7 @@ typedef struct {
> CoQueue bounce_available;
>
> /* The value of the "mem-region-alignment" property */
> -size_t mem_region_alignment;
> +uint64_t mem_region_alignment;
>
> /* Can we skip adding/deleting blkio_mem_regions? */
> bool needs_mem_regions;
> --
> 2.43.0
>
Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit
On Tue, Jan 30, 2024 at 09:51:59AM +0100, Kevin Wolf wrote:
> Am 29.01.2024 um 19:53 hat Richard W.M. Jones geschrieben:
> > With GCC 14 the code failed to compile on i686 (and was wrong for any
> > version of GCC):
> >
> > ../block/blkio.c: In function ‘blkio_file_open’:
> > ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’
> > from incompatible pointer type [-Wincompatible-pointer-types]
> > 857 |>mem_region_alignment);
> > |^~~~
> > ||
> > |size_t * {aka unsigned int *}
> > In file included from ../block/blkio.c:12:
> > /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> > unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
> >49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> > *value);
> > |
> > ~~^
> >
> > Signed-off-by: Richard W.M. Jones
>
> Why not simply make BDRVBlkioState.mem_region_alignment a uint64_t
> instead of keeping it size_t and doing an additional conversion with
> a check that requires an #if (probably to avoid a warning on 64 bit
> hosts because the condition is never true)?
The smaller change (attached) does work on i686, but this worries me a
little (although it doesn't give any error or warning):
if (((uintptr_t)host | size) % s->mem_region_alignment) {
error_setg(errp, "unaligned buf %p with size %zu", host, size);
return BMRR_FAIL;
}
Rich.
> Kevin
>
> > block/blkio.c | 12 +++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/blkio.c b/block/blkio.c
> > index 0a0a6c0f5fd..52d78935147 100644
> > --- a/block/blkio.c
> > +++ b/block/blkio.c
> > @@ -794,6 +794,7 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > const char *blkio_driver = bs->drv->protocol_name;
> > BDRVBlkioState *s = bs->opaque;
> > int ret;
> > +uint64_t val;
> >
> > ret = blkio_create(blkio_driver, >blkio);
> > if (ret < 0) {
> > @@ -854,7 +855,7 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> > *options, int flags,
> >
> > ret = blkio_get_uint64(s->blkio,
> > "mem-region-alignment",
> > - >mem_region_alignment);
> > + );
> > if (ret < 0) {
> > error_setg_errno(errp, -ret,
> > "failed to get mem-region-alignment: %s",
> > @@ -862,6 +863,15 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > blkio_destroy(>blkio);
> > return ret;
> > }
> > +#if HOST_LONG_BITS == 32
> > +if (val > SIZE_MAX) {
> > +error_setg_errno(errp, ERANGE,
> > + "mem-region-alignment too large for size_t");
> > +blkio_destroy(>blkio);
> > +return -ERANGE;
> > +}
> > +#endif
> > +s->mem_region_alignment = (size_t)val;
> >
> > ret = blkio_get_bool(s->blkio,
> > "may-pin-mem-regions",
> > --
> > 2.43.0
> >
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
>From 500f3a81652dcefa79a4864c1f3fa6747c16952e Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones"
Date: Mon, 29 Jan 2024 18:20:46 +
Subject: [PATCH] block/blkio: Make s->mem_region_alignment be 64 bits
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
With GCC 14 the code failed to compile on i686 (and was wrong for any
version of GCC):
../block/blkio.c: In function ‘blkio_file_open’:
../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
incompatible pointer type [-Wincompatible-pointer-types]
857 |>mem_region_alignment);
|^~~~
||
|size_t * {aka unsigned int *}
In file included from ../block/blkio.c:12:
/usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
*value);
| ~~^
Signed-off-by: Richard W.M. Jones
---
block/blkio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blkio.c b/block/blkio.c
index 0a0a6c0f5fd..bc2f21784c7 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -68,7 +68,7 @@ typedef struct {
CoQueue bounce_available;
/*
Re: [PATCH v3 6/6] hw/xen: convert stderr prints to error/warn reports
Manos Pitsidianakis writes: > According to the QEMU Coding Style document: > >> Do not use printf(), fprintf() or monitor_printf(). Instead, use >> error_report() or error_vreport() from error-report.h. This ensures the >> error is reported in the right place (current monitor or stderr), and in >> a uniform format. >> Use error_printf() & friends to print additional information. > > This commit changes fprintfs that report warnings and errors to the > appropriate report functions. > > Reviewed-by: Philippe Mathieu-Daudé > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v3 5/6] hw/xen/xen-hvm-common.c: convert DPRINTF to tracepoints
Manos Pitsidianakis writes: > Tracing DPRINTFs to stderr might not be desired. A developer that relies > on tracepoints should be able to opt-in to each tracepoint and rely on > QEMU's log redirection, instead of stderr by default. > > This commit converts DPRINTFs in this file that are used for tracing > into tracepoints. > > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v3 4/6] hw/xen/xen-mapcache.c: convert DPRINTF to tracepoints
Manos Pitsidianakis writes: > Tracing DPRINTFs to stderr might not be desired. A developer that relies > on tracepoints should be able to opt-in to each tracepoint and rely on > QEMU's log redirection, instead of stderr by default. > > This commit converts DPRINTFs in this file that are used for tracing > into tracepoints. > > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v3 3/6] hw/arm/xen_arm.c: convert DPRINTF to trace events and error/warn reports
Manos Pitsidianakis writes: > Tracing DPRINTFs to stderr might not be desired. A developer that relies > on trace events should be able to opt-in to each trace event and rely on > QEMU's log redirection, instead of stderr by default. > > This commit converts DPRINTFs in this file that are used for tracing > into trace events. Errors or warnings are converted to error_report and > warn_report calls. > > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH v3 2/6] hw/arm/z2: convert DPRINTF to trace events and guest errors
Manos Pitsidianakis writes: > Tracing DPRINTFs to stderr might not be desired. A developer that relies > on trace events should be able to opt-in to each trace event and rely on > QEMU's log redirection, instead of stderr by default. > > This commit converts DPRINTFs in this file that are used for tracing > into trace events. DPRINTFs that report guest errors are logged with > LOG_GUEST_ERROR. > > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH] hw/hyperv: Include missing headers
On Mon, 29 Jan 2024 19:00, Philippe Mathieu-Daudé wrote:
Include missing headers in order to avoid when refactoring
unrelated headers:
hw/hyperv/hyperv.c:33:18: error: field ‘msg_page_mr’ has incomplete type
33 | MemoryRegion msg_page_mr;
| ^~~
hw/hyperv/hyperv.c: In function ‘synic_update’:
hw/hyperv/hyperv.c:64:13: error: implicit declaration of function
‘memory_region_del_subregion’ [-Werror=implicit-function-declaration]
64 | memory_region_del_subregion(get_system_memory(),
| ^~~
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_signal_event’:
hw/hyperv/hyperv.c:683:17: error: implicit declaration of function ‘ldq_phys’;
did you mean ‘ldub_phys’? [-Werror=implicit-function-declaration]
683 | param = ldq_phys(_space_memory, addr);
| ^~~~
| ldub_phys
hw/hyperv/hyperv.c:683:17: error: nested extern declaration of ‘ldq_phys’
[-Werror=nested-externs]
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_retreive_dbg_data’:
hw/hyperv/hyperv.c:792:24: error: ‘TARGET_PAGE_SIZE’ undeclared (first use in
this function); did you mean ‘TARGET_PAGE_BITS’?
792 | msg.u.recv.count = TARGET_PAGE_SIZE - sizeof(*debug_data_out);
|^~~~
|TARGET_PAGE_BITS
hw/hyperv/hyperv.c: In function ‘hyperv_syndbg_send’:
hw/hyperv/hyperv.c:885:16: error: ‘HV_SYNDBG_STATUS_INVALID’ undeclared (first
use in this function)
885 | return HV_SYNDBG_STATUS_INVALID;
|^~~~
Signed-off-by: Philippe Mathieu-Daudé
---
BTW who maintains this code?
$ ./scripts/get_maintainer.pl -f hw/hyperv/hyperv.c
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches! Use common sense.
---
hw/hyperv/hyperv.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 57b402b956..6c4a18dd0e 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -12,6 +12,7 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "exec/memory.h"
#include "sysemu/kvm.h"
#include "qemu/bitops.h"
#include "qemu/error-report.h"
@@ -21,6 +22,9 @@
#include "qemu/rcu_queue.h"
#include "hw/hyperv/hyperv.h"
#include "qom/object.h"
+#include "target/i386/kvm/hyperv-proto.h"
+#include "target/i386/cpu.h"
+#include "exec/cpu-all.h"
struct SynICState {
DeviceState parent_obj;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
[PATCH v3] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, check if src BDS has refcnt > 1. If so, the BDS
will not be removed after blockcommit, and we should make the base image
RO. Otherwise check recursively if there is a parent BDS of src BDS and
reopen the base BDS in RO in this case.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 38 --
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..52a7fee75e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -652,6 +653,32 @@ static void coroutine_fn
mirror_wait_for_all_io(MirrorBlockJob *s)
}
}
+/*
+ * Check recursively if there is a parent BDS referenced more than
+ * min_refcnt times. This argument is needed because at the first
+ * call there is a bds referenced in blockcommit.
+ */
+static bool bdrv_chain_has_significant_parent(BlockDriverState *bs)
+{
+BdrvChild *parent;
+BlockDriverState *parent_bs;
+
+QLIST_FOREACH(parent, >parents, next) {
+if (!(parent->klass->parent_is_bds)) {
+continue;
+}
+parent_bs = parent->opaque;
+if (parent_bs->drv && !parent_bs->drv->is_filter) {
+return true;
+}
+if (bdrv_chain_has_significant_parent(parent_bs)) {
+return true;
+}
+}
+
+return false;
+}
+
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
@@ -793,6 +820,11 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+if (s->base_ro && !bdrv_is_read_only(target_bs) &&
+(src->refcnt > 1 || bdrv_chain_has_significant_parent(src))) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1715,6 +1747,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1798,6 +1831,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2027,7 +2061,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2056,7 +2090,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
2.40.1
Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit
Am 29.01.2024 um 19:53 hat Richard W.M. Jones geschrieben:
> With GCC 14 the code failed to compile on i686 (and was wrong for any
> version of GCC):
>
> ../block/blkio.c: In function ‘blkio_file_open’:
> ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from
> incompatible pointer type [-Wincompatible-pointer-types]
> 857 |>mem_region_alignment);
> |^~~~
> ||
> |size_t * {aka unsigned int *}
> In file included from ../block/blkio.c:12:
> /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long
> unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
>49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t
> *value);
> |
> ~~^
>
> Signed-off-by: Richard W.M. Jones
Why not simply make BDRVBlkioState.mem_region_alignment a uint64_t
instead of keeping it size_t and doing an additional conversion with
a check that requires an #if (probably to avoid a warning on 64 bit
hosts because the condition is never true)?
Kevin
> block/blkio.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/block/blkio.c b/block/blkio.c
> index 0a0a6c0f5fd..52d78935147 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -794,6 +794,7 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> *options, int flags,
> const char *blkio_driver = bs->drv->protocol_name;
> BDRVBlkioState *s = bs->opaque;
> int ret;
> +uint64_t val;
>
> ret = blkio_create(blkio_driver, >blkio);
> if (ret < 0) {
> @@ -854,7 +855,7 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> ret = blkio_get_uint64(s->blkio,
> "mem-region-alignment",
> - >mem_region_alignment);
> + );
> if (ret < 0) {
> error_setg_errno(errp, -ret,
> "failed to get mem-region-alignment: %s",
> @@ -862,6 +863,15 @@ static int blkio_file_open(BlockDriverState *bs, QDict
> *options, int flags,
> blkio_destroy(>blkio);
> return ret;
> }
> +#if HOST_LONG_BITS == 32
> +if (val > SIZE_MAX) {
> +error_setg_errno(errp, ERANGE,
> + "mem-region-alignment too large for size_t");
> +blkio_destroy(>blkio);
> +return -ERANGE;
> +}
> +#endif
> +s->mem_region_alignment = (size_t)val;
>
> ret = blkio_get_bool(s->blkio,
> "may-pin-mem-regions",
> --
> 2.43.0
>
Re: [PATCH] pflash: fix sectors vs bytes confusion in blk_pread_nonzeroes()
On 30/1/24 01:27, Stefan Hajnoczi wrote:
The following expression is incorrect because blk_pread_nonzeroes()
deals in units of bytes, not sectors:
bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS)
^^^
BDRV_REQUEST_MAX_BYTES is the appropriate constant.
Fixes: a4b15a8b9ef2 ("pflash: Only read non-zero parts of backend image")
Cc: Xiang Zheng
Signed-off-by: Stefan Hajnoczi
---
hw/block/block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ff503002aa 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -30,7 +30,7 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
BlockDriverState *bs = blk_bs(blk);
for (;;) {
-bytes = MIN(size - offset, BDRV_REQUEST_MAX_SECTORS);
+bytes = MIN(size - offset, BDRV_REQUEST_MAX_BYTES);
if (bytes <= 0) {
return 0;
}
Reviewed-by: Philippe Mathieu-Daudé
