Re: [PATCH v4 01/34] modules: add modinfo macros
On Thu, Jun 24, 2021 at 12:38:03PM +0200, Gerd Hoffmann wrote: > Add macros for module info annotations. > > Instead of having that module meta-data stored in lists in util/module.c > place directly in the module source code. > [...] > +/* module implements QOM type */ > +#define module_obj(name) modinfo(obj, name) Can we make OBJECT_DEFINE_TYPE*() use this macro automatically? -- Eduardo
[PATCH 08/11] block: check for sys/disk.h
From: Joelle van Dyne
Some BSD platforms do not have this header.
Reviewed-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Joelle van Dyne
Message-Id: <20210315180341.31638-...@getutm.app>
Reviewed-by: Max Reitz
Signed-off-by: Paolo Bonzini
---
block.c | 2 +-
meson.build | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 3f456892d0..1d37f133a8 100644
--- a/block.c
+++ b/block.c
@@ -54,7 +54,7 @@
#ifdef CONFIG_BSD
#include
#include
-#ifndef __DragonFly__
+#if defined(HAVE_SYS_DISK_H)
#include
#endif
#endif
diff --git a/meson.build b/meson.build
index bb3a5be796..a95a9fbcbf 100644
--- a/meson.build
+++ b/meson.build
@@ -1250,6 +1250,7 @@ config_host_data.set('HAVE_SYS_IOCCOM_H',
cc.has_header('sys/ioccom.h'))
config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
config_host_data.set('HAVE_SYSTEM_FUNCTION', cc.has_function('system', prefix:
'#include '))
config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
+config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix:
'#include '))
--
2.31.1
[PATCH 09/11] block: try BSD disk size ioctls one after another
Try all the possible ioctls for disk size as long as they are
supported, to keep the #if ladder simple.
Extracted and cleaned up from a patch by Joelle van Dyne and
Warner Losh.
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index e56bb491a1..f16d987c07 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2327,39 +2327,37 @@ static int64_t raw_getlength(BlockDriverState *bs)
again:
#endif
if (!fstat(fd, ) && (S_IFCHR & sb.st_mode)) {
+size = 0;
#ifdef DIOCGMEDIASIZE
-if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)))
-#elif defined(DIOCGPART)
-{
-struct partinfo pi;
-if (ioctl(fd, DIOCGPART, ) == 0)
-size = pi.media_size;
-else
-size = 0;
+if (ioctl(fd, DIOCGMEDIASIZE, (off_t *))) {
+size = 0;
+}
+#endif
+#ifdef DIOCGPART
+if (size == 0) {
+struct partinfo pi;
+if (ioctl(fd, DIOCGPART, ) == 0) {
+size = pi.media_size;
+}
}
-if (size == 0)
#endif
#if defined(__APPLE__) && defined(__MACH__)
-{
+if (size == 0) {
uint64_t sectors = 0;
uint32_t sector_size = 0;
if (ioctl(fd, DKIOCGETBLOCKCOUNT, ) == 0
&& ioctl(fd, DKIOCGETBLOCKSIZE, _size) == 0) {
size = sectors * sector_size;
-} else {
-size = lseek(fd, 0LL, SEEK_END);
-if (size < 0) {
-return -errno;
-}
}
}
-#else
-size = lseek(fd, 0LL, SEEK_END);
+#endif
+if (size == 0) {
+size = lseek(fd, 0LL, SEEK_END);
+}
if (size < 0) {
return -errno;
}
-#endif
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
switch(s->type) {
case FTYPE_CD:
--
2.31.1
[PATCH 11/11] file-posix: handle EINTR during ioctl
Similar to other handle_aiocb_* functions, handle_aiocb_ioctl needs to cater
for the possibility that ioctl is interrupted by a signal. Otherwise, the
I/O is incorrectly reported as a failure to the guest.
Reported-by: Gordon Watson
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 74b8216077..a26eab0ac3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1347,7 +1347,9 @@ static int handle_aiocb_ioctl(void *opaque)
RawPosixAIOData *aiocb = opaque;
int ret;
-ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
+do {
+ret = ioctl(aiocb->aio_fildes, aiocb->ioctl.cmd, aiocb->ioctl.buf);
+} while (ret == -1 && errno == EINTR);
if (ret == -1) {
return -errno;
}
--
2.31.1
[PATCH 07/11] block: feature detection for host block support
From: Joelle van Dyne
On Darwin (iOS), there are no system level APIs for directly accessing
host block devices. We detect this at configure time.
Signed-off-by: Joelle van Dyne
Message-Id: <20210315180341.31638-...@getutm.app>
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 33 ++---
meson.build | 6 +-
qapi/block-core.json | 14 ++
3 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index ea102483b0..e56bb491a1 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -42,6 +42,8 @@
#include "scsi/constants.h"
#if defined(__APPLE__) && (__MACH__)
+#include
+#if defined(HAVE_HOST_BLOCK_DEVICE)
#include
#include
#include
@@ -52,6 +54,7 @@
//#include
#include
#include
+#endif /* defined(HAVE_HOST_BLOCK_DEVICE) */
#endif
#ifdef __sun__
@@ -178,7 +181,17 @@ typedef struct BDRVRawReopenState {
bool check_cache_dropped;
} BDRVRawReopenState;
-static int fd_open(BlockDriverState *bs);
+static int fd_open(BlockDriverState *bs)
+{
+BDRVRawState *s = bs->opaque;
+
+/* this is just to ensure s->fd is sane (its called by io ops) */
+if (s->fd >= 0) {
+return 0;
+}
+return -EIO;
+}
+
static int64_t raw_getlength(BlockDriverState *bs);
typedef struct RawPosixAIOData {
@@ -3033,6 +3046,7 @@ static BlockStatsSpecific
*raw_get_specific_stats(BlockDriverState *bs)
return stats;
}
+#if defined(HAVE_HOST_BLOCK_DEVICE)
static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs)
{
BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
@@ -3042,6 +3056,7 @@ static BlockStatsSpecific
*hdev_get_specific_stats(BlockDriverState *bs)
return stats;
}
+#endif /* HAVE_HOST_BLOCK_DEVICE */
static QemuOptsList raw_create_opts = {
.name = "raw-create-opts",
@@ -3257,6 +3272,8 @@ BlockDriver bdrv_file = {
/***/
/* host device */
+#if defined(HAVE_HOST_BLOCK_DEVICE)
+
#if defined(__APPLE__) && defined(__MACH__)
static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
CFIndex maxPathSize, int flags);
@@ -3549,16 +3566,6 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int
req, void *buf)
}
#endif /* linux */
-static int fd_open(BlockDriverState *bs)
-{
-BDRVRawState *s = bs->opaque;
-
-/* this is just to ensure s->fd is sane (its called by io ops) */
-if (s->fd >= 0)
-return 0;
-return -EIO;
-}
-
static coroutine_fn int
hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
{
@@ -3882,6 +3889,8 @@ static BlockDriver bdrv_host_cdrom = {
};
#endif /* __FreeBSD__ */
+#endif /* HAVE_HOST_BLOCK_DEVICE */
+
static void bdrv_file_init(void)
{
/*
@@ -3889,6 +3898,7 @@ static void bdrv_file_init(void)
* registered last will get probed first.
*/
bdrv_register(_file);
+#if defined(HAVE_HOST_BLOCK_DEVICE)
bdrv_register(_host_device);
#ifdef __linux__
bdrv_register(_host_cdrom);
@@ -3896,6 +3906,7 @@ static void bdrv_file_init(void)
#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
bdrv_register(_host_cdrom);
#endif
+#endif /* HAVE_HOST_BLOCK_DEVICE */
}
block_init(bdrv_file_init);
diff --git a/meson.build b/meson.build
index 62de7ac106..bb3a5be796 100644
--- a/meson.build
+++ b/meson.build
@@ -183,7 +183,7 @@ if targetos == 'windows'
include_directories:
include_directories('.'))
elif targetos == 'darwin'
coref = dependency('appleframeworks', modules: 'CoreFoundation')
- iokit = dependency('appleframeworks', modules: 'IOKit')
+ iokit = dependency('appleframeworks', modules: 'IOKit', required: false)
elif targetos == 'sunos'
socket = [cc.find_library('socket'),
cc.find_library('nsl'),
@@ -1147,6 +1147,9 @@ if get_option('cfi')
add_global_link_arguments(cfi_flags, native: false, language: ['c', 'cpp',
'objc'])
endif
+have_host_block_device = (targetos != 'darwin' or
+cc.has_header('IOKit/storage/IOMedia.h'))
+
#
# config-host.h #
#
@@ -1246,6 +1249,7 @@ config_host_data.set('HAVE_PTY_H', cc.has_header('pty.h'))
config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
config_host_data.set('HAVE_SYSTEM_FUNCTION', cc.has_function('system', prefix:
'#include '))
+config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
config_host_data.set('CONFIG_PREADV', cc.has_function('preadv', prefix:
'#include '))
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2ea294129e..a54f37dbef 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -897,7 +897,8 @@
'discriminator': 'driver',
'data': {
'file': 'BlockStatsSpecificFile',
- 'host_device': 'BlockStatsSpecificFile',
+
[PATCH 10/11] block: detect DKIOCGETBLOCKCOUNT/SIZE before use
From: Joelle van Dyne
iOS hosts do not have these defined so we fallback to the
default behaviour.
Co-authored-by: Warner Losh
Signed-off-by: Joelle van Dyne
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index f16d987c07..74b8216077 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2341,7 +2341,7 @@ again:
}
}
#endif
-#if defined(__APPLE__) && defined(__MACH__)
+#if defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)
if (size == 0) {
uint64_t sectors = 0;
uint32_t sector_size = 0;
--
2.31.1
[PATCH 05/11] block: add max_hw_transfer to BlockLimits
For block host devices, I/O can happen through either the kernel file
descriptor I/O system calls (preadv/pwritev, io_submit, io_uring)
or the SCSI passthrough ioctl SG_IO.
In the latter case, the size of each transfer can be limited by the
HBA, while for file descriptor I/O the kernel is able to split and
merge I/O in smaller pieces as needed. Applying the HBA limits to
file descriptor I/O results in more system calls and suboptimal
performance, so this patch splits the max_transfer limit in two:
max_transfer remains valid and is used in general, while max_hw_transfer
is limited to the maximum hardware size. max_hw_transfer can then be
included by the scsi-generic driver in the block limits page, to ensure
that the stricter hardware limit is used.
Signed-off-by: Paolo Bonzini
---
block/block-backend.c | 13 +
block/file-posix.c | 2 +-
block/io.c | 2 ++
hw/scsi/scsi-generic.c | 2 +-
include/block/block_int.h | 7 +++
include/sysemu/block-backend.h | 1 +
6 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 6e37582740..deb55c272e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1953,6 +1953,19 @@ uint32_t blk_get_request_alignment(BlockBackend *blk)
return bs ? bs->bl.request_alignment : BDRV_SECTOR_SIZE;
}
+/* Returns the maximum hardware transfer length, in bytes; guaranteed nonzero
*/
+uint64_t blk_get_max_hw_transfer(BlockBackend *blk)
+{
+BlockDriverState *bs = blk_bs(blk);
+uint64_t max = INT_MAX;
+
+if (bs) {
+max = MIN_NON_ZERO(max, bs->bl.max_hw_transfer);
+max = MIN_NON_ZERO(max, bs->bl.max_transfer);
+}
+return ROUND_DOWN(max, blk_get_request_alignment(blk));
+}
+
/* Returns the maximum transfer length, in bytes; guaranteed nonzero */
uint32_t blk_get_max_transfer(BlockBackend *blk)
{
diff --git a/block/file-posix.c b/block/file-posix.c
index 6db690baf2..88e58d2863 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1232,7 +1232,7 @@ static void raw_refresh_limits(BlockDriverState *bs,
Error **errp)
int ret = sg_get_max_transfer_length(s->fd);
if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
-bs->bl.max_transfer = pow2floor(ret);
+bs->bl.max_hw_transfer = pow2floor(ret);
}
ret = sg_get_max_segments(s->fd);
diff --git a/block/io.c b/block/io.c
index 323854d063..dd93364258 100644
--- a/block/io.c
+++ b/block/io.c
@@ -127,6 +127,8 @@ static void bdrv_merge_limits(BlockLimits *dst, const
BlockLimits *src)
{
dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
dst->max_transfer = MIN_NON_ZERO(dst->max_transfer, src->max_transfer);
+dst->max_hw_transfer = MIN_NON_ZERO(dst->max_hw_transfer,
+src->max_hw_transfer);
dst->opt_mem_alignment = MAX(dst->opt_mem_alignment,
src->opt_mem_alignment);
dst->min_mem_alignment = MAX(dst->min_mem_alignment,
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index b6c4143dc7..665baf900e 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -179,7 +179,7 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq *r,
SCSIDevice *s, int len)
(r->req.cmd.buf[1] & 0x01)) {
page = r->req.cmd.buf[2];
if (page == 0xb0) {
-uint32_t max_transfer = blk_get_max_transfer(s->conf.blk);
+uint64_t max_transfer = blk_get_max_hw_transfer(s->conf.blk);
uint32_t max_iov = blk_get_max_iov(s->conf.blk);
assert(max_transfer);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 057d88b1fc..f1a54db0f8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -695,6 +695,13 @@ typedef struct BlockLimits {
* clamped down. */
uint32_t max_transfer;
+/* Maximal hardware transfer length in bytes. Applies whenever
+ * transfers to the device bypass the kernel I/O scheduler, for
+ * example with SG_IO. If larger than max_transfer or if zero,
+ * blk_get_max_hw_transfer will fall back to max_transfer.
+ */
+uint64_t max_hw_transfer;
+
/* memory alignment, in bytes so that no bounce buffer is needed */
size_t min_mem_alignment;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 5423e3d9c6..9ac5f7bbd3 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -208,6 +208,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag);
int blk_get_flags(BlockBackend *blk);
uint32_t blk_get_request_alignment(BlockBackend *blk);
uint32_t blk_get_max_transfer(BlockBackend *blk);
+uint64_t blk_get_max_hw_transfer(BlockBackend *blk);
int blk_get_max_iov(BlockBackend *blk);
void blk_set_guest_block_size(BlockBackend *blk, int align);
void *blk_try_blockalign(BlockBackend *blk,
[PATCH 06/11] file-posix: try BLKSECTGET on block devices too, do not round to power of 2
bs->sg is only true for character devices, but block devices can also
be used with scsi-block and scsi-generic. Unfortunately BLKSECTGET
returns bytes in an int for /dev/sgN devices, and sectors in a short
for block devices, so account for that in the code.
The maximum transfer also need not be a power of 2 (for example I have
seen disks with 1280 KiB maximum transfer) so there's no need to pass
the result through pow2floor.
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 67 ++
1 file changed, 38 insertions(+), 29 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 88e58d2863..ea102483b0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1147,22 +1147,27 @@ static void raw_reopen_abort(BDRVReopenState *state)
s->reopen_state = NULL;
}
-static int sg_get_max_transfer_length(int fd)
+static int hdev_get_max_hw_transfer(int fd, struct stat *st)
{
#ifdef BLKSECTGET
-int max_bytes = 0;
-
-if (ioctl(fd, BLKSECTGET, _bytes) == 0) {
-return max_bytes;
+if (S_ISBLK(st->st_mode)) {
+unsigned short max_sectors = 0;
+if (ioctl(fd, BLKSECTGET, _sectors) == 0) {
+return max_sectors * 512;
+}
} else {
-return -errno;
+int max_bytes = 0;
+if (ioctl(fd, BLKSECTGET, _bytes) == 0) {
+return max_bytes;
+}
}
+return -errno;
#else
return -ENOSYS;
#endif
}
-static int sg_get_max_segments(int fd)
+static int hdev_get_max_segments(int fd, struct stat *st)
{
#ifdef CONFIG_LINUX
char buf[32];
@@ -1171,26 +1176,20 @@ static int sg_get_max_segments(int fd)
int ret;
int sysfd = -1;
long max_segments;
-struct stat st;
-if (fstat(fd, )) {
-ret = -errno;
-goto out;
-}
-
-if (S_ISCHR(st.st_mode)) {
+if (S_ISCHR(st->st_mode)) {
if (ioctl(fd, SG_GET_SG_TABLESIZE, ) == 0) {
return ret;
}
return -ENOTSUP;
}
-if (!S_ISBLK(st.st_mode)) {
+if (!S_ISBLK(st->st_mode)) {
return -ENOTSUP;
}
sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
-major(st.st_rdev), minor(st.st_rdev));
+major(st->st_rdev), minor(st->st_rdev));
sysfd = open(sysfspath, O_RDONLY);
if (sysfd == -1) {
ret = -errno;
@@ -1227,23 +1226,33 @@ out:
static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
{
BDRVRawState *s = bs->opaque;
-
-if (bs->sg) {
-int ret = sg_get_max_transfer_length(s->fd);
-
-if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
-bs->bl.max_hw_transfer = pow2floor(ret);
-}
-
-ret = sg_get_max_segments(s->fd);
-if (ret > 0) {
-bs->bl.max_iov = ret;
-}
-}
+struct stat st;
raw_probe_alignment(bs, s->fd, errp);
bs->bl.min_mem_alignment = s->buf_align;
bs->bl.opt_mem_alignment = MAX(s->buf_align, qemu_real_host_page_size);
+
+/*
+ * Maximum transfers are best effort, so it is okay to ignore any
+ * errors. That said, based on the man page errors in fstat would be
+ * very much unexpected; the only possible case seems to be ENOMEM.
+ */
+if (fstat(s->fd, )) {
+return;
+}
+
+if (bs->sg || S_ISBLK(st.st_mode)) {
+int ret = hdev_get_max_hw_transfer(s->fd, );
+
+if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+bs->bl.max_hw_transfer = ret;
+}
+
+ret = hdev_get_max_segments(s->fd, );
+if (ret > 0) {
+bs->bl.max_iov = ret;
+}
+}
}
static int check_for_dasd(int fd)
--
2.31.1
[PATCH v5 00/11] block: file-posix queue
New patches: - 3/4 (for review comments), - 9 (split for ease of review), - 11 (new bugfix) v1->v2: add missing patch v2->v3: add max_hw_transfer to BlockLimits v3->v4: fix compilation after patch 1, tweak commit messages according to Vladimir's review v4->v5: round down max_transfer and max_hw_transfer to request alignment checkpatch fixes return -ENOTSUP, -not -EIO if block limits ioctls fail handle host_cdrom like host_device in QAPI split "block: try BSD disk size ioctls one after another" new bugfix patch "file-posix: handle EINTR during ioctl" Joelle van Dyne (3): block: feature detection for host block support block: check for sys/disk.h block: detect DKIOCGETBLOCKCOUNT/SIZE before use Paolo Bonzini (8): file-posix: fix max_iov for /dev/sg devices scsi-generic: pass max_segments via max_iov field in BlockLimits osdep: provide ROUND_DOWN macro block-backend: align max_transfer to request alignment block: add max_hw_transfer to BlockLimits file-posix: try BLKSECTGET on block devices too, do not round to power of 2 block: try BSD disk size ioctls one after another file-posix: handle EINTR during ioctl block.c| 2 +- block/block-backend.c | 19 - block/file-posix.c | 144 - block/io.c | 2 + hw/scsi/scsi-generic.c | 6 +- include/block/block_int.h | 7 ++ include/qemu/osdep.h | 28 +-- include/sysemu/block-backend.h | 1 + meson.build| 7 +- qapi/block-core.json | 14 +++- 10 files changed, 156 insertions(+), 74 deletions(-) -- 2.31.1
[PATCH 02/11] scsi-generic: pass max_segments via max_iov field in BlockLimits
I/O to a disk via read/write is not limited by the number of segments allowed
by the host adapter; the kernel can split requests if needed, and the limit
imposed by the host adapter can be very low (256k or so) to avoid that SG_IO
returns EINVAL if memory is heavily fragmented.
Since this value is only interesting for SG_IO-based I/O, do not include
it in the max_transfer and only take it into account when patching the
block limits VPD page in the scsi-generic device.
Signed-off-by: Paolo Bonzini
Reviewed-by: Max Reitz
---
block/file-posix.c | 3 +--
hw/scsi/scsi-generic.c | 6 --
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index b8dc19ce1a..6db690baf2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1237,8 +1237,7 @@ static void raw_refresh_limits(BlockDriverState *bs,
Error **errp)
ret = sg_get_max_segments(s->fd);
if (ret > 0) {
-bs->bl.max_transfer = MIN(bs->bl.max_transfer,
- ret * qemu_real_host_page_size);
+bs->bl.max_iov = ret;
}
}
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 40e039864f..b6c4143dc7 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -179,10 +179,12 @@ static int scsi_handle_inquiry_reply(SCSIGenericReq *r,
SCSIDevice *s, int len)
(r->req.cmd.buf[1] & 0x01)) {
page = r->req.cmd.buf[2];
if (page == 0xb0) {
-uint32_t max_transfer =
-blk_get_max_transfer(s->conf.blk) / s->blocksize;
+uint32_t max_transfer = blk_get_max_transfer(s->conf.blk);
+uint32_t max_iov = blk_get_max_iov(s->conf.blk);
assert(max_transfer);
+max_transfer = MIN_NON_ZERO(max_transfer, max_iov *
qemu_real_host_page_size)
+/ s->blocksize;
stl_be_p(>buf[8], max_transfer);
/* Also take care of the opt xfer len. */
stl_be_p(>buf[12],
--
2.31.1
[PATCH 04/11] block-backend: align max_transfer to request alignment
Block device requests must be aligned to bs->bl.request_alignment.
It makes sense for drivers to align bs->bl.max_transfer the same
way; however when there is no specified limit, blk_get_max_transfer
just returns INT_MAX. Since the contract of the function does not
specify that INT_MAX means "no maximum", just align the outcome
of the function (whether INT_MAX or bs->bl.max_transfer) before
returning it.
Signed-off-by: Paolo Bonzini
---
block/block-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 15f1ea4288..6e37582740 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1957,12 +1957,12 @@ uint32_t blk_get_request_alignment(BlockBackend *blk)
uint32_t blk_get_max_transfer(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
-uint32_t max = 0;
+uint32_t max = INT_MAX;
if (bs) {
-max = bs->bl.max_transfer;
+max = MIN_NON_ZERO(max, bs->bl.max_transfer);
}
-return MIN_NON_ZERO(max, INT_MAX);
+return ROUND_DOWN(max, blk_get_request_alignment(blk));
}
int blk_get_max_iov(BlockBackend *blk)
--
2.31.1
[PATCH 03/11] osdep: provide ROUND_DOWN macro
osdep.h provides a ROUND_UP macro to hide bitwise operations for the
purpose of rounding a number up to a power of two; add a ROUND_DOWN
macro that does the same with truncation towards zero.
While at it, change the formatting of some comments.
Signed-off-by: Paolo Bonzini
---
include/qemu/osdep.h | 28 ++--
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 0a54bf7be8..c3656b755a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -319,11 +319,16 @@ extern "C" {
})
#endif
-/* Round number down to multiple */
+/*
+ * Round number down to multiple. Safe when m is not a power of 2 (see
+ * ROUND_DOWN for a faster version when a power of 2 is guaranteed).
+ */
#define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m))
-/* Round number up to multiple. Safe when m is not a power of 2 (see
- * ROUND_UP for a faster version when a power of 2 is guaranteed) */
+/*
+ * Round number up to multiple. Safe when m is not a power of 2 (see
+ * ROUND_UP for a faster version when a power of 2 is guaranteed).
+ */
#define QEMU_ALIGN_UP(n, m) QEMU_ALIGN_DOWN((n) + (m) - 1, (m))
/* Check if n is a multiple of m */
@@ -340,11 +345,22 @@ extern "C" {
/* Check if pointer p is n-bytes aligned */
#define QEMU_PTR_IS_ALIGNED(p, n) QEMU_IS_ALIGNED((uintptr_t)(p), (n))
-/* Round number up to multiple. Requires that d be a power of 2 (see
+/*
+ * Round number down to multiple. Requires that d be a power of 2 (see
* QEMU_ALIGN_UP for a safer but slower version on arbitrary
- * numbers); works even if d is a smaller type than n. */
+ * numbers); works even if d is a smaller type than n.
+ */
+#ifndef ROUND_DOWN
+#define ROUND_DOWN(n, d) ((n) & -(0 ? (n) : (d)))
+#endif
+
+/*
+ * Round number up to multiple. Requires that d be a power of 2 (see
+ * QEMU_ALIGN_UP for a safer but slower version on arbitrary
+ * numbers); works even if d is a smaller type than n.
+ */
#ifndef ROUND_UP
-#define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))
+#define ROUND_UP(n, d) ROUND_DOWN((n) + (d) - 1, (d))
#endif
#ifndef DIV_ROUND_UP
--
2.31.1
[PATCH 01/11] file-posix: fix max_iov for /dev/sg devices
Even though it was only called for devices that have bs->sg set (which
must be character devices), sg_get_max_segments looked at /sys/dev/block
which only works for block devices.
On Linux the sg driver has its own way to provide the maximum number of
iovecs in a scatter/gather list, so add support for it. The block device
path is kept because it will be reinstated in the next patches.
Signed-off-by: Paolo Bonzini
Reviewed-by: Max Reitz
---
block/file-posix.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index b3fbb9bd63..b8dc19ce1a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1178,6 +1178,17 @@ static int sg_get_max_segments(int fd)
goto out;
}
+if (S_ISCHR(st.st_mode)) {
+if (ioctl(fd, SG_GET_SG_TABLESIZE, ) == 0) {
+return ret;
+}
+return -ENOTSUP;
+}
+
+if (!S_ISBLK(st.st_mode)) {
+return -ENOTSUP;
+}
+
sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
major(st.st_rdev), minor(st.st_rdev));
sysfd = open(sysfspath, O_RDONLY);
--
2.31.1
Re: [PATCH v4 00/34] modules: add meta-data database
* Gerd Hoffmann (kra...@redhat.com) wrote: > On Thu, Jun 24, 2021 at 04:01:25PM +0100, Dr. David Alan Gilbert wrote: > > * Gerd Hoffmann (kra...@redhat.com) wrote: > > > This patch series adds support for module meta-data. Today this is > > > either hard-coded in qemu (see qemu_load_module_for_opts) or handled > > > with manually maintained lists in util/module (see module_deps[] and > > > qom_modules[]). This series replaced that scheme with annotation > > > macros, so the meta-data can go into the module source code and -- for > > > example -- the module_obj() annotations can go next to the TypeInfo > > > struct for the object class. > > > > So this is slightly off-topic for the series; but kind of relevant, > > but... > > Is there a way to inhibit module loading after a given point? > > We could block loading after machine initialization. > Has implications for hotplug though. Yes; I was thinking perhaps a command to explicitly disable autoloading if people worried about it. > > I ask, because there's a fairly well known security escalation that > > takes advantage of NSS loading of PAM modules; typically you have > > your nice sandboxed application, you write out your nasty .so into the > > sandbox and then somehow get your application to trigger the PAM module > > load. > > Now, what stops the same attack here? > > Placing a new .so at some random directory wouldn't work, qemu only > loads modules from the search path (but I guess the same is true for > pam). Yes, I'm failing to find the CVE I vaguely remember about the details of how it was messed up. Dave > With this patch series applied all modules are listed the in modinfo.c > database (even if we don't have any metadata about them), so we could > easily limit loading to modules known at compile time. Not sure how > much that alone would improve security though, when the attacker is able > to write to the qemu module directory it isn't much of a problem to just > overwrite one of the existing modules. > > We could try work with hashes or signatures stored in modinfo ... > > take care, > Gerd > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v4 30/34] monitor: allow register hmp commands
On Thu, Jun 24, 2021 at 03:55:29PM +0100, Dr. David Alan Gilbert wrote:
> * Gerd Hoffmann (kra...@redhat.com) wrote:
> > Allow commands having a NULL cmd pointer, add a function to set the
> > pointer later. Use case: allow modules implement hmp commands.
> >
> > Signed-off-by: Gerd Hoffmann
>
> So this is OK, so
>
> Acked-by: Dr. David Alan Gilbert
>
> however, I can imagine:
> a) Auto load as you suggest
Not sure about that. The tcg monitor commands are pointless when you
picked another accelerator, and "info usbhost" would probably also be
most useful when trouble-shooting usb-host issues. That's why I left
it as FIXME question for now. But can certainly be done, we can add
something along the lines of 'module_hmp("info usbhost");' to the
meta-data database and autoload based on that (or use it for more
verbose error messages).
> c) Don't actually define the command in the tables at all; make
> the module actually add the command to the table.
Another possible approach. I don't see a need for modules to expand the
list of commands though, so I only set the function pointer for existing
table entries ...
take care,
Gerd
Re: [PATCH v4 00/34] modules: add meta-data database
On Thu, Jun 24, 2021 at 04:01:25PM +0100, Dr. David Alan Gilbert wrote: > * Gerd Hoffmann (kra...@redhat.com) wrote: > > This patch series adds support for module meta-data. Today this is > > either hard-coded in qemu (see qemu_load_module_for_opts) or handled > > with manually maintained lists in util/module (see module_deps[] and > > qom_modules[]). This series replaced that scheme with annotation > > macros, so the meta-data can go into the module source code and -- for > > example -- the module_obj() annotations can go next to the TypeInfo > > struct for the object class. > > So this is slightly off-topic for the series; but kind of relevant, > but... > Is there a way to inhibit module loading after a given point? We could block loading after machine initialization. Has implications for hotplug though. > I ask, because there's a fairly well known security escalation that > takes advantage of NSS loading of PAM modules; typically you have > your nice sandboxed application, you write out your nasty .so into the > sandbox and then somehow get your application to trigger the PAM module > load. > Now, what stops the same attack here? Placing a new .so at some random directory wouldn't work, qemu only loads modules from the search path (but I guess the same is true for pam). With this patch series applied all modules are listed the in modinfo.c database (even if we don't have any metadata about them), so we could easily limit loading to modules known at compile time. Not sure how much that alone would improve security though, when the attacker is able to write to the qemu module directory it isn't much of a problem to just overwrite one of the existing modules. We could try work with hashes or signatures stored in modinfo ... take care, Gerd
Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again
> On Jun 24, 2021, at 4:56 AM, Peter Maydell wrote: > > On Thu, 24 Jun 2021 at 11:27, Tom Yan wrote: >> I really think we should get (/ have gotten) things clear first. What >> exactly is the bug we have been talking about here? I mean like, where >> does it occur and what's the nature of it. >> >> 1. Is it specific to a certain type / model of backend / physical >> storage device that will be made use of by qemu for the emulated >> storage? (I presume not since you mention about image, unless you >> irrationally related/bound the emulated storage type and the physical >> storage type together.) >> >> 2. Does it have anything to do with a certain flaw in qemu itself? >> Like the code that does read/write operation is flawed that it cannot >> be handled by a certain *proper* backend device? >> >> 3. Or is it actually a bug in a certain driver / firmware blob that >> will be used by an *emulated* device in the guest? In that case, can >> we emulate another model so that it won't be using the problematic >> driver / firmware? > > Definitely agreed -- before we start changing QEMU code we need > to identify clearly (a) what the real hardware does and (b) what > the situation was we were originally trying to fix. Real SD and MMC cards do not have power-of-two constraints in the number of blocks. None of the SD/MMC bridges I’ve ever dealt with have a power-of-two constraint on the size of the card. The only constraint on the size related to the card is the block size. If non-power-of-2 sized cards fail, that’s a cat-1 sev-1 bug that needs to be fixed, rather than a warning be generated. Warner signature.asc Description: Message signed with OpenPGP
Re: [PATCH v4 00/34] modules: add meta-data database
* Gerd Hoffmann (kra...@redhat.com) wrote: > This patch series adds support for module meta-data. Today this is > either hard-coded in qemu (see qemu_load_module_for_opts) or handled > with manually maintained lists in util/module (see module_deps[] and > qom_modules[]). This series replaced that scheme with annotation > macros, so the meta-data can go into the module source code and -- for > example -- the module_obj() annotations can go next to the TypeInfo > struct for the object class. So this is slightly off-topic for the series; but kind of relevant, but... Is there a way to inhibit module loading after a given point? I ask, because there's a fairly well known security escalation that takes advantage of NSS loading of PAM modules; typically you have your nice sandboxed application, you write out your nasty .so into the sandbox and then somehow get your application to trigger the PAM module load. Now, what stops the same attack here? Dave > Patches 1-3 put the infrastructure in place: Add the annotation macros, > add a script to collect the meta-data, add a script to compile the > meta-data into C source code which we can then add to qemu. > > Patch 4 - check module dependencies (Jose, new in v4). > > Patches 5-13 add annotations macros to the modules we have. > > Patches 14-16 put the modinfo database into use and remove the > module_deps[] and qom_modules[] lists. > > Patch 16 adds two tracepoints for easier trouble-shooting. > > Patches 18-20 add support for target-specific modules. > > Patches 21-24 add documentation for all of the above (new in v4, was > separate series). > > Patches 25-29 start building accelerators modular. So far it is > only qtest (all archs) and a small fraction of tcg (x86 only). > > Patches 30-34 add support for registering hmp commands so they can > be implemented as module (new in v4, was separate series). > > take care, > Gerd > > Gerd Hoffmann (33): > modules: add modinfo macros > modules: collect module meta-data > modules: generate modinfo.c > modules: add qxl module annotations > modules: add virtio-gpu module annotations > modules: add chardev module annotations > modules: add audio module annotations > modules: add usb-redir module annotations > modules: add ccid module annotations > modules: add ui module annotations > modules: add s390x module annotations > modules: add block module annotations > modules: use modinfo for dependencies > modules: use modinfo for qom load > modules: use modinfo for qemu opts load > modules: add tracepoints > modules: check arch and block load on mismatch > modules: check arch on qom lookup > modules: target-specific module build infrastructure > modules: add documentation for module sourcesets > modules: add module_obj() note to QOM docs > modules: module.h kerneldoc annotations > modules: hook up modules.h to docs build > accel: autoload modules > accel: add qtest module annotations > accel: build qtest modular > accel: add tcg module annotations > accel: build tcg modular > monitor: allow register hmp commands > usb: drop usb_host_dev_is_scsi_storage hook > monitor/usb: register 'info usbhost' dynamically > usb: build usb-host as module > monitor/tcg: move tcg hmp commands to accel/tcg, register them > dynamically > > Jose R. Ziviani (1): > modules: check if all dependencies can be satisfied > > scripts/modinfo-collect.py | 67 +++ > scripts/modinfo-generate.py | 97 > include/hw/usb.h| 7 +- > include/monitor/monitor.h | 3 + > include/qemu/module.h | 74 > accel/accel-common.c| 2 +- > accel/accel-softmmu.c | 2 +- > accel/qtest/qtest.c | 2 + > accel/tcg/hmp.c | 29 + > accel/tcg/tcg-accel-ops.c | 1 + > accel/tcg/tcg-all.c | 1 + > audio/spiceaudio.c | 2 + > block/iscsi-opts.c | 1 + > chardev/baum.c | 1 + > chardev/spice.c | 4 + > hw/display/qxl.c| 4 + > hw/display/vhost-user-gpu-pci.c | 1 + > hw/display/vhost-user-gpu.c | 1 + > hw/display/vhost-user-vga.c | 1 + > hw/display/virtio-gpu-base.c| 1 + > hw/display/virtio-gpu-gl.c | 3 + > hw/display/virtio-gpu-pci-gl.c | 3 + > hw/display/virtio-gpu-pci.c | 2 + > hw/display/virtio-gpu.c | 1 + > hw/display/virtio-vga-gl.c | 3 + > hw/display/virtio-vga.c | 2 + > hw/ppc/spapr.c | 2 +- > hw/s390x/virtio-ccw-gpu.c | 3 + > hw/usb/ccid-card-emulated.c | 1 + > hw/usb/ccid-card-passthru.c | 1 + > hw/usb/dev-storage-bot.c| 1 + > hw/usb/dev-storage-classic.c| 1 + > hw/usb/dev-uas.c| 1 + > hw/usb/host-libusb.c| 38 ++ > hw/usb/host-stub.c | 45 --- >
Re: [PATCH v4 34/34] monitor/tcg: move tcg hmp commands to accel/tcg, register them dynamically
* Gerd Hoffmann (kra...@redhat.com) wrote:
> One more little step towards modular tcg ...
>
> Signed-off-by: Gerd Hoffmann
Acked-by: Dr. David Alan Gilbert
> ---
> accel/tcg/hmp.c | 29 +
> monitor/misc.c| 18 --
> accel/tcg/meson.build | 1 +
> hmp-commands-info.hx | 2 --
> 4 files changed, 30 insertions(+), 20 deletions(-)
> create mode 100644 accel/tcg/hmp.c
>
> diff --git a/accel/tcg/hmp.c b/accel/tcg/hmp.c
> new file mode 100644
> index ..a6e72fdb3ed6
> --- /dev/null
> +++ b/accel/tcg/hmp.c
> @@ -0,0 +1,29 @@
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "exec/exec-all.h"
> +#include "monitor/monitor.h"
> +#include "sysemu/tcg.h"
> +
> +static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> +{
> +if (!tcg_enabled()) {
> +error_report("JIT information is only available with accel=tcg");
> +return;
> +}
> +
> +dump_exec_info();
> +dump_drift_info();
> +}
> +
> +static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
> +{
> +dump_opcount_info();
> +}
> +
> +static void hmp_tcg_register(void)
> +{
> +monitor_register_hmp("jit", true, hmp_info_jit);
> +monitor_register_hmp("opcount", true, hmp_info_opcount);
> +}
> +
> +type_init(hmp_tcg_register);
> diff --git a/monitor/misc.c b/monitor/misc.c
> index ad476c6e51ea..b28874d6dc59 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -320,24 +320,6 @@ static void hmp_info_registers(Monitor *mon, const QDict
> *qdict)
> }
> }
>
> -#ifdef CONFIG_TCG
> -static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> -{
> -if (!tcg_enabled()) {
> -error_report("JIT information is only available with accel=tcg");
> -return;
> -}
> -
> -dump_exec_info();
> -dump_drift_info();
> -}
> -
> -static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
> -{
> -dump_opcount_info();
> -}
> -#endif
> -
> static void hmp_info_sync_profile(Monitor *mon, const QDict *qdict)
> {
> int64_t max = qdict_get_try_int(qdict, "max", 10);
> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
> index 0ae9180282e3..137a1a44cc0a 100644
> --- a/accel/tcg/meson.build
> +++ b/accel/tcg/meson.build
> @@ -15,6 +15,7 @@ specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_ss)
>
> specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
>'cputlb.c',
> + 'hmp.c',
> ))
>
> tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ce42aef47acb..27206ac049df 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -274,7 +274,6 @@ ERST
> .args_type = "",
> .params = "",
> .help = "show dynamic compiler info",
> -.cmd= hmp_info_jit,
> },
> #endif
>
> @@ -289,7 +288,6 @@ ERST
> .args_type = "",
> .params = "",
> .help = "show dynamic compiler opcode counters",
> -.cmd= hmp_info_opcount,
> },
> #endif
>
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v4 30/34] monitor: allow register hmp commands
* Gerd Hoffmann (kra...@redhat.com) wrote:
> Allow commands having a NULL cmd pointer, add a function to set the
> pointer later. Use case: allow modules implement hmp commands.
>
> Signed-off-by: Gerd Hoffmann
So this is OK, so
Acked-by: Dr. David Alan Gilbert
however, I can imagine:
a) Auto load as you suggest
b) Making the error better 'Command ... needs module '
c) Don't actually define the command in the tables at all; make
the module actually add the command to the table.
Dave
> ---
> include/monitor/monitor.h | 3 +++
> monitor/hmp.c | 7 +++
> monitor/misc.c| 16
> 3 files changed, 26 insertions(+)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1211d6e6d69f..1a8a369b50b2 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -51,4 +51,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
> void monitor_fdset_dup_fd_remove(int dup_fd);
> int64_t monitor_fdset_dup_fd_find(int dup_fd);
>
> +void monitor_register_hmp(const char *name, bool info,
> + void (*cmd)(Monitor *mon, const QDict *qdict));
> +
> #endif /* MONITOR_H */
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 6c0b33a0b19d..d50c3124e1e1 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1089,6 +1089,13 @@ void handle_hmp_command(MonitorHMP *mon, const char
> *cmdline)
> return;
> }
>
> +if (!cmd->cmd) {
> +/* FIXME: is it useful to try autoload modules here ??? */
> +monitor_printf(>common, "Command \"%.*s\" is not available.\n",
> + (int)(cmdline - cmd_start), cmd_start);
> +return;
> +}
> +
> qdict = monitor_parse_arguments(>common, , cmd);
> if (!qdict) {
> while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 1539e18557f0..ad476c6e51ea 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -1974,6 +1974,22 @@ static void sortcmdlist(void)
>compare_mon_cmd);
> }
>
> +void monitor_register_hmp(const char *name, bool info,
> + void (*cmd)(Monitor *mon, const QDict *qdict))
> +{
> +HMPCommand *table = info ? hmp_info_cmds : hmp_cmds;
> +
> +while (table->name != NULL) {
> +if (strcmp(table->name, name) == 0) {
> +g_assert(table->cmd == NULL);
> +table->cmd = cmd;
> +return;
> +}
> +table++;
> +}
> +g_assert_not_reached();
> +}
> +
> void monitor_init_globals(void)
> {
> monitor_init_globals_core();
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v4 00/34] modules: add meta-data database
Hello Gerd, Reviewed and tested successfully here. Thank you! Reviewed-by: Jose R. Ziviani On Thu, Jun 24, 2021 at 12:38:02PM +0200, Gerd Hoffmann wrote: > This patch series adds support for module meta-data. Today this is > either hard-coded in qemu (see qemu_load_module_for_opts) or handled > with manually maintained lists in util/module (see module_deps[] and > qom_modules[]). This series replaced that scheme with annotation > macros, so the meta-data can go into the module source code and -- for > example -- the module_obj() annotations can go next to the TypeInfo > struct for the object class. > > Patches 1-3 put the infrastructure in place: Add the annotation macros, > add a script to collect the meta-data, add a script to compile the > meta-data into C source code which we can then add to qemu. > > Patch 4 - check module dependencies (Jose, new in v4). > > Patches 5-13 add annotations macros to the modules we have. > > Patches 14-16 put the modinfo database into use and remove the > module_deps[] and qom_modules[] lists. > > Patch 16 adds two tracepoints for easier trouble-shooting. > > Patches 18-20 add support for target-specific modules. > > Patches 21-24 add documentation for all of the above (new in v4, was > separate series). > > Patches 25-29 start building accelerators modular. So far it is > only qtest (all archs) and a small fraction of tcg (x86 only). > > Patches 30-34 add support for registering hmp commands so they can > be implemented as module (new in v4, was separate series). > > take care, > Gerd > > Gerd Hoffmann (33): > modules: add modinfo macros > modules: collect module meta-data > modules: generate modinfo.c > modules: add qxl module annotations > modules: add virtio-gpu module annotations > modules: add chardev module annotations > modules: add audio module annotations > modules: add usb-redir module annotations > modules: add ccid module annotations > modules: add ui module annotations > modules: add s390x module annotations > modules: add block module annotations > modules: use modinfo for dependencies > modules: use modinfo for qom load > modules: use modinfo for qemu opts load > modules: add tracepoints > modules: check arch and block load on mismatch > modules: check arch on qom lookup > modules: target-specific module build infrastructure > modules: add documentation for module sourcesets > modules: add module_obj() note to QOM docs > modules: module.h kerneldoc annotations > modules: hook up modules.h to docs build > accel: autoload modules > accel: add qtest module annotations > accel: build qtest modular > accel: add tcg module annotations > accel: build tcg modular > monitor: allow register hmp commands > usb: drop usb_host_dev_is_scsi_storage hook > monitor/usb: register 'info usbhost' dynamically > usb: build usb-host as module > monitor/tcg: move tcg hmp commands to accel/tcg, register them > dynamically > > Jose R. Ziviani (1): > modules: check if all dependencies can be satisfied > > scripts/modinfo-collect.py | 67 +++ > scripts/modinfo-generate.py | 97 > include/hw/usb.h| 7 +- > include/monitor/monitor.h | 3 + > include/qemu/module.h | 74 > accel/accel-common.c| 2 +- > accel/accel-softmmu.c | 2 +- > accel/qtest/qtest.c | 2 + > accel/tcg/hmp.c | 29 + > accel/tcg/tcg-accel-ops.c | 1 + > accel/tcg/tcg-all.c | 1 + > audio/spiceaudio.c | 2 + > block/iscsi-opts.c | 1 + > chardev/baum.c | 1 + > chardev/spice.c | 4 + > hw/display/qxl.c| 4 + > hw/display/vhost-user-gpu-pci.c | 1 + > hw/display/vhost-user-gpu.c | 1 + > hw/display/vhost-user-vga.c | 1 + > hw/display/virtio-gpu-base.c| 1 + > hw/display/virtio-gpu-gl.c | 3 + > hw/display/virtio-gpu-pci-gl.c | 3 + > hw/display/virtio-gpu-pci.c | 2 + > hw/display/virtio-gpu.c | 1 + > hw/display/virtio-vga-gl.c | 3 + > hw/display/virtio-vga.c | 2 + > hw/ppc/spapr.c | 2 +- > hw/s390x/virtio-ccw-gpu.c | 3 + > hw/usb/ccid-card-emulated.c | 1 + > hw/usb/ccid-card-passthru.c | 1 + > hw/usb/dev-storage-bot.c| 1 + > hw/usb/dev-storage-classic.c| 1 + > hw/usb/dev-uas.c| 1 + > hw/usb/host-libusb.c| 38 ++ > hw/usb/host-stub.c | 45 --- > hw/usb/redirect.c | 1 + > monitor/hmp.c | 7 ++ > monitor/misc.c | 34 +++--- > softmmu/vl.c| 24 ++-- > stubs/module-opts.c | 4 - > ui/egl-headless.c | 4 + > ui/gtk.c| 4 + > ui/sdl2.c | 4 + >
[RFC PATCH 10/10] hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 70dde5ae0b8..43a59b34ee8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1010,6 +1010,20 @@ static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd,
SDRequest req)
return sd_r2_i;
}
+static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
+{
+switch (sd->state) {
+case sd_identification_state:
+case sd_standby_state:
+sd->state = sd_standby_state;
+sd_set_rca(sd);
+return sd_r6;
+
+default:
+return sd_invalid_state_for_cmd(sd, req);
+}
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1049,19 +1063,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (req.cmd) {
/* Basic commands (Class 0 and Class 1) */
-case 3:/* CMD3: SEND_RELATIVE_ADDR */
-switch (sd->state) {
-case sd_identification_state:
-case sd_standby_state:
-sd->state = sd_standby_state;
-sd_set_rca(sd);
-return sd_r6;
-
-default:
-break;
-}
-break;
-
case 4:/* CMD4: SEND_DSR */
switch (sd->state) {
case sd_standby_state:
@@ -2106,6 +2107,7 @@ static const SDProto sd_proto_sd = {
[0] = sd_cmd_GO_IDLE_STATE,
[1] = sd_cmd_illegal,
[2] = sd_cmd_ALL_SEND_CID,
+[3] = sd_cmd_SEND_RELATIVE_ADDR,
[5] = sd_cmd_illegal,
[52 ... 54] = sd_cmd_illegal,
[58]= sd_cmd_illegal,
--
2.31.1
[RFC PATCH 03/10] hw/sd: Move proto_name to SDProto structure
Introduce a new structure to hold the bus protocol specific
fields: SDProto. The first field is the protocol name.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d71ec81c22a..a1cc8ab0be8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -88,6 +88,10 @@ enum SDCardStates {
sd_disconnect_state,
};
+typedef struct SDProto {
+const char *name;
+} SDProto;
+
struct SDState {
DeviceState parent_obj;
@@ -112,6 +116,7 @@ struct SDState {
/* Runtime changeables */
+const SDProto *proto; /* Bus protocol */
uint32_t mode;/* current card mode, one of SDCardModes */
int32_t state;/* current card state, one of SDCardStates */
uint32_t vhs;
@@ -138,7 +143,6 @@ struct SDState {
qemu_irq readonly_cb;
qemu_irq inserted_cb;
QEMUTimer *ocr_power_timer;
-const char *proto_name;
bool enable;
uint8_t dat_lines;
bool cmd_line;
@@ -951,8 +955,8 @@ static bool address_in_range(SDState *sd, const char *desc,
static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
{
-qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
- req.cmd, sd_state_name(sd->state));
+qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong state: %s\n",
+ sd->proto->name, req.cmd, sd_state_name(sd->state));
return sd_illegal;
}
@@ -966,7 +970,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
* However there is no ACMD55, so we want to trace this particular case.
*/
if (req.cmd != 55 || sd->expecting_acmd) {
-trace_sdcard_normal_command(sd->proto_name,
+trace_sdcard_normal_command(sd->proto->name,
sd_cmd_name(req.cmd), req.cmd,
req.arg, sd_state_name(sd->state));
}
@@ -1526,7 +1530,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_app_command(SDState *sd,
SDRequest req)
{
-trace_sdcard_app_command(sd->proto_name, sd_acmd_name(req.cmd),
+trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
req.cmd, req.arg, sd_state_name(sd->state));
sd->card_status |= APP_CMD;
switch (req.cmd) {
@@ -1820,7 +1824,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
return;
-trace_sdcard_write_data(sd->proto_name,
+trace_sdcard_write_data(sd->proto->name,
sd_acmd_name(sd->current_cmd),
sd->current_cmd, value);
switch (sd->current_cmd) {
@@ -1976,7 +1980,7 @@ uint8_t sd_read_byte(SDState *sd)
io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
-trace_sdcard_read_data(sd->proto_name,
+trace_sdcard_read_data(sd->proto->name,
sd_acmd_name(sd->current_cmd),
sd->current_cmd, io_len);
switch (sd->current_cmd) {
@@ -2095,6 +2099,14 @@ void sd_enable(SDState *sd, bool enable)
sd->enable = enable;
}
+static const SDProto sd_proto_spi = {
+.name = "SPI",
+};
+
+static const SDProto sd_proto_sd = {
+.name = "SD",
+};
+
static void sd_instance_init(Object *obj)
{
SDState *sd = SD_CARD(obj);
@@ -2115,7 +2127,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
SDState *sd = SD_CARD(dev);
int ret;
-sd->proto_name = sd->spi ? "SPI" : "SD";
+sd->proto = sd->spi ? _proto_spi : _proto_sd;
switch (sd->spec_version) {
case SD_PHY_SPECv1_10_VERS
--
2.31.1
[RFC PATCH 09/10] hw/sd: Add sd_cmd_ALL_SEND_CID() handler
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b63d99f7da0..70dde5ae0b8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -999,6 +999,17 @@ static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd,
SDRequest req)
return sd_r1;
}
+static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_ready_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+sd->state = sd_identification_state;
+
+return sd_r2_i;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1038,17 +1049,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (req.cmd) {
/* Basic commands (Class 0 and Class 1) */
-case 2:/* CMD2: ALL_SEND_CID */
-switch (sd->state) {
-case sd_ready_state:
-sd->state = sd_identification_state;
-return sd_r2_i;
-
-default:
-break;
-}
-break;
-
case 3:/* CMD3: SEND_RELATIVE_ADDR */
switch (sd->state) {
case sd_identification_state:
@@ -2105,6 +2105,7 @@ static const SDProto sd_proto_sd = {
.cmd = {
[0] = sd_cmd_GO_IDLE_STATE,
[1] = sd_cmd_illegal,
+[2] = sd_cmd_ALL_SEND_CID,
[5] = sd_cmd_illegal,
[52 ... 54] = sd_cmd_illegal,
[58]= sd_cmd_illegal,
--
2.31.1
[RFC PATCH 02/10] hw/sd: Extract address_in_range() helper, log invalid accesses
Multiple commands have to check the address requested is valid.
Extract this code pattern as a new address_in_range() helper, and
log invalid accesses as guest errors.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 288ae059e3d..d71ec81c22a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -937,6 +937,18 @@ static void sd_lock_command(SDState *sd)
sd->card_status &= ~CARD_IS_LOCKED;
}
+static bool address_in_range(SDState *sd, const char *desc,
+ uint64_t addr, uint32_t length)
+{
+if (addr + length > sd->size) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s offset %lu > card %lu [%%%u]\n",
+ desc, addr, sd->size, length);
+sd->card_status |= ADDRESS_ERROR;
+return false;
+}
+return true;
+}
+
static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
{
qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
@@ -1226,8 +1238,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (sd->state) {
case sd_transfer_state:
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "READ_BLOCK", addr, sd->blk_len)) {
return sd_r1;
}
@@ -1272,8 +1283,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (sd->state) {
case sd_transfer_state:
-if (addr + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "WRITE_BLOCK", addr, sd->blk_len)) {
return sd_r1;
}
@@ -1333,8 +1343,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (sd->state) {
case sd_transfer_state:
-if (addr >= sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
return sd_r1b;
}
@@ -1356,8 +1365,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (sd->state) {
case sd_transfer_state:
-if (addr >= sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
return sd_r1b;
}
@@ -1832,8 +1840,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
if (sd->data_offset == 0) {
/* Start of the block - let's check the address is valid */
-if (sd->data_start + sd->blk_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
+ sd->data_start, sd->blk_len)) {
break;
}
if (sd->size <= SDSC_MAX_CAPACITY) {
@@ -2005,8 +2013,8 @@ uint8_t sd_read_byte(SDState *sd)
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
if (sd->data_offset == 0) {
-if (sd->data_start + io_len > sd->size) {
-sd->card_status |= ADDRESS_ERROR;
+if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
+ sd->data_start, io_len)) {
return 0x00;
}
BLK_READ_BLOCK(sd->data_start, io_len);
--
2.31.1
[RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type
Add 2 command handler arrays in SDProto, for CMD and ACMD.
Have sd_normal_command() / sd_app_command() use these arrays:
if an command handler is registered, call it, otherwise fall
back to current code base.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a1cc8ab0be8..ce1eec0374f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -88,8 +88,12 @@ enum SDCardStates {
sd_disconnect_state,
};
+typedef sd_rsp_type_t (*sd_cmd_handler)(SDState *sd, SDRequest req);
+
typedef struct SDProto {
const char *name;
+sd_cmd_handler cmd[SDMMC_CMD_MAX];
+sd_cmd_handler acmd[SDMMC_CMD_MAX];
} SDProto;
struct SDState {
@@ -994,6 +998,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_illegal;
}
+if (sd->proto->cmd[req.cmd]) {
+return sd->proto->cmd[req.cmd](sd, req);
+}
+
switch (req.cmd) {
/* Basic commands (Class 0 and Class 1) */
case 0:/* CMD0: GO_IDLE_STATE */
@@ -1533,6 +1541,11 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
req.cmd, req.arg, sd_state_name(sd->state));
sd->card_status |= APP_CMD;
+
+if (sd->proto->acmd[req.cmd]) {
+return sd->proto->acmd[req.cmd](sd, req);
+}
+
switch (req.cmd) {
case 6:/* ACMD6: SET_BUS_WIDTH */
if (sd->spi) {
--
2.31.1
[RFC PATCH 08/10] hw/sd: Add sd_cmd_SEND_OP_CMD() handler
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3ef6aca89da..b63d99f7da0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -992,6 +992,13 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd,
SDRequest req)
return sd->spi ? sd_r1 : sd_r0;
}
+static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
+{
+sd->state = sd_transfer_state;
+
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1031,10 +1038,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (req.cmd) {
/* Basic commands (Class 0 and Class 1) */
-case 1:/* CMD1: SEND_OP_CMD */
-sd->state = sd_transfer_state;
-return sd_r1;
-
case 2:/* CMD2: ALL_SEND_CID */
switch (sd->state) {
case sd_ready_state:
@@ -1579,11 +1582,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
break;
case 41: /* ACMD41: SD_APP_OP_COND */
-if (sd->spi) {
-/* SEND_OP_CMD */
-sd->state = sd_transfer_state;
-return sd_r1;
-}
if (sd->state != sd_idle_state) {
break;
}
@@ -2088,6 +2086,7 @@ static const SDProto sd_proto_spi = {
.name = "SPI",
.cmd = {
[0] = sd_cmd_GO_IDLE_STATE,
+[1] = sd_cmd_SEND_OP_CMD,
[2 ... 4] = sd_cmd_illegal,
[5] = sd_cmd_illegal,
[7] = sd_cmd_illegal,
@@ -2097,6 +2096,7 @@ static const SDProto sd_proto_spi = {
},
.cmd = {
[6] = sd_cmd_unimplemented,
+[41]= sd_cmd_SEND_OP_CMD,
},
};
--
2.31.1
[RFC PATCH 07/10] hw/sd: Add sd_cmd_GO_IDLE_STATE() handler
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2647fd26566..3ef6aca89da 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -982,6 +982,16 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd,
SDRequest req)
return sd_illegal;
}
+static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_inactive_state) {
+sd->state = sd_idle_state;
+sd_reset(DEVICE(sd));
+}
+
+return sd->spi ? sd_r1 : sd_r0;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1021,18 +1031,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
switch (req.cmd) {
/* Basic commands (Class 0 and Class 1) */
-case 0:/* CMD0: GO_IDLE_STATE */
-switch (sd->state) {
-case sd_inactive_state:
-return sd->spi ? sd_r1 : sd_r0;
-
-default:
-sd->state = sd_idle_state;
-sd_reset(DEVICE(sd));
-return sd->spi ? sd_r1 : sd_r0;
-}
-break;
-
case 1:/* CMD1: SEND_OP_CMD */
sd->state = sd_transfer_state;
return sd_r1;
@@ -2089,6 +2087,7 @@ void sd_enable(SDState *sd, bool enable)
static const SDProto sd_proto_spi = {
.name = "SPI",
.cmd = {
+[0] = sd_cmd_GO_IDLE_STATE,
[2 ... 4] = sd_cmd_illegal,
[5] = sd_cmd_illegal,
[7] = sd_cmd_illegal,
@@ -2104,6 +2103,7 @@ static const SDProto sd_proto_spi = {
static const SDProto sd_proto_sd = {
.name = "SD",
.cmd = {
+[0] = sd_cmd_GO_IDLE_STATE,
[1] = sd_cmd_illegal,
[5] = sd_cmd_illegal,
[52 ... 54] = sd_cmd_illegal,
--
2.31.1
[RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0215bdb3689..2647fd26566 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -973,6 +973,15 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest
req)
return sd_illegal;
}
+/* Commands that are recognised but not yet implemented. */
+static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
+{
+qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
+ sd->proto->name, req.cmd);
+
+return sd_illegal;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1522,9 +1531,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
switch (req.cmd) {
case 6:/* ACMD6: SET_BUS_WIDTH */
-if (sd->spi) {
-goto unimplemented_spi_cmd;
-}
switch (sd->state) {
case sd_transfer_state:
sd->sd_status[0] &= 0x3f;
@@ -1655,12 +1661,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
default:
/* Fall back to standard commands. */
return sd_normal_command(sd, req);
-
-unimplemented_spi_cmd:
-/* Commands that are recognised but not yet implemented in SPI mode.
*/
-qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
- req.cmd);
-return sd_illegal;
}
qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
@@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
[26]= sd_cmd_illegal,
[52 ... 54] = sd_cmd_illegal,
},
+.cmd = {
+[6] = sd_cmd_unimplemented,
+},
};
static const SDProto sd_proto_sd = {
--
2.31.1
[RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler
Log illegal commands as GUEST_ERROR.
Note: we are logging back the SDIO commands (CMD5, CMD52-54).
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 57 ++
1 file changed, 23 insertions(+), 34 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ce1eec0374f..0215bdb3689 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd,
SDRequest req)
return sd_illegal;
}
+static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
+{
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
+ sd->proto->name, req.cmd);
+
+return sd_illegal;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 1:/* CMD1: SEND_OP_CMD */
-if (!sd->spi)
-goto bad_cmd;
-
sd->state = sd_transfer_state;
return sd_r1;
case 2:/* CMD2: ALL_SEND_CID */
-if (sd->spi)
-goto bad_cmd;
switch (sd->state) {
case sd_ready_state:
sd->state = sd_identification_state;
@@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 3:/* CMD3: SEND_RELATIVE_ADDR */
-if (sd->spi)
-goto bad_cmd;
switch (sd->state) {
case sd_identification_state:
case sd_standby_state:
@@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 4:/* CMD4: SEND_DSR */
-if (sd->spi)
-goto bad_cmd;
switch (sd->state) {
case sd_standby_state:
break;
@@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
}
break;
-case 5: /* CMD5: reserved for SDIO cards */
-return sd_illegal;
-
case 6:/* CMD6: SWITCH_FUNCTION */
switch (sd->mode) {
case sd_data_transfer_mode:
@@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 7:/* CMD7: SELECT/DESELECT_CARD */
-if (sd->spi)
-goto bad_cmd;
switch (sd->state) {
case sd_standby_state:
if (sd->rca != rca)
@@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 15: /* CMD15: GO_INACTIVE_STATE */
-if (sd->spi)
-goto bad_cmd;
switch (sd->mode) {
case sd_data_transfer_mode:
if (sd->rca != rca)
@@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 26: /* CMD26: PROGRAM_CID */
-if (sd->spi)
-goto bad_cmd;
switch (sd->state) {
case sd_transfer_state:
sd->state = sd_receivingdata_state;
@@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
}
break;
-case 52 ... 54:
-/* CMD52, CMD53, CMD54: reserved for SDIO cards
- * (see the SDIO Simplified Specification V2.0)
- * Handle as illegal command but do not complain
- * on stderr, as some OSes may use these in their
- * probing for presence of an SDIO card.
- */
-return sd_illegal;
-
/* Application specific commands (Class 8) */
case 55: /* CMD55: APP_CMD */
switch (sd->state) {
@@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
break;
case 58:/* CMD58: READ_OCR (SPI) */
-if (!sd->spi) {
-goto bad_cmd;
-}
return sd_r3;
case 59:/* CMD59: CRC_ON_OFF (SPI) */
-if (!sd->spi) {
-goto bad_cmd;
-}
return sd_r1;
default:
-bad_cmd:
qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
return sd_illegal;
}
@@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
static const SDProto sd_proto_spi = {
.name = "SPI",
+.cmd = {
+[2 ... 4] = sd_cmd_illegal,
+[5] = sd_cmd_illegal,
+[7] = sd_cmd_illegal,
+[15]= sd_cmd_illegal,
+[26]= sd_cmd_illegal,
+[52 ... 54] = sd_cmd_illegal,
+},
};
static const SDProto sd_proto_sd = {
.name = "SD",
+.cmd = {
+[1] = sd_cmd_illegal,
+[5] = sd_cmd_illegal,
+[52 ... 54] = sd_cmd_illegal,
+[58]= sd_cmd_illegal,
+[59]= sd_cmd_illegal,
+},
};
static void sd_instance_init(Object *obj)
--
2.31.1
[RFC PATCH 01/10] hw/sd: When card is in wrong state, log which state it is
We report the card is in an inconsistent state, but don't precise
in which state it is. Add this information, as it is useful when
debugging problems.
Since we will reuse this code, extract as sd_invalid_state_for_cmd()
helper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 282d39a7042..288ae059e3d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -937,6 +937,14 @@ static void sd_lock_command(SDState *sd)
sd->card_status &= ~CARD_IS_LOCKED;
}
+static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
+{
+qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
+ req.cmd, sd_state_name(sd->state));
+
+return sd_illegal;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint32_t rca = 0x;
@@ -1504,8 +1512,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_illegal;
}
-qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
-return sd_illegal;
+return sd_invalid_state_for_cmd(sd, req);
}
static sd_rsp_type_t sd_app_command(SDState *sd,
--
2.31.1
[RFC PATCH 00/10] hw/sd: Start splitting SD vs SPI protocols
Hi Cédric, After our discussion yesterday about how to add support for MMC (and eMMC) I looked at how to easily add these bus protocols, which might have commands quite different, avoiding to have big unreadable if/else statements. I'm not yet happy enough with the result but it is a starting point which keeps things still simple. What I'm wondering is if we could include the command classes (as another dimension in the array). Also if we could use the older spec version supported as base set of commands, and if the user asks for more recent spec version, for each version we overwrite the array of commands. Thoughts? Phil. Philippe Mathieu-Daudé (10): hw/sd: When card is in wrong state, log which state it is hw/sd: Extract address_in_range() helper, log invalid accesses hw/sd: Move proto_name to SDProto structure hw/sd: Introduce sd_cmd_handler type hw/sd: Add sd_cmd_illegal() handler hw/sd: Add sd_cmd_unimplemented() handler hw/sd: Add sd_cmd_GO_IDLE_STATE() handler hw/sd: Add sd_cmd_SEND_OP_CMD() handler hw/sd: Add sd_cmd_ALL_SEND_CID() handler hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler hw/sd/sd.c | 251 ++--- 1 file changed, 143 insertions(+), 108 deletions(-) -- 2.31.1
Re: [RFC] block/mirror: fix file-system went to read-only after block-mirror
Patchew URL: https://patchew.org/QEMU/20210624120635.54573-1-caojinh...@huawei.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210624120635.54573-1-caojinh...@huawei.com Subject: [RFC] block/mirror: fix file-system went to read-only after block-mirror === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20210624120635.54573-1-caojinh...@huawei.com -> patchew/20210624120635.54573-1-caojinh...@huawei.com Switched to a new branch 'test' 6e2ba54 block/mirror: fix file-system went to read-only after block-mirror === OUTPUT BEGIN === ERROR: Missing Signed-off-by: line(s) total: 1 errors, 0 warnings, 19 lines checked Commit 6e2ba547f042 (block/mirror: fix file-system went to read-only after block-mirror) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20210624120635.54573-1-caojinh...@huawei.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[RFC] block/mirror: fix file-system went to read-only after block-mirror
1) Configure the VM disk as prdm.
...
...
Mount the disk in guest and keep the disk writing data continuously during
block-mirror,
the file-system went to read-only after block-mirror.
2) This commit 6cdbceb12cf[mirror: Add filter-node-name to blockdev-mirror]
introduces
mirror_top_bs which does not set default function for
mirror_top_bs->drv->bdrv_co_ioctl.
3) The function bdrv_co_ioctl in block/io.c will be called during block-mirror,
in this
function, the judgment is as follow:
---
if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
co.ret = -ENOTSUP;
goto out;
}
---
The mirror_top_bs does not set drv->bdrv_aio_ioctl or drv->bdrv_co_ioctl which
result this
return -ENOTSUP. So the file-system went to read-only after block-mirror.
4) This patch set a default function for mirror_top_bs->drv->bdrv_aio_ioctl,
fix this problem.
---
block/mirror.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index 019f6deaa5..63b788ec39 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1480,6 +1480,12 @@ static int coroutine_fn
bdrv_mirror_top_flush(BlockDriverState *bs)
return bdrv_co_flush(bs->backing->bs);
}
+static int coroutine_fn bdrv_mirror_top_ioctl(BlockDriverState *bs,
+unsigned long int req, void *buf)
+{
+return 0;
+}
+
static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes, BdrvRequestFlags flags)
{
@@ -1555,6 +1561,7 @@ static BlockDriver bdrv_mirror_top = {
.bdrv_co_pwrite_zeroes = bdrv_mirror_top_pwrite_zeroes,
.bdrv_co_pdiscard = bdrv_mirror_top_pdiscard,
.bdrv_co_flush = bdrv_mirror_top_flush,
+.bdrv_co_ioctl = bdrv_mirror_top_ioctl,
.bdrv_refresh_filename = bdrv_mirror_top_refresh_filename,
.bdrv_child_perm= bdrv_mirror_top_child_perm,
--
2.27.0
[PULL 0/1] Block patch
The following changes since commit b22726abdfa54592d6ad88f65b0297c0e8b363e2: Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-06-22 16:07:53 +0100) are available in the Git repository at: https://github.com/XanClic/qemu.git tags/pull-block-2021-06-24 for you to fetch changes up to 32a9a245d719a883eef2cbf07d2cf89efa0206d0: block/snapshot: Clarify goto fallback behavior (2021-06-24 09:49:04 +0200) Block patch: - Fix Coverity complaint in block/snapshot.c Max Reitz (1): block/snapshot: Clarify goto fallback behavior block/snapshot.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) -- 2.31.1
Re: [PATCH v2 2/6] block: block-status cache for data regions
On 24.06.21 12:05, Vladimir Sementsov-Ogievskiy wrote:
23.06.2021 18:01, Max Reitz wrote:
As we have attempted before
(https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06451.html,
"file-posix: Cache lseek result for data regions";
https://lists.nongnu.org/archive/html/qemu-block/2021-02/msg00934.html,
"file-posix: Cache next hole"), this patch seeks to reduce the number of
SEEK_DATA/HOLE operations the file-posix driver has to perform. The
main difference is that this time it is implemented as part of the
general block layer code.
The problem we face is that on some filesystems or in some
circumstances, SEEK_DATA/HOLE is unreasonably slow. Given the
implementation is outside of qemu, there is little we can do about its
performance.
We have already introduced the want_zero parameter to
bdrv_co_block_status() to reduce the number of SEEK_DATA/HOLE calls
unless we really want zero information; but sometimes we do want that
information, because for files that consist largely of zero areas,
special-casing those areas can give large performance boosts. So the
real problem is with files that consist largely of data, so that
inquiring the block status does not gain us much performance, but where
such an inquiry itself takes a lot of time.
To address this, we want to cache data regions. Most of the time, when
bad performance is reported, it is in places where the image is iterated
over from start to end (qemu-img convert or the mirror job), so a simple
yet effective solution is to cache only the current data region.
(Note that only caching data regions but not zero regions means that
returning false information from the cache is not catastrophic: Treating
zeroes as data is fine. While we try to invalidate the cache on zero
writes and discards, such incongruences may still occur when there are
other processes writing to the image.)
We only use the cache for nodes without children (i.e. protocol nodes),
because that is where the problem is: Drivers that rely on block-status
implementations outside of qemu (e.g. SEEK_DATA/HOLE).
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/307
Signed-off-by: Max Reitz
I'm new to RCU, so my review can't be reliable..
Yeah, well, same here. :)
---
include/block/block_int.h | 47 ++
block.c | 84 +++
block/io.c | 61 ++--
3 files changed, 189 insertions(+), 3 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a8f9598102..fcb599dd1c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -832,6 +832,22 @@ struct BdrvChild {
QLIST_ENTRY(BdrvChild) next_parent;
};
+/*
+ * Allows bdrv_co_block_status() to cache one data region for a
+ * protocol node.
+ *
+ * @valid: Whether the cache is valid (should be accessed with atomic
+ * functions so this can be reset by RCU readers)
+ * @data_start: Offset where we know (or strongly assume) is data
+ * @data_end: Offset where the data region ends (which is not
necessarily
+ * the start of a zeroed region)
+ */
+typedef struct BdrvBlockStatusCache {
+ bool valid;
+ int64_t data_start;
+ int64_t data_end;
+} BdrvBlockStatusCache;
+
struct BlockDriverState {
/* Protected by big QEMU lock or read-only after opening. No
special
* locking needed during I/O...
@@ -997,6 +1013,11 @@ struct BlockDriverState {
/* BdrvChild links to this node may never be frozen */
bool never_freeze;
+
+ /* Lock for block-status cache RCU writers */
+ CoMutex bsc_modify_lock;
+ /* Always non-NULL, but must only be dereferenced under an RCU
read guard */
+ BdrvBlockStatusCache *block_status_cache;> };
struct BlockBackendRootState {
@@ -1422,4 +1443,30 @@ static inline BlockDriverState
*bdrv_primary_bs(BlockDriverState *bs)
*/
void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
+/**
+ * Check whether the given offset is in the cached block-status data
+ * region.
+ *
+ * If it is, and @pnum is not NULL, *pnum is set to
+ * `bsc.data_end - offset`, i.e. how many bytes, starting from
+ * @offset, are data (according to the cache).
+ * Otherwise, *pnum is not touched.
+ */
+bool bdrv_bsc_is_data(BlockDriverState *bs, int64_t offset, int64_t
*pnum);
+
+/**
+ * If [offset, offset + bytes) overlaps with the currently cached
+ * block-status region, invalidate the cache.
+ *
+ * (To be used by I/O paths that cause data regions to be zero or
+ * holes.)
+ */
+void bdrv_bsc_invalidate_range(BlockDriverState *bs,
+ int64_t offset, int64_t bytes);
+
+/**
+ * Mark the range [offset, offset + bytes) as a data region.
+ */
+void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t
bytes);
+
#endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index 3f456892d0..9ab9459f7a 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,8 @@
#include
[PULL 1/1] block/snapshot: Clarify goto fallback behavior
In the bdrv_snapshot_goto() fallback code, we work with a pointer to
either bs->file or bs->backing. We detach that child, close the node
(with .bdrv_close()), apply the snapshot on the child node, and then
re-open the node (with .bdrv_open()).
In order for .bdrv_open() to attach the same child node that we had
before, we pass "file={child-node}" or "backing={child-node}" to it.
Therefore, when .bdrv_open() has returned success, we can assume that
bs->file or bs->backing (respectively) points to our original child
again. This is verified by an assertion.
All of this is not immediately clear from a quick glance at the code,
so add a comment to the assertion what it is for, and why it is valid.
It certainly confused Coverity.
Reported-by: Coverity (CID 1452774)
Signed-off-by: Max Reitz
Message-Id: <20210503095418.31521-1-mre...@redhat.com>
[mreitz: s/close/detach/]
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/snapshot.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 6702c75e42..ccacda8bd5 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
qobject_unref(file_options);
g_free(subqdict_prefix);
+/* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr
*/
qdict_put_str(options, (*fallback_ptr)->name,
bdrv_get_node_name(fallback_bs));
+/* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
if (drv->bdrv_close) {
drv->bdrv_close(bs);
}
+/* .bdrv_open() will re-attach it */
bdrv_unref_child(bs, *fallback_ptr);
*fallback_ptr = NULL;
@@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret < 0 ? ret : open_ret;
}
-assert(fallback_bs == (*fallback_ptr)->bs);
+/*
+ * fallback_ptr is >file or >backing. *fallback_ptr
+ * was closed above and set to NULL, but the .bdrv_open() call
+ * has opened it again, because we set the respective option
+ * (with the qdict_put_str() call above).
+ * Assert that .bdrv_open() has attached some child on
+ * *fallback_ptr, and that it has attached the one we wanted
+ * it to (i.e., fallback_bs).
+ */
+assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
bdrv_unref(fallback_bs);
return ret;
}
--
2.31.1
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
On 24.06.21 12:25, Vladimir Sementsov-Ogievskiy wrote:
24.06.2021 13:16, Max Reitz wrote:
On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
23.06.2021 18:01, Max Reitz wrote:
.bdrv_co_block_status() implementations are free to return a *pnum
that
exceeds @bytes, because bdrv_co_block_status() in block/io.c will
clamp
*pnum as necessary.
On the other hand, if drivers' implementations return values for *pnum
that are as large as possible, our recently introduced block-status
cache will become more effective.
So, make a note in block_int.h that @bytes is no upper limit for
*pnum.
Suggested-by: Eric Blake
Signed-off-by: Max Reitz
---
include/block/block_int.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fcb599dd1c..f85b96ed99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -347,6 +347,11 @@ struct BlockDriver {
* clamped to bdrv_getlength() and aligned to request_alignment,
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero
value.
+ *
+ * Note that @bytes is just a hint on how big of a region the
+ * caller wants to inspect. It is not a limit on *pnum.
+ * Implementations are free to return larger values of *pnum if
+ * doing so does not incur a performance penalty.
Worth mention that the cache will benefit of it?
Oh, right, absolutely. Like so:
"block/io.c's bdrv_co_block_status() will clamp *pnum before
returning it to its caller, but it itself can still make use of the
unclamped *pnum value. Specifically, the block-status cache for
protocol nodes will benefit from storing as large a region as possible."
Sounds good. Do you mean this as an addition or substitution? If the
latter, I'd keep "if doing so does not incur a performance penalty
I meant it as an addition, just a new paragraph.
Max
Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again
On Thu, 24 Jun 2021 at 11:27, Tom Yan wrote: > I really think we should get (/ have gotten) things clear first. What > exactly is the bug we have been talking about here? I mean like, where > does it occur and what's the nature of it. > > 1. Is it specific to a certain type / model of backend / physical > storage device that will be made use of by qemu for the emulated > storage? (I presume not since you mention about image, unless you > irrationally related/bound the emulated storage type and the physical > storage type together.) > > 2. Does it have anything to do with a certain flaw in qemu itself? > Like the code that does read/write operation is flawed that it cannot > be handled by a certain *proper* backend device? > > 3. Or is it actually a bug in a certain driver / firmware blob that > will be used by an *emulated* device in the guest? In that case, can > we emulate another model so that it won't be using the problematic > driver / firmware? Definitely agreed -- before we start changing QEMU code we need to identify clearly (a) what the real hardware does and (b) what the situation was we were originally trying to fix. thanks -- PMM
[PATCH v4 33/34] usb: build usb-host as module
Drop one more shared library dependency (libusb) from core qemu.
Signed-off-by: Gerd Hoffmann
---
hw/usb/host-libusb.c | 1 +
hw/usb/meson.build | 8 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 2b7f87872ce3..c0f314462aaf 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1777,6 +1777,7 @@ static TypeInfo usb_host_dev_info = {
.class_init= usb_host_class_initfn,
.instance_init = usb_host_instance_init,
};
+module_obj(TYPE_USB_HOST_DEVICE);
static void usb_host_register_types(void)
{
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 3d8f2ae99302..0a6029ec9797 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -72,8 +72,12 @@ if config_host.has_key('CONFIG_USB_REDIR')
endif
# usb pass-through
-softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_USB_LIBUSB', libusb],
- if_true: files('host-libusb.c'))
+if config_host.has_key('CONFIG_USB_LIBUSB')
+ usbhost_ss = ss.source_set()
+ usbhost_ss.add(when: ['CONFIG_USB', libusb],
+ if_true: files('host-libusb.c'))
+ hw_usb_modules += {'host': usbhost_ss}
+endif
softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN', libusb], if_true:
files('xen-usb.c'))
--
2.31.1
[PATCH v4 26/34] accel: add qtest module annotations
Add module annotations for qtest so autoloading works.
Signed-off-by: Gerd Hoffmann
---
accel/qtest/qtest.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index edb29f6fa4c0..7e6b8110d52b 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -45,6 +45,7 @@ static const TypeInfo qtest_accel_type = {
.parent = TYPE_ACCEL,
.class_init = qtest_accel_class_init,
};
+module_obj(TYPE_QTEST_ACCEL);
static void qtest_accel_ops_class_init(ObjectClass *oc, void *data)
{
@@ -61,6 +62,7 @@ static const TypeInfo qtest_accel_ops_type = {
.class_init = qtest_accel_ops_class_init,
.abstract = true,
};
+module_obj(ACCEL_OPS_NAME("qtest"));
static void qtest_type_init(void)
{
--
2.31.1
[PATCH v4 25/34] accel: autoload modules
Call module_object_class_by_name() instead of object_class_by_name()
for objects possibly implemented as module
Signed-off-by: Gerd Hoffmann
---
accel/accel-common.c | 2 +-
accel/accel-softmmu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/accel/accel-common.c b/accel/accel-common.c
index cf07f78421d6..7b8ec7e0f72a 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -44,7 +44,7 @@ static const TypeInfo accel_type = {
AccelClass *accel_find(const char *opt_name)
{
char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
-AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
+AccelClass *ac = ACCEL_CLASS(module_object_class_by_name(class_name));
g_free(class_name);
return ac;
}
diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 50fa5acaa401..67276e4f5222 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -72,7 +72,7 @@ void accel_init_ops_interfaces(AccelClass *ac)
g_assert(ac_name != NULL);
ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
-ops = ACCEL_OPS_CLASS(object_class_by_name(ops_name));
+ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
g_free(ops_name);
/*
--
2.31.1
[PATCH v4 29/34] accel: build tcg modular
Build tcg accel ops as module.
Which is only a small fraction of tcg.
Also only x86 for now.
Signed-off-by: Gerd Hoffmann
---
accel/tcg/meson.build | 5 -
meson.build | 14 +-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index 1236ac7b910b..0ae9180282e3 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -15,8 +15,11 @@ specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_ss)
specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
'cputlb.c',
+))
+
+tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
'tcg-accel-ops.c',
'tcg-accel-ops-mttcg.c',
'tcg-accel-ops-icount.c',
- 'tcg-accel-ops-rr.c'
+ 'tcg-accel-ops-rr.c',
))
diff --git a/meson.build b/meson.build
index d2cacc145a91..9f1ca4177073 100644
--- a/meson.build
+++ b/meson.build
@@ -92,6 +92,8 @@ if cpu in ['x86', 'x86_64']
}
endif
+modular_tcg = ['i386-softmmu', 'x86_64-softmmu']
+
edk2_targets = [ 'arm-softmmu', 'aarch64-softmmu', 'i386-softmmu',
'x86_64-softmmu' ]
install_edk2_blobs = false
if get_option('install_blobs')
@@ -1311,6 +1313,11 @@ foreach target : target_dirs
elif sym == 'CONFIG_XEN' and have_xen_pci_passthrough
config_target += { 'CONFIG_XEN_PCI_PASSTHROUGH': 'y' }
endif
+ if target in modular_tcg
+config_target += { 'CONFIG_TCG_MODULAR': 'y' }
+ else
+config_target += { 'CONFIG_TCG_BUILTIN': 'y' }
+ endif
accel_kconfig += [ sym + '=y' ]
endif
endforeach
@@ -1782,6 +1789,7 @@ util_ss = ss.source_set()
# accel modules
qtest_module_ss = ss.source_set()
+tcg_module_ss = ss.source_set()
modules = {}
target_modules = {}
@@ -2022,7 +2030,11 @@ subdir('tests/qtest/libqos')
subdir('tests/qtest/fuzz')
# accel modules
-target_modules += { 'accel' : { 'qtest': qtest_module_ss }}
+tcg_real_module_ss = ss.source_set()
+tcg_real_module_ss.add_all(when: 'CONFIG_TCG_MODULAR', if_true: tcg_module_ss)
+specific_ss.add_all(when: 'CONFIG_TCG_BUILTIN', if_true: tcg_module_ss)
+target_modules += { 'accel' : { 'qtest': qtest_module_ss,
+'tcg': tcg_real_module_ss }}
# Library dependencies #
--
2.31.1
[PATCH v4 27/34] accel: build qtest modular
Allow building accelerators as module.
Start with qtest as first user.
Signed-off-by: Gerd Hoffmann
---
accel/qtest/meson.build | 8 ++--
meson.build | 6 ++
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/accel/qtest/meson.build b/accel/qtest/meson.build
index a2f327645980..4c6560029336 100644
--- a/accel/qtest/meson.build
+++ b/accel/qtest/meson.build
@@ -1,6 +1,2 @@
-qtest_ss = ss.source_set()
-qtest_ss.add(files(
- 'qtest.c',
-))
-
-specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true:
qtest_ss)
+qtest_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'],
+if_true: files('qtest.c'))
diff --git a/meson.build b/meson.build
index 76020b43fb32..d2cacc145a91 100644
--- a/meson.build
+++ b/meson.build
@@ -1780,6 +1780,9 @@ trace_ss = ss.source_set()
user_ss = ss.source_set()
util_ss = ss.source_set()
+# accel modules
+qtest_module_ss = ss.source_set()
+
modules = {}
target_modules = {}
hw_arch = {}
@@ -2018,6 +2021,9 @@ specific_ss.add_all(when: 'CONFIG_LINUX_USER', if_true:
linux_user_ss)
subdir('tests/qtest/libqos')
subdir('tests/qtest/fuzz')
+# accel modules
+target_modules += { 'accel' : { 'qtest': qtest_module_ss }}
+
# Library dependencies #
--
2.31.1
[PATCH v4 18/34] modules: check arch and block load on mismatch
Add module_allow_arch() to set the target architecture.
In case a module is limited to some arch verify arches
match and ignore the module if not.
Signed-off-by: Gerd Hoffmann
---
include/qemu/module.h | 1 +
softmmu/vl.c | 3 +++
util/module.c | 29 +
3 files changed, 33 insertions(+)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index a98748d501d3..7f4b1af8198c 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -72,6 +72,7 @@ void module_call_init(module_init_type type);
bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);
void module_load_qom_one(const char *type);
void module_load_qom_all(void);
+void module_allow_arch(const char *arch);
/*
* module info annotation macros
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5c26e80126db..a70e7b4658e8 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -126,6 +126,8 @@
#include "sysemu/iothread.h"
#include "qemu/guest-random.h"
+#include "config-host.h"
+
#define MAX_VIRTIO_CONSOLES 1
typedef struct BlockdevOptionsQueueEntry {
@@ -2725,6 +2727,7 @@ void qemu_init(int argc, char **argv, char **envp)
#ifdef CONFIG_MODULES
module_init_info(qemu_modinfo);
+module_allow_arch(TARGET_NAME);
#endif
qemu_init_subsystems();
diff --git a/util/module.c b/util/module.c
index acaaecad56c9..065aed09ffef 100644
--- a/util/module.c
+++ b/util/module.c
@@ -117,12 +117,33 @@ static const QemuModinfo module_info_stub[] = { {
/* end of list */
} };
static const QemuModinfo *module_info = module_info_stub;
+static const char *module_arch;
void module_init_info(const QemuModinfo *info)
{
module_info = info;
}
+void module_allow_arch(const char *arch)
+{
+module_arch = arch;
+}
+
+static bool module_check_arch(const QemuModinfo *modinfo)
+{
+if (modinfo->arch) {
+if (!module_arch) {
+/* no arch set -> ignore all */
+return false;
+}
+if (strcmp(module_arch, modinfo->arch) != 0) {
+/* mismatch */
+return false;
+}
+}
+return true;
+}
+
static int module_load_file(const char *fname, bool mayfail, bool
export_symbols)
{
GModule *g_module;
@@ -224,6 +245,13 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
g_hash_table_add(loaded_modules, module_name);
for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (modinfo->arch) {
+if (strcmp(modinfo->name, module_name) == 0) {
+if (!module_check_arch(modinfo)) {
+return false;
+}
+}
+}
if (modinfo->deps) {
if (strcmp(modinfo->name, module_name) == 0) {
/* we depend on other module(s) */
@@ -345,6 +373,7 @@ void qemu_load_module_for_opts(const char *group)
#else
+void module_allow_arch(const char *arch) {}
void qemu_load_module_for_opts(const char *group) {}
void module_load_qom_one(const char *type) {}
void module_load_qom_all(void) {}
--
2.31.1
[PATCH v4 34/34] monitor/tcg: move tcg hmp commands to accel/tcg, register them dynamically
One more little step towards modular tcg ...
Signed-off-by: Gerd Hoffmann
---
accel/tcg/hmp.c | 29 +
monitor/misc.c| 18 --
accel/tcg/meson.build | 1 +
hmp-commands-info.hx | 2 --
4 files changed, 30 insertions(+), 20 deletions(-)
create mode 100644 accel/tcg/hmp.c
diff --git a/accel/tcg/hmp.c b/accel/tcg/hmp.c
new file mode 100644
index ..a6e72fdb3ed6
--- /dev/null
+++ b/accel/tcg/hmp.c
@@ -0,0 +1,29 @@
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "exec/exec-all.h"
+#include "monitor/monitor.h"
+#include "sysemu/tcg.h"
+
+static void hmp_info_jit(Monitor *mon, const QDict *qdict)
+{
+if (!tcg_enabled()) {
+error_report("JIT information is only available with accel=tcg");
+return;
+}
+
+dump_exec_info();
+dump_drift_info();
+}
+
+static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
+{
+dump_opcount_info();
+}
+
+static void hmp_tcg_register(void)
+{
+monitor_register_hmp("jit", true, hmp_info_jit);
+monitor_register_hmp("opcount", true, hmp_info_opcount);
+}
+
+type_init(hmp_tcg_register);
diff --git a/monitor/misc.c b/monitor/misc.c
index ad476c6e51ea..b28874d6dc59 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -320,24 +320,6 @@ static void hmp_info_registers(Monitor *mon, const QDict
*qdict)
}
}
-#ifdef CONFIG_TCG
-static void hmp_info_jit(Monitor *mon, const QDict *qdict)
-{
-if (!tcg_enabled()) {
-error_report("JIT information is only available with accel=tcg");
-return;
-}
-
-dump_exec_info();
-dump_drift_info();
-}
-
-static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
-{
-dump_opcount_info();
-}
-#endif
-
static void hmp_info_sync_profile(Monitor *mon, const QDict *qdict)
{
int64_t max = qdict_get_try_int(qdict, "max", 10);
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index 0ae9180282e3..137a1a44cc0a 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -15,6 +15,7 @@ specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_ss)
specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
'cputlb.c',
+ 'hmp.c',
))
tcg_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: files(
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ce42aef47acb..27206ac049df 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -274,7 +274,6 @@ ERST
.args_type = "",
.params = "",
.help = "show dynamic compiler info",
-.cmd= hmp_info_jit,
},
#endif
@@ -289,7 +288,6 @@ ERST
.args_type = "",
.params = "",
.help = "show dynamic compiler opcode counters",
-.cmd= hmp_info_opcount,
},
#endif
--
2.31.1
[PATCH v4 19/34] modules: check arch on qom lookup
With target-specific modules we can have multiple modules implementing
the same object. Therefore we have to check the target arch on lookup
to find the correct module.
Signed-off-by: Gerd Hoffmann
---
util/module.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/util/module.c b/util/module.c
index 065aed09ffef..6bb4ad915a1c 100644
--- a/util/module.c
+++ b/util/module.c
@@ -329,6 +329,9 @@ void module_load_qom_one(const char *type)
if (!modinfo->objs) {
continue;
}
+if (!module_check_arch(modinfo)) {
+continue;
+}
for (sl = modinfo->objs; *sl != NULL; sl++) {
if (strcmp(type, *sl) == 0) {
module_load_one("", modinfo->name, false);
@@ -349,6 +352,9 @@ void module_load_qom_all(void)
if (!modinfo->objs) {
continue;
}
+if (!module_check_arch(modinfo)) {
+continue;
+}
module_load_one("", modinfo->name, false);
}
module_loaded_qom_all = true;
--
2.31.1
[PATCH v4 24/34] modules: hook up modules.h to docs build
Signed-off-by: Gerd Hoffmann Reviewed-by: Paolo Bonzini --- docs/devel/index.rst | 1 + docs/devel/modules.rst | 5 + 2 files changed, 6 insertions(+) create mode 100644 docs/devel/modules.rst diff --git a/docs/devel/index.rst b/docs/devel/index.rst index 977c3893bdaf..ba90badbbd6d 100644 --- a/docs/devel/index.rst +++ b/docs/devel/index.rst @@ -41,6 +41,7 @@ Contents: s390-dasd-ipl clocks qom + modules block-coroutine-wrapper multi-process ebpf_rss diff --git a/docs/devel/modules.rst b/docs/devel/modules.rst new file mode 100644 index ..066f347b89ba --- /dev/null +++ b/docs/devel/modules.rst @@ -0,0 +1,5 @@ + +Qemu modules + + +.. kernel-doc:: include/qemu/module.h -- 2.31.1
[PATCH v4 32/34] monitor/usb: register 'info usbhost' dynamically
Signed-off-by: Gerd Hoffmann
---
hw/usb/host-libusb.c | 1 +
hw/usb/host-stub.c | 40
hmp-commands-info.hx | 1 -
hw/usb/meson.build | 4 +---
4 files changed, 2 insertions(+), 44 deletions(-)
delete mode 100644 hw/usb/host-stub.c
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index e6d21aa8e1d3..2b7f87872ce3 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1781,6 +1781,7 @@ static TypeInfo usb_host_dev_info = {
static void usb_host_register_types(void)
{
type_register_static(_host_dev_info);
+monitor_register_hmp("usbhost", true, hmp_info_usbhost);
}
type_init(usb_host_register_types)
diff --git a/hw/usb/host-stub.c b/hw/usb/host-stub.c
deleted file mode 100644
index bbe69baa390f..
--- a/hw/usb/host-stub.c
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * Stub host USB redirector
- *
- * Copyright (c) 2005 Fabrice Bellard
- *
- * Copyright (c) 2008 Max Krasnyansky
- * Support for host device auto connect & disconnect
- * Major rewrite to support fully async operation
- *
- * Copyright 2008 TJ
- * Added flexible support for /dev/bus/usb /sys/bus/usb/devices in
addition
- * to the legacy /proc/bus/usb USB device discovery and handling
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#include "qemu/osdep.h"
-#include "hw/usb.h"
-#include "monitor/monitor.h"
-
-void hmp_info_usbhost(Monitor *mon, const QDict *qdict)
-{
-monitor_printf(mon, "USB host devices not supported\n");
-}
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index fb59c27200cb..ce42aef47acb 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -368,7 +368,6 @@ ERST
.args_type = "",
.params = "",
.help = "show host USB devices",
-.cmd= hmp_info_usbhost,
},
SRST
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index f357270d0b6b..3d8f2ae99302 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -73,9 +73,7 @@ endif
# usb pass-through
softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_USB_LIBUSB', libusb],
- if_true: files('host-libusb.c'),
- if_false: files('host-stub.c'))
-softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('host-stub.c'))
+ if_true: files('host-libusb.c'))
softmmu_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN', libusb], if_true:
files('xen-usb.c'))
--
2.31.1
[PATCH v4 30/34] monitor: allow register hmp commands
Allow commands having a NULL cmd pointer, add a function to set the
pointer later. Use case: allow modules implement hmp commands.
Signed-off-by: Gerd Hoffmann
---
include/monitor/monitor.h | 3 +++
monitor/hmp.c | 7 +++
monitor/misc.c| 16
3 files changed, 26 insertions(+)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1211d6e6d69f..1a8a369b50b2 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -51,4 +51,7 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int flags);
void monitor_fdset_dup_fd_remove(int dup_fd);
int64_t monitor_fdset_dup_fd_find(int dup_fd);
+void monitor_register_hmp(const char *name, bool info,
+ void (*cmd)(Monitor *mon, const QDict *qdict));
+
#endif /* MONITOR_H */
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 6c0b33a0b19d..d50c3124e1e1 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1089,6 +1089,13 @@ void handle_hmp_command(MonitorHMP *mon, const char
*cmdline)
return;
}
+if (!cmd->cmd) {
+/* FIXME: is it useful to try autoload modules here ??? */
+monitor_printf(>common, "Command \"%.*s\" is not available.\n",
+ (int)(cmdline - cmd_start), cmd_start);
+return;
+}
+
qdict = monitor_parse_arguments(>common, , cmd);
if (!qdict) {
while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
diff --git a/monitor/misc.c b/monitor/misc.c
index 1539e18557f0..ad476c6e51ea 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1974,6 +1974,22 @@ static void sortcmdlist(void)
compare_mon_cmd);
}
+void monitor_register_hmp(const char *name, bool info,
+ void (*cmd)(Monitor *mon, const QDict *qdict))
+{
+HMPCommand *table = info ? hmp_info_cmds : hmp_cmds;
+
+while (table->name != NULL) {
+if (strcmp(table->name, name) == 0) {
+g_assert(table->cmd == NULL);
+table->cmd = cmd;
+return;
+}
+table++;
+}
+g_assert_not_reached();
+}
+
void monitor_init_globals(void)
{
monitor_init_globals_core();
--
2.31.1
[PATCH v4 31/34] usb: drop usb_host_dev_is_scsi_storage hook
Introduce an usb device flag instead, set it when usb-host looks at the
device descriptors anyway. Also set it for emulated storage devices,
for consistency. Add an inline helper function to check the flag.
Signed-off-by: Gerd Hoffmann
Acked-by: David Gibson
---
include/hw/usb.h | 7 ++-
hw/ppc/spapr.c | 2 +-
hw/usb/dev-storage-bot.c | 1 +
hw/usb/dev-storage-classic.c | 1 +
hw/usb/dev-uas.c | 1 +
hw/usb/host-libusb.c | 36 +++-
hw/usb/host-stub.c | 5 -
7 files changed, 17 insertions(+), 36 deletions(-)
diff --git a/include/hw/usb.h b/include/hw/usb.h
index 436e07b30404..33668dd0a99a 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -219,6 +219,7 @@ enum USBDeviceFlags {
USB_DEV_FLAG_IS_HOST,
USB_DEV_FLAG_MSOS_DESC_ENABLE,
USB_DEV_FLAG_MSOS_DESC_IN_USE,
+USB_DEV_FLAG_IS_SCSI_STORAGE,
};
/* definition of a USB device */
@@ -465,7 +466,6 @@ void usb_generic_async_ctrl_complete(USBDevice *s,
USBPacket *p);
/* usb-linux.c */
void hmp_info_usbhost(Monitor *mon, const QDict *qdict);
-bool usb_host_dev_is_scsi_storage(USBDevice *usbdev);
/* usb ports of the VM */
@@ -561,6 +561,11 @@ const char *usb_device_get_product_desc(USBDevice *dev);
const USBDesc *usb_device_get_usb_desc(USBDevice *dev);
+static inline bool usb_device_is_scsi_storage(USBDevice *dev)
+{
+return dev->flags & (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
+}
+
/* quirks.c */
/* In bulk endpoints are streaming data sources (iow behave like isoc eps) */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4dd90b75cc52..f83a081af0f1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3106,7 +3106,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p,
BusState *bus,
*/
if (strcmp("usb-host", qdev_fw_name(dev)) == 0) {
USBDevice *usbdev = CAST(USBDevice, dev, TYPE_USB_DEVICE);
-if (usb_host_dev_is_scsi_storage(usbdev)) {
+if (usb_device_is_scsi_storage(usbdev)) {
return g_strdup_printf("storage@%s/disk", usbdev->port->path);
}
}
diff --git a/hw/usb/dev-storage-bot.c b/hw/usb/dev-storage-bot.c
index 6aad026d1133..68ebaca10c66 100644
--- a/hw/usb/dev-storage-bot.c
+++ b/hw/usb/dev-storage-bot.c
@@ -32,6 +32,7 @@ static void usb_msd_bot_realize(USBDevice *dev, Error **errp)
usb_desc_create_serial(dev);
usb_desc_init(dev);
+dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
if (d->hotplugged) {
s->dev.auto_attach = 0;
}
diff --git a/hw/usb/dev-storage-classic.c b/hw/usb/dev-storage-classic.c
index 00cb34b22f02..3d017a4e6791 100644
--- a/hw/usb/dev-storage-classic.c
+++ b/hw/usb/dev-storage-classic.c
@@ -64,6 +64,7 @@ static void usb_msd_storage_realize(USBDevice *dev, Error
**errp)
usb_desc_create_serial(dev);
usb_desc_init(dev);
+dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
scsi_bus_new(>bus, sizeof(s->bus), DEVICE(dev),
_msd_scsi_info_storage, NULL);
scsi_dev = scsi_bus_legacy_add_drive(>bus, blk, 0, !!s->removable,
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index d2bd85d3f6bb..263056231c79 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -926,6 +926,7 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
QTAILQ_INIT(>requests);
uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
+dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
scsi_bus_new(>bus, sizeof(uas->bus), DEVICE(dev),
_uas_scsi_info, NULL);
}
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 2518306f527f..e6d21aa8e1d3 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -770,6 +770,13 @@ static void usb_host_speed_compat(USBHostDevice *s)
for (i = 0; i < conf->bNumInterfaces; i++) {
for (a = 0; a < conf->interface[i].num_altsetting; a++) {
intf = >interface[i].altsetting[a];
+
+if (intf->bInterfaceClass == LIBUSB_CLASS_MASS_STORAGE &&
+intf->bInterfaceSubClass == 6) { /* SCSI */
+udev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
+break;
+}
+
for (e = 0; e < intf->bNumEndpoints; e++) {
endp = >endpoint[e];
type = endp->bmAttributes & 0x3;
@@ -1893,35 +1900,6 @@ static void usb_host_auto_check(void *unused)
timer_mod(usb_auto_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 2000);
}
-/**
- * Check whether USB host device has a USB mass storage SCSI interface
- */
-bool usb_host_dev_is_scsi_storage(USBDevice *ud)
-{
-USBHostDevice *uhd = USB_HOST_DEVICE(ud);
-struct libusb_config_descriptor *conf;
-const struct libusb_interface_descriptor *intf;
-bool is_scsi_storage = false;
-int i;
-
-if (!uhd || libusb_get_active_config_descriptor(uhd->dev, ) != 0) {
-return false;
[PATCH v4 22/34] modules: add module_obj() note to QOM docs
Signed-off-by: Gerd Hoffmann Reviewed-by: Paolo Bonzini --- docs/devel/qom.rst | 8 1 file changed, 8 insertions(+) diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst index 42d0dc4f4da8..e5fe3597cd82 100644 --- a/docs/devel/qom.rst +++ b/docs/devel/qom.rst @@ -87,6 +87,14 @@ specific type: #define MY_DEVICE(obj) \ OBJECT_CHECK(MyDevice, obj, TYPE_MY_DEVICE) +In case the ObjectClass implementation can be built as module a +module_obj() line must be added to make sure qemu loads the module +when the object is needed. + +.. code-block:: c + + module_obj(TYPE_MY_DEVICE); + Class Initialization -- 2.31.1
[PATCH v4 28/34] accel: add tcg module annotations
Add module annotations for tcg so autoloading works.
Signed-off-by: Gerd Hoffmann
---
accel/tcg/tcg-accel-ops.c | 1 +
accel/tcg/tcg-all.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 7191315aeed4..1a8e8390bd60 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -124,6 +124,7 @@ static const TypeInfo tcg_accel_ops_type = {
.class_init = tcg_accel_ops_class_init,
.abstract = true,
};
+module_obj(ACCEL_OPS_NAME("tcg"));
static void tcg_accel_ops_register_types(void)
{
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 00803f76d870..d6336a9c966d 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -238,6 +238,7 @@ static const TypeInfo tcg_accel_type = {
.class_init = tcg_accel_class_init,
.instance_size = sizeof(TCGState),
};
+module_obj(TYPE_TCG_ACCEL);
static void register_accel_types(void)
{
--
2.31.1
[PATCH v4 23/34] modules: module.h kerneldoc annotations
Signed-off-by: Gerd Hoffmann
Reviewed-by: Paolo Bonzini
---
include/qemu/module.h | 59 +--
1 file changed, 45 insertions(+), 14 deletions(-)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 7f4b1af8198c..8bc80535a4d4 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -74,11 +74,18 @@ void module_load_qom_one(const char *type);
void module_load_qom_all(void);
void module_allow_arch(const char *arch);
-/*
- * module info annotation macros
+/**
+ * DOC: module info annotation macros
*
- * scripts/modinfo-collect.py will collect module info,
- * using the preprocessor and -DQEMU_MODINFO
+ * `scripts/modinfo-collect.py` will collect module info,
+ * using the preprocessor and -DQEMU_MODINFO.
+ *
+ * `scripts/modinfo-generate.py` will create a module meta-data database
+ * from the collected information so qemu knows about module
+ * dependencies and QOM objects implemented by modules.
+ *
+ * See `*.modinfo` and `modinfo.c` in the build directory to check the
+ * script results.
*/
#ifdef QEMU_MODINFO
# define modinfo(kind, value) \
@@ -87,24 +94,48 @@ void module_allow_arch(const char *arch);
# define modinfo(kind, value)
#endif
-/* module implements QOM type */
+/**
+ * module_obj
+ *
+ * @name: QOM type.
+ *
+ * This module implements QOM type @name.
+ */
#define module_obj(name) modinfo(obj, name)
-/* module has a dependency on */
+/**
+ * module_dep
+ *
+ * @name: module name
+ *
+ * This module depends on module @name.
+ */
#define module_dep(name) modinfo(dep, name)
-/* module is for target architecture */
+/**
+ * module_arch
+ *
+ * @arch: target architecture
+ *
+ * This module is for target architecture @arch.
+ *
+ * Note that target-dependent modules are tagged automatically, so
+ * this is only needed in case target-independent modules should be
+ * restricted. Use case example: the ccw bus is implemented by s390x
+ * only.
+ */
#define module_arch(name) modinfo(arch, name)
-/* module registers QemuOpts */
+/**
+ * module_opts
+ *
+ * @name: QemuOpts name
+ *
+ * This module registers QemuOpts @name.
+ */
#define module_opts(name) modinfo(opts, name)
-/*
- * module info database
- *
- * scripts/modinfo-generate.c will build this using the data collected
- * by scripts/modinfo-collect.py
- */
+/* module info database (created by scripts/modinfo-generate.py) */
typedef struct QemuModinfo QemuModinfo;
struct QemuModinfo {
const char *name;
--
2.31.1
[PATCH v4 13/34] modules: add block module annotations
Signed-off-by: Gerd Hoffmann
---
block/iscsi-opts.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/iscsi-opts.c b/block/iscsi-opts.c
index afaf8837d6c1..4f2da405e645 100644
--- a/block/iscsi-opts.c
+++ b/block/iscsi-opts.c
@@ -68,3 +68,4 @@ static void iscsi_block_opts_init(void)
}
block_init(iscsi_block_opts_init);
+module_opts("iscsi");
--
2.31.1
[PATCH v4 20/34] modules: target-specific module build infrastructure
Signed-off-by: Gerd Hoffmann
---
meson.build | 37 +
1 file changed, 37 insertions(+)
diff --git a/meson.build b/meson.build
index 6ae2b9ab9443..76020b43fb32 100644
--- a/meson.build
+++ b/meson.build
@@ -1781,6 +1781,7 @@ user_ss = ss.source_set()
util_ss = ss.source_set()
modules = {}
+target_modules = {}
hw_arch = {}
target_arch = {}
target_softmmu_arch = {}
@@ -2060,6 +2061,42 @@ foreach d, list : modules
endforeach
endforeach
+foreach d, list : target_modules
+ foreach m, module_ss : list
+if enable_modules and targetos != 'windows'
+ foreach target : target_dirs
+if target.endswith('-softmmu')
+ config_target = config_target_mak[target]
+ config_target += config_host
+ target_inc = [include_directories('target' /
config_target['TARGET_BASE_ARCH'])]
+ c_args = ['-DNEED_CPU_H',
+'-DCONFIG_TARGET="@0@-config-target.h"'.format(target),
+'-DCONFIG_DEVICES="@0@-config-devices.h"'.format(target)]
+ target_module_ss = module_ss.apply(config_target, strict: false)
+ if target_module_ss.sources() != []
+module_name = d + '-' + m + '-' + config_target['TARGET_NAME']
+sl = static_library(module_name,
+[genh, target_module_ss.sources()],
+dependencies: [modulecommon,
target_module_ss.dependencies()],
+include_directories: target_inc,
+c_args: c_args,
+pic: true)
+softmmu_mods += sl
+# FIXME: Should use sl.extract_all_objects(recursive: true) too.
+modinfo_files += custom_target(module_name + '.modinfo',
+ output: module_name + '.modinfo',
+ input: target_module_ss.sources(),
+ capture: true,
+ command: [modinfo_collect,
'--target', target, '@INPUT@'])
+ endif
+endif
+ endforeach
+else
+ specific_ss.add_all(module_ss)
+endif
+ endforeach
+endforeach
+
if enable_modules
modinfo_src = custom_target('modinfo.c',
output: 'modinfo.c',
--
2.31.1
[PATCH v4 21/34] modules: add documentation for module sourcesets
Signed-off-by: Gerd Hoffmann
Reviewed-by: Paolo Bonzini
---
docs/devel/build-system.rst | 17 +
1 file changed, 17 insertions(+)
diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst
index 7ef36f42d0f5..fd1650442ecc 100644
--- a/docs/devel/build-system.rst
+++ b/docs/devel/build-system.rst
@@ -272,6 +272,23 @@ Target-dependent emulator sourcesets:
target_arch += {'arm': arm_ss}
target_softmmu_arch += {'arm': arm_softmmu_ss}
+Module sourcesets:
+ There are two dictionaries for modules: `modules` is used for
+ target-independent modules and `target_modules` is used for
+ target-dependent modules. When modules are disabled the `module`
+ source sets are added to `softmmu_ss` and the `target_modules`
+ source sets are added to `specific_ss`.
+
+ Both dictionaries are nested. One dictionary is created per
+ subdirectory, and these per-subdirectory dictionaries are added to
+ the toplevel dictionaries. For example::
+
+hw_display_modules = {}
+qxl_ss = ss.source_set()
+...
+hw_display_modules += { 'qxl': qxl_ss }
+modules += { 'hw-display': hw_display_modules }
+
Utility sourcesets:
All binaries link with a static library `libqemuutil.a`. This library
is built from several sourcesets; most of them however host generated
--
2.31.1
[PATCH v4 15/34] modules: use modinfo for qom load
Use module database to figure which module implements a given QOM type.
Drop hard-coded object list.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Paolo Bonzini
---
util/module.c | 77 ---
1 file changed, 24 insertions(+), 53 deletions(-)
diff --git a/util/module.c b/util/module.c
index 7d7b69cbdaca..745ae0fb20ed 100644
--- a/util/module.c
+++ b/util/module.c
@@ -280,80 +280,51 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
return success;
}
-/*
- * Building devices and other qom objects modular is mostly useful in
- * case they have dependencies to external shared libraries, so we can
- * cut down the core qemu library dependencies. Which is the case for
- * only a very few devices & objects.
- *
- * So with the expectation that this will be rather the exception than
- * the rule and the list will not gain that many entries, go with a
- * simple manually maintained list for now.
- *
- * The list must be sorted by module (module_load_qom_all() needs this).
- */
-static struct {
-const char *type;
-const char *prefix;
-const char *module;
-} const qom_modules[] = {
-{ "ccid-card-passthru","hw-", "usb-smartcard" },
-{ "ccid-card-emulated","hw-", "usb-smartcard" },
-{ "usb-redir", "hw-", "usb-redirect" },
-{ "qxl-vga", "hw-", "display-qxl" },
-{ "qxl", "hw-", "display-qxl" },
-{ "virtio-gpu-device", "hw-", "display-virtio-gpu"},
-{ "virtio-gpu-gl-device", "hw-", "display-virtio-gpu-gl" },
-{ "vhost-user-gpu","hw-", "display-virtio-gpu"},
-{ "virtio-gpu-pci-base", "hw-", "display-virtio-gpu-pci" },
-{ "virtio-gpu-pci","hw-", "display-virtio-gpu-pci" },
-{ "virtio-gpu-gl-pci", "hw-", "display-virtio-gpu-pci-gl" },
-{ "vhost-user-gpu-pci","hw-", "display-virtio-gpu-pci" },
-{ "virtio-gpu-ccw","hw-", "s390x-virtio-gpu-ccw" },
-{ "virtio-vga-base", "hw-", "display-virtio-vga"},
-{ "virtio-vga","hw-", "display-virtio-vga"},
-{ "virtio-vga-gl", "hw-", "display-virtio-vga-gl" },
-{ "vhost-user-vga","hw-", "display-virtio-vga"},
-{ "chardev-braille", "chardev-", "baum" },
-{ "chardev-spicevmc", "chardev-", "spice"},
-{ "chardev-spiceport", "chardev-", "spice"},
-};
+#ifdef CONFIG_MODULES
static bool module_loaded_qom_all;
void module_load_qom_one(const char *type)
{
-int i;
+const QemuModinfo *modinfo;
+const char **sl;
if (!type) {
return;
}
-for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
-if (strcmp(qom_modules[i].type, type) == 0) {
-module_load_one(qom_modules[i].prefix,
-qom_modules[i].module,
-false);
-return;
+
+for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (!modinfo->objs) {
+continue;
+}
+for (sl = modinfo->objs; *sl != NULL; sl++) {
+if (strcmp(type, *sl) == 0) {
+module_load_one("", modinfo->name, false);
+}
}
}
}
void module_load_qom_all(void)
{
-int i;
+const QemuModinfo *modinfo;
if (module_loaded_qom_all) {
return;
}
-for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
-if (i > 0 && (strcmp(qom_modules[i - 1].module,
- qom_modules[i].module) == 0 &&
- strcmp(qom_modules[i - 1].prefix,
- qom_modules[i].prefix) == 0)) {
-/* one module implementing multiple types -> load only once */
+
+for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (!modinfo->objs) {
continue;
}
-module_load_one(qom_modules[i].prefix, qom_modules[i].module, true);
+module_load_one("", modinfo->name, false);
}
module_loaded_qom_all = true;
}
+
+#else
+
+void module_load_qom_one(const char *type) {}
+void module_load_qom_all(void) {}
+
+#endif
--
2.31.1
[PATCH v4 16/34] modules: use modinfo for qemu opts load
Use module database to figure which module adds given QemuOpts group.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Paolo Bonzini
---
softmmu/vl.c| 17 -
stubs/module-opts.c | 4
util/module.c | 19 +++
3 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e001505d79f7..5c26e80126db 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2675,23 +2675,6 @@ void qmp_x_exit_preconfig(Error **errp)
}
}
-#ifdef CONFIG_MODULES
-void qemu_load_module_for_opts(const char *group)
-{
-static bool spice_tried;
-if (g_str_equal(group, "spice") && !spice_tried) {
-ui_module_load_one("spice-core");
-spice_tried = true;
-}
-
-static bool iscsi_tried;
-if (g_str_equal(group, "iscsi") && !iscsi_tried) {
-block_module_load_one("iscsi");
-iscsi_tried = true;
-}
-}
-#endif
-
void qemu_init(int argc, char **argv, char **envp)
{
QemuOpts *opts;
diff --git a/stubs/module-opts.c b/stubs/module-opts.c
index a7d0e4ad6ead..5412429ea869 100644
--- a/stubs/module-opts.c
+++ b/stubs/module-opts.c
@@ -1,6 +1,2 @@
#include "qemu/osdep.h"
#include "qemu/config-file.h"
-
-void qemu_load_module_for_opts(const char *group)
-{
-}
diff --git a/util/module.c b/util/module.c
index 745ae0fb20ed..a9ec2da9972e 100644
--- a/util/module.c
+++ b/util/module.c
@@ -20,6 +20,7 @@
#include "qemu/queue.h"
#include "qemu/module.h"
#include "qemu/cutils.h"
+#include "qemu/config-file.h"
#ifdef CONFIG_MODULE_UPGRADES
#include "qemu-version.h"
#endif
@@ -322,8 +323,26 @@ void module_load_qom_all(void)
module_loaded_qom_all = true;
}
+void qemu_load_module_for_opts(const char *group)
+{
+const QemuModinfo *modinfo;
+const char **sl;
+
+for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (!modinfo->opts) {
+continue;
+}
+for (sl = modinfo->opts; *sl != NULL; sl++) {
+if (strcmp(group, *sl) == 0) {
+module_load_one("", modinfo->name, false);
+}
+}
+}
+}
+
#else
+void qemu_load_module_for_opts(const char *group) {}
void module_load_qom_one(const char *type) {}
void module_load_qom_all(void) {}
--
2.31.1
[PATCH v4 14/34] modules: use modinfo for dependencies
Use module database for module dependencies.
Drop hard-coded dependency list.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Paolo Bonzini
---
util/module.c | 55 ---
1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/util/module.c b/util/module.c
index 8d3e8275b9f7..7d7b69cbdaca 100644
--- a/util/module.c
+++ b/util/module.c
@@ -182,28 +182,6 @@ static int module_load_file(const char *fname, bool
mayfail, bool export_symbols
out:
return ret;
}
-
-static const struct {
-const char *name;
-const char *dep;
-} module_deps[] = {
-{ "audio-spice","ui-spice-core" },
-{ "chardev-spice", "ui-spice-core" },
-{ "hw-display-qxl", "ui-spice-core" },
-{ "ui-spice-app", "ui-spice-core" },
-{ "ui-spice-app", "chardev-spice" },
-
-{ "hw-display-virtio-gpu-gl", "hw-display-virtio-gpu" },
-{ "hw-display-virtio-gpu-pci-gl", "hw-display-virtio-gpu-pci" },
-{ "hw-display-virtio-vga-gl", "hw-display-virtio-vga" },
-
-#ifdef CONFIG_OPENGL
-{ "ui-egl-headless", "ui-opengl"},
-{ "ui-gtk", "ui-opengl"},
-{ "ui-sdl", "ui-opengl"},
-{ "ui-spice-core", "ui-opengl"},
-#endif
-};
#endif
bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
@@ -219,9 +197,11 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
char *dirs[5];
char *module_name;
int i = 0, n_dirs = 0;
-int ret, dep;
+int ret;
bool export_symbols = false;
static GHashTable *loaded_modules;
+const QemuModinfo *modinfo;
+const char **sl;
if (!g_module_supported()) {
fprintf(stderr, "Module is not supported by system.\n");
@@ -234,23 +214,30 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
module_name = g_strdup_printf("%s%s", prefix, lib_name);
-for (dep = 0; dep < ARRAY_SIZE(module_deps); dep++) {
-if (strcmp(module_name, module_deps[dep].name) == 0) {
-/* we depend on another module */
-module_load_one("", module_deps[dep].dep, false);
-}
-if (strcmp(module_name, module_deps[dep].dep) == 0) {
-/* another module depends on us */
-export_symbols = true;
-}
-}
-
if (g_hash_table_contains(loaded_modules, module_name)) {
g_free(module_name);
return true;
}
g_hash_table_add(loaded_modules, module_name);
+for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+if (modinfo->deps) {
+if (strcmp(modinfo->name, module_name) == 0) {
+/* we depend on other module(s) */
+for (sl = modinfo->deps; *sl != NULL; sl++) {
+module_load_one("", *sl, false);
+}
+} else {
+for (sl = modinfo->deps; *sl != NULL; sl++) {
+if (strcmp(module_name, *sl) == 0) {
+/* another module depends on us */
+export_symbols = true;
+}
+}
+}
+}
+}
+
search_dir = getenv("QEMU_MODULE_DIR");
if (search_dir != NULL) {
dirs[n_dirs++] = g_strdup_printf("%s", search_dir);
--
2.31.1
[PATCH v4 17/34] modules: add tracepoints
One for module load and one for qom type lookup.
Signed-off-by: Gerd Hoffmann
---
util/module.c | 3 +++
util/trace-events | 4
2 files changed, 7 insertions(+)
diff --git a/util/module.c b/util/module.c
index a9ec2da9972e..acaaecad56c9 100644
--- a/util/module.c
+++ b/util/module.c
@@ -24,6 +24,7 @@
#ifdef CONFIG_MODULE_UPGRADES
#include "qemu-version.h"
#endif
+#include "trace.h"
typedef struct ModuleEntry
{
@@ -176,6 +177,7 @@ static int module_load_file(const char *fname, bool
mayfail, bool export_symbols
ret = 0;
}
+trace_module_load_module(fname);
QTAILQ_FOREACH_SAFE(e, _init_list, node, next) {
QTAILQ_REMOVE(_init_list, e, node);
g_free(e);
@@ -294,6 +296,7 @@ void module_load_qom_one(const char *type)
return;
}
+trace_module_lookup_object_type(type);
for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
if (!modinfo->objs) {
continue;
diff --git a/util/trace-events b/util/trace-events
index 806cac14a762..c8f53d7d9fc3 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -100,3 +100,7 @@ uffd_create_fd_api_failed(int err) "errno: %i"
uffd_create_fd_api_noioctl(uint64_t ioctl_req, uint64_t ioctl_supp)
"ioctl_req: 0x%" PRIx64 "ioctl_supp: 0x%" PRIx64
uffd_register_memory_failed(void *addr, uint64_t length, uint64_t mode, int
err) "addr: %p length: %" PRIu64 " mode: 0x%" PRIx64 " errno: %i"
uffd_unregister_memory_failed(void *addr, uint64_t length, int err) "addr: %p
length: %" PRIu64 " errno: %i"
+
+# module.c
+module_load_module(const char *name) "file %s"
+module_lookup_object_type(const char *name) "name %s"
--
2.31.1
[PATCH v4 10/34] modules: add ccid module annotations
Signed-off-by: Gerd Hoffmann
---
hw/usb/ccid-card-emulated.c | 1 +
hw/usb/ccid-card-passthru.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 5c76bed77aa0..6c8c0355e099 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -612,6 +612,7 @@ static const TypeInfo emulated_card_info = {
.instance_size = sizeof(EmulatedState),
.class_init= emulated_class_initfn,
};
+module_obj(TYPE_EMULATED_CCID);
static void ccid_card_emulated_register_types(void)
{
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 7212d0d7fb5e..fa3040fb7154 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -414,6 +414,7 @@ static const TypeInfo passthru_card_info = {
.instance_size = sizeof(PassthruState),
.class_init= passthru_class_initfn,
};
+module_obj(TYPE_CCID_PASSTHRU);
static void ccid_card_passthru_register_types(void)
{
--
2.31.1
[PATCH v4 12/34] modules: add s390x module annotations
Signed-off-by: Gerd Hoffmann
---
hw/s390x/virtio-ccw-gpu.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/s390x/virtio-ccw-gpu.c b/hw/s390x/virtio-ccw-gpu.c
index 75a9e4bb3908..5868a2a07093 100644
--- a/hw/s390x/virtio-ccw-gpu.c
+++ b/hw/s390x/virtio-ccw-gpu.c
@@ -59,6 +59,7 @@ static const TypeInfo virtio_ccw_gpu = {
.instance_init = virtio_ccw_gpu_instance_init,
.class_init= virtio_ccw_gpu_class_init,
};
+module_obj(TYPE_VIRTIO_GPU_CCW);
static void virtio_ccw_gpu_register(void)
{
@@ -68,3 +69,5 @@ static void virtio_ccw_gpu_register(void)
}
type_init(virtio_ccw_gpu_register)
+
+module_arch("s390x");
--
2.31.1
[PATCH v4 09/34] modules: add usb-redir module annotations
Signed-off-by: Gerd Hoffmann
---
hw/usb/redirect.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 6a75b0dc4ab2..4ec9326e0582 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -2608,6 +2608,7 @@ static const TypeInfo usbredir_dev_info = {
.class_init= usbredir_class_initfn,
.instance_init = usbredir_instance_init,
};
+module_obj(TYPE_USB_REDIR);
static void usbredir_register_types(void)
{
--
2.31.1
[PATCH v4 06/34] modules: add virtio-gpu module annotations
Signed-off-by: Gerd Hoffmann
---
hw/display/vhost-user-gpu-pci.c | 1 +
hw/display/vhost-user-gpu.c | 1 +
hw/display/vhost-user-vga.c | 1 +
hw/display/virtio-gpu-base.c| 1 +
hw/display/virtio-gpu-gl.c | 3 +++
hw/display/virtio-gpu-pci-gl.c | 3 +++
hw/display/virtio-gpu-pci.c | 2 ++
hw/display/virtio-gpu.c | 1 +
hw/display/virtio-vga-gl.c | 3 +++
hw/display/virtio-vga.c | 2 ++
10 files changed, 18 insertions(+)
diff --git a/hw/display/vhost-user-gpu-pci.c b/hw/display/vhost-user-gpu-pci.c
index a02b23ecaf11..daefcf710159 100644
--- a/hw/display/vhost-user-gpu-pci.c
+++ b/hw/display/vhost-user-gpu-pci.c
@@ -43,6 +43,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_gpu_pci_info
= {
.instance_size = sizeof(VhostUserGPUPCI),
.instance_init = vhost_user_gpu_pci_initfn,
};
+module_obj(TYPE_VHOST_USER_GPU_PCI);
static void vhost_user_gpu_pci_register_types(void)
{
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 6cdaa1c73b9b..32ef0061f924 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -596,6 +596,7 @@ static const TypeInfo vhost_user_gpu_info = {
.instance_finalize = vhost_user_gpu_instance_finalize,
.class_init = vhost_user_gpu_class_init,
};
+module_obj(TYPE_VHOST_USER_GPU);
static void vhost_user_gpu_register_types(void)
{
diff --git a/hw/display/vhost-user-vga.c b/hw/display/vhost-user-vga.c
index a34a99856d73..072c9c65bc75 100644
--- a/hw/display/vhost-user-vga.c
+++ b/hw/display/vhost-user-vga.c
@@ -44,6 +44,7 @@ static const VirtioPCIDeviceTypeInfo vhost_user_vga_info = {
.instance_size = sizeof(VhostUserVGA),
.instance_init = vhost_user_vga_inst_initfn,
};
+module_obj(TYPE_VHOST_USER_VGA);
static void vhost_user_vga_register_types(void)
{
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index dd294276cb38..c8da4806e0bb 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -256,6 +256,7 @@ static const TypeInfo virtio_gpu_base_info = {
.class_init = virtio_gpu_base_class_init,
.abstract = true
};
+module_obj(TYPE_VIRTIO_GPU_BASE);
static void
virtio_register_types(void)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index d971b480806a..7ab93bf8c829 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -154,6 +154,7 @@ static const TypeInfo virtio_gpu_gl_info = {
.instance_size = sizeof(VirtIOGPUGL),
.class_init = virtio_gpu_gl_class_init,
};
+module_obj(TYPE_VIRTIO_GPU_GL);
static void virtio_register_types(void)
{
@@ -161,3 +162,5 @@ static void virtio_register_types(void)
}
type_init(virtio_register_types)
+
+module_dep("hw-display-virtio-gpu");
diff --git a/hw/display/virtio-gpu-pci-gl.c b/hw/display/virtio-gpu-pci-gl.c
index 902dda345275..99b14a07185e 100644
--- a/hw/display/virtio-gpu-pci-gl.c
+++ b/hw/display/virtio-gpu-pci-gl.c
@@ -46,6 +46,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_gl_pci_info =
{
.instance_size = sizeof(VirtIOGPUGLPCI),
.instance_init = virtio_gpu_gl_initfn,
};
+module_obj(TYPE_VIRTIO_GPU_GL_PCI);
static void virtio_gpu_gl_pci_register_types(void)
{
@@ -53,3 +54,5 @@ static void virtio_gpu_gl_pci_register_types(void)
}
type_init(virtio_gpu_gl_pci_register_types)
+
+module_dep("hw-display-virtio-gpu-pci");
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d742a30aecf7..e36eee0c409b 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -64,6 +64,7 @@ static const TypeInfo virtio_gpu_pci_base_info = {
.class_init = virtio_gpu_pci_base_class_init,
.abstract = true
};
+module_obj(TYPE_VIRTIO_GPU_PCI_BASE);
#define TYPE_VIRTIO_GPU_PCI "virtio-gpu-pci"
typedef struct VirtIOGPUPCI VirtIOGPUPCI;
@@ -90,6 +91,7 @@ static const VirtioPCIDeviceTypeInfo virtio_gpu_pci_info = {
.instance_size = sizeof(VirtIOGPUPCI),
.instance_init = virtio_gpu_initfn,
};
+module_obj(TYPE_VIRTIO_GPU_PCI);
static void virtio_gpu_pci_register_types(void)
{
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index e183f4ecdaa5..6b7f643951fe 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1427,6 +1427,7 @@ static const TypeInfo virtio_gpu_info = {
.class_size = sizeof(VirtIOGPUClass),
.class_init = virtio_gpu_class_init,
};
+module_obj(TYPE_VIRTIO_GPU);
static void virtio_register_types(void)
{
diff --git a/hw/display/virtio-vga-gl.c b/hw/display/virtio-vga-gl.c
index c971340ebb1a..f22549097c5e 100644
--- a/hw/display/virtio-vga-gl.c
+++ b/hw/display/virtio-vga-gl.c
@@ -36,6 +36,7 @@ static VirtioPCIDeviceTypeInfo virtio_vga_gl_info = {
.instance_size = sizeof(VirtIOVGAGL),
.instance_init = virtio_vga_gl_inst_initfn,
};
+module_obj(TYPE_VIRTIO_VGA_GL);
static void virtio_vga_register_types(void)
{
@@ -45,3 +46,5 @@ static void virtio_vga_register_types(void)
[PATCH v4 11/34] modules: add ui module annotations
Signed-off-by: Gerd Hoffmann
---
ui/egl-headless.c | 4
ui/gtk.c | 4
ui/sdl2.c | 4
ui/spice-app.c| 3 +++
ui/spice-core.c | 5 +
5 files changed, 20 insertions(+)
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index da377a74af69..75404e0e8700 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -213,3 +213,7 @@ static void register_egl(void)
}
type_init(register_egl);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/gtk.c b/ui/gtk.c
index 98046f577b9d..376b4d528daa 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2333,3 +2333,7 @@ static void register_gtk(void)
}
type_init(register_gtk);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/sdl2.c b/ui/sdl2.c
index a203cb0239e1..36d9010cb6c1 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -918,3 +918,7 @@ static void register_sdl1(void)
}
type_init(register_sdl1);
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
diff --git a/ui/spice-app.c b/ui/spice-app.c
index 4325ac2d9c54..641f4a9d53e3 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -221,3 +221,6 @@ static void register_spice_app(void)
}
type_init(register_spice_app);
+
+module_dep("ui-spice-core");
+module_dep("chardev-spice");
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 272d19b0c152..86d43783acac 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -1037,3 +1037,8 @@ static void spice_register_config(void)
qemu_add_opts(_spice_opts);
}
opts_init(spice_register_config);
+module_opts("spice");
+
+#ifdef CONFIG_OPENGL
+module_dep("ui-opengl");
+#endif
--
2.31.1
Re: [PATCH v2 2/2] iotests/307: Test iothread conflict for exports
24.06.2021 11:38, Max Reitz wrote: Passing fixed-iothread=true should make iothread conflicts fatal, whereas fixed-iothread=false should not. Combine the second case with an error condition that is checked after the iothread is handled, to verify that qemu does not crash if there is such an error after changing the iothread failed. Signed-off-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy Tested-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
[PATCH v4 08/34] modules: add audio module annotations
Signed-off-by: Gerd Hoffmann
---
audio/spiceaudio.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/audio/spiceaudio.c b/audio/spiceaudio.c
index 999bfbde47c5..a8d370fe6f31 100644
--- a/audio/spiceaudio.c
+++ b/audio/spiceaudio.c
@@ -317,3 +317,5 @@ static void register_audio_spice(void)
audio_driver_register(_audio_driver);
}
type_init(register_audio_spice);
+
+module_dep("ui-spice-core");
--
2.31.1
[PATCH v4 05/34] modules: add qxl module annotations
Signed-off-by: Gerd Hoffmann
---
hw/display/qxl.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 6e1f8ff1b2a7..84f99088e0a0 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2522,6 +2522,7 @@ static const TypeInfo qxl_primary_info = {
.parent= TYPE_PCI_QXL,
.class_init= qxl_primary_class_init,
};
+module_obj("qxl-vga");
static void qxl_secondary_class_init(ObjectClass *klass, void *data)
{
@@ -2538,6 +2539,7 @@ static const TypeInfo qxl_secondary_info = {
.parent= TYPE_PCI_QXL,
.class_init= qxl_secondary_class_init,
};
+module_obj("qxl");
static void qxl_register_types(void)
{
@@ -2547,3 +2549,5 @@ static void qxl_register_types(void)
}
type_init(qxl_register_types)
+
+module_dep("ui-spice-core");
--
2.31.1
[PATCH v4 07/34] modules: add chardev module annotations
Signed-off-by: Gerd Hoffmann
---
chardev/baum.c | 1 +
chardev/spice.c | 4
2 files changed, 5 insertions(+)
diff --git a/chardev/baum.c b/chardev/baum.c
index 5deca778bc44..79d618e35045 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -680,6 +680,7 @@ static const TypeInfo char_braille_type_info = {
.instance_finalize = char_braille_finalize,
.class_init = char_braille_class_init,
};
+module_obj(TYPE_CHARDEV_BRAILLE);
static void register_types(void)
{
diff --git a/chardev/spice.c b/chardev/spice.c
index 1104426e3a11..3ffb3fdc0dac 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -366,6 +366,7 @@ static const TypeInfo char_spice_type_info = {
.class_init = char_spice_class_init,
.abstract = true,
};
+module_obj(TYPE_CHARDEV_SPICE);
static void char_spicevmc_class_init(ObjectClass *oc, void *data)
{
@@ -396,6 +397,7 @@ static const TypeInfo char_spiceport_type_info = {
.parent = TYPE_CHARDEV_SPICE,
.class_init = char_spiceport_class_init,
};
+module_obj(TYPE_CHARDEV_SPICEPORT);
static void register_types(void)
{
@@ -405,3 +407,5 @@ static void register_types(void)
}
type_init(register_types);
+
+module_dep("ui-spice-core");
--
2.31.1
[PATCH v4 02/34] modules: collect module meta-data
Add script to collect the module meta-data from the source code,
store the results in *.modinfo files.
Signed-off-by: Gerd Hoffmann
---
scripts/modinfo-collect.py | 67 ++
meson.build| 16 +
2 files changed, 83 insertions(+)
create mode 100755 scripts/modinfo-collect.py
diff --git a/scripts/modinfo-collect.py b/scripts/modinfo-collect.py
new file mode 100755
index ..4acb188c3e89
--- /dev/null
+++ b/scripts/modinfo-collect.py
@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+import os
+import sys
+import json
+import shlex
+import subprocess
+
+def find_command(src, target, compile_commands):
+for command in compile_commands:
+if command['file'] != src:
+continue
+if target != '' and command['command'].find(target) == -1:
+continue
+return command['command']
+return 'false'
+
+def process_command(src, command):
+skip = False
+arg = False
+out = []
+for item in shlex.split(command):
+if arg:
+out.append(x)
+arg = False
+continue
+if skip:
+skip = False
+continue
+if item == '-MF' or item == '-MQ' or item == '-o':
+skip = True
+continue
+if item == '-c':
+skip = True
+continue
+out.append(item)
+out.append('-DQEMU_MODINFO')
+out.append('-E')
+out.append(src)
+return out
+
+def main(args):
+target = ''
+if args[0] == '--target':
+args.pop(0)
+target = args.pop(0)
+print("MODINFO_DEBUG target %s" % target)
+arch = target[:-8] # cut '-softmmu'
+print("MODINFO_START arch \"%s\" MODINFO_END" % arch)
+with open('compile_commands.json') as f:
+compile_commands = json.load(f)
+for src in args:
+print("MODINFO_DEBUG src %s" % src)
+command = find_command(src, target, compile_commands)
+cmdline = process_command(src, command)
+print("MODINFO_DEBUG cmd", cmdline)
+result = subprocess.run(cmdline, stdout = subprocess.PIPE,
+universal_newlines = True)
+if result.returncode != 0:
+sys.exit(result.returncode)
+for line in result.stdout.split('\n'):
+if line.find('MODINFO') != -1:
+print(line)
+
+if __name__ == "__main__":
+main(sys.argv[1:])
diff --git a/meson.build b/meson.build
index d8a92666fbcd..30ca06c6c2d4 100644
--- a/meson.build
+++ b/meson.build
@@ -2021,6 +2021,9 @@ subdir('tests/qtest/fuzz')
# Library dependencies #
+modinfo_collect = find_program('scripts/modinfo-collect.py')
+modinfo_files = []
+
block_mods = []
softmmu_mods = []
foreach d, list : modules
@@ -2034,6 +2037,19 @@ foreach d, list : modules
else
softmmu_mods += sl
endif
+ if module_ss.sources() != []
+# FIXME: Should use sl.extract_all_objects(recursive: true) as
+# input. Sources can be used multiple times but objects are
+# unique when it comes to lookup in compile_commands.json.
+# Depnds on a mesion version with
+# https://github.com/mesonbuild/meson/pull/8900
+modinfo_files += custom_target(d + '-' + m + '.modinfo',
+ output: d + '-' + m + '.modinfo',
+ input: module_ss.sources(),
+ capture: true,
+ build_by_default: true, # to be removed
when added to a target
+ command: [modinfo_collect, '@INPUT@'])
+ endif
else
if d == 'block'
block_ss.add_all(module_ss)
--
2.31.1
[PATCH v4 03/34] modules: generate modinfo.c
Add script to generate C source with a small
database containing the module meta-data.
Signed-off-by: Gerd Hoffmann
---
scripts/modinfo-generate.py | 84 +
include/qemu/module.h | 17
softmmu/vl.c| 4 ++
util/module.c | 11 +
meson.build | 13 +-
5 files changed, 128 insertions(+), 1 deletion(-)
create mode 100755 scripts/modinfo-generate.py
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
new file mode 100755
index ..a6d98a6bc4cc
--- /dev/null
+++ b/scripts/modinfo-generate.py
@@ -0,0 +1,84 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+import os
+import sys
+
+def print_array(name, values):
+if len(values) == 0:
+return
+list = ", ".join(values)
+print(".%s = ((const char*[]){ %s, NULL })," % (name, list))
+
+def parse_line(line):
+kind = ""
+data = ""
+get_kind = False
+get_data = False
+for item in line.split():
+if item == "MODINFO_START":
+get_kind = True
+continue
+if item.startswith("MODINFO_END"):
+get_data = False
+continue
+if get_kind:
+kind = item
+get_kind = False
+get_data = True
+continue
+if get_data:
+data += " " + item
+continue
+return (kind, data)
+
+def generate(name, lines):
+arch = ""
+objs = []
+deps = []
+opts = []
+for line in lines:
+if line.find("MODINFO_START") != -1:
+(kind, data) = parse_line(line)
+if kind == 'obj':
+objs.append(data)
+elif kind == 'dep':
+deps.append(data)
+elif kind == 'opts':
+opts.append(data)
+elif kind == 'arch':
+arch = data;
+else:
+print("unknown:", kind)
+exit(1)
+
+print(".name = \"%s\"," % name)
+if arch != "":
+print(".arch = %s," % arch)
+print_array("objs", objs)
+print_array("deps", deps)
+print_array("opts", opts)
+print("},{");
+
+def print_pre():
+print("/* generated by scripts/modinfo-generate.py */")
+print("#include \"qemu/osdep.h\"")
+print("#include \"qemu/module.h\"")
+print("const QemuModinfo qemu_modinfo[] = {{")
+
+def print_post():
+print("/* end of list */")
+print("}};")
+
+def main(args):
+print_pre()
+for modinfo in args:
+with open(modinfo) as f:
+lines = f.readlines()
+print("/* %s */" % modinfo)
+(basename, ext) = os.path.splitext(modinfo)
+generate(basename, lines)
+print_post()
+
+if __name__ == "__main__":
+main(sys.argv[1:])
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 81ef086da023..a98748d501d3 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -98,4 +98,21 @@ void module_load_qom_all(void);
/* module registers QemuOpts */
#define module_opts(name) modinfo(opts, name)
+/*
+ * module info database
+ *
+ * scripts/modinfo-generate.c will build this using the data collected
+ * by scripts/modinfo-collect.py
+ */
+typedef struct QemuModinfo QemuModinfo;
+struct QemuModinfo {
+const char *name;
+const char *arch;
+const char **objs;
+const char **deps;
+const char **opts;
+};
+extern const QemuModinfo qemu_modinfo[];
+void module_init_info(const QemuModinfo *info);
+
#endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index feb4d201f30f..e001505d79f7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2740,6 +2740,10 @@ void qemu_init(int argc, char **argv, char **envp)
error_init(argv[0]);
qemu_init_exec_dir(argv[0]);
+#ifdef CONFIG_MODULES
+module_init_info(qemu_modinfo);
+#endif
+
qemu_init_subsystems();
/* first pass of option parsing */
diff --git a/util/module.c b/util/module.c
index eee8ff2de136..8d3e8275b9f7 100644
--- a/util/module.c
+++ b/util/module.c
@@ -110,6 +110,17 @@ void module_call_init(module_init_type type)
}
#ifdef CONFIG_MODULES
+
+static const QemuModinfo module_info_stub[] = { {
+/* end of list */
+} };
+static const QemuModinfo *module_info = module_info_stub;
+
+void module_init_info(const QemuModinfo *info)
+{
+module_info = info;
+}
+
static int module_load_file(const char *fname, bool mayfail, bool
export_symbols)
{
GModule *g_module;
diff --git a/meson.build b/meson.build
index 30ca06c6c2d4..6ae2b9ab9443 100644
--- a/meson.build
+++ b/meson.build
@@ -2022,6 +2022,7 @@ subdir('tests/qtest/fuzz')
modinfo_collect = find_program('scripts/modinfo-collect.py')
+modinfo_generate = find_program('scripts/modinfo-generate.py')
modinfo_files = []
block_mods = []
@@ -2047,7 +2048,6 @@ foreach d, list : modules
output: d + '-' + m + '.modinfo',
[PATCH v4 04/34] modules: check if all dependencies can be satisfied
From: "Jose R. Ziviani"
Verifies if all dependencies are correctly listed in the modinfo.c too
and stop the builds if they're not.
Signed-off-by: Jose R. Ziviani
Signed-off-by: Gerd Hoffmann
---
scripts/modinfo-generate.py | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/modinfo-generate.py b/scripts/modinfo-generate.py
index a6d98a6bc4cc..f559eed0077a 100755
--- a/scripts/modinfo-generate.py
+++ b/scripts/modinfo-generate.py
@@ -59,6 +59,7 @@ def generate(name, lines):
print_array("deps", deps)
print_array("opts", opts)
print("},{");
+return deps
def print_pre():
print("/* generated by scripts/modinfo-generate.py */")
@@ -71,14 +72,26 @@ def print_post():
print("}};")
def main(args):
+deps = {}
print_pre()
for modinfo in args:
with open(modinfo) as f:
lines = f.readlines()
print("/* %s */" % modinfo)
(basename, ext) = os.path.splitext(modinfo)
-generate(basename, lines)
+deps[basename] = generate(basename, lines)
print_post()
+flattened_deps = {flat.strip('" ') for dep in deps.values() for flat in
dep}
+error = False
+for dep in flattened_deps:
+if dep not in deps.keys():
+print("Dependency {} cannot be satisfied".format(dep),
+ file=sys.stderr)
+error = True
+
+if error:
+exit(1)
+
if __name__ == "__main__":
main(sys.argv[1:])
--
2.31.1
[PATCH v4 01/34] modules: add modinfo macros
Add macros for module info annotations. Instead of having that module meta-data stored in lists in util/module.c place directly in the module source code. Signed-off-by: Gerd Hoffmann --- include/qemu/module.h | 25 + 1 file changed, 25 insertions(+) diff --git a/include/qemu/module.h b/include/qemu/module.h index 944d403cbd15..81ef086da023 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -73,4 +73,29 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail); void module_load_qom_one(const char *type); void module_load_qom_all(void); +/* + * module info annotation macros + * + * scripts/modinfo-collect.py will collect module info, + * using the preprocessor and -DQEMU_MODINFO + */ +#ifdef QEMU_MODINFO +# define modinfo(kind, value) \ +MODINFO_START kind value MODINFO_END +#else +# define modinfo(kind, value) +#endif + +/* module implements QOM type */ +#define module_obj(name) modinfo(obj, name) + +/* module has a dependency on */ +#define module_dep(name) modinfo(dep, name) + +/* module is for target architecture */ +#define module_arch(name) modinfo(arch, name) + +/* module registers QemuOpts */ +#define module_opts(name) modinfo(opts, name) + #endif -- 2.31.1
[PATCH v4 00/34] modules: add meta-data database
This patch series adds support for module meta-data. Today this is either hard-coded in qemu (see qemu_load_module_for_opts) or handled with manually maintained lists in util/module (see module_deps[] and qom_modules[]). This series replaced that scheme with annotation macros, so the meta-data can go into the module source code and -- for example -- the module_obj() annotations can go next to the TypeInfo struct for the object class. Patches 1-3 put the infrastructure in place: Add the annotation macros, add a script to collect the meta-data, add a script to compile the meta-data into C source code which we can then add to qemu. Patch 4 - check module dependencies (Jose, new in v4). Patches 5-13 add annotations macros to the modules we have. Patches 14-16 put the modinfo database into use and remove the module_deps[] and qom_modules[] lists. Patch 16 adds two tracepoints for easier trouble-shooting. Patches 18-20 add support for target-specific modules. Patches 21-24 add documentation for all of the above (new in v4, was separate series). Patches 25-29 start building accelerators modular. So far it is only qtest (all archs) and a small fraction of tcg (x86 only). Patches 30-34 add support for registering hmp commands so they can be implemented as module (new in v4, was separate series). take care, Gerd Gerd Hoffmann (33): modules: add modinfo macros modules: collect module meta-data modules: generate modinfo.c modules: add qxl module annotations modules: add virtio-gpu module annotations modules: add chardev module annotations modules: add audio module annotations modules: add usb-redir module annotations modules: add ccid module annotations modules: add ui module annotations modules: add s390x module annotations modules: add block module annotations modules: use modinfo for dependencies modules: use modinfo for qom load modules: use modinfo for qemu opts load modules: add tracepoints modules: check arch and block load on mismatch modules: check arch on qom lookup modules: target-specific module build infrastructure modules: add documentation for module sourcesets modules: add module_obj() note to QOM docs modules: module.h kerneldoc annotations modules: hook up modules.h to docs build accel: autoload modules accel: add qtest module annotations accel: build qtest modular accel: add tcg module annotations accel: build tcg modular monitor: allow register hmp commands usb: drop usb_host_dev_is_scsi_storage hook monitor/usb: register 'info usbhost' dynamically usb: build usb-host as module monitor/tcg: move tcg hmp commands to accel/tcg, register them dynamically Jose R. Ziviani (1): modules: check if all dependencies can be satisfied scripts/modinfo-collect.py | 67 +++ scripts/modinfo-generate.py | 97 include/hw/usb.h| 7 +- include/monitor/monitor.h | 3 + include/qemu/module.h | 74 accel/accel-common.c| 2 +- accel/accel-softmmu.c | 2 +- accel/qtest/qtest.c | 2 + accel/tcg/hmp.c | 29 + accel/tcg/tcg-accel-ops.c | 1 + accel/tcg/tcg-all.c | 1 + audio/spiceaudio.c | 2 + block/iscsi-opts.c | 1 + chardev/baum.c | 1 + chardev/spice.c | 4 + hw/display/qxl.c| 4 + hw/display/vhost-user-gpu-pci.c | 1 + hw/display/vhost-user-gpu.c | 1 + hw/display/vhost-user-vga.c | 1 + hw/display/virtio-gpu-base.c| 1 + hw/display/virtio-gpu-gl.c | 3 + hw/display/virtio-gpu-pci-gl.c | 3 + hw/display/virtio-gpu-pci.c | 2 + hw/display/virtio-gpu.c | 1 + hw/display/virtio-vga-gl.c | 3 + hw/display/virtio-vga.c | 2 + hw/ppc/spapr.c | 2 +- hw/s390x/virtio-ccw-gpu.c | 3 + hw/usb/ccid-card-emulated.c | 1 + hw/usb/ccid-card-passthru.c | 1 + hw/usb/dev-storage-bot.c| 1 + hw/usb/dev-storage-classic.c| 1 + hw/usb/dev-uas.c| 1 + hw/usb/host-libusb.c| 38 ++ hw/usb/host-stub.c | 45 --- hw/usb/redirect.c | 1 + monitor/hmp.c | 7 ++ monitor/misc.c | 34 +++--- softmmu/vl.c| 24 ++-- stubs/module-opts.c | 4 - ui/egl-headless.c | 4 + ui/gtk.c| 4 + ui/sdl2.c | 4 + ui/spice-app.c | 3 + ui/spice-core.c | 5 + util/module.c | 200 ++-- accel/qtest/meson.build | 8 +- accel/tcg/meson.build | 6 +- docs/devel/build-system.rst | 17 +++ docs/devel/index.rst| 1 + docs/devel/modules.rst | 5 + docs/devel/qom.rst | 8
Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again
Hi,
On Thu, 24 Jun 2021 at 02:01, Philippe Mathieu-Daudé wrote:
>
> In commit a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card
> sizes") we tried to protect us from CVE-2020-13253 by only allowing
> card with power-of-2 sizes. However doing so we disrupted valid user
> cases. As a compromise, allow any card size, but warn only power of 2
> sizes are supported, still suggesting the user how to increase a
> card with 'qemu-img resize'.
>
> Cc: Tom Yan
> Cc: Warner Losh
> Buglink: https://bugs.launchpad.net/qemu/+bug/1910586
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 25 +
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c8dd11bad1..cab4aab1475 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2131,23 +2131,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
> blk_size = blk_getlength(sd->blk);
> if (blk_size > 0 && !is_power_of_2(blk_size)) {
> int64_t blk_size_aligned = pow2ceil(blk_size);
> -char *blk_size_str;
> +g_autofree char *blk_size_s = size_to_str(blk_size);
> +g_autofree char *blk_size_aligned_s =
> size_to_str(blk_size_aligned);
>
> -blk_size_str = size_to_str(blk_size);
> -error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> -g_free(blk_size_str);
> -
> -blk_size_str = size_to_str(blk_size_aligned);
> -error_append_hint(errp,
> - "SD card size has to be a power of 2, e.g.
> %s.\n"
> - "You can resize disk images with"
> - " 'qemu-img resize '\n"
> - "(note that this will lose data if you make
> the"
> - " image smaller than it currently is).\n",
> - blk_size_str);
> -g_free(blk_size_str);
> -
> -return;
> +warn_report("SD card size is not a power of 2 (%s). It might
> work"
> +" but is not supported by QEMU. If possible, resize"
> +" your disk image to %s with:",
> +blk_size_s, blk_size_aligned_s);
> +warn_report(" 'qemu-img resize '");
> +warn_report("(note that this will lose data if you make the"
> +" image smaller than it currently is).");
Not trying to be picky, but I don't think this is much better. IMHO
it's quite irresponsible to give a warning like that, leaving users in
a state like "Should I use it or not then?", without giving a concrete
reference to what exactly might/would lead to the warned problem.
I really think we should get (/ have gotten) things clear first. What
exactly is the bug we have been talking about here? I mean like, where
does it occur and what's the nature of it.
1. Is it specific to a certain type / model of backend / physical
storage device that will be made use of by qemu for the emulated
storage? (I presume not since you mention about image, unless you
irrationally related/bound the emulated storage type and the physical
storage type together.)
2. Does it have anything to do with a certain flaw in qemu itself?
Like the code that does read/write operation is flawed that it cannot
be handled by a certain *proper* backend device?
3. Or is it actually a bug in a certain driver / firmware blob that
will be used by an *emulated* device in the guest? In that case, can
we emulate another model so that it won't be using the problematic
driver / firmware?
Also, could you provide any information / reference to what the bug
really is? Like a bug report (for the problem itself, not some vague
claim that qemu should workaround the problem)?
> }
>
> ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> --
> 2.31.1
>
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
24.06.2021 13:16, Max Reitz wrote:
On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
23.06.2021 18:01, Max Reitz wrote:
.bdrv_co_block_status() implementations are free to return a *pnum that
exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp
*pnum as necessary.
On the other hand, if drivers' implementations return values for *pnum
that are as large as possible, our recently introduced block-status
cache will become more effective.
So, make a note in block_int.h that @bytes is no upper limit for *pnum.
Suggested-by: Eric Blake
Signed-off-by: Max Reitz
---
include/block/block_int.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fcb599dd1c..f85b96ed99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -347,6 +347,11 @@ struct BlockDriver {
* clamped to bdrv_getlength() and aligned to request_alignment,
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero value.
+ *
+ * Note that @bytes is just a hint on how big of a region the
+ * caller wants to inspect. It is not a limit on *pnum.
+ * Implementations are free to return larger values of *pnum if
+ * doing so does not incur a performance penalty.
Worth mention that the cache will benefit of it?
Oh, right, absolutely. Like so:
"block/io.c's bdrv_co_block_status() will clamp *pnum before returning it to its
caller, but it itself can still make use of the unclamped *pnum value. Specifically, the
block-status cache for protocol nodes will benefit from storing as large a region as
possible."
Sounds good. Do you mean this as an addition or substitution? If the latter, I'd keep
"if doing so does not incur a performance penalty"
--
Best regards,
Vladimir
Re: [PATCH v2 1/2] block/export: Conditionally ignore set-context error
24.06.2021 11:38, Max Reitz wrote:
When invoking block-export-add with some iothread and
fixed-iothread=false, and changing the node's iothread fails, the error
is supposed to be ignored.
However, it is still stored in *errp, which is wrong. If a second error
occurs, the "*errp must be NULL" assertion in error_setv() fails:
qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion
`*errp == NULL' failed.
So if fixed-iothread=false, we should ignore the error by passing NULL
to bdrv_try_set_aio_context().
Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef
("block/export: add iothread and fixed-iothread options")
Signed-off-by: Max Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again
On Wed, Jun 23, 2021 at 08:00:21PM +0200, Philippe Mathieu-Daudé wrote:
> In commit a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card
> sizes") we tried to protect us from CVE-2020-13253 by only allowing
> card with power-of-2 sizes. However doing so we disrupted valid user
> cases. As a compromise, allow any card size, but warn only power of 2
> sizes are supported, still suggesting the user how to increase a
> card with 'qemu-img resize'.
>
> Cc: Tom Yan
> Cc: Warner Losh
> Buglink: https://bugs.launchpad.net/qemu/+bug/1910586
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 25 +
> 1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c8dd11bad1..cab4aab1475 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2131,23 +2131,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
> blk_size = blk_getlength(sd->blk);
> if (blk_size > 0 && !is_power_of_2(blk_size)) {
> int64_t blk_size_aligned = pow2ceil(blk_size);
> -char *blk_size_str;
> +g_autofree char *blk_size_s = size_to_str(blk_size);
> +g_autofree char *blk_size_aligned_s =
> size_to_str(blk_size_aligned);
>
> -blk_size_str = size_to_str(blk_size);
> -error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> -g_free(blk_size_str);
> -
> -blk_size_str = size_to_str(blk_size_aligned);
> -error_append_hint(errp,
> - "SD card size has to be a power of 2, e.g.
> %s.\n"
> - "You can resize disk images with"
> - " 'qemu-img resize '\n"
> - "(note that this will lose data if you make
> the"
> - " image smaller than it currently is).\n",
> - blk_size_str);
> -g_free(blk_size_str);
> -
> -return;
> +warn_report("SD card size is not a power of 2 (%s). It might
> work"
> +" but is not supported by QEMU. If possible, resize"
> +" your disk image to %s with:",
> +blk_size_s, blk_size_aligned_s);
> +warn_report(" 'qemu-img resize '");
> +warn_report("(note that this will lose data if you make the"
> +" image smaller than it currently is).");
In what scenarios will non-power of 2 not work and what is the effect ?
Is it a QEMU bug or not ?
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote:
23.06.2021 18:01, Max Reitz wrote:
.bdrv_co_block_status() implementations are free to return a *pnum that
exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp
*pnum as necessary.
On the other hand, if drivers' implementations return values for *pnum
that are as large as possible, our recently introduced block-status
cache will become more effective.
So, make a note in block_int.h that @bytes is no upper limit for *pnum.
Suggested-by: Eric Blake
Signed-off-by: Max Reitz
---
include/block/block_int.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fcb599dd1c..f85b96ed99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -347,6 +347,11 @@ struct BlockDriver {
* clamped to bdrv_getlength() and aligned to request_alignment,
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero value.
+ *
+ * Note that @bytes is just a hint on how big of a region the
+ * caller wants to inspect. It is not a limit on *pnum.
+ * Implementations are free to return larger values of *pnum if
+ * doing so does not incur a performance penalty.
Worth mention that the cache will benefit of it?
Oh, right, absolutely. Like so:
"block/io.c's bdrv_co_block_status() will clamp *pnum before returning
it to its caller, but it itself can still make use of the unclamped
*pnum value. Specifically, the block-status cache for protocol nodes
will benefit from storing as large a region as possible."
?
Max
Re: [PATCH v2 2/6] block: block-status cache for data regions
23.06.2021 18:01, Max Reitz wrote:
As we have attempted before
(https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06451.html,
"file-posix: Cache lseek result for data regions";
https://lists.nongnu.org/archive/html/qemu-block/2021-02/msg00934.html,
"file-posix: Cache next hole"), this patch seeks to reduce the number of
SEEK_DATA/HOLE operations the file-posix driver has to perform. The
main difference is that this time it is implemented as part of the
general block layer code.
The problem we face is that on some filesystems or in some
circumstances, SEEK_DATA/HOLE is unreasonably slow. Given the
implementation is outside of qemu, there is little we can do about its
performance.
We have already introduced the want_zero parameter to
bdrv_co_block_status() to reduce the number of SEEK_DATA/HOLE calls
unless we really want zero information; but sometimes we do want that
information, because for files that consist largely of zero areas,
special-casing those areas can give large performance boosts. So the
real problem is with files that consist largely of data, so that
inquiring the block status does not gain us much performance, but where
such an inquiry itself takes a lot of time.
To address this, we want to cache data regions. Most of the time, when
bad performance is reported, it is in places where the image is iterated
over from start to end (qemu-img convert or the mirror job), so a simple
yet effective solution is to cache only the current data region.
(Note that only caching data regions but not zero regions means that
returning false information from the cache is not catastrophic: Treating
zeroes as data is fine. While we try to invalidate the cache on zero
writes and discards, such incongruences may still occur when there are
other processes writing to the image.)
We only use the cache for nodes without children (i.e. protocol nodes),
because that is where the problem is: Drivers that rely on block-status
implementations outside of qemu (e.g. SEEK_DATA/HOLE).
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/307
Signed-off-by: Max Reitz
I'm new to RCU, so my review can't be reliable..
---
include/block/block_int.h | 47 ++
block.c | 84 +++
block/io.c| 61 ++--
3 files changed, 189 insertions(+), 3 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a8f9598102..fcb599dd1c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -832,6 +832,22 @@ struct BdrvChild {
QLIST_ENTRY(BdrvChild) next_parent;
};
+/*
+ * Allows bdrv_co_block_status() to cache one data region for a
+ * protocol node.
+ *
+ * @valid: Whether the cache is valid (should be accessed with atomic
+ * functions so this can be reset by RCU readers)
+ * @data_start: Offset where we know (or strongly assume) is data
+ * @data_end: Offset where the data region ends (which is not necessarily
+ *the start of a zeroed region)
+ */
+typedef struct BdrvBlockStatusCache {
+bool valid;
+int64_t data_start;
+int64_t data_end;
+} BdrvBlockStatusCache;
+
struct BlockDriverState {
/* Protected by big QEMU lock or read-only after opening. No special
* locking needed during I/O...
@@ -997,6 +1013,11 @@ struct BlockDriverState {
/* BdrvChild links to this node may never be frozen */
bool never_freeze;
+
+/* Lock for block-status cache RCU writers */
+CoMutex bsc_modify_lock;
+/* Always non-NULL, but must only be dereferenced under an RCU read guard
*/
+BdrvBlockStatusCache *block_status_cache;> };
struct BlockBackendRootState {
@@ -1422,4 +1443,30 @@ static inline BlockDriverState
*bdrv_primary_bs(BlockDriverState *bs)
*/
void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
+/**
+ * Check whether the given offset is in the cached block-status data
+ * region.
+ *
+ * If it is, and @pnum is not NULL, *pnum is set to
+ * `bsc.data_end - offset`, i.e. how many bytes, starting from
+ * @offset, are data (according to the cache).
+ * Otherwise, *pnum is not touched.
+ */
+bool bdrv_bsc_is_data(BlockDriverState *bs, int64_t offset, int64_t *pnum);
+
+/**
+ * If [offset, offset + bytes) overlaps with the currently cached
+ * block-status region, invalidate the cache.
+ *
+ * (To be used by I/O paths that cause data regions to be zero or
+ * holes.)
+ */
+void bdrv_bsc_invalidate_range(BlockDriverState *bs,
+ int64_t offset, int64_t bytes);
+
+/**
+ * Mark the range [offset, offset + bytes) as a data region.
+ */
+void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t bytes);
+
#endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index 3f456892d0..9ab9459f7a 100644
--- a/block.c
+++ b/block.c
@@ -49,6 +49,8 @@
#include "qemu/timer.h"
#include "qemu/cutils.h"
#include "qemu/id.h"
+#include
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
23.06.2021 18:01, Max Reitz wrote:
.bdrv_co_block_status() implementations are free to return a *pnum that
exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp
*pnum as necessary.
On the other hand, if drivers' implementations return values for *pnum
that are as large as possible, our recently introduced block-status
cache will become more effective.
So, make a note in block_int.h that @bytes is no upper limit for *pnum.
Suggested-by: Eric Blake
Signed-off-by: Max Reitz
---
include/block/block_int.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fcb599dd1c..f85b96ed99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -347,6 +347,11 @@ struct BlockDriver {
* clamped to bdrv_getlength() and aligned to request_alignment,
* as well as non-NULL pnum, map, and file; in turn, the driver
* must return an error or set pnum to an aligned non-zero value.
+ *
+ * Note that @bytes is just a hint on how big of a region the
+ * caller wants to inspect. It is not a limit on *pnum.
+ * Implementations are free to return larger values of *pnum if
+ * doing so does not incur a performance penalty.
Worth mention that the cache will benefit of it?
Reviewed-by: Vladimir Sementsov-Ogievskiy
*/
int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
--
Best regards,
Vladimir
[PATCH v2 2/2] iotests/307: Test iothread conflict for exports
Passing fixed-iothread=true should make iothread conflicts fatal,
whereas fixed-iothread=false should not.
Combine the second case with an error condition that is checked after
the iothread is handled, to verify that qemu does not crash if there is
such an error after changing the iothread failed.
Signed-off-by: Max Reitz
---
tests/qemu-iotests/307 | 15 +++
tests/qemu-iotests/307.out | 8
2 files changed, 23 insertions(+)
diff --git a/tests/qemu-iotests/307 b/tests/qemu-iotests/307
index c7685347bc..b429b5aa50 100755
--- a/tests/qemu-iotests/307
+++ b/tests/qemu-iotests/307
@@ -41,9 +41,11 @@ with iotests.FilePath('image') as img, \
iotests.log('=== Launch VM ===')
vm.add_object('iothread,id=iothread0')
+vm.add_object('iothread,id=iothread1')
vm.add_blockdev(f'file,filename={img},node-name=file')
vm.add_blockdev(f'{iotests.imgfmt},file=file,node-name=fmt')
vm.add_blockdev('raw,file=file,node-name=ro,read-only=on')
+vm.add_blockdev('null-co,node-name=null')
vm.add_device(f'id=scsi0,driver=virtio-scsi,iothread=iothread0')
vm.launch()
@@ -74,6 +76,19 @@ with iotests.FilePath('image') as img, \
vm.qmp_log('query-block-exports')
iotests.qemu_nbd_list_log('-k', socket)
+iotests.log('\n=== Add export with conflicting iothread ===')
+
+vm.qmp_log('device_add', id='sdb', driver='scsi-hd', drive='null')
+
+# Should fail because of fixed-iothread
+vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+ iothread='iothread1', fixed_iothread=True, writable=True)
+
+# Should ignore the iothread conflict, but then fail because of the
+# permission conflict (and not crash)
+vm.qmp_log('block-export-add', id='export1', type='nbd', node_name='null',
+ iothread='iothread1', fixed_iothread=False, writable=True)
+
iotests.log('\n=== Add a writable export ===')
# This fails because share-rw=off
diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out
index 4b0c7e155a..ec8d2be0e0 100644
--- a/tests/qemu-iotests/307.out
+++ b/tests/qemu-iotests/307.out
@@ -51,6 +51,14 @@ exports available: 1
base:allocation
+=== Add export with conflicting iothread ===
+{"execute": "device_add", "arguments": {"drive": "null", "driver": "scsi-hd",
"id": "sdb"}}
+{"return": {}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": true, "id":
"export1", "iothread": "iothread1", "node-name": "null", "type": "nbd",
"writable": true}}
+{"error": {"class": "GenericError", "desc": "Cannot change iothread of active
block backend"}}
+{"execute": "block-export-add", "arguments": {"fixed-iothread": false, "id":
"export1", "iothread": "iothread1", "node-name": "null", "type": "nbd",
"writable": true}}
+{"error": {"class": "GenericError", "desc": "Permission conflict on node
'null': permissions 'write' are both required by an unnamed block device (uses
node 'null' as 'root' child) and unshared by block device 'sdb' (uses node
'null' as 'root' child)."}}
+
=== Add a writable export ===
{"execute": "block-export-add", "arguments": {"description": "This is the
writable second export", "id": "export1", "name": "export1", "node-name":
"fmt", "type": "nbd", "writable": true, "writethrough": true}}
{"error": {"class": "GenericError", "desc": "Permission conflict on node
'fmt': permissions 'write' are both required by an unnamed block device (uses
node 'fmt' as 'root' child) and unshared by block device 'sda' (uses node 'fmt'
as 'root' child)."}}
--
2.31.1
[PATCH v2 1/2] block/export: Conditionally ignore set-context error
When invoking block-export-add with some iothread and
fixed-iothread=false, and changing the node's iothread fails, the error
is supposed to be ignored.
However, it is still stored in *errp, which is wrong. If a second error
occurs, the "*errp must be NULL" assertion in error_setv() fails:
qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion
`*errp == NULL' failed.
So if fixed-iothread=false, we should ignore the error by passing NULL
to bdrv_try_set_aio_context().
Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef
("block/export: add iothread and fixed-iothread options")
Signed-off-by: Max Reitz
---
block/export/export.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/export/export.c b/block/export/export.c
index fec7d9f738..6d3b9964c8 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -111,6 +111,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
if (export->has_iothread) {
IOThread *iothread;
AioContext *new_ctx;
+Error **set_context_errp;
iothread = iothread_by_id(export->iothread);
if (!iothread) {
@@ -120,7 +121,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error
**errp)
new_ctx = iothread_get_aio_context(iothread);
-ret = bdrv_try_set_aio_context(bs, new_ctx, errp);
+/* Ignore errors with fixed-iothread=false */
+set_context_errp = fixed_iothread ? errp : NULL;
+ret = bdrv_try_set_aio_context(bs, new_ctx, set_context_errp);
if (ret == 0) {
aio_context_release(ctx);
aio_context_acquire(new_ctx);
--
2.31.1
[PATCH v2 0/2] block/export: Conditionally ignore set-context error
Hi, I had this v2 lying around for quite some time but for some reason never sent it. I probably just forgot. Sorry. v1: https://lists.nongnu.org/archive/html/qemu-block/2021-04/msg00584.html Changes in v2: - Letting `bdrv_try_set_aio_context()` return an error and then just discarding it conditionally is kind of not right. If we want to ignore the error, decide so from the start: Depending on `fixed_iothread`, pass either `errp` or `NULL`. - Patch 2: Reference output has changed because of 30ebb9aa920. git-backport-diff against v1: Key: [] : patches are identical [] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/2:[0009] [FC] 'block/export: Conditionally ignore set-context error' 002/2:[0002] [FC] 'iotests/307: Test iothread conflict for exports' Max Reitz (2): block/export: Conditionally ignore set-context error iotests/307: Test iothread conflict for exports block/export/export.c | 5 - tests/qemu-iotests/307 | 15 +++ tests/qemu-iotests/307.out | 8 3 files changed, 27 insertions(+), 1 deletion(-) -- 2.31.1
Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again
On 6/24/21 4:50 AM, Alexander Bulekov wrote: > On 210623 2000, Philippe Mathieu-Daudé wrote: >> Hi Ubi-Wan Kenubi and Tom, >> >> In commit a9bcedd (SD card size has to be power of 2) we decided >> to restrict SD card size to avoid security problems (CVE-2020-13253) >> but this became not practical to some users. >> >> This RFC series tries to remove the limitation, keeping our >> functional tests working. It is unfinished work because I had to >> attend other topics, but sending it early as RFC to get feedback. >> I'll keep working when I get more time, except if one if you can >> help me. >> >> Alexander, could you generate a qtest reproducer with the fuzzer >> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054 > > I think that bug was already fixed - the reproducer no logner causes a > timeout on 6.0. Did I misunderstand something? That bug was fixed but now I'm changing the code and would like to feel sure I'm not re-introducing the problem, so having the reproducer in the tree would help. > I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3 > config. The only problem it found is this assert() (that exists without the > patch anyways): > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 Sigh. > Let me know if this is something you think I should report on gitlab.. Yes please :( > I'll leave the fuzzer running for another 24h or so, but otherwise I'm > happy to leave a Tested-by, once there is a V1 series > -Alex Thanks! Phil.
Re: [PATCH v4 1/7] file-posix: fix max_iov for /dev/sg devices
23.06.2021 18:42, Max Reitz wrote:
On 08.06.21 21:14, Vladimir Sementsov-Ogievskiy wrote:
08.06.2021 16:16, Paolo Bonzini wrote:
Even though it was only called for devices that have bs->sg set (which
must be character devices), sg_get_max_segments looked at /sys/dev/block
which only works for block devices.
On Linux the sg driver has its own way to provide the maximum number of
iovecs in a scatter/gather list, so add support for it. The block device
path is kept because it will be reinstated in the next patches.
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index f37dfc10b3..536998a1d6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1180,6 +1180,17 @@ static int sg_get_max_segments(int fd)
goto out;
}
+ if (S_ISCHR(st.st_mode)) {
Why not check "if (bs->sg) {" instead? It seems to be more consistent with
issuing SG_ ioctl. Or what I miss?
I dismissed this in v3, because I didn’t understand why you’d raise this point.
The function is called sg_*(), and it’s only called if bs->sg is true anyway.
So clearly we can use SG_ ioctls, because the whole function is intended only for
SG devices anyway.
This time, I looked forward, and perhaps starting at patch 4 I can understand
where you’re coming from, because then the function is used for host devices in
general.
So now I don’t particularly mind. I think it’s still clear that if there’s a host
device here that’s a character device, then that’s going to be an SG device, so I
don’t really have a preference between S_ISCHR() and bs->sg.
If I understand all correctly:
In this patch we don't need neither S_ISCHR nor bs->sg check: they both must pass
for sg devices. Starting from patch 4 we'll need here if (bs->sg) check.
+ if (ioctl(fd, SG_GET_SG_TABLESIZE, ) == 0) {
+ return ret;
+ }
+ return -ENOTSUP;
+ }
+
+ if (!S_ISBLK(st.st_mode)) {
+ return -ENOTSUP;
+ }
+
sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/max_segments",
major(st.st_rdev), minor(st.st_rdev));
sysfd = open(sysfspath, O_RDONLY);
--
Best regards,
Vladimir
[PATCH v5 3/5] block-copy: move progress_set_remaining in block_copy_task_end
Moving this function in task_end ensures to update the progress
anyways, even if there is an error.
It also helps in next patch, allowing task_end to have only
one critical section.
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-copy.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index bbcc53ff70..d44c41549e 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -248,6 +248,9 @@ static void coroutine_fn block_copy_task_end(BlockCopyTask
*task, int ret)
bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, task->bytes);
}
QLIST_REMOVE(task, list);
+progress_set_remaining(task->s->progress,
+ bdrv_get_dirty_count(task->s->copy_bitmap) +
+ task->s->in_flight_bytes);
qemu_co_queue_restart_all(>wait_queue);
}
@@ -638,9 +641,6 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
}
if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
block_copy_task_end(task, 0);
-progress_set_remaining(s->progress,
- bdrv_get_dirty_count(s->copy_bitmap) +
- s->in_flight_bytes);
trace_block_copy_skip_range(s, task->offset, task->bytes);
offset = task_end(task);
bytes = end - offset;
--
2.31.1
[PATCH v5 4/5] block-copy: add CoMutex lock
Group various structures fields, to better understand what we need to
protect with a lock and what doesn't need it.
Then, add a CoMutex to protect concurrent access of block-copy
data structures. This mutex also protects .copy_bitmap, because its thread-safe
API does not prevent it from assigning two tasks to the same
bitmap region.
Exceptions to the lock:
- .sleep_state is handled in the series "coroutine: new sleep/wake API"
and thus here left as TODO.
- .finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch, because are used also outside
coroutines.
- .skip_unallocated is atomic. Including it under the mutex would
increase the critical sections and make them also much more complex.
We can have it as atomic since it is only written from outside and
read by block-copy coroutines.
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-copy.c | 155 +
1 file changed, 116 insertions(+), 39 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index d44c41549e..52878ba57a 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -39,7 +39,7 @@ typedef enum {
static coroutine_fn int block_copy_task_entry(AioTask *task);
typedef struct BlockCopyCallState {
-/* IN parameters. Initialized in block_copy_async() and never changed. */
+/* Fields initialized in block_copy_async() and never changed. */
BlockCopyState *s;
int64_t offset;
int64_t bytes;
@@ -48,33 +48,60 @@ typedef struct BlockCopyCallState {
bool ignore_ratelimit;
BlockCopyAsyncCallbackFunc cb;
void *cb_opaque;
-
/* Coroutine where async block-copy is running */
Coroutine *co;
-/* To reference all call states from BlockCopyState */
-QLIST_ENTRY(BlockCopyCallState) list;
-
-/* State */
-int ret;
+/* Fields whose state changes throughout the execution */
bool finished;
-QemuCoSleep sleep;
+QemuCoSleep sleep; /* TODO: protect API with a lock */
bool cancelled;
+/* To reference all call states from BlockCopyState */
+QLIST_ENTRY(BlockCopyCallState) list;
-/* OUT parameters */
+/*
+ * Fields that report information about return values and erros.
+ * Protected by lock in BlockCopyState.
+ */
bool error_is_read;
+/*
+ * @ret is set concurrently by tasks under mutex. Only set once by first
+ * failed task (and untouched if no task failed).
+ * After finishing (call_state->finished is true), it is not modified
+ * anymore and may be safely read without mutex.
+ */
+int ret;
} BlockCopyCallState;
typedef struct BlockCopyTask {
AioTask task;
+/*
+ * Fields initialized in block_copy_task_create()
+ * and never changed.
+ */
BlockCopyState *s;
BlockCopyCallState *call_state;
int64_t offset;
-int64_t bytes;
+/*
+ * @method can also be set again in the while loop of
+ * block_copy_dirty_clusters(), but it is never accessed concurrently
+ * because the only other function that reads it is
+ * block_copy_task_entry() and it is invoked afterwards in the same
+ * iteration.
+ */
BlockCopyMethod method;
-QLIST_ENTRY(BlockCopyTask) list;
+
+/*
+ * Fields whose state changes throughout the execution
+ * Protected by lock in BlockCopyState.
+ */
CoQueue wait_queue; /* coroutines blocked on this task */
+/*
+ * Only protect the case of parallel read while updating @bytes
+ * value in block_copy_task_shrink().
+ */
+int64_t bytes;
+QLIST_ENTRY(BlockCopyTask) list;
} BlockCopyTask;
static int64_t task_end(BlockCopyTask *task)
@@ -90,17 +117,25 @@ typedef struct BlockCopyState {
*/
BdrvChild *source;
BdrvChild *target;
-BdrvDirtyBitmap *copy_bitmap;
-int64_t in_flight_bytes;
+
+/*
+ * Fields initialized in block_copy_state_new()
+ * and never changed.
+ */
int64_t cluster_size;
-BlockCopyMethod method;
int64_t max_transfer;
uint64_t len;
-QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls
*/
-QLIST_HEAD(, BlockCopyCallState) calls;
-
BdrvRequestFlags write_flags;
+/*
+ * Fields whose state changes throughout the execution
+ * Protected by lock.
+ */
+CoMutex lock;
+int64_t in_flight_bytes;
+BlockCopyMethod method;
+QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls
*/
+QLIST_HEAD(, BlockCopyCallState) calls;
/*
* skip_unallocated:
*
@@ -115,15 +150,15 @@ typedef struct BlockCopyState {
* skip unallocated regions, clear them in the copy_bitmap, and invoke
* block_copy_reset_unallocated() every time it does.
*/
-bool skip_unallocated;
-
+bool skip_unallocated; /* atomic */
+/* State fields that use a thread-safe API */
+BdrvDirtyBitmap *copy_bitmap;
ProgressMeter *progress;
-
[PATCH v5 2/5] block-copy: streamline choice of copy_range vs. read/write
From: Paolo Bonzini
Put the logic to determine the copy size in a separate function, so
that there is a simple state machine for the possible methods of
copying data from one BlockDriverState to the other.
Use .method instead of .copy_range as in-out argument, and
include also .zeroes as an additional copy method.
While at it, store the common computation of block_copy_max_transfer
into a new field of BlockCopyState, and make sure that we always
obey max_transfer; that's more efficient even for the
COPY_RANGE_READ_WRITE case.
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Emanuele Giuseppe Esposito
Signed-off-by: Paolo Bonzini
---
block/block-copy.c | 176 +++--
1 file changed, 90 insertions(+), 86 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index f0dbb4912b..bbcc53ff70 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -28,6 +28,14 @@
#define BLOCK_COPY_MAX_WORKERS 64
#define BLOCK_COPY_SLICE_TIME 1ULL /* ns */
+typedef enum {
+COPY_READ_WRITE_CLUSTER,
+COPY_READ_WRITE,
+COPY_WRITE_ZEROES,
+COPY_RANGE_SMALL,
+COPY_RANGE_FULL
+} BlockCopyMethod;
+
static coroutine_fn int block_copy_task_entry(AioTask *task);
typedef struct BlockCopyCallState {
@@ -64,8 +72,7 @@ typedef struct BlockCopyTask {
BlockCopyCallState *call_state;
int64_t offset;
int64_t bytes;
-bool zeroes;
-bool copy_range;
+BlockCopyMethod method;
QLIST_ENTRY(BlockCopyTask) list;
CoQueue wait_queue; /* coroutines blocked on this task */
} BlockCopyTask;
@@ -86,8 +93,8 @@ typedef struct BlockCopyState {
BdrvDirtyBitmap *copy_bitmap;
int64_t in_flight_bytes;
int64_t cluster_size;
-bool use_copy_range;
-int64_t copy_size;
+BlockCopyMethod method;
+int64_t max_transfer;
uint64_t len;
QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls
*/
QLIST_HEAD(, BlockCopyCallState) calls;
@@ -149,6 +156,24 @@ static bool coroutine_fn
block_copy_wait_one(BlockCopyState *s, int64_t offset,
return true;
}
+static int64_t block_copy_chunk_size(BlockCopyState *s)
+{
+switch (s->method) {
+case COPY_READ_WRITE_CLUSTER:
+return s->cluster_size;
+case COPY_READ_WRITE:
+case COPY_RANGE_SMALL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER),
+ s->max_transfer);
+case COPY_RANGE_FULL:
+return MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+ s->max_transfer);
+default:
+/* Cannot have COPY_WRITE_ZEROES here. */
+abort();
+}
+}
+
/*
* Search for the first dirty area in offset/bytes range and create task at
* the beginning of it.
@@ -158,8 +183,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState
*s,
int64_t offset, int64_t bytes)
{
BlockCopyTask *task;
-int64_t max_chunk = MIN_NON_ZERO(s->copy_size, call_state->max_chunk);
+int64_t max_chunk;
+max_chunk = MIN_NON_ZERO(block_copy_chunk_size(s), call_state->max_chunk);
if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
offset, offset + bytes,
max_chunk, , ))
@@ -183,7 +209,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState
*s,
.call_state = call_state,
.offset = offset,
.bytes = bytes,
-.copy_range = s->use_copy_range,
+.method = s->method,
};
qemu_co_queue_init(>wait_queue);
QLIST_INSERT_HEAD(>tasks, task, list);
@@ -267,28 +293,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
BdrvChild *target,
.len = bdrv_dirty_bitmap_size(copy_bitmap),
.write_flags = write_flags,
.mem = shres_create(BLOCK_COPY_MAX_MEM),
+.max_transfer = QEMU_ALIGN_DOWN(
+block_copy_max_transfer(source, target),
+cluster_size),
};
-if (block_copy_max_transfer(source, target) < cluster_size) {
+if (s->max_transfer < cluster_size) {
/*
* copy_range does not respect max_transfer. We don't want to bother
* with requests smaller than block-copy cluster size, so fallback to
* buffered copying (read and write respect max_transfer on their
* behalf).
*/
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
} else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
/* Compression supports only cluster-size writes and no copy-range. */
-s->use_copy_range = false;
-s->copy_size = cluster_size;
+s->method = COPY_READ_WRITE_CLUSTER;
} else {
/*
- * We enable copy-range, but keep small copy_size, until first
+ * If copy range enabled, start with
[PATCH v5 5/5] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState
By adding acquire/release pairs, we ensure that .ret and .error_is_read
fields are written by block_copy_dirty_clusters before .finished is true,
and that they are read by API user after .finished is true.
The atomic here are necessary because the fields are concurrently modified
in coroutines, and read outside.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/block-copy.h | 2 ++
block/block-copy.c | 37 ++---
2 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 338f2ea7fd..5c8278895c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -18,6 +18,8 @@
#include "block/block.h"
#include "qemu/co-shared-resource.h"
+/* All APIs are thread-safe */
+
typedef void (*BlockCopyAsyncCallbackFunc)(void *opaque);
typedef struct BlockCopyState BlockCopyState;
typedef struct BlockCopyCallState BlockCopyCallState;
diff --git a/block/block-copy.c b/block/block-copy.c
index 52878ba57a..594d9f4aec 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -52,9 +52,9 @@ typedef struct BlockCopyCallState {
Coroutine *co;
/* Fields whose state changes throughout the execution */
-bool finished;
+bool finished; /* atomic */
QemuCoSleep sleep; /* TODO: protect API with a lock */
-bool cancelled;
+bool cancelled; /* atomic */
/* To reference all call states from BlockCopyState */
QLIST_ENTRY(BlockCopyCallState) list;
@@ -667,7 +667,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
-while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
+while (bytes && aio_task_pool_status(aio) == 0 &&
+ !qatomic_read(_state->cancelled)) {
BlockCopyTask *task;
int64_t status_bytes;
@@ -779,7 +780,7 @@ static int coroutine_fn
block_copy_common(BlockCopyCallState *call_state)
do {
ret = block_copy_dirty_clusters(call_state);
-if (ret == 0 && !call_state->cancelled) {
+if (ret == 0 && !qatomic_read(_state->cancelled)) {
WITH_QEMU_LOCK_GUARD(>lock) {
/*
* Check that there is no task we still need to
@@ -815,9 +816,9 @@ static int coroutine_fn
block_copy_common(BlockCopyCallState *call_state)
* 2. We have waited for some intersecting block-copy request
*It may have failed and produced new dirty bits.
*/
-} while (ret > 0 && !call_state->cancelled);
+} while (ret > 0 && !qatomic_read(_state->cancelled));
-call_state->finished = true;
+qatomic_store_release(_state->finished, true);
if (call_state->cb) {
call_state->cb(call_state->cb_opaque);
@@ -880,44 +881,50 @@ void block_copy_call_free(BlockCopyCallState *call_state)
return;
}
-assert(call_state->finished);
+assert(qatomic_read(_state->finished));
g_free(call_state);
}
bool block_copy_call_finished(BlockCopyCallState *call_state)
{
-return call_state->finished;
+return qatomic_read(_state->finished);
}
bool block_copy_call_succeeded(BlockCopyCallState *call_state)
{
-return call_state->finished && !call_state->cancelled &&
-call_state->ret == 0;
+return qatomic_load_acquire(_state->finished) &&
+ !qatomic_read(_state->cancelled) &&
+ call_state->ret == 0;
}
bool block_copy_call_failed(BlockCopyCallState *call_state)
{
-return call_state->finished && !call_state->cancelled &&
-call_state->ret < 0;
+return qatomic_load_acquire(_state->finished) &&
+ !qatomic_read(_state->cancelled) &&
+ call_state->ret < 0;
}
bool block_copy_call_cancelled(BlockCopyCallState *call_state)
{
-return call_state->cancelled;
+return qatomic_read(_state->cancelled);
}
int block_copy_call_status(BlockCopyCallState *call_state, bool *error_is_read)
{
-assert(call_state->finished);
+assert(qatomic_load_acquire(_state->finished));
if (error_is_read) {
*error_is_read = call_state->error_is_read;
}
return call_state->ret;
}
+/*
+ * Note that cancelling and finishing are racy.
+ * User can cancel a block-copy that is already finished.
+ */
void block_copy_call_cancel(BlockCopyCallState *call_state)
{
-call_state->cancelled = true;
+qatomic_set(_state->cancelled, true);
block_copy_kick(call_state);
}
--
2.31.1
[PATCH v5 1/5] block-copy: small refactor in block_copy_task_entry and block_copy_common
Use a local variable instead of referencing BlockCopyState through a
BlockCopyCallState or BlockCopyTask every time.
This is in preparation for next patches.
No functional change intended.
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Emanuele Giuseppe Esposito
---
block/block-copy.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 943e30b7e6..f0dbb4912b 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -452,14 +452,15 @@ static void
block_copy_handle_copy_range_result(BlockCopyState *s,
static coroutine_fn int block_copy_task_entry(AioTask *task)
{
BlockCopyTask *t = container_of(task, BlockCopyTask, task);
+BlockCopyState *s = t->s;
bool error_is_read = false;
bool copy_range = t->copy_range;
int ret;
-ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
+ret = block_copy_do_copy(s, t->offset, t->bytes, t->zeroes,
_range, _is_read);
if (t->copy_range) {
-block_copy_handle_copy_range_result(t->s, copy_range);
+block_copy_handle_copy_range_result(s, copy_range);
}
if (ret < 0) {
if (!t->call_state->ret) {
@@ -467,9 +468,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
t->call_state->error_is_read = error_is_read;
}
} else {
-progress_work_done(t->s->progress, t->bytes);
+progress_work_done(s->progress, t->bytes);
}
-co_put_to_shres(t->s->mem, t->bytes);
+co_put_to_shres(s->mem, t->bytes);
block_copy_task_end(t, ret);
return ret;
@@ -714,14 +715,15 @@ void block_copy_kick(BlockCopyCallState *call_state)
static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
{
int ret;
+BlockCopyState *s = call_state->s;
-QLIST_INSERT_HEAD(_state->s->calls, call_state, list);
+QLIST_INSERT_HEAD(>calls, call_state, list);
do {
ret = block_copy_dirty_clusters(call_state);
if (ret == 0 && !call_state->cancelled) {
-ret = block_copy_wait_one(call_state->s, call_state->offset,
+ret = block_copy_wait_one(s, call_state->offset,
call_state->bytes);
}
--
2.31.1
[PATCH v5 0/5] block-copy: protect block-copy internal structures
This serie of patches aims to reduce the usage of the AioContexlock in block-copy, by introducing smaller granularity locks thus on making the block layer thread safe. This serie depends on my previous serie that brings thread safety to the smaller API used by block-copy, like ratelimit, progressmeter abd co-shared-resource. What's missing for block-copy to be fully thread-safe is fixing the CoSleep API to allow cross-thread sleep and wakeup. Paolo is working on it. Patch 1 provides a small refactoring, patch 2 introduces the .method field in BlockCopyState, to be used instead of .use_copy_range, .copy_size and .zeros. Patch 3 provide a refactoring in preparation to the lock added in patch 4 on BlockCopyTask, BlockCopyCallState and BlockCopyState. Patch 5 uses load_acquire/store_release to make sure BlockCopyCallState OUT fields are updated before finished is set to true. Based-on: <20210518094058.25952-1-eespo...@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito --- v5: * Squash patch 3 (improve comments) with patch 5 (add CoMutex). * Better comments in block-copy, drop IN/OUT/State categories * Remove some load_acquire in patch 6, replace them with atomic reads Emanuele Giuseppe Esposito (4): block-copy: small refactor in block_copy_task_entry and block_copy_common block-copy: move progress_set_remaining in block_copy_task_end block-copy: add CoMutex lock block-copy: atomic .cancelled and .finished fields in BlockCopyCallState Paolo Bonzini (1): block-copy: streamline choice of copy_range vs. read/write include/block/block-copy.h | 2 + block/block-copy.c | 368 +++-- 2 files changed, 231 insertions(+), 139 deletions(-) -- 2.31.1
