Re: [PATCH v5 13/13] tests/qtest/vhost-user-test: add a test case for memory-backend-shm
On 23.05.24 16:55, Stefano Garzarella wrote: `memory-backend-shm` can be used with vhost-user devices, so let's add a new test case for it. Acked-by: Thomas Huth Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 12/13] tests/qtest/vhost-user-blk-test: use memory-backend-shm
On 23.05.24 16:55, Stefano Garzarella wrote: `memory-backend-memfd` is available only on Linux while the new `memory-backend-shm` can be used on any POSIX-compliant operating system. Let's use it so we can run the test in multiple environments. Acked-by: Thomas Huth Acked-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Signed-off-by: Stefano Garzarella --- tests/qtest/vhost-user-blk-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c index 117b9acd10..e945f6abf2 100644 --- a/tests/qtest/vhost-user-blk-test.c +++ b/tests/qtest/vhost-user-blk-test.c @@ -906,7 +906,7 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances, vhost_user_blk_bin); g_string_append_printf(cmd_line, -" -object memory-backend-memfd,id=mem,size=256M,share=on " +" -object memory-backend-shm,id=mem,size=256M,share=on " Can we simplifya nd drop the share=on? Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 07/13] vhost-user: enable frontends on any POSIX system
On 23.05.24 16:55, Stefano Garzarella wrote: The vhost-user protocol is not really Linux-specific so let's enable vhost-user frontends for any POSIX system. In vhost_net.c we use VHOST_FILE_UNBIND which is defined in a Linux specific header, let's define it for other systems as well. Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 06/13] contrib/vhost-user-*: use QEMU bswap helper functions
On 23.05.24 16:55, Stefano Garzarella wrote: Let's replace the calls to le*toh() and htole*() with qemu/bswap.h helpers to make the code more portable. Suggested-by: Philippe Mathieu-Daudé Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 04/13] vhost-user-server: do not set memory fd non-blocking
On 23.05.24 16:55, Stefano Garzarella wrote: In vhost-user-server we set all fd received from the other peer in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.) it's not really needed, because we don't use these fd with blocking operations, but only to map memory. In addition, in some systems this operation can fail (e.g. in macOS setting an fd returned by shm_open() non-blocking fails with errno = ENOTTY). So, let's avoid setting fd non-blocking for those messages that we know carry memory fd (e.g. VHOST_USER_ADD_MEM_REG, VHOST_USER_SET_MEM_TABLE). Reviewed-by: Daniel P. Berrangé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 05/13] contrib/vhost-user-blk: fix bind() using the right size of the address
On 23.05.24 16:55, Stefano Garzarella wrote: On macOS passing `-s /tmp/vhost.socket` parameter to the vhost-user-blk application, the bind was done on `/tmp/vhost.socke` pathname, missing the last character. This sounds like one of the portability problems described in the unix(7) manpage: Pathname sockets When binding a socket to a pathname, a few rules should be observed for maximum portability and ease of coding: • The pathname in sun_path should be null-terminated. • The length of the pathname, including the terminating null byte, should not exceed the size of sun_path. • The addrlen argument that describes the enclosing sockaddr_un structure should have a value of at least: offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path)+1 or, more simply, addrlen can be specified as sizeof(struct sockaddr_un). So let's follow the last advice and simplify the code as well. Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 03/13] libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported
On 23.05.24 16:55, Stefano Garzarella wrote: libvhost-user will panic when receiving VHOST_USER_GET_INFLIGHT_FD message if MFD_ALLOW_SEALING is not defined, since it's not able to create a memfd. VHOST_USER_GET_INFLIGHT_FD is used only if VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD is negotiated. So, let's mask that feature if the backend is not able to properly handle these messages. Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v5 02/13] libvhost-user: fail vu_message_write() if sendmsg() is failing
On 23.05.24 16:55, Stefano Garzarella wrote: In vu_message_write() we use sendmsg() to send the message header, then a write() to send the payload. If sendmsg() fails we should avoid sending the payload, since we were unable to send the header. Discovered before fixing the issue with the previous patch, where sendmsg() failed on macOS due to wrong parameters, but the frontend still sent the payload which the backend incorrectly interpreted as a wrong header. Reviewed-by: Daniel P. Berrangé Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Acked-by: Stefan Hajnoczi Signed-off-by: Stefano Garzarella --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH for-9.1 v3 09/11] hostmem: add a new memory backend based on POSIX shm_open()
On 08.04.24 09:58, Stefano Garzarella wrote:
On Thu, Apr 04, 2024 at 04:09:34PM +0200, David Hildenbrand wrote:
On 04.04.24 14:23, Stefano Garzarella wrote:
shm_open() creates and opens a new POSIX shared memory object.
A POSIX shared memory object allows creating memory backend with an
associated file descriptor that can be shared with external processes
(e.g. vhost-user).
The new `memory-backend-shm` can be used as an alternative when
`memory-backend-memfd` is not available (Linux only), since shm_open()
should be provided by any POSIX-compliant operating system.
This backend mimics memfd, allocating memory that is practically
anonymous. In theory shm_open() requires a name, but this is allocated
for a short time interval and shm_unlink() is called right after
shm_open(). After that, only fd is shared with external processes
(e.g., vhost-user) as if it were associated with anonymous memory.
In the future we may also allow the user to specify the name to be
passed to shm_open(), but for now we keep the backend simple, mimicking
anonymous memory such as memfd.
Signed-off-by: Stefano Garzarella
---
v3
- enriched commit message and documentation to highlight that we
want to mimic memfd (David)
---
docs/system/devices/vhost-user.rst | 5 +-
qapi/qom.json | 17 +
backends/hostmem-shm.c | 118 +
backends/meson.build | 1 +
qemu-options.hx| 11 +++
5 files changed, 150 insertions(+), 2 deletions(-)
create mode 100644 backends/hostmem-shm.c
diff --git a/docs/system/devices/vhost-user.rst
b/docs/system/devices/vhost-user.rst
index 9b2da106ce..35259d8ec7 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -98,8 +98,9 @@ Shared memory object
In order for the daemon to access the VirtIO queues to process the
requests it needs access to the guest's address space. This is
-achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
-objects. A reference to a file-descriptor which can access this object
+achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
+``memory-backend-shm`` objects.
+A reference to a file-descriptor which can access this object
will be passed via the socket as part of the protocol negotiation.
Currently the shared memory object needs to match the size of the main
diff --git a/qapi/qom.json b/qapi/qom.json
index 85e6b4f84a..5252ec69e3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -721,6 +721,19 @@
'*hugetlbsize': 'size',
'*seal': 'bool' } }
+##
+# @MemoryBackendShmProperties:
+#
+# Properties for memory-backend-shm objects.
+#
+# The @share boolean option is true by default with shm.
+#
+# Since: 9.1
+##
+{ 'struct': 'MemoryBackendShmProperties',
+ 'base': 'MemoryBackendProperties',
+ 'data': { } }
+
Acked-by: David Hildenbrand
One comment: we should maybe just forbid setting share=off. it doesn't
make any sense and it can even result in an unexpected double memory
consumption. We missed doing that for memfd, unfortunately.
Good point!
IIUC the `share` property is defined by the parent `hostmem`, so I
should find a way to override the property here and disable the setter,
or add an option to `hostmem` to make the property non-writable.
Right, or simply fail later when you would find "share=off" in
shm_backend_memory_alloc().
When ever supporting named shmem_open(), it could make sense for VM
snapshotting. Right now it doesn't really make any sense.
--
Cheers,
David / dhildenb
Re: [PATCH for-9.1 v3 09/11] hostmem: add a new memory backend based on POSIX shm_open()
On 04.04.24 14:23, Stefano Garzarella wrote:
shm_open() creates and opens a new POSIX shared memory object.
A POSIX shared memory object allows creating memory backend with an
associated file descriptor that can be shared with external processes
(e.g. vhost-user).
The new `memory-backend-shm` can be used as an alternative when
`memory-backend-memfd` is not available (Linux only), since shm_open()
should be provided by any POSIX-compliant operating system.
This backend mimics memfd, allocating memory that is practically
anonymous. In theory shm_open() requires a name, but this is allocated
for a short time interval and shm_unlink() is called right after
shm_open(). After that, only fd is shared with external processes
(e.g., vhost-user) as if it were associated with anonymous memory.
In the future we may also allow the user to specify the name to be
passed to shm_open(), but for now we keep the backend simple, mimicking
anonymous memory such as memfd.
Signed-off-by: Stefano Garzarella
---
v3
- enriched commit message and documentation to highlight that we
want to mimic memfd (David)
---
docs/system/devices/vhost-user.rst | 5 +-
qapi/qom.json | 17 +
backends/hostmem-shm.c | 118 +
backends/meson.build | 1 +
qemu-options.hx| 11 +++
5 files changed, 150 insertions(+), 2 deletions(-)
create mode 100644 backends/hostmem-shm.c
diff --git a/docs/system/devices/vhost-user.rst
b/docs/system/devices/vhost-user.rst
index 9b2da106ce..35259d8ec7 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -98,8 +98,9 @@ Shared memory object
In order for the daemon to access the VirtIO queues to process the
requests it needs access to the guest's address space. This is
-achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
-objects. A reference to a file-descriptor which can access this object
+achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
+``memory-backend-shm`` objects.
+A reference to a file-descriptor which can access this object
will be passed via the socket as part of the protocol negotiation.
Currently the shared memory object needs to match the size of the main
diff --git a/qapi/qom.json b/qapi/qom.json
index 85e6b4f84a..5252ec69e3 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -721,6 +721,19 @@
'*hugetlbsize': 'size',
'*seal': 'bool' } }
+##
+# @MemoryBackendShmProperties:
+#
+# Properties for memory-backend-shm objects.
+#
+# The @share boolean option is true by default with shm.
+#
+# Since: 9.1
+##
+{ 'struct': 'MemoryBackendShmProperties',
+ 'base': 'MemoryBackendProperties',
+ 'data': { } }
+
Acked-by: David Hildenbrand
One comment: we should maybe just forbid setting share=off. it doesn't
make any sense and it can even result in an unexpected double memory
consumption. We missed doing that for memfd, unfortunately.
--
Cheers,
David / dhildenb
Re: [PATCH for-9.1 v2 09/11] hostmem: add a new memory backend based on POSIX shm_open()
So I thought that for now we only support the "anonymous" mode, and if in the future we have a use case where the user wants to specify the name, we can add it later. Okay, so for now you really only want an anonymous fd that behaves like a memfd, got it. Likely we should somehow fail if the "POSIX shared memory object" already exists. Hmm. `O_CREAT | O_EXCL` should provide just this behavior. From shm_open(3) manpage: O_EXCL If O_CREAT was also specified, and a shared memory object with the given name already exists, return an error. The check for the existence of the object, and its creation if it does not exist, are performed atomically. Cool! That said, if you think it's already useful from the beginning, I can add the name as an optional parameter. I'm also not quite sure if "host_memory_backend_get_name()" should be used for the purpose here. What problem do you see? As an alternative I thought of a static counter. If it's really just an "internal UUID", as we'll remove the file using shm_unlink(), then the name in fact shouldn't matter and this would be fine. Correct? Right. It's a name that will only "appear" in the system for a very short time since we call shm_unlink() right after shm_open(). So I just need the unique name to prevent several QEMUs launched at the same time from colliding with the name. Okay, essentially emulating tmpfile(), but for weird shmem_open() :) So I assume if we ever want to have non-anonymous fds here, we'd pass in a new property "name", and change the logic how we open/unlink. Makes sense. Exactly! I can clarify this in the commit description as well. Thanks for the helpful comments! If there is anything I need to change for v3, please tell me ;-) Besides some patch description extension, makes sense to me :) -- Cheers, David / dhildenb
Re: [PATCH for-9.1 v2 09/11] hostmem: add a new memory backend based on POSIX shm_open()
On 27.03.24 11:23, Stefano Garzarella wrote: On Tue, Mar 26, 2024 at 03:45:52PM +0100, David Hildenbrand wrote: +mode = 0; +oflag = O_RDWR | O_CREAT | O_EXCL; +backend_name = host_memory_backend_get_name(backend); + +/* + * Some operating systems allow creating anonymous POSIX shared memory + * objects (e.g. FreeBSD provides the SHM_ANON constant), but this is not + * defined by POSIX, so let's create a unique name. + * + * From Linux's shm_open(3) man-page: + * For portable use, a shared memory object should be identified + * by a name of the form /somename;" + */ +g_string_printf(shm_name, "/qemu-" FMT_pid "-shm-%s", getpid(), +backend_name); Hm, shouldn't we just let the user specify a name, and if no name was specified, generate one ourselves? I thought about it and initially did it that way, but then some problems came up so I tried to keep it as simple as possible for the user and for our use case (having an fd associated with memory and sharing it with other processes). The problems I had were: - what mode_t to use if the object does not exist and needs to be created? > - exclude O_EXCL if the user passes the name since they may have already created it? I'd handle both like we handle files. But I understand now that you really just want to "simulate" memfd. - call shm_unlink() only at object deallocation? Right, we don't really do that for ordinary files, they keep existing. We only have the "discard-data" property to free up memory. For memfd, it just happens "automatically". They cannot be looked up by name, and once the last user closes the fd, it gets destroyed. So I thought that for now we only support the "anonymous" mode, and if in the future we have a use case where the user wants to specify the name, we can add it later. Okay, so for now you really only want an anonymous fd that behaves like a memfd, got it. Likely we should somehow fail if the "POSIX shared memory object" already exists. Hmm. That said, if you think it's already useful from the beginning, I can add the name as an optional parameter. I'm also not quite sure if "host_memory_backend_get_name()" should be used for the purpose here. What problem do you see? As an alternative I thought of a static counter. If it's really just an "internal UUID", as we'll remove the file using shm_unlink(), then the name in fact shouldn't matter and this would be fine. Correct? So I assume if we ever want to have non-anonymous fds here, we'd pass in a new property "name", and change the logic how we open/unlink. Makes sense. -- Cheers, David / dhildenb
Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing
I was wondering if we could see some partial sendmsg()/write succeeding. Meaning, we transferred some bytes but not all, and we'd actually need to loop ... Yep, true, but I would fix it in another patch/series if you agree. Absolutely. -- Cheers, David / dhildenb
Re: [PATCH for-9.1 v2 09/11] hostmem: add a new memory backend based on POSIX shm_open()
+mode = 0; +oflag = O_RDWR | O_CREAT | O_EXCL; +backend_name = host_memory_backend_get_name(backend); + +/* + * Some operating systems allow creating anonymous POSIX shared memory + * objects (e.g. FreeBSD provides the SHM_ANON constant), but this is not + * defined by POSIX, so let's create a unique name. + * + * From Linux's shm_open(3) man-page: + * For portable use, a shared memory object should be identified + * by a name of the form /somename;" + */ +g_string_printf(shm_name, "/qemu-" FMT_pid "-shm-%s", getpid(), +backend_name); Hm, shouldn't we just let the user specify a name, and if no name was specified, generate one ourselves? I'm also not quite sure if "host_memory_backend_get_name()" should be used for the purpose here. -- Cheers, David / dhildenb
Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing
On 26.03.24 15:34, Eric Blake wrote:
On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
In vu_message_write() we use sendmsg() to send the message header,
then a write() to send the payload.
If sendmsg() fails we should avoid sending the payload, since we
were unable to send the header.
Discovered before fixing the issue with the previous patch, where
sendmsg() failed on macOS due to wrong parameters, but the frontend
still sent the payload which the backend incorrectly interpreted
as a wrong header.
Signed-off-by: Stefano Garzarella
---
subprojects/libvhost-user/libvhost-user.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index 22bea0c775..a11afd1960 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg
*vmsg)
rc = sendmsg(conn_fd, &msg, 0);
} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+if (rc <= 0) {
Is rejecting a 0 return value correct? Technically, a 0 return means
a successful write of 0 bytes - but then again, it is unwise to
attempt to send an fd or other auxilliary ddata without at least one
regular byte, so we should not be attempting a write of 0 bytes. So I
guess this one is okay, although I might have used < instead of <=.
I was wondering if we could see some partial sendmsg()/write succeeding.
Meaning, we transferred some bytes but not all, and we'd actually need
to loop ...
--
Cheers,
David / dhildenb
Re: [PATCH for-9.1 v2 01/11] libvhost-user: set msg.msg_control to NULL when it is empty
On 26.03.24 14:39, Stefano Garzarella wrote:
On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
the `struct msghdr` has the field `msg_controllen` set to 0, but
`msg_control` is not NULL.
Signed-off-by: Stefano Garzarella
---
subprojects/libvhost-user/libvhost-user.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/subprojects/libvhost-user/libvhost-user.c
b/subprojects/libvhost-user/libvhost-user.c
index a879149fef..22bea0c775 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg
*vmsg)
memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
} else {
msg.msg_controllen = 0;
+msg.msg_control = NULL;
}
do {
Reviewed-by: David Hildenbrand
--
Cheers,
David / dhildenb
Re: [PATCH 9/9] hostmem-file: support POSIX shm_open()
On 28.02.24 12:47, Stefano Garzarella wrote: Add a new `shm` bool option for `-object memory-backend-file`. When this option is set to true, the POSIX shm_open(3) is used instead of open(2). So a file will not be created in the filesystem, but a "POSIX shared memory object" will be instantiated. In Linux this turns into a file in /dev/shm, but in other OSes this may not happen (for example in macOS or FreeBSD nothing is shown in any filesystem). This new feature is useful when we need to share guest memory with another process (e.g. vhost-user backend), but we don't have memfd_create() or any special filesystems (e.g. /dev/shm) available as in macOS. Signed-off-by: Stefano Garzarella --- I am not sure this is the best way to support shm_open() in QEMU. Other solutions I had in mind were: - create a new memory-backend-shm - extend memory-backend-memfd to use shm_open() on systems where memfd is not available (problem: shm_open wants a name to assign to the object, but we can do a workaround by using a random name and do the unlink right away) Any preference/suggestion? Both sound like reasonable options, and IMHO better than hostmem-file with things that are not necessarily "real" files. Regarding memory-backend-memfd, we similarly have to pass a name to memfd_create(), although for different purpose: "The name supplied in name is used as a filename and will be displayed as the target of the corresponding symbolic link in the directory /proc/self/fd/". So we simply pass TYPE_MEMORY_BACKEND_MEMFD. Likely, memory-backend-shm that directly maps to shm_open() and only provides properties reasonable for shm_open() is the cleanest approach. So that would currently be my preference :) -- Cheers, David / dhildenb
Re: [PATCH-for-9.0 v2 04/19] system/physmem: Do not include 'hw/xen/xen.h' but 'sysemu/xen.h'
On 14.11.23 15:38, Philippe Mathieu-Daudé wrote: physmem.c doesn't use any declaration from "hw/xen/xen.h", it only requires "sysemu/xen.h" and "system/xen-mapcache.h". Suggested-by: David Woodhouse Signed-off-by: Philippe Mathieu-Daudé --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [RFC PATCH 26/78] target/s390x: add fallthrough pseudo-keyword
On 13.10.23 09:47, Emmanouil Pitsidianakis wrote: In preparation of raising -Wimplicit-fallthrough to 5, replace all fall-through comments with the fallthrough attribute pseudo-keyword. Signed-off-by: Emmanouil Pitsidianakis --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [RFC PATCH 07/78] hw/virtio/virtio-balloon.c: add fallthrough pseudo-keyword
On 13.10.23 09:47, Emmanouil Pitsidianakis wrote: In preparation of raising -Wimplicit-fallthrough to 5, replace all fall-through comments with the fallthrough attribute pseudo-keyword. Signed-off-by: Emmanouil Pitsidianakis --- Reviewed-by: David Hildenbrand -- Cheers, David / dhildenb
Re: [PATCH v2] qdict: Preserve order for iterating qdict elements
On 04.09.23 18:38, Daniel P. Berrangé wrote: On Sat, Sep 02, 2023 at 05:40:40PM +0800, William Tsai wrote: Changing the structure of qdict so that it can preserve order when iterating qdict. This will fix array_properties as it relies on `len-` prefixed argument to be set first. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090 Signed-off-by: William Tsai This is a variation of what Markus illustrated a year ago https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html I wasn't a particular fan of that approach at the time. I've made an alternative proposal here which avoids the broader impact of this QDict change: https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00652.html Just a note regarding s390x CPU models (and how they are also affected, but it probably doesn't matter because it never 100% worked that way on all interfaces). I recall that I thought the order of parameters worked for s390x CPU models, where we support feature groups (due to the huge number of CPU features). But this might only have worked for the "-cpu" parameter, which parses them in-order and sets properties in-order. So when mixing a feature group with contained features, the end result might not be deterministic in other cases thatn "-pu" (CPU hotplug via "-device", but also qapi CPU model operations). For example, one might want to enable all but some features of a group, or disable all but some features of a group. Note that I doubt that there are really users of that, but it's possible on the QEMU cmdline. I guess it never really worked with qapi CPU model operations in general (baseline, comparison, expansion, ...) because these operations all rely on qdict as well (see cpu_model_from_info()). So they should never return something non-deterministic. To highlight one case that could now fail: $ ./qemu-system-s390x -smp 1,maxcpus=2 -cpu qemu,msa2=off,kimd-sha-512=on -nographic -nodefaults -monitor stdio -S -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on QEMU 8.1.50 monitor - type 'help' for more information qemu-system-s390x: warning: 'msa5-base' requires 'klmd-sha-512'. qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: warning: 'msa5-base' requires 'kimd-sha-512'. qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: warning: 'msa5-base' requires 'klmd-sha-512'. qemu-system-s390x: -device qemu-s390x-cpu,core-id=1,msa2=off,kimd-sha-512=on: Mixed CPU models are not supported on s390x. Note that using "-device qemu-s390x-cpu,core-id=1" instead works as expected, as cpu_common_parse_features() registers all settings as global properties for that CPU type. Further, feature groups might not be used by actual users that way. CPU model expansion (s390_feat_bitmap_to_ascii()) only reports a feature group when all features are contained, so most of libvirt should be fine, unless someone decides to manually specify a non-deterministic CPU model as above. So maybe one can conclude that specifying "msa2=off,kimd-sha-512=on" is similar to "kimd-sha-512=off,kimd-sha-512=on", and which setting "wins" is not deterministic. -- Cheers, David / dhildenb
Re: [PATCH 06/11] hw/virtio/virtio-mem: Use qemu_ram_get_fd() helper
On 23.05.23 18:35, Philippe Mathieu-Daudé wrote: Avoid accessing RAMBlock internals, use the provided qemu_ram_get_fd() getter to get the file descriptor. Signed-off-by: Philippe Mathieu-Daudé --- hw/virtio/virtio-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index 538b695c29..74e63bd47a 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -135,7 +135,7 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb) * anonymous RAM. In any other case, reading unplugged *can* populate a * fresh page, consuming actual memory. */ -return !qemu_ram_is_shared(rb) && rb->fd < 0 && +return !qemu_ram_is_shared(rb) && qemu_ram_get_fd(rb) < 0 && qemu_ram_pagesize(rb) == qemu_real_host_page_size(); } #endif /* VIRTIO_MEM_HAS_LEGACY_GUESTS */ Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH 03/16] qapi: Fix misspelled references
On 04.04.23 13:58, Markus Armbruster wrote: query-cpu-definitions returns a list of CpuDefinitionInfo, but documentation claims CpuDefInfo, which doesn't exist. query-migrate-capabilities returns a list of MigrationCapabilityStatus, but documentation claims MigrationCapabilitiesStatus, which doesn't exist. balloon and query-balloon can fail with KVMMissingCap, but documentation claims KvmMissingCap, which doesn't exist. Fix the documentation. Fixes: e4e31c6324af (qapi: add query-cpu-definitions command (v2)) Fixes: bbf6da32b5bd (Add migration capabilities) Fixes: d72f326431e2 (qapi: Convert balloon) Fixes: 96637bcdf9e0 (qapi: Convert query-balloon) Signed-off-by: Markus Armbruster --- qapi/machine-target.json | 2 +- qapi/machine.json| 4 ++-- qapi/migration.json | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/qapi/machine-target.json b/qapi/machine-target.json index 2e267fa458..b94fbdb65e 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -331,7 +331,7 @@ # # Return a list of supported virtual CPU definitions # -# Returns: a list of CpuDefInfo +# Returns: a list of CpuDefinitionInfo # # Since: 1.2 ## diff --git a/qapi/machine.json b/qapi/machine.json index 604b686e59..8c3c58c763 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1029,7 +1029,7 @@ # # Returns: - Nothing on success # - If the balloon driver is enabled but not functional because the KVM -#kernel module cannot support it, KvmMissingCap +#kernel module cannot support it, KVMMissingCap # - If no balloon device is present, DeviceNotActive # # Notes: This command just issues a request to the guest. When it returns, @@ -1067,7 +1067,7 @@ # # Returns: - @BalloonInfo on success # - If the balloon driver is enabled but not functional because the KVM -#kernel module cannot support it, KvmMissingCap +#kernel module cannot support it, KVMMissingCap # - If no balloon device is present, DeviceNotActive # # Since: 0.14 diff --git a/qapi/migration.json b/qapi/migration.json index c84fa10e86..87c174dca2 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -531,7 +531,7 @@ # # Returns information about the current migration capabilities status # -# Returns: @MigrationCapabilitiesStatus +# Returns: @MigrationCapabilityStatus # # Since: 1.2 # Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH v6 09/13] block: add BlockRAMRegistrar
On 13.10.22 20:05, Stefan Hajnoczi wrote:
On Fri, Oct 07, 2022 at 12:51:21PM +0200, Stefano Garzarella wrote:
On Thu, Oct 06, 2022 at 05:35:03PM -0400, Stefan Hajnoczi wrote:
Emulated devices and other BlockBackend users wishing to take advantage
of blk_register_buf() all have the same repetitive job: register
RAMBlocks with the BlockBackend using RAMBlockNotifier.
Add a BlockRAMRegistrar API to do this. A later commit will use this
from hw/block/virtio-blk.c.
Signed-off-by: Stefan Hajnoczi
---
MAINTAINERS | 1 +
include/sysemu/block-ram-registrar.h | 37 ++
block/block-ram-registrar.c | 58
block/meson.build| 1 +
4 files changed, 97 insertions(+)
create mode 100644 include/sysemu/block-ram-registrar.h
create mode 100644 block/block-ram-registrar.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0dcae6168a..91aed2cdc7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2498,6 +2498,7 @@ F: block*
F: block/
F: hw/block/
F: include/block/
+F: include/sysemu/block-*.h
F: qemu-img*
F: docs/tools/qemu-img.rst
F: qemu-io*
diff --git a/include/sysemu/block-ram-registrar.h
b/include/sysemu/block-ram-registrar.h
new file mode 100644
index 00..d8b2f7942b
--- /dev/null
+++ b/include/sysemu/block-ram-registrar.h
@@ -0,0 +1,37 @@
+/*
+ * BlockBackend RAM Registrar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef BLOCK_RAM_REGISTRAR_H
+#define BLOCK_RAM_REGISTRAR_H
+
+#include "exec/ramlist.h"
+
+/**
+ * struct BlockRAMRegistrar:
+ *
+ * Keeps RAMBlock memory registered with a BlockBackend using
+ * blk_register_buf() including hotplugged memory.
+ *
+ * Emulated devices or other BlockBackend users initialize a BlockRAMRegistrar
+ * with blk_ram_registrar_init() before submitting I/O requests with the
+ * BDRV_REQ_REGISTERED_BUF flag set.
+ */
+typedef struct {
+BlockBackend *blk;
+RAMBlockNotifier notifier;
+bool ok;
+} BlockRAMRegistrar;
+
+void blk_ram_registrar_init(BlockRAMRegistrar *r, BlockBackend *blk);
+void blk_ram_registrar_destroy(BlockRAMRegistrar *r);
+
+/* Have all RAMBlocks been registered successfully? */
+static inline bool blk_ram_registrar_ok(BlockRAMRegistrar *r)
+{
+return r->ok;
+}
+
+#endif /* BLOCK_RAM_REGISTRAR_H */
diff --git a/block/block-ram-registrar.c b/block/block-ram-registrar.c
new file mode 100644
index 00..25dbafa789
--- /dev/null
+++ b/block/block-ram-registrar.c
@@ -0,0 +1,58 @@
+/*
+ * BlockBackend RAM Registrar
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/block-ram-registrar.h"
+#include "qapi/error.h"
+
+static void ram_block_added(RAMBlockNotifier *n, void *host, size_t size,
+size_t max_size)
+{
+BlockRAMRegistrar *r = container_of(n, BlockRAMRegistrar, notifier);
+Error *err = NULL;
+
+if (!r->ok) {
+return; /* don't try again if we've already failed */
+}
The segfault I was seeing is gone, though, and I'm getting a doubt.
Here we basically just report the error and prevent new regions from being
registered. The VM still starts though and the blkio driver works as if
nothing happened.
For drivers that require all regions to be registered, this can cause
problems, so should we stop the VM in case of failure or put the blkio
driver in a state such that IOs are not submitted?
Or maybe it's okay and then the device will somehow report the error when it
can't find the mapped region?
The BlockDriver supports the fast path for registered bufs but also has
a slow path using bounce buffers. When registered bufs fail it uses
bounce buffers. That's why the guest still boots.
So, that means if we hotplug memory and would be out of slots, it would
still continue working? Would there be some kind of a warning to let the
user know that this might come with a performance regression?
--
Thanks,
David / dhildenb
Re: [PATCH v6 08/13] numa: use QLIST_FOREACH_SAFE() for RAM block notifiers
On 06.10.22 23:35, Stefan Hajnoczi wrote:
Make list traversal work when a callback removes a notifier
mid-traversal. This is a cleanup to prevent bugs in the future.
Signed-off-by: Stefan Hajnoczi
---
hw/core/numa.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/core/numa.c b/hw/core/numa.c
index 31e6fe1caa..ea24a5fa8c 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -857,8 +857,9 @@ void ram_block_notifier_remove(RAMBlockNotifier *n)
void ram_block_notify_add(void *host, size_t size, size_t max_size)
{
RAMBlockNotifier *notifier;
+RAMBlockNotifier *next;
-QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
+QLIST_FOREACH_SAFE(notifier, &ram_list.ramblock_notifiers, next, next) {
if (notifier->ram_block_added) {
notifier->ram_block_added(notifier, host, size, max_size);
}
@@ -868,8 +869,9 @@ void ram_block_notify_add(void *host, size_t size, size_t
max_size)
void ram_block_notify_remove(void *host, size_t size, size_t max_size)
{
RAMBlockNotifier *notifier;
+RAMBlockNotifier *next;
-QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
+QLIST_FOREACH_SAFE(notifier, &ram_list.ramblock_notifiers, next, next) {
if (notifier->ram_block_removed) {
notifier->ram_block_removed(notifier, host, size, max_size);
}
@@ -879,8 +881,9 @@ void ram_block_notify_remove(void *host, size_t size,
size_t max_size)
void ram_block_notify_resize(void *host, size_t old_size, size_t new_size)
{
RAMBlockNotifier *notifier;
+RAMBlockNotifier *next;
-QLIST_FOREACH(notifier, &ram_list.ramblock_notifiers, next) {
+QLIST_FOREACH_SAFE(notifier, &ram_list.ramblock_notifiers, next, next) {
if (notifier->ram_block_resized) {
notifier->ram_block_resized(notifier, host, old_size, new_size);
}
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [RFC v4 11/11] virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint
On 30.08.22 22:16, Stefan Hajnoczi wrote:
> On Thu, Aug 25, 2022 at 09:43:16AM +0200, David Hildenbrand wrote:
>> On 23.08.22 21:22, Stefan Hajnoczi wrote:
>>> On Tue, Aug 23, 2022 at 10:01:59AM +0200, David Hildenbrand wrote:
>>>> On 23.08.22 00:24, Stefan Hajnoczi wrote:
>>>>> Register guest RAM using BlockRAMRegistrar and set the
>>>>> BDRV_REQ_REGISTERED_BUF flag so block drivers can optimize memory
>>>>> accesses in I/O requests.
>>>>>
>>>>> This is for vdpa-blk, vhost-user-blk, and other I/O interfaces that rely
>>>>> on DMA mapping/unmapping.
>>>>
>>>> Can you explain why we're monitoring RAMRegistrar to hook into "guest
>>>> RAM" and not go the usual path of the MemoryListener?
>>>
>>> The requirements are similar to VFIO, which uses RAMBlockNotifier. We
>>
>> Only VFIO NVME uses RAMBlockNotifier. Ordinary VFIO uses the MemoryListener.
>>
>> Maybe the difference is that ordinary VFIO has to replicate the actual
>> guest physical memory layout, and VFIO NVME is only interested in
>> possible guest RAM inside guest physical memory.
>>
>>> need to learn about all guest RAM because that's where I/O buffers are
>>> located.
>>>
>>> Do you think RAMBlockNotifier should be avoided?
>>
>> I assume it depends on the use case. For saying "this might be used for
>> I/O" it might be good enough I guess.
>>
>>>
>>>> What will BDRV_REQ_REGISTERED_BUF actually do? Pin all guest memory in
>>>> the worst case such as io_uring fixed buffers would do ( I hope not ).
>>>
>>> BLK_REQ_REGISTERED_BUF is a hint that no bounce buffer is necessary
>>> because the I/O buffer is located in memory that was previously
>>> registered with bdrv_registered_buf().
>>>
>>> The RAMBlockNotifier calls bdrv_register_buf() to let the libblkio
>>> driver know about RAM. Some libblkio drivers ignore this hint, io_uring
>>> may use the fixed buffers feature, vhost-user sends the shared memory
>>> file descriptors to the vhost device server, and VFIO/vhost may pin
>>> pages.
>>>
>>> So the blkio block driver doesn't add anything new, it's the union of
>>> VFIO/vhost/vhost-user/etc memory requirements.
>>
>> The issue is if that backend pins memory inside any of these regions.
>> Then, you're instantly incompatible to anything the relies on sparse
>> RAMBlocks, such as memory ballooning or virtio-mem, and have to properly
>> fence it.
>>
>> In that case, you'd have to successfully trigger
>> ram_block_discard_disable(true) first, before pinning. Who would do that
>> now conditionally, just like e.g., VFIO does?
>>
>> io_uring fixed buffers would be one such example that pins memory and is
>> problematic. vfio (unless on s390x) is another example, as you point out.
>
> Okay, I think libblkio needs to expose a bool property called
> "mem-regions-pinned" so QEMU whether or not the registered buffers will
> be pinned.
>
> Then the QEMU BlockDriver can do:
>
> if (mem_regions_pinned) {
> if (ram_block_discard_disable(true) < 0) {
> ...fail to open block device...
> }
> }
>
> Does that sound right?
Yes, I think so.
>
> Is "pinned" the best word to describe this or is there a more general
> characteristic we are looking for?
pinning should be the right term. We want to express that all user page
tables will immediately get populated and that a kernel subsystem will
take longterm references on mapped page that will go out of sync as soon
as we discard memory e.g., using madvise(MADV_DONTEED).
We just should not confuse it with memlock / locking into memory, which
are yet different semantics (e.g., don't swap it out).
>
>>
>> This has to be treated with care. Another thing to consider is that
>> different backends might only support a limited number of such regions.
>> I assume there is a way for QEMU to query this limit upfront? It might
>> be required for memory hot(un)plug to figure out how many memory slots
>> we actually have (for ordinary DIMMs, and if we ever want to make this
>> compatible to virtio-mem, it might be required as well when the backend
>> pins memory).
>
> Yes, libblkio reports the maximum number of blkio_mem_regions supported
> by the device. The property is called "max-mem-regions".
>
> The QEMU BlockDriver currently doesn't use this information. Are there
> any QEMU APIs that should be called to propagate this value?
I assume we have to do exactly the same thing as e.g.,
vhost_has_free_slot()/kvm_has_free_slot() does.
Especially, hw/mem/memory-device.c needs care and
slots_limit/used_memslots handling in hw/virtio/vhost.c might be
relevant as well.
Note that I have some patches pending that extend that handling, by also
providing how many used+free slots there are, such as:
https://lore.kernel.org/all/20211027124531.57561-3-da...@redhat.com/
--
Thanks,
David / dhildenb
Re: [RFC v4 11/11] virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint
On 23.08.22 21:22, Stefan Hajnoczi wrote: > On Tue, Aug 23, 2022 at 10:01:59AM +0200, David Hildenbrand wrote: >> On 23.08.22 00:24, Stefan Hajnoczi wrote: >>> Register guest RAM using BlockRAMRegistrar and set the >>> BDRV_REQ_REGISTERED_BUF flag so block drivers can optimize memory >>> accesses in I/O requests. >>> >>> This is for vdpa-blk, vhost-user-blk, and other I/O interfaces that rely >>> on DMA mapping/unmapping. >> >> Can you explain why we're monitoring RAMRegistrar to hook into "guest >> RAM" and not go the usual path of the MemoryListener? > > The requirements are similar to VFIO, which uses RAMBlockNotifier. We Only VFIO NVME uses RAMBlockNotifier. Ordinary VFIO uses the MemoryListener. Maybe the difference is that ordinary VFIO has to replicate the actual guest physical memory layout, and VFIO NVME is only interested in possible guest RAM inside guest physical memory. > need to learn about all guest RAM because that's where I/O buffers are > located. > > Do you think RAMBlockNotifier should be avoided? I assume it depends on the use case. For saying "this might be used for I/O" it might be good enough I guess. > >> What will BDRV_REQ_REGISTERED_BUF actually do? Pin all guest memory in >> the worst case such as io_uring fixed buffers would do ( I hope not ). > > BLK_REQ_REGISTERED_BUF is a hint that no bounce buffer is necessary > because the I/O buffer is located in memory that was previously > registered with bdrv_registered_buf(). > > The RAMBlockNotifier calls bdrv_register_buf() to let the libblkio > driver know about RAM. Some libblkio drivers ignore this hint, io_uring > may use the fixed buffers feature, vhost-user sends the shared memory > file descriptors to the vhost device server, and VFIO/vhost may pin > pages. > > So the blkio block driver doesn't add anything new, it's the union of > VFIO/vhost/vhost-user/etc memory requirements. The issue is if that backend pins memory inside any of these regions. Then, you're instantly incompatible to anything the relies on sparse RAMBlocks, such as memory ballooning or virtio-mem, and have to properly fence it. In that case, you'd have to successfully trigger ram_block_discard_disable(true) first, before pinning. Who would do that now conditionally, just like e.g., VFIO does? io_uring fixed buffers would be one such example that pins memory and is problematic. vfio (unless on s390x) is another example, as you point out. This has to be treated with care. Another thing to consider is that different backends might only support a limited number of such regions. I assume there is a way for QEMU to query this limit upfront? It might be required for memory hot(un)plug to figure out how many memory slots we actually have (for ordinary DIMMs, and if we ever want to make this compatible to virtio-mem, it might be required as well when the backend pins memory). -- Thanks, David / dhildenb
Re: [RFC v4 11/11] virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint
On 23.08.22 00:24, Stefan Hajnoczi wrote: > Register guest RAM using BlockRAMRegistrar and set the > BDRV_REQ_REGISTERED_BUF flag so block drivers can optimize memory > accesses in I/O requests. > > This is for vdpa-blk, vhost-user-blk, and other I/O interfaces that rely > on DMA mapping/unmapping. Can you explain why we're monitoring RAMRegistrar to hook into "guest RAM" and not go the usual path of the MemoryListener? What will BDRV_REQ_REGISTERED_BUF actually do? Pin all guest memory in the worst case such as io_uring fixed buffers would do ( I hope not ). -- Thanks, David / dhildenb
Re: [RFC 2/8] numa: call ->ram_block_removed() in ram_block_notifer_remove()
On 23.03.22 12:17, Stefan Hajnoczi wrote: > When a RAMBlockNotifier is added, ->ram_block_added() is called with all > existing RAMBlocks. There is no equivalent ->ram_block_removed() call > when a RAMBlockNotifier is removed. > > The util/vfio-helpers.c code (the sole user of RAMBlockNotifier) is fine > with this asymmetry because it does not rely on RAMBlockNotifier for > cleanup. It walks its internal list of DMA mappings and unmaps them by > itself. > > Future users of RAMBlockNotifier may not have an internal data structure > that records added RAMBlocks so they will need ->ram_block_removed() > callbacks. > > This patch makes ram_block_notifier_remove() symmetric with respect to > callbacks. Now util/vfio-helpers.c needs to unmap remaining DMA mappings > after ram_block_notifier_remove() has been called. This is necessary > since users like block/nvme.c may create additional DMA mappings that do > not originate from the RAMBlockNotifier. > > Cc: David Hildenbrand > Signed-off-by: Stefan Hajnoczi Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH v3 09/10] hw/dma: Move ScatterGatherEntry / QEMUSGList declarations around
On 11.01.22 19:43, Philippe Mathieu-Daudé wrote:
> In the next commit we will use the dma_addr_t type in the QEMUSGList
> structure. Since currently dma_addr_t is defined after QEMUSGList,
> move the declarations to have dma_addr_t defined first. This is a
> pure code-movement patch.
Oh, that was the underlying reason for the movement. Anyhow, this
certainly makes the next patch easier to review
Reviewed-by: David Hildenbrand
>
> Suggested-by: David Hildenbrand
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/sysemu/dma.h | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 0db2478a506..c992d9d5d6b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -15,22 +15,11 @@
> #include "block/block.h"
> #include "block/accounting.h"
>
> -typedef struct ScatterGatherEntry ScatterGatherEntry;
> -
> typedef enum {
> DMA_DIRECTION_TO_DEVICE = 0,
> DMA_DIRECTION_FROM_DEVICE = 1,
> } DMADirection;
>
> -struct QEMUSGList {
> -ScatterGatherEntry *sg;
> -int nsg;
> -int nalloc;
> -size_t size;
> -DeviceState *dev;
> -AddressSpace *as;
> -};
> -
> /*
> * When an IOMMU is present, bus addresses become distinct from
> * CPU/memory physical addresses and may be a different size. Because
> @@ -43,6 +32,17 @@ typedef uint64_t dma_addr_t;
> #define DMA_ADDR_BITS 64
> #define DMA_ADDR_FMT "%" PRIx64
>
> +typedef struct ScatterGatherEntry ScatterGatherEntry;
> +
> +struct QEMUSGList {
> +ScatterGatherEntry *sg;
> +int nsg;
> +int nalloc;
> +size_t size;
> +DeviceState *dev;
> +AddressSpace *as;
> +};
> +
> static inline void dma_barrier(AddressSpace *as, DMADirection dir)
> {
> /*
--
Thanks,
David / dhildenb
Re: [PATCH v3 01/10] stubs: Restrict fw_cfg to system emulation
On 11.01.22 19:43, Philippe Mathieu-Daudé wrote:
> fw_cfg_arch_key_name() stub is only required for sysemu.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> stubs/meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 71469c1d50a..363f6fa785d 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -11,7 +11,6 @@
> stub_ss.add(files('dump.c'))
> stub_ss.add(files('error-printf.c'))
> stub_ss.add(files('fdset.c'))
> -stub_ss.add(files('fw_cfg.c'))
> stub_ss.add(files('gdbstub.c'))
> stub_ss.add(files('get-vm-name.c'))
> if linux_io_uring.found()
> @@ -51,6 +50,7 @@
>stub_ss.add(files('replay-tools.c'))
> endif
> if have_system
> + stub_ss.add(files('fw_cfg.c'))
>stub_ss.add(files('semihost.c'))
>stub_ss.add(files('usb-dev-stub.c'))
>stub_ss.add(files('xen-hw-stub.c'))
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v3 02/10] hw/nvram: Restrict fw_cfg QOM interface to sysemu and tools
On 11.01.22 19:43, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> fw_cfg QOM interface is required by system emulation and
> qemu-storage-daemon. User-mode emulation doesn't need it.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/nvram/meson.build | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
> index 202a5466e63..f5ee9f6b88c 100644
> --- a/hw/nvram/meson.build
> +++ b/hw/nvram/meson.build
> @@ -1,5 +1,7 @@
> -# QOM interfaces must be available anytime QOM is used.
> -qom_ss.add(files('fw_cfg-interface.c'))
> +if have_system or have_tools
> + # QOM interfaces must be available anytime QOM is used.
> + qom_ss.add(files('fw_cfg-interface.c'))
> +endif
>
> softmmu_ss.add(files('fw_cfg.c'))
> softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> Update the obvious places where dma_addr_t should be used
> (instead of uint64_t, hwaddr, size_t, int32_t types).
>
> This allows to have &dma_addr_t type portable on 32/64-bit
> hosts.
>
> Move QEMUSGList declaration after dma_addr_t declaration
> so this structure can use the new type.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/sysemu/dma.h | 22 +++---
> hw/nvme/ctrl.c| 2 +-
> hw/rdma/rdma_utils.c | 2 +-
> hw/scsi/megasas.c | 10 +-
> softmmu/dma-helpers.c | 6 +++---
> 5 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 0db2478a506..7a8ae4fcd0b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -15,22 +15,11 @@
> #include "block/block.h"
> #include "block/accounting.h"
>
> -typedef struct ScatterGatherEntry ScatterGatherEntry;
> -
> typedef enum {
> DMA_DIRECTION_TO_DEVICE = 0,
> DMA_DIRECTION_FROM_DEVICE = 1,
> } DMADirection;
>
> -struct QEMUSGList {
> -ScatterGatherEntry *sg;
> -int nsg;
> -int nalloc;
> -size_t size;
> -DeviceState *dev;
> -AddressSpace *as;
> -};
> -
> /*
> * When an IOMMU is present, bus addresses become distinct from
> * CPU/memory physical addresses and may be a different size. Because
> @@ -43,6 +32,17 @@ typedef uint64_t dma_addr_t;
> #define DMA_ADDR_BITS 64
> #define DMA_ADDR_FMT "%" PRIx64
>
> +typedef struct ScatterGatherEntry ScatterGatherEntry;
> +
> +struct QEMUSGList {
> +ScatterGatherEntry *sg;
> +int nsg;
> +int nalloc;
> +dma_addr_t size;
> +DeviceState *dev;
> +AddressSpace *as;
> +};
Changing one member while moving is sneaky. Why the move in this patch?
Apart from that and Peters comment
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 7/9] hw/dma: Fix format string issues using dma_addr_t
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/ide/ahci.c| 2 +-
> hw/rdma/trace-events | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 205dfdc6622..6c727dd0c08 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1159,7 +1159,7 @@ static void process_ncq_command(AHCIState *s, int port,
> const uint8_t *cmd_fis,
> ahci_populate_sglist(ad, &ncq_tfs->sglist, ncq_tfs->cmdh, size, 0);
>
> if (ncq_tfs->sglist.size < size) {
> -error_report("ahci: PRDT length for NCQ command (0x%zx) "
> +error_report("ahci: PRDT length for NCQ command (0x" DMA_ADDR_FMT ")
> "
> "is smaller than the requested size (0x%zx)",
> ncq_tfs->sglist.size, size);
> ncq_err(ncq_tfs);
> diff --git a/hw/rdma/trace-events b/hw/rdma/trace-events
> index 9accb149734..c23175120e1 100644
> --- a/hw/rdma/trace-events
> +++ b/hw/rdma/trace-events
> @@ -27,5 +27,5 @@ rdma_rm_alloc_qp(uint32_t rm_qpn, uint32_t backend_qpn,
> uint8_t qp_type) "rm_qpn
> rdma_rm_modify_qp(uint32_t qpn, uint32_t attr_mask, int qp_state, uint8_t
> sgid_idx) "qpn=0x%x, attr_mask=0x%x, qp_state=%d, sgid_idx=%d"
>
> # rdma_utils.c
> -rdma_pci_dma_map(uint64_t addr, void *vaddr, uint64_t len) "0x%"PRIx64" ->
> %p (len=%" PRId64")"
> +rdma_pci_dma_map(uint64_t addr, void *vaddr, uint64_t len) "0x%"PRIx64" ->
> %p (len=%" PRIu64")"
> rdma_pci_dma_unmap(void *vaddr) "%p"
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 6/9] hw/scsi: Rename SCSIRequest::resid as 'residual'
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote: > From: Philippe Mathieu-Daudé > > The 'resid' field is slightly confusing and could be > interpreted as some ID. Rename it as 'residual' which > is clearer to review. No logical change. > > Signed-off-by: Philippe Mathieu-Daudé > Signed-off-by: Philippe Mathieu-Daudé > --- Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [PATCH v2 5/9] hw/rdma/rdma_utils: Rename rdma_pci_dma_map 'len' argument
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> Various APIs use 'pval' naming for 'pointer to val'.
> rdma_pci_dma_map() uses 'plen' for 'PCI length', but since
> 'PCI' is already explicit in the function name, simplify
> and rename the argument 'len'. No logical change.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/rdma/rdma_utils.h | 2 +-
> hw/rdma/rdma_utils.c | 14 +++---
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/rdma/rdma_utils.h b/hw/rdma/rdma_utils.h
> index 9fd0efd940b..0c6414e7e0a 100644
> --- a/hw/rdma/rdma_utils.h
> +++ b/hw/rdma/rdma_utils.h
> @@ -38,7 +38,7 @@ typedef struct RdmaProtectedGSList {
> GSList *list;
> } RdmaProtectedGSList;
>
> -void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t plen);
> +void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t len);
> void rdma_pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t len);
> void rdma_protected_gqueue_init(RdmaProtectedGQueue *list);
> void rdma_protected_gqueue_destroy(RdmaProtectedGQueue *list);
> diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
> index 98df58f6897..61cb8ede0fd 100644
> --- a/hw/rdma/rdma_utils.c
> +++ b/hw/rdma/rdma_utils.c
> @@ -17,29 +17,29 @@
> #include "trace.h"
> #include "rdma_utils.h"
>
> -void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t plen)
> +void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t len)
> {
> void *p;
> -hwaddr len = plen;
> +hwaddr pci_len = len;
>
> if (!addr) {
> rdma_error_report("addr is NULL");
> return NULL;
> }
>
> -p = pci_dma_map(dev, addr, &len, DMA_DIRECTION_TO_DEVICE);
> +p = pci_dma_map(dev, addr, &pci_len, DMA_DIRECTION_TO_DEVICE);
> if (!p) {
> rdma_error_report("pci_dma_map fail, addr=0x%"PRIx64", len=%"PRId64,
> - addr, len);
> + addr, pci_len);
> return NULL;
> }
>
> -if (len != plen) {
> - rdma_pci_dma_unmap(dev, p, len);
> +if (pci_len != len) {
> +rdma_pci_dma_unmap(dev, p, pci_len);
> return NULL;
> }
>
> -trace_rdma_pci_dma_map(addr, p, len);
> +trace_rdma_pci_dma_map(addr, p, pci_len);
>
> return p;
> }
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 9/9] hw/dma: Let dma_buf_read() / dma_buf_write() propagate MemTxResult
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote: > From: Philippe Mathieu-Daudé > > Since the previous commit, dma_buf_rw() returns a MemTxResult > type. Do not discard it, return it to the caller. > > Since both dma_buf_read/dma_buf_write functions were previously > returning the QEMUSGList size not consumed, add an extra argument > where the unconsummed size can be stored. > > Update the few callers. I feel like this patch doesn't fit into the context of this series. Especially as the cover letter mentiones "Finally we replace misuses with dma_addr_t typedef." Am I missing something? -- Thanks, David / dhildenb
Re: [PATCH v2 4/9] hw/dma: Remove CONFIG_USER_ONLY check
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> DMA API should not be included in user-mode emulation.
> If so, build should fail. Remove the CONFIG_USER_ONLY check.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/sysemu/dma.h | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index b3faef41b2f..0db2478a506 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -31,8 +31,6 @@ struct QEMUSGList {
> AddressSpace *as;
> };
>
> -#ifndef CONFIG_USER_ONLY
> -
> /*
> * When an IOMMU is present, bus addresses become distinct from
> * CPU/memory physical addresses and may be a different size. Because
> @@ -288,7 +286,6 @@ void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev,
> int alloc_hint,
>AddressSpace *as);
> void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len);
> void qemu_sglist_destroy(QEMUSGList *qsg);
> -#endif
>
> typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov,
> BlockCompletionFunc *cb, void *cb_opaque,
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 2/9] hw/pci: Restrict pci-bus stub to sysemu
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> Neither tools nor user-mode emulation require the PCI bus stub.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> stubs/meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 363f6fa785d..d359cbe1ad7 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -26,7 +26,6 @@
> stub_ss.add(files('module-opts.c'))
> stub_ss.add(files('monitor.c'))
> stub_ss.add(files('monitor-core.c'))
> -stub_ss.add(files('pci-bus.c'))
> stub_ss.add(files('qemu-timer-notify-cb.c'))
> stub_ss.add(files('qmp_memory_device.c'))
> stub_ss.add(files('qmp-command-available.c'))
> @@ -51,6 +50,7 @@
> endif
> if have_system
>stub_ss.add(files('fw_cfg.c'))
> + stub_ss.add(files('pci-bus.c'))
>stub_ss.add(files('semihost.c'))
>stub_ss.add(files('usb-dev-stub.c'))
>stub_ss.add(files('xen-hw-stub.c'))
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v8 5/8] qmp: decode feature & status bits in virtio-status
On 28.10.21 09:56, Jonah Palmer wrote: > On 10/27/21 08:18, Laurent Vivier wrote: >> On 27/10/2021 13:59, David Hildenbrand wrote: >>> On 27.10.21 13:41, Jonah Palmer wrote: >>>> From: Laurent Vivier >>>> >>>> Display feature names instead of bitmaps for host, guest, and >>>> backend for VirtIODevice. >>>> >>>> Display status names instead of bitmaps for VirtIODevice. >>>> >>>> Display feature names instead of bitmaps for backend, protocol, >>>> acked, and features (hdev->features) for vhost devices. >>>> >>>> Decode features according to device type. Decode status >>>> according to configuration status bitmap (config_status_map). >>>> Decode vhost user protocol features according to vhost user >>>> protocol bitmap (vhost_user_protocol_map). >>>> >>>> Transport features are on the first line. Undecoded bits >>>> (if any) are stored in a separate field. Vhost device field >>>> wont show if there's no vhost active for a given VirtIODevice. >>>> >>>> Signed-off-by: Jonah Palmer >>>> --- >>>> hw/block/virtio-blk.c | 28 ++ >>>> hw/char/virtio-serial-bus.c | 11 + >>>> hw/display/virtio-gpu-base.c | 18 +- >>>> hw/input/virtio-input.c | 11 +- >>>> hw/net/virtio-net.c | 47 >>>> hw/scsi/virtio-scsi.c | 17 ++ >>>> hw/virtio/vhost-user-fs.c | 10 + >>>> hw/virtio/vhost-vsock-common.c | 10 + >>>> hw/virtio/virtio-balloon.c | 14 + >>>> hw/virtio/virtio-crypto.c | 10 + >>>> hw/virtio/virtio-iommu.c | 14 + >>>> hw/virtio/virtio.c | 273 ++- >>>> include/hw/virtio/vhost.h | 3 + >>>> include/hw/virtio/virtio.h | 17 ++ >>>> qapi/virtio.json | 577 >>>> ++--- >>> >>> Any particular reason we're not handling virtio-mem? >>> >>> (there is only a single feature bit so far, a second one to be >>> introduced soon) >>> >> >> I think this is because the v1 of the series has been written in March >> 2020 and it has not been update when virtio-mem has been added (June >> 2020). >> >> Thanks, >> Laurent > > Oops, I think I just might've missed this device. I can add in support for > virtio-mem > in the next revision! Cool, thanks! I consider the introspection interface very valuable! -- Thanks, David / dhildenb
Re: [PATCH v8 5/8] qmp: decode feature & status bits in virtio-status
On 27.10.21 13:41, Jonah Palmer wrote: > From: Laurent Vivier > > Display feature names instead of bitmaps for host, guest, and > backend for VirtIODevice. > > Display status names instead of bitmaps for VirtIODevice. > > Display feature names instead of bitmaps for backend, protocol, > acked, and features (hdev->features) for vhost devices. > > Decode features according to device type. Decode status > according to configuration status bitmap (config_status_map). > Decode vhost user protocol features according to vhost user > protocol bitmap (vhost_user_protocol_map). > > Transport features are on the first line. Undecoded bits > (if any) are stored in a separate field. Vhost device field > wont show if there's no vhost active for a given VirtIODevice. > > Signed-off-by: Jonah Palmer > --- > hw/block/virtio-blk.c | 28 ++ > hw/char/virtio-serial-bus.c| 11 + > hw/display/virtio-gpu-base.c | 18 +- > hw/input/virtio-input.c| 11 +- > hw/net/virtio-net.c| 47 > hw/scsi/virtio-scsi.c | 17 ++ > hw/virtio/vhost-user-fs.c | 10 + > hw/virtio/vhost-vsock-common.c | 10 + > hw/virtio/virtio-balloon.c | 14 + > hw/virtio/virtio-crypto.c | 10 + > hw/virtio/virtio-iommu.c | 14 + > hw/virtio/virtio.c | 273 ++- > include/hw/virtio/vhost.h | 3 + > include/hw/virtio/virtio.h | 17 ++ > qapi/virtio.json | 577 > ++--- Any particular reason we're not handling virtio-mem? (there is only a single feature bit so far, a second one to be introduced soon) -- Thanks, David / dhildenb
Re: [PATCH v2 0/3] virtio: increase VIRTQUEUE_MAX_SIZE to 32k
On 04.10.21 21:38, Christian Schoenebeck wrote: At the moment the maximum transfer size with virtio is limited to 4M (1024 * PAGE_SIZE). This series raises this limit to its maximum theoretical possible transfer size of 128M (32k pages) according to the virtio specs: https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-240006 I'm missing the "why do we care". Can you comment on that? -- Thanks, David / dhildenb
Re: [PATCH 3/4] hw/virtio/virtio-balloon: Remove the "class" property
On 03.02.21 18:18, Thomas Huth wrote:
This property was only required for compatibility reasons in the
pc-1.0 machine type and earlier. Now that these machine types have
been removed, the property is not useful anymore.
Signed-off-by: Thomas Huth
---
hw/virtio/virtio-balloon-pci.c | 11 +--
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/hw/virtio/virtio-balloon-pci.c b/hw/virtio/virtio-balloon-pci.c
index a2c5cc7207..79a3ba979a 100644
--- a/hw/virtio/virtio-balloon-pci.c
+++ b/hw/virtio/virtio-balloon-pci.c
@@ -34,21 +34,13 @@ struct VirtIOBalloonPCI {
VirtIOPCIProxy parent_obj;
VirtIOBalloon vdev;
};
-static Property virtio_balloon_pci_properties[] = {
-DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
-DEFINE_PROP_END_OF_LIST(),
-};
static void virtio_balloon_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
{
VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
-if (vpci_dev->class_code != PCI_CLASS_OTHERS &&
-vpci_dev->class_code != PCI_CLASS_MEMORY_RAM) { /* qemu < 1.1 */
-vpci_dev->class_code = PCI_CLASS_OTHERS;
-}
-
+vpci_dev->class_code = PCI_CLASS_OTHERS;
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -59,7 +51,6 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
k->realize = virtio_balloon_pci_realize;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-device_class_set_props(dc, virtio_balloon_pci_properties);
pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_BALLOON;
pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
Acked-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH 1/2] sysemu/runstate: Let runstate_is_running() return bool
On 11.01.21 16:20, Philippe Mathieu-Daudé wrote:
> runstate_check() returns a boolean. runstate_is_running()
> returns what runstate_check() returns, also a boolean.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/sysemu/runstate.h | 2 +-
> softmmu/runstate.c| 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index e557f470d42..3ab35a039a0 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -6,7 +6,7 @@
>
> bool runstate_check(RunState state);
> void runstate_set(RunState new_state);
> -int runstate_is_running(void);
> +bool runstate_is_running(void);
> bool runstate_needs_reset(void);
> bool runstate_store(char *str, size_t size);
>
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 636aab0addb..c7a67147d17 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -217,7 +217,7 @@ void runstate_set(RunState new_state)
> current_run_state = new_state;
> }
>
> -int runstate_is_running(void)
> +bool runstate_is_running(void)
> {
> return runstate_check(RUN_STATE_RUNNING);
> }
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
On 22.09.20 08:56, Paolo Bonzini wrote: > On 22/09/20 08:45, David Hildenbrand wrote: >>> It's certainly a good idea but it's quite verbose. >>> >>> What about using atomic__* as the prefix? It is not very common in QEMU >>> but there are some cases (and I cannot think of anything better). >> >> aqomic_*, lol :) > > Actually qatomic_ would be a good one, wouldn't it? agreed! -- Thanks, David / dhildenb
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
On 22.09.20 08:27, Paolo Bonzini wrote:
> On 21/09/20 18:23, Stefan Hajnoczi wrote:
>> clang's C11 atomic_fetch_*() functions only take a C11 atomic type
>> pointer argument. QEMU uses direct types (int, etc) and this causes a
>> compiler error when a QEMU code calls these functions in a source file
>> that also included via a system header file:
>>
>> $ CC=clang CXX=clang++ ./configure ... && make
>> ../util/async.c:79:17: error: address argument to atomic operation must be
>> a pointer to _Atomic type ('unsigned int *' invalid)
>>
>> Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
>> used by . Prefix QEMU's APIs with qemu_ so that atomic.h
>> and can co-exist.
>>
>> This patch was generated using:
>>
>> $ git diff | grep -o '\> >/tmp/changed_identifiers
>> $ for identifier in $(>sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l
>> "\<$identifier\>") \
>> done
>
> It's certainly a good idea but it's quite verbose.
>
> What about using atomic__* as the prefix? It is not very common in QEMU
> but there are some cases (and I cannot think of anything better).
>
aqomic_*, lol :)
> Paolo
>
--
Thanks,
David / dhildenb
Re: Suspicious QOM types without instance/class size
On 20.08.20 23:55, Eduardo Habkost wrote: > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. > > WARNING: hw/arm/armsse.c:1159:1: class_size should be set to > sizeof(ARMSSEClass)? > WARNING: hw/audio/hda-codec.c:900:1: instance_size should be set to > sizeof(HDAAudioState)? > ERROR: hw/core/register.c:328:1: instance_size should be set to > sizeof(RegisterInfo)? > WARNING: hw/input/adb.c:310:1: class_size should be set to > sizeof(ADBDeviceClass)? > WARNING: hw/isa/isa-superio.c:181:1: instance_size should be set to > sizeof(ISASuperIODevice)? > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to > sizeof(PnvLpcController)? > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to > sizeof(SpaprDrc)? > WARNING: hw/rtc/m48t59-isa.c:156:1: class_size should be set to > sizeof(M48txxISADeviceClass)? > WARNING: hw/rtc/m48t59.c:691:1: class_size should be set to > sizeof(M48txxSysBusDeviceClass)? > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > sizeof(VirtioCcwBusClass)? The parent of TYPE_VIRTIO_CCW_BUS is TYPE_VIRTIO_BUS. typedef struct VirtioBusClass VirtioCcwBusClass; So I guess the sizes match? Anyhow, setting doesn't hurt. -- Thanks, David / dhildenb
Re: [PATCH 4/7] hw/virtio/virtio-balloon: Rename timer field including 'ms' unit
On 16.06.20 09:51, Philippe Mathieu-Daudé wrote:
> To make code review easier, append the timer unit (milli-seconds)
> to its variable name.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/hw/virtio/virtio-balloon.h | 2 +-
> hw/virtio/virtio-balloon.c | 14 --
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-balloon.h
> b/include/hw/virtio/virtio-balloon.h
> index d49fef00ce..8a85fb1b88 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -50,7 +50,7 @@ typedef struct VirtIOBalloon {
> uint64_t stats[VIRTIO_BALLOON_S_NR];
> VirtQueueElement *stats_vq_elem;
> size_t stats_vq_offset;
> -QEMUTimer *stats_timer;
> +QEMUTimer *stats_timer_ms;
> IOThread *iothread;
> QEMUBH *free_page_bh;
> /*
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 10507b2a43..ad67cd53e4 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -197,16 +197,17 @@ static bool balloon_stats_enabled(const VirtIOBalloon
> *s)
> static void balloon_stats_destroy_timer(VirtIOBalloon *s)
> {
> if (balloon_stats_enabled(s)) {
> -timer_del(s->stats_timer);
> -timer_free(s->stats_timer);
> -s->stats_timer = NULL;
> +timer_del(s->stats_timer_ms);
> +timer_free(s->stats_timer_ms);
> +s->stats_timer_ms = NULL;
> s->stats_poll_interval = 0;
> }
> }
>
> static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
> {
> -timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs *
> 1000);
> +timer_mod(s->stats_timer_ms,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
> }
>
> static void balloon_stats_poll_cb(void *opaque)
> @@ -315,8 +316,9 @@ static void balloon_stats_set_poll_interval(Object *obj,
> Visitor *v,
> }
>
> /* create a new timer */
> -g_assert(s->stats_timer == NULL);
> -s->stats_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, balloon_stats_poll_cb,
> s);
> +g_assert(s->stats_timer_ms == NULL);
> +s->stats_timer_ms = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> + balloon_stats_poll_cb, s);
> s->stats_poll_interval = value;
> balloon_stats_change_timer(s, 0);
> }
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v3 3/9] qapi/misc.json: Correct balloon documentation
On 25.05.20 17:06, Philippe Mathieu-Daudé wrote:
> The documentation incorrectly uses the "size of the balloon"
> description when it should be "logical size of the VM". Fix it.
>
> The relation between both values is:
>
> logical_vm_size = vm_ram_size - balloon_size
>
> Reported-by: David Hildenbrand
> Suggested-by: David Hildenbrand
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> qapi/misc.json | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 2c48ffaa62..446fc8ff83 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -191,7 +191,8 @@
> #
> # Information about the guest balloon device.
> #
> -# @actual: the number of bytes the balloon currently contains
> +# @actual: the logical size of the VM in bytes
> +# Formula used: logical_vm_size = vm_ram_size - balloon_size
> #
> # Since: 0.14.0
> #
> @@ -227,7 +228,8 @@
> # Emitted when the guest changes the actual BALLOON level. This value is
Side note: I have no idea what an "actual BALLOON level" is. "When the
guest changes the balloon size, and thereby, the logical size of the
VM". But it's unrelated to your changes ...
> # equivalent to the @actual field return by the 'query-balloon' command
> #
> -# @actual: actual level of the guest memory balloon in bytes
> +# @actual: the logical size of the VM in bytes
> +# Formula used: logical_vm_size = vm_ram_size - balloon_size
> #
> # Note: this event is rate-limited.
> #
> @@ -756,7 +758,10 @@
> #
> # Request the balloon driver to change its balloon size.
> #
> -# @value: the target size of the balloon in bytes
> +# @value: the target logical size of the VM in bytes
> +# We can deduce the size of the balloon using this formula:
> +#logical_vm_size = vm_ram_size - balloon_size
> +# From it we have: balloon_size = vm_ram_size - @value
> #
> # Returns: - Nothing on success
> # - If the balloon driver is enabled but not functional because the
> KVM
> @@ -774,6 +779,8 @@
> # -> { "execute": "balloon", "arguments": { "value": 536870912 } }
> # <- { "return": {} }
> #
> +# With a 2.5GiB guest this command inflated the ballon to 3GiB.
s/ballon/balloon/
> +#
> ##
> { 'command': 'balloon', 'data': {'value': 'int'} }
>
>
Thanks!
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v3 4/9] qapi/misc: Restrict balloon-related commands to machine code
On 26.05.20 09:35, David Hildenbrand wrote:
> On 25.05.20 17:06, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> qapi/machine.json | 90 ++
>> qapi/misc.json | 90 --
>> include/sysemu/balloon.h | 2 +-
>> balloon.c | 2 +-
>> hw/virtio/virtio-balloon.c | 2 +-
>> monitor/hmp-cmds.c | 1 +
>> 6 files changed, 94 insertions(+), 93 deletions(-)
>>
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index ca7d58f0c9..ae42d69495 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -921,3 +921,93 @@
>>'data': 'NumaOptions',
>>'allow-preconfig': true
>> }
>> +
>> +##
>> +# @balloon:
>> +#
>> +# Request the balloon driver to change its balloon size.
>> +#
>> +# @value: the target logical size of the VM in bytes
>> +# We can deduce the size of the balloon using this formula:
>> +#logical_vm_size = vm_ram_size - balloon_size
>> +# From it we have: balloon_size = vm_ram_size - @value
>> +#
>> +# Returns: - Nothing on success
>> +# - If the balloon driver is enabled but not functional because
>> the KVM
>> +#kernel module cannot support it, KvmMissingCap
>> +# - If no balloon device is present, DeviceNotActive
>> +#
>> +# Notes: This command just issues a request to the guest. When it returns,
>> +#the balloon size may not have changed. A guest can change the
>> balloon
>> +#size independent of this command.
>> +#
>> +# Since: 0.14.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
>> +# <- { "return": {} }
>> +#
>> +# With a 2.5GiB guest this command inflated the ballon to 3GiB.
>> +#
>> +##
>> +{ 'command': 'balloon', 'data': {'value': 'int'} }
>> +
>> +##
>> +# @BalloonInfo:
>> +#
>> +# Information about the guest balloon device.
>> +#
>> +# @actual: the logical size of the VM in bytes
>> +# Formula used: logical_vm_size = vm_ram_size - balloon_size
>> +#
>> +# Since: 0.14.0
>> +#
>> +##
>> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
>> +
>> +##
>> +# @query-balloon:
>> +#
>> +# Return information about the balloon device.
>> +#
>> +# Returns: - @BalloonInfo on success
>> +# - If the balloon driver is enabled but not functional because
>> the KVM
>> +#kernel module cannot support it, KvmMissingCap
>> +# - If no balloon device is present, DeviceNotActive
>> +#
>> +# Since: 0.14.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "query-balloon" }
>> +# <- { "return": {
>> +# "actual": 1073741824,
>> +# }
>> +#}
>> +#
>> +##
>> +{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
>> +
>> +##
>> +# @BALLOON_CHANGE:
>> +#
>> +# Emitted when the guest changes the actual BALLOON level. This value is
>> +# equivalent to the @actual field return by the 'query-balloon' command
>> +#
>> +# @actual: the logical size of the VM in bytes
>> +# Formula used: logical_vm_size = vm_ram_size - balloon_size
>> +#
>> +# Note: this event is rate-limited.
>> +#
>> +# Since: 1.2
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "BALLOON_CHANGE",
>> +# "data": { "actual": 944766976 },
>> +# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>> +#
>> +##
>> +{ 'event': 'BALLOON_CHANGE',
>> + 'data': { 'actual': 'int' } }
>> diff --git a/qapi/misc.json b/qapi/misc.json
>> index 446fc8ff83..26b5115638 100644
>> --- a/qapi/misc.json
>> +++ b/qapi/misc.json
>> @@ -186,65 +186,6 @@
>> { 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
>>'allow-preconfig': true }
>>
>> -##
>> -# @BalloonInfo:
>> -#
>> -# Information about the guest balloo
Re: [PATCH v3 4/9] qapi/misc: Restrict balloon-related commands to machine code
e
> KVM
> -#kernel module cannot support it, KvmMissingCap
> -# - If no balloon device is present, DeviceNotActive
> -#
> -# Since: 0.14.0
> -#
> -# Example:
> -#
> -# -> { "execute": "query-balloon" }
> -# <- { "return": {
> -# "actual": 1073741824,
> -# }
> -#}
> -#
> -##
> -{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
> -
> -##
> -# @BALLOON_CHANGE:
> -#
> -# Emitted when the guest changes the actual BALLOON level. This value is
> -# equivalent to the @actual field return by the 'query-balloon' command
> -#
> -# @actual: the logical size of the VM in bytes
> -# Formula used: logical_vm_size = vm_ram_size - balloon_size
> -#
> -# Note: this event is rate-limited.
> -#
> -# Since: 1.2
> -#
> -# Example:
> -#
> -# <- { "event": "BALLOON_CHANGE",
> -# "data": { "actual": 944766976 },
> -# "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
> -#
> -##
> -{ 'event': 'BALLOON_CHANGE',
> - 'data': { 'actual': 'int' } }
> -
> ##
> # @PciMemoryRange:
> #
> @@ -753,37 +694,6 @@
> ##
> { 'command': 'inject-nmi' }
>
> -##
> -# @balloon:
> -#
> -# Request the balloon driver to change its balloon size.
> -#
> -# @value: the target logical size of the VM in bytes
> -# We can deduce the size of the balloon using this formula:
> -#logical_vm_size = vm_ram_size - balloon_size
> -# From it we have: balloon_size = vm_ram_size - @value
> -#
> -# Returns: - Nothing on success
> -# - If the balloon driver is enabled but not functional because the
> KVM
> -#kernel module cannot support it, KvmMissingCap
> -# - If no balloon device is present, DeviceNotActive
> -#
> -# Notes: This command just issues a request to the guest. When it returns,
> -#the balloon size may not have changed. A guest can change the
> balloon
> -#size independent of this command.
> -#
> -# Since: 0.14.0
> -#
> -# Example:
> -#
> -# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
> -# <- { "return": {} }
> -#
> -# With a 2.5GiB guest this command inflated the ballon to 3GiB.
> -#
> -##
> -{ 'command': 'balloon', 'data': {'value': 'int'} }
> -
> ##
> # @human-monitor-command:
> #
> diff --git a/include/sysemu/balloon.h b/include/sysemu/balloon.h
> index aea0c44985..b3de4b92b9 100644
> --- a/include/sysemu/balloon.h
> +++ b/include/sysemu/balloon.h
> @@ -15,7 +15,7 @@
> #define QEMU_BALLOON_H
>
> #include "exec/cpu-common.h"
> -#include "qapi/qapi-types-misc.h"
> +#include "qapi/qapi-types-machine.h"
>
> typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target);
> typedef void (QEMUBalloonStatus)(void *opaque, BalloonInfo *info);
> diff --git a/balloon.c b/balloon.c
> index f104b42961..ee9c59252d 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -30,7 +30,7 @@
> #include "sysemu/balloon.h"
> #include "trace-root.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-machine.h"
> #include "qapi/qmp/qerror.h"
>
> static QEMUBalloonEvent *balloon_event_fn;
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 065cd450f1..ec3aac1e80 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -24,7 +24,7 @@
> #include "hw/virtio/virtio-balloon.h"
> #include "exec/address-spaces.h"
> #include "qapi/error.h"
> -#include "qapi/qapi-events-misc.h"
> +#include "qapi/qapi-events-machine.h"
> #include "qapi/visitor.h"
> #include "trace.h"
> #include "qemu/error-report.h"
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 9c61e769ca..376590c073 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -32,6 +32,7 @@
> #include "qapi/qapi-commands-block.h"
> #include "qapi/qapi-commands-char.h"
> #include "qapi/qapi-commands-control.h"
> +#include "qapi/qapi-commands-machine.h"
> #include "qapi/qapi-commands-migration.h"
> #include "qapi/qapi-commands-misc.h"
> #include "qapi/qapi-commands-net.h"
>
Reviewed-by: David Hildenbrand
I yet have to craft a patch to fixup the wrong documentation :)
--
Thanks,
David / dhildenb
Re: [PATCH v2 3/8] qapi/misc: Restrict balloon-related commands to machine code
On 17.03.20 12:03, Philippe Mathieu-Daudé wrote:
> Hi David,
>
> On 3/16/20 10:05 AM, David Hildenbrand wrote:
>> On 16.03.20 01:03, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> qapi/machine.json | 83 ++
>>> qapi/misc.json | 83 --
>>> include/sysemu/balloon.h | 2 +-
>>> balloon.c | 2 +-
>>> hw/virtio/virtio-balloon.c | 2 +-
>>> monitor/hmp-cmds.c | 1 +
>>> 6 files changed, 87 insertions(+), 86 deletions(-)
>>>
>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>> index 07ffc07ba2..c096efbea3 100644
>>> --- a/qapi/machine.json
>>> +++ b/qapi/machine.json
>>> @@ -915,3 +915,86 @@
>>> 'data': 'NumaOptions',
>>> 'allow-preconfig': true
>>> }
>>> +
>>> +##
>>> +# @balloon:
>>> +#
>>> +# Request the balloon driver to change its balloon size.
>>> +#
>>> +# @value: the target size of the balloon in bytes
>>
>> Not related to your patch. The description of most of this stuff is wrong.
>>
>> It's not the target size of the balloon, it's the target logical size of
>> the VM (logical_vm_size = vm_ram_size - balloon_size)
>>
>> -> balloon_size = vm_ram_size - @value
>>
>> E.g., "balloon 1024" with a 3G guest means "please inflate the balloon
>> to 2048"
>>
>>> +#
>>> +# Returns: - Nothing on success
>>> +# - If the balloon driver is enabled but not functional because
>>> the KVM
>>> +#kernel module cannot support it, KvmMissingCap
>>> +# - If no balloon device is present, DeviceNotActive
>>> +#
>>> +# Notes: This command just issues a request to the guest. When it returns,
>>> +#the balloon size may not have changed. A guest can change the
>>> balloon
>>> +#size independent of this command.
>>> +#
>>> +# Since: 0.14.0
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'balloon', 'data': {'value': 'int'} }
>>> +
>>> +##
>>> +# @BalloonInfo:
>>> +#
>>> +# Information about the guest balloon device.
>>> +#
>>> +# @actual: the number of bytes the balloon currently contains
>>
>> Dito
>>
>> @actual is the logical size of the VM (logical_vm_size = vm_ram_size -
>> balloon_size)
>>
>>> +#
>>> +# Since: 0.14.0
>>> +#
>>> +##
>>> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
>>> +
>>> +##
>>> +# @query-balloon:
>>> +#
>>> +# Return information about the balloon device.
>>> +#
>>> +# Returns: - @BalloonInfo on success
>>> +# - If the balloon driver is enabled but not functional because
>>> the KVM
>>> +#kernel module cannot support it, KvmMissingCap
>>> +# - If no balloon device is present, DeviceNotActive
>>> +#
>>> +# Since: 0.14.0
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "query-balloon" }
>>> +# <- { "return": {
>>> +# "actual": 1073741824,
>>> +# }
>>> +#}
>>> +#
>>> +##
>>> +{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
>>> +
>>> +##
>>> +# @BALLOON_CHANGE:
>>> +#
>>> +# Emitted when the guest changes the actual BALLOON level. This value is
>>> +# equivalent to the @actual field return by the 'query-balloon' command
>>> +#
>>> +# @actual: actual level of the guest memory balloon in bytes
>>
>> Dito
>>
>> @actual is the logical size of the VM (vm_ram_size - balloon_size)
>>
>>
>> Most probably we want to pull this description fix out. #badinterface
>
> Since you understand how ballooning works, do you mind sending a patch
> with description fixed? :)
Will add it to my todo list, so your patch can go in first :)
Thanks
--
Thanks,
David / dhildenb
Re: [PATCH v2 3/8] qapi/misc: Restrict balloon-related commands to machine code
On 16.03.20 01:03, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> qapi/machine.json | 83 ++
> qapi/misc.json | 83 --
> include/sysemu/balloon.h | 2 +-
> balloon.c | 2 +-
> hw/virtio/virtio-balloon.c | 2 +-
> monitor/hmp-cmds.c | 1 +
> 6 files changed, 87 insertions(+), 86 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 07ffc07ba2..c096efbea3 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -915,3 +915,86 @@
>'data': 'NumaOptions',
>'allow-preconfig': true
> }
> +
> +##
> +# @balloon:
> +#
> +# Request the balloon driver to change its balloon size.
> +#
> +# @value: the target size of the balloon in bytes
Not related to your patch. The description of most of this stuff is wrong.
It's not the target size of the balloon, it's the target logical size of
the VM (logical_vm_size = vm_ram_size - balloon_size)
-> balloon_size = vm_ram_size - @value
E.g., "balloon 1024" with a 3G guest means "please inflate the balloon
to 2048"
> +#
> +# Returns: - Nothing on success
> +# - If the balloon driver is enabled but not functional because the
> KVM
> +#kernel module cannot support it, KvmMissingCap
> +# - If no balloon device is present, DeviceNotActive
> +#
> +# Notes: This command just issues a request to the guest. When it returns,
> +#the balloon size may not have changed. A guest can change the
> balloon
> +#size independent of this command.
> +#
> +# Since: 0.14.0
> +#
> +# Example:
> +#
> +# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'balloon', 'data': {'value': 'int'} }
> +
> +##
> +# @BalloonInfo:
> +#
> +# Information about the guest balloon device.
> +#
> +# @actual: the number of bytes the balloon currently contains
Dito
@actual is the logical size of the VM (logical_vm_size = vm_ram_size -
balloon_size)
> +#
> +# Since: 0.14.0
> +#
> +##
> +{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
> +
> +##
> +# @query-balloon:
> +#
> +# Return information about the balloon device.
> +#
> +# Returns: - @BalloonInfo on success
> +# - If the balloon driver is enabled but not functional because the
> KVM
> +#kernel module cannot support it, KvmMissingCap
> +# - If no balloon device is present, DeviceNotActive
> +#
> +# Since: 0.14.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-balloon" }
> +# <- { "return": {
> +# "actual": 1073741824,
> +# }
> +#}
> +#
> +##
> +{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
> +
> +##
> +# @BALLOON_CHANGE:
> +#
> +# Emitted when the guest changes the actual BALLOON level. This value is
> +# equivalent to the @actual field return by the 'query-balloon' command
> +#
> +# @actual: actual level of the guest memory balloon in bytes
Dito
@actual is the logical size of the VM (vm_ram_size - balloon_size)
Most probably we want to pull this description fix out. #badinterface
--
Thanks,
David / dhildenb
Re: [PATCH 2/2] misc: Replace zero-length arrays with flexible array member (manual)
CPUArchId cpus[];
> } CPUArchIdList;
>
> /**
> diff --git a/include/hw/s390x/event-facility.h
> b/include/hw/s390x/event-facility.h
> index bdc32a3c09..700a610f33 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -122,7 +122,7 @@ typedef struct MDBO {
>
> typedef struct MDB {
> MdbHeader header;
> -MDBO mdbo[0];
> +MDBO mdbo[];
> } QEMU_PACKED MDB;
>
> typedef struct SclpMsg {
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index c54413b78c..cd7b24359f 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -132,7 +132,7 @@ typedef struct ReadInfo {
> uint16_t highest_cpu;
> uint8_t _reserved5[124 - 122]; /* 122-123 */
> uint32_t hmfai;
> -struct CPUEntry entries[0];
> +struct CPUEntry entries[];
> } QEMU_PACKED ReadInfo;
>
> typedef struct ReadCpuInfo {
> @@ -142,7 +142,7 @@ typedef struct ReadCpuInfo {
> uint16_t nr_standby;/* 12-13 */
> uint16_t offset_standby;/* 14-15 */
> uint8_t reserved0[24-16]; /* 16-23 */
> -struct CPUEntry entries[0];
> +struct CPUEntry entries[];
> } QEMU_PACKED ReadCpuInfo;
>
> typedef struct ReadStorageElementInfo {
> @@ -151,7 +151,7 @@ typedef struct ReadStorageElementInfo {
> uint16_t assigned;
> uint16_t standby;
> uint8_t _reserved0[16 - 14]; /* 14-15 */
> -uint32_t entries[0];
> +uint32_t entries[];
> } QEMU_PACKED ReadStorageElementInfo;
>
> typedef struct AttachStorageElement {
> @@ -159,7 +159,7 @@ typedef struct AttachStorageElement {
> uint8_t _reserved0[10 - 8]; /* 8-9 */
> uint16_t assigned;
> uint8_t _reserved1[16 - 12]; /* 12-15 */
> -uint32_t entries[0];
> +uint32_t entries[];
> } QEMU_PACKED AttachStorageElement;
>
> typedef struct AssignStorage {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 20e909d997..8466051bc9 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -187,7 +187,7 @@ typedef struct VmdkMetaData {
> typedef struct VmdkGrainMarker {
> uint64_t lba;
> uint32_t size;
> -uint8_t data[0];
> +uint8_t data[];
> } QEMU_PACKED VmdkGrainMarker;
>
> enum {
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index c420dc066e..2b5f37b6a2 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -31,7 +31,7 @@
> typedef struct OprtnsCommand {
> EventBufferHeader header;
> MDMSU message_unit;
> -char data[0];
> +char data[];
> } QEMU_PACKED OprtnsCommand;
>
> /* max size for line-mode data in 4K SCCB page */
> diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> index 1fa124dab9..5c7664905e 100644
> --- a/hw/char/sclpconsole.c
> +++ b/hw/char/sclpconsole.c
> @@ -25,7 +25,7 @@
>
> typedef struct ASCIIConsoleData {
> EventBufferHeader ebh;
> -char data[0];
> +char data[];
> } QEMU_PACKED ASCIIConsoleData;
>
> /* max size for ASCII data in 4K SCCB page */
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 50cf95b781..64f928fc7d 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -193,7 +193,7 @@ typedef struct VirtioThinintInfo {
> typedef struct VirtioRevInfo {
> uint16_t revision;
> uint16_t length;
> -uint8_t data[0];
> +uint8_t data[];
> } QEMU_PACKED VirtioRevInfo;
>
> /* Specify where the virtqueues for the subchannel are in guest memory. */
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6..0e840cc579 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -347,7 +347,7 @@ typedef struct ChscResp {
> uint16_t len;
> uint16_t code;
> uint32_t param;
> -char data[0];
> +char data[];
> } QEMU_PACKED ChscResp;
>
> #define CHSC_MIN_RESP_LEN 0x0008
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)
.08246523f2 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -328,7 +328,7 @@ struct setup_data {
> uint64_t next;
> uint32_t type;
> uint32_t len;
> -uint8_t data[0];
> +uint8_t data[];
> } __attribute__((packed));
>
>
> diff --git a/hw/misc/omap_l4.c b/hw/misc/omap_l4.c
> index 61b6df564a..54aeaecd69 100644
> --- a/hw/misc/omap_l4.c
> +++ b/hw/misc/omap_l4.c
> @@ -24,7 +24,7 @@ struct omap_l4_s {
> MemoryRegion *address_space;
> hwaddr base;
> int ta_num;
> -struct omap_target_agent_s ta[0];
> +struct omap_target_agent_s ta[];
> };
>
> struct omap_l4_s *omap_l4_init(MemoryRegion *address_space,
> diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
> index 07f09549ed..ca6f591c84 100644
> --- a/hw/nvram/eeprom93xx.c
> +++ b/hw/nvram/eeprom93xx.c
> @@ -86,7 +86,7 @@ struct _eeprom_t {
> uint8_t addrbits;
> uint16_t size;
> uint16_t data;
> -uint16_t contents[0];
> +uint16_t contents[];
> };
>
> /* Code for saving and restoring of EEPROM state. */
> diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
> index bd6db858de..8050287a6c 100644
> --- a/hw/rdma/vmw/pvrdma_qp_ops.c
> +++ b/hw/rdma/vmw/pvrdma_qp_ops.c
> @@ -34,13 +34,13 @@ typedef struct CompHandlerCtx {
> /* Send Queue WQE */
> typedef struct PvrdmaSqWqe {
> struct pvrdma_sq_wqe_hdr hdr;
> -struct pvrdma_sge sge[0];
> +struct pvrdma_sge sge[];
> } PvrdmaSqWqe;
>
> /* Recv Queue WQE */
> typedef struct PvrdmaRqWqe {
> struct pvrdma_rq_wqe_hdr hdr;
> -struct pvrdma_sge sge[0];
> +struct pvrdma_sge sge[];
> } PvrdmaRqWqe;
>
> /*
> diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
> index 9a78ad928b..6210427544 100644
> --- a/hw/usb/dev-network.c
> +++ b/hw/usb/dev-network.c
> @@ -626,7 +626,7 @@ static const uint32_t oid_supported_list[] =
> struct rndis_response {
> QTAILQ_ENTRY(rndis_response) entries;
> uint32_t length;
> -uint8_t buf[0];
> +uint8_t buf[];
> };
>
> typedef struct USBNetState {
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 02693a26ad..ef72738ced 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -227,7 +227,7 @@ typedef struct QEMU_PACKED CCID_Parameter {
> typedef struct QEMU_PACKED CCID_DataBlock {
> CCID_BULK_IN b;
> uint8_t bChainParameter;
> -uint8_t abData[0];
> +uint8_t abData[];
> } CCID_DataBlock;
>
> /* 6.1.4 PC_to_RDR_XfrBlock */
> @@ -235,7 +235,7 @@ typedef struct QEMU_PACKED CCID_XferBlock {
> CCID_Header hdr;
> uint8_t bBWI; /* Block Waiting Timeout */
> uint16_t wLevelParameter; /* XXX currently unused */
> -uint8_t abData[0];
> +uint8_t abData[];
> } CCID_XferBlock;
>
> typedef struct QEMU_PACKED CCID_IccPowerOn {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index b2d415e5dd..b6c8ef5bc0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -54,7 +54,7 @@ typedef struct VRingAvail
> {
> uint16_t flags;
> uint16_t idx;
> -uint16_t ring[0];
> +uint16_t ring[];
> } VRingAvail;
>
> typedef struct VRingUsedElem
> @@ -67,7 +67,7 @@ typedef struct VRingUsed
> {
> uint16_t flags;
> uint16_t idx;
> -VRingUsedElem ring[0];
> +VRingUsedElem ring[];
> } VRingUsed;
>
> typedef struct VRingMemoryRegionCaches {
> diff --git a/net/queue.c b/net/queue.c
> index 61276ca4be..0164727e39 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -46,7 +46,7 @@ struct NetPacket {
> unsigned flags;
> int size;
> NetPacketSent *sent_cb;
> -uint8_t data[0];
> +uint8_t data[];
> };
>
> struct NetQueue {
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [Qemu-block] [RFC] error: auto propagated local_err
On 19.09.19 10:20, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:53, David Hildenbrand wrote:
>> On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.09.2019 10:32, David Hildenbrand wrote:
>>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> Here is a proposal (three of them, actually) of auto propagation for
>>>>> local_err, to not call error_propagate on every exit point, when we
>>>>> deal with local_err.
>>>>>
>>>>> It also may help make Greg's series[1] about error_append_hint smaller.
>>>>>
>>>>> See definitions and examples below.
>>>>>
>>>>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>>>>> it, the idea will touch same code (and may be more).
>>>>>
>>>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>> ---
>>>>>include/qapi/error.h | 102 +++
>>>>>block.c | 63 --
>>>>>block/backup.c | 8 +++-
>>>>>block/gluster.c | 7 +++
>>>>>4 files changed, 144 insertions(+), 36 deletions(-)
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index 3f95141a01..083e061014 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>>>>>ErrorClass err_class, const char *fmt, ...)
>>>>>GCC_FMT_ATTR(6, 7);
>>>>>
>>>>> +typedef struct ErrorPropagator {
>>>>> +Error **errp;
>>>>> +Error *local_err;
>>>>> +} ErrorPropagator;
>>>>> +
>>>>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>>>>> +{
>>>>> +if (prop->local_err) {
>>>>> +error_propagate(prop->errp, prop->local_err);
>>>>> +}
>>>>> +}
>>>>> +
>>>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
>>>>> error_propagator_cleanup);
>>>>> +
>>>>> +/*
>>>>> + * ErrorPropagationPair
>>>>> + *
>>>>> + * [Error *local_err, Error **errp]
>>>>> + *
>>>>> + * First element is local_err, second is original errp, which is
>>>>> propagation
>>>>> + * target. Yes, errp has a bit another type, so it should be converted.
>>>>> + *
>>>>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>>>>> + * as it's type is compatible.
>>>>> + */
>>>>> +typedef Error *ErrorPropagationPair[2];
>>>>> +
>>>>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair
>>>>> *arr)
>>>>> +{
>>>>> +Error *local_err = (*arr)[0];
>>>>> +Error **errp = (Error **)(*arr)[1];
>>>>> +
>>>>> +if (local_err) {
>>>>> +error_propagate(errp, local_err);
>>>>> +}
>>>>> +}
>>>>> +
>>>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>>>>> + error_propagation_pair_cleanup);
>>>>> +
>>>>> +/*
>>>>> + * DEF_AUTO_ERRP
>>>>> + *
>>>>> + * Define auto_errp variable, which may be used instead of errp, and
>>>>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>>>>> + * used for error_append_hint(). auto_errp is automatically propagated
>>>>> + * to errp at function exit.
>>>>> + */
>>>>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>>>>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>>>>> +
>>>>> +
>>>>> +/*
>>>>> + * Another variant:
>>>>> + * Pros:
>>>>> + * - normal structure instead of cheating with array
>>>>> + * - we can directly use errp, if it's not NULL and don't point to
>>>>
Re: [Qemu-block] [RFC] error: auto propagated local_err
On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:32, David Hildenbrand wrote:
>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is a proposal (three of them, actually) of auto propagation for
>>> local_err, to not call error_propagate on every exit point, when we
>>> deal with local_err.
>>>
>>> It also may help make Greg's series[1] about error_append_hint smaller.
>>>
>>> See definitions and examples below.
>>>
>>> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
>>> it, the idea will touch same code (and may be more).
>>>
>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> include/qapi/error.h | 102 +++
>>> block.c | 63 --
>>> block/backup.c | 8 +++-
>>> block/gluster.c | 7 +++
>>> 4 files changed, 144 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 3f95141a01..083e061014 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
>>> ErrorClass err_class, const char *fmt, ...)
>>> GCC_FMT_ATTR(6, 7);
>>>
>>> +typedef struct ErrorPropagator {
>>> +Error **errp;
>>> +Error *local_err;
>>> +} ErrorPropagator;
>>> +
>>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>>> +{
>>> +if (prop->local_err) {
>>> +error_propagate(prop->errp, prop->local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator,
>>> error_propagator_cleanup);
>>> +
>>> +/*
>>> + * ErrorPropagationPair
>>> + *
>>> + * [Error *local_err, Error **errp]
>>> + *
>>> + * First element is local_err, second is original errp, which is
>>> propagation
>>> + * target. Yes, errp has a bit another type, so it should be converted.
>>> + *
>>> + * ErrorPropagationPair may be used as errp, which points to local_err,
>>> + * as it's type is compatible.
>>> + */
>>> +typedef Error *ErrorPropagationPair[2];
>>> +
>>> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair
>>> *arr)
>>> +{
>>> +Error *local_err = (*arr)[0];
>>> +Error **errp = (Error **)(*arr)[1];
>>> +
>>> +if (local_err) {
>>> +error_propagate(errp, local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
>>> + error_propagation_pair_cleanup);
>>> +
>>> +/*
>>> + * DEF_AUTO_ERRP
>>> + *
>>> + * Define auto_errp variable, which may be used instead of errp, and
>>> + * *auto_errp may be safely checked to be zero or not, and may be safely
>>> + * used for error_append_hint(). auto_errp is automatically propagated
>>> + * to errp at function exit.
>>> + */
>>> +#define DEF_AUTO_ERRP(auto_errp, errp) \
>>> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
>>> +
>>> +
>>> +/*
>>> + * Another variant:
>>> + * Pros:
>>> + * - normal structure instead of cheating with array
>>> + * - we can directly use errp, if it's not NULL and don't point to
>>> + * error_abort or error_fatal
>>> + * Cons:
>>> + * - we need to define two variables instead of one
>>> + */
>>> +typedef struct ErrorPropagationStruct {
>>> +Error *local_err;
>>> +Error **errp;
>>> +} ErrorPropagationStruct;
>>> +
>>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
>>> *prop)
>>> +{
>>> +if (prop->local_err) {
>>> +error_propagate(prop->errp, prop->local_err);
>>> +}
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
>>> + error_propagation_struct_cleanup);
>>> +
>>> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
>>> +
Re: [Qemu-block] [RFC] error: auto propagated local_err
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is a proposal (three of them, actually) of auto propagation for
> local_err, to not call error_propagate on every exit point, when we
> deal with local_err.
>
> It also may help make Greg's series[1] about error_append_hint smaller.
>
> See definitions and examples below.
>
> I'm cc-ing to this RFC everyone from series[1] CC list, as if we like
> it, the idea will touch same code (and may be more).
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qapi/error.h | 102 +++
> block.c | 63 --
> block/backup.c | 8 +++-
> block/gluster.c | 7 +++
> 4 files changed, 144 insertions(+), 36 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..083e061014 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,108 @@ void error_set_internal(Error **errp,
> ErrorClass err_class, const char *fmt, ...)
> GCC_FMT_ATTR(6, 7);
>
> +typedef struct ErrorPropagator {
> +Error **errp;
> +Error *local_err;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ErrorPropagationPair
> + *
> + * [Error *local_err, Error **errp]
> + *
> + * First element is local_err, second is original errp, which is propagation
> + * target. Yes, errp has a bit another type, so it should be converted.
> + *
> + * ErrorPropagationPair may be used as errp, which points to local_err,
> + * as it's type is compatible.
> + */
> +typedef Error *ErrorPropagationPair[2];
> +
> +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr)
> +{
> +Error *local_err = (*arr)[0];
> +Error **errp = (Error **)(*arr)[1];
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair,
> + error_propagation_pair_cleanup);
> +
> +/*
> + * DEF_AUTO_ERRP
> + *
> + * Define auto_errp variable, which may be used instead of errp, and
> + * *auto_errp may be safely checked to be zero or not, and may be safely
> + * used for error_append_hint(). auto_errp is automatically propagated
> + * to errp at function exit.
> + */
> +#define DEF_AUTO_ERRP(auto_errp, errp) \
> +g_auto(ErrorPropagationPair) (auto_errp) = {NULL, (Error *)(errp)}
> +
> +
> +/*
> + * Another variant:
> + * Pros:
> + * - normal structure instead of cheating with array
> + * - we can directly use errp, if it's not NULL and don't point to
> + * error_abort or error_fatal
> + * Cons:
> + * - we need to define two variables instead of one
> + */
> +typedef struct ErrorPropagationStruct {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagationStruct;
> +
> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct
> *prop)
> +{
> +if (prop->local_err) {
> +error_propagate(prop->errp, prop->local_err);
> +}
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct,
> + error_propagation_struct_cleanup);
> +
> +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
> +Error **auto_errp = \
> +((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal)
> ? \
> +&__auto_errp_prop.local_err : \
> +(errp);
> +
> +/*
> + * Third variant:
> + * Pros:
> + * - simpler movement for functions which don't have local_err yet
> + * the only thing to do is to call one macro at function start.
> + * This extremely simplifies Greg's series
> + * Cons:
> + * - looks like errp shadowing.. Still seems safe.
> + * - must be after all definitions of local variables and before any
> + * code.
> + * - like v2, several statements in one open macro
> + */
> +#define MAKE_ERRP_SAFE(errp) \
> +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \
> +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { \
> +(errp) = &__auto_errp_prop.local_err; \
> +}
Using that idea, what about something like this:
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8bfb6684cb..043ad69f8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
}
+typedef struct ErrorPropagator {
+Error **errp;
+Error *local_err;
+} ErrorPropagator;
Re: [Qemu-block] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()
On 17.09.19 13:24, Cornelia Huck wrote: > On Tue, 17 Sep 2019 12:21:34 +0200 > Greg Kurz wrote: > >> Ensure that hints are added even if errp is &error_fatal or &error_abort. >> >> Signed-off-by: Greg Kurz >> --- >> hw/s390x/s390-ccw.c |6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) > > Reviewed-by: Cornelia Huck > > Can also take this via the s390 tree, let me know what would work best. > Thanks for fixing the cc list :) Reviewed-by: David Hildenbrand -- Thanks, David / dhildenb
Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] coroutine: add x86 specific coroutine backend
On 13.03.19 14:24, Paolo Bonzini wrote: > On 13/03/19 12:49, Peter Maydell wrote: >> On Mon, 11 Mar 2019 at 12:49, Paolo Bonzini wrote: >>> >>> This is in preparation for CET support for x86. >> >> What's CET ? > > It's basically shadow stacks and tracking of indirect branch > targets---hardware support for what others calls control-flow integrity. > I've now sent the full series, which consists of these patches (with a > few cleanups) followed by CET support. > >>> I know this is very late, but if possible I'd like to have this in 4.0 >>> as an experimental backend. >>> >>> Paolo Bonzini (3): >>> qemugdb: fix licensing >>> qemugdb: allow adding support for other coroutine backends >>> coroutine: add x86 specific coroutine backend >> >> I can't say I'm terribly enthusiastic about having >> native-asm specific versions of the coroutine code. >> It seems like the kind of thing that's going to be >> fragile against changes in libc implementation, calling >> convention, etc. > > I thought the same before I started writing it, but surprisingly it does > not require any of that. Since the asm clobbers all registers except > the frame and stack pointer, the compiler will just push/pop all > caller-save registers automatically in qemu_coroutine_switch and > qemu_coroutine_new's prolog and epilog. This also means that porting it > to other architectures should require only a rewrite of the CO_SWITCH macro. So the clobber list in the inline asm actually only hinders the compiler to play cheap tricks when trying to optimize - so it is forced to save/restore all caller-save register, right? Sounds sane to me, porting it shouldn't be too hard (however I can't tell when I will have time to look into the details). > > I can try porting it to ARM and/or aarch64 to prove that point, or > perhaps David will enjoy beating me and doing an s390 port. :) > > Paolo > -- Thanks, David / dhildenb
Re: [Qemu-block] [Qemu-devel] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()
On 20.08.2018 15:28, Paolo Bonzini wrote:
> On 18/08/2018 04:56, Philippe Mathieu-Daudé wrote:
>> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>>
>> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>>
>> block/sheepdog.c: In function 'find_vdi_name':
>> block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
>> destination size [-Werror=stringop-truncation]
>>strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>^~
>>
>> Reported-by: Howard Spoelstra
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>>
>> block/sheepdog.c | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index b229a664d9..5dc3d0c39e 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const
>> char *filename,
>> SheepdogVdiReq hdr;
>> SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
>> unsigned int wlen, rlen = 0;
>> -char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
>> +/* Ensures that the buffer is zero-filled,
>> + * which is desirable since we'll soon be sending those bytes, and
>> + * don't want the send_req to read uninitialized data.
>> + */
>> +char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
>>
>> fd = connect_to_sdog(s, errp);
>> if (fd < 0) {
>> return fd;
>> }
>>
>> -/* This pair of strncpy calls ensures that the buffer is zero-filled,
>> - * which is desirable since we'll soon be sending those bytes, and
>> - * don't want the send_req to read uninitialized data.
>> - */
>> -strncpy(buf, filename, SD_MAX_VDI_LEN);
>> -strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>> +g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
>> +g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>
>> memset(&hdr, 0, sizeof(hdr));
>> if (lock) {
>>
>
> The protocol doesn't require (as far as I can see) the strings to be
> NULL-terminated, therefore strncpy is the right function to use here.
>
> However, we should have a check on the length of filename and tag, so
> that no truncation is done. This applies to both strncpy and g_strlcpy.
>
> Indeed I find g_strlcpy to be harmful because it encourages a style
> where truncations happen silently. There are very few cases where
> silent truncation is the right thing to do, and in several cases where
> you _have to have_ fixed-size buffers, those buffers are sent on the
> wire---and then g_strlcpy is wrong, while strncpy is just as good as
> memset+strlen+memcpy (and shorter).
Yes, convinced, let's disable the warning.
>
> Paolo
>
--
Thanks,
David / dhildenb
Re: [Qemu-block] [PATCH] sheepdog: Replace strncpy() by g_strlcpy()
On 18.08.2018 04:56, Philippe Mathieu-Daudé wrote:
> Fedora 29 comes with GCC 8.1 which added the 'stringop-truncation' checks.
>
> Replace the strncpy() calls by g_strlcpy() to avoid the following warning:
>
> block/sheepdog.c: In function 'find_vdi_name':
> block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals
> destination size [-Werror=stringop-truncation]
>strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>^~
>
> Reported-by: Howard Spoelstra
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> See http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg03723.html
>
> block/sheepdog.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index b229a664d9..5dc3d0c39e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1224,19 +1224,19 @@ static int find_vdi_name(BDRVSheepdogState *s, const
> char *filename,
> SheepdogVdiReq hdr;
> SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr;
> unsigned int wlen, rlen = 0;
> -char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
> +/* Ensures that the buffer is zero-filled,
> + * which is desirable since we'll soon be sending those bytes, and
> + * don't want the send_req to read uninitialized data.
> + */
I this really necessary? The two g_strlcpy should still fill the whole
buffer, no?
> +char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN] = { };
>
> fd = connect_to_sdog(s, errp);
> if (fd < 0) {
> return fd;
> }
>
> -/* This pair of strncpy calls ensures that the buffer is zero-filled,
> - * which is desirable since we'll soon be sending those bytes, and
> - * don't want the send_req to read uninitialized data.
> - */
> -strncpy(buf, filename, SD_MAX_VDI_LEN);
> -strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> +g_strlcpy(buf, filename, SD_MAX_VDI_LEN);
> +g_strlcpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>
> memset(&hdr, 0, sizeof(hdr));
> if (lock) {
>
--
Thanks,
David / dhildenb
Re: [Qemu-block] [PATCH v2 1/3] iotests: use -ccw on s390x for 040, 139, and 182
On 13.09.2017 11:10, Cornelia Huck wrote:
> The default cpu model on s390x does not provide zPCI, which is
> not yet wired up on tcg. Moreover, virtio-ccw is the standard
> on s390x, so use the -ccw instead of the -pci versions of virtio
> devices on s390x.
>
> Reviewed-by: Kevin Wolf
> Signed-off-by: Cornelia Huck
> ---
> tests/qemu-iotests/040 | 6 +-
> tests/qemu-iotests/139 | 12 ++--
> tests/qemu-iotests/182 | 13 +++--
> 3 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index 95b7510571..c284d08796 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -82,7 +82,11 @@ class TestSingleDrive(ImageCommitTestCase):
> qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
> qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
> mid_img)
> self.vm = iotests.VM().add_drive(test_img,
> "node-name=top,backing.node-name=mid,backing.backing.node-name=base",
> interface="none")
> -self.vm.add_device("virtio-scsi-pci")
> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> +self.vm.add_device("virtio-scsi-ccw")
> +else:
> +self.vm.add_device("virtio-scsi-pci")
> +
> self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
> self.vm.launch()
>
> diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
> index 50cf40fbd5..f8f02808a9 100644
> --- a/tests/qemu-iotests/139
> +++ b/tests/qemu-iotests/139
> @@ -25,13 +25,21 @@ import time
>
> base_img = os.path.join(iotests.test_dir, 'base.img')
> new_img = os.path.join(iotests.test_dir, 'new.img')
> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> +default_virtio_blk = 'virtio-blk-ccw'
> +else:
> +default_virtio_blk = 'virtio-blk-pci'
simply "virtio_blk" ?
>
> class TestBlockdevDel(iotests.QMPTestCase):
>
> def setUp(self):
> iotests.qemu_img('create', '-f', iotests.imgfmt, base_img, '1M')
> self.vm = iotests.VM()
> -self.vm.add_device("virtio-scsi-pci,id=virtio-scsi")
> +if iotests.qemu_default_machine == 's390-ccw-virtio':
> +self.vm.add_device("virtio-scsi-ccw,id=virtio-scsi")
> +else:
> +self.vm.add_device("virtio-scsi-pci,id=virtio-scsi")
> +
> self.vm.launch()
>
> def tearDown(self):
> @@ -87,7 +95,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
> self.checkBlockDriverState(node, expect_error)
>
> # Add a device model
> -def addDeviceModel(self, device, backend, driver = 'virtio-blk-pci'):
> +def addDeviceModel(self, device, backend, driver = default_virtio_blk):
> result = self.vm.qmp('device_add', id = device,
> driver = driver, drive = backend)
> self.assert_qmp(result, 'return', {})
> diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
> index 7ecbb22604..2e078ceed8 100755
> --- a/tests/qemu-iotests/182
> +++ b/tests/qemu-iotests/182
> @@ -45,17 +45,26 @@ _supported_os Linux
>
> size=32M
>
> +case "$QEMU_DEFAULT_MACHINE" in
> + s390-ccw-virtio)
> + virtioblk=virtio-blk-ccw
> + ;;
> + *)
> + virtioblk=virtio-blk-pci
s/virtioblk/virtio_blk/ ?
(due to "virtio_scsi" in next patch)
> + ;;
> +esac
> +
> _make_test_img $size
>
> echo "Starting QEMU"
> _launch_qemu -drive file=$TEST_IMG,if=none,id=drive0,file.locking=on \
> --device virtio-blk-pci,drive=drive0
> +-device $virtioblk,drive=drive0
>
> echo
> echo "Starting a second QEMU using the same image should fail"
> echo 'quit' | $QEMU -monitor stdio \
> -drive file=$TEST_IMG,if=none,id=drive0,file.locking=on \
> --device virtio-blk-pci,drive=drive0 2>&1 | _filter_testdir 2>&1 |
> +-device $virtioblk,drive=drive0 2>&1 | _filter_testdir 2>&1 |
> _filter_qemu |
> sed -e '/falling back to POSIX file/d' \
> -e '/locks can be lost unexpectedly/d'
>
--
Thanks,
David
Re: [Qemu-block] [PATCH v2 3/3] iotests: use virtio aliases for 067
On 13.09.2017 11:10, Cornelia Huck wrote:
> The default cpu model on s390x does not provide zPCI, which is
> not yet wired up on tcg. Moreover, virtio-ccw is the standard
> on s390x.
>
> Using virtio-scsi will implicitly pick the right device, so just
> switch to that for simplicity.
>
> Signed-off-by: Cornelia Huck
> ---
> tests/qemu-iotests/067 | 3 ++-
> tests/qemu-iotests/067.out | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> index 5d4ca4bc61..cbb3da286a 100755
> --- a/tests/qemu-iotests/067
> +++ b/tests/qemu-iotests/067
> @@ -141,7 +141,7 @@ echo
> echo === Empty drive with -device and device_del ===
> echo
>
> -run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0 < +run_qemu -device virtio-scsi -device scsi-cd,id=cd0 < { "execute": "qmp_capabilities" }
> { "execute": "query-block" }
> { "execute": "device_del", "arguments": { "id": "cd0" } }
> @@ -150,6 +150,7 @@ run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0
> < { "execute": "quit" }
> EOF
>
> +
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out
> index bd70557ddc..58e83c4505 100644
> --- a/tests/qemu-iotests/067.out
> +++ b/tests/qemu-iotests/067.out
> @@ -419,7 +419,7 @@ Testing:
>
> === Empty drive with -device and device_del ===
>
> -Testing: -device virtio-scsi-pci -device scsi-cd,id=cd0
> +Testing: -device virtio-scsi -device scsi-cd,id=cd0
> {
> QMP_VERSION
> }
>
Certainly not wrong to use the old alias in some tests, as long as we
test both variants.
Reviewed-by: David Hildenbrand
--
Thanks,
David
Re: [Qemu-block] [PATCH v2 2/3] iotests: use -ccw on s390x for 051
On 13.09.2017 11:10, Cornelia Huck wrote: > The default cpu model on s390x does not provide zPCI, which is > not yet wired up on tcg. Moreover, virtio-ccw is the standard > on s390x, so use the -ccw instead of the -pci versions of virtio > devices on s390x. > > Signed-off-by: Cornelia Huck > --- > tests/qemu-iotests/051| 12 +++- > tests/qemu-iotests/051.out| 2 +- > tests/qemu-iotests/051.pc.out | 2 +- > 3 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 > index c8cfc764bc..dba8816c9f 100755 > --- a/tests/qemu-iotests/051 > +++ b/tests/qemu-iotests/051 > @@ -103,7 +103,17 @@ echo > echo === Device without drive === > echo > > -run_qemu -device virtio-scsi-pci -device scsi-hd > +case "$QEMU_DEFAULT_MACHINE" in > + s390-ccw-virtio) > + virtio_scsi=virtio-scsi-ccw > + ;; > + *) > + virtio_scsi=virtio-scsi-pci > + ;; > +esac > + > +run_qemu -device $virtio_scsi -device scsi-hd | > +sed -e "s/$virtio_scsi/VIRTIO_SCSI/" > > echo > echo === Overriding backing file === > diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out > index 4d3b1ff316..e3c6eaba57 100644 > --- a/tests/qemu-iotests/051.out > +++ b/tests/qemu-iotests/051.out > @@ -49,7 +49,7 @@ QEMU_PROG: -drive > file=TEST_DIR/t.qcow2,driver=qcow2,format=qcow2: Cannot specif > > === Device without drive === > > -Testing: -device virtio-scsi-pci -device scsi-hd > +Testing: -device VIRTIO_SCSI -device scsi-hd > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) QEMU_PROG: -device scsi-hd: drive property not set > > diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out > index 76d7205460..ae7801b44b 100644 > --- a/tests/qemu-iotests/051.pc.out > +++ b/tests/qemu-iotests/051.pc.out > @@ -49,7 +49,7 @@ QEMU_PROG: -drive > file=TEST_DIR/t.qcow2,driver=qcow2,format=qcow2: Cannot specif > > === Device without drive === > > -Testing: -device virtio-scsi-pci -device scsi-hd > +Testing: -device VIRTIO_SCSI -device scsi-hd > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) QEMU_PROG: -device scsi-hd: drive property not set > > Reviewed-by: David Hildenbrand -- Thanks, David
