[PATCH] console: avoid passing con=NULL to graphic_hw_update_done() In graphic_hw_update(), first select an existing console, a specific-console or active_console(if not specified), then updating the
Signed-off-by: lichun
---
ui/console.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/ui/console.c b/ui/console.c
index e8e5970..e07d2c3 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -270,10 +270,11 @@ void graphic_hw_update_done(QemuConsole *con)
void graphic_hw_update(QemuConsole *con)
{
bool async = false;
+con = con ? con : active_console;
if (!con) {
-con = active_console;
+return;
}
-if (con && con->hw_ops->gfx_update) {
+if (con->hw_ops->gfx_update) {
con->hw_ops->gfx_update(con->hw);
async = con->hw_ops->gfx_update_async;
}
--
1.8.3.1
Re: [PATCH 1/2] block: make bdrv_drop_intermediate() less wrong
05.11.2020 18:14, Alberto Garcia wrote:
On Sat 31 Oct 2020 01:35:01 PM CET, Vladimir Sementsov-Ogievskiy wrote:
-QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
/* ... */
+QLIST_FOREACH_SAFE(c, &base->parents, next_parent, next) {
I also wonder, is top->parents and base->parents guaranteed to be the
same list in this case?
If not you could store the list of top->parents before calling
bdrv_replace_node() and use it afterwards.
QLIST_FOREACH(c, &top->parents, next_parent) {
parents = g_slist_prepend(parents, c);
}
Berto
Hmm... We should not touch other parents of base, which it had prior to
bdrv_replace_node().
On the other hand, it should be safe to call update_filename for not-updated
parents..
And we should keep in mind that bdrv_replace_node may replace not all children.
Still, in this
case we must replace all of them. Seems, we need additional argument for
bdrv_replace_node()
to fail, if it want's to skip some children replacement.
So, I'm going to resend to add this parameter to bdrv_replace_node() and will
use your
suggestion to be stricter on what we update, thanks!
--
Best regards,
Vladimir
Re: [PATCH] console: avoid passing con=NULL to graphic_hw_update_done() In graphic_hw_update(), first select an existing console, a specific-console or active_console(if not specified), then updating
Hi,
If you have an long commit message put it into the body not the subject
please.
On Sat, Nov 07, 2020 at 01:03:39AM +0800, lichun wrote:
> Signed-off-by: lichun
> ---
> ui/console.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index e8e5970..e07d2c3 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -270,10 +270,11 @@ void graphic_hw_update_done(QemuConsole *con)
> void graphic_hw_update(QemuConsole *con)
> {
> bool async = false;
> +con = con ? con : active_console;
con should not be NULL at this point.
Can you trigger a NULL pointer dereference here somehow?
thanks,
Gerd
Re: Re: [PATCH] console: avoid passing con=NULL to graphic_hw_update_done() In graphic_hw_update(), first select an existing console, a specific-console or active_console(if not specified), then updat
> Hi,
>
>If you have an long commit message put it into the body not the subject
>please.
Okey, I should leave a blank line.
>
>On Sat, Nov 07, 2020 at 01:03:39AM +0800, lichun wrote:
>> Signed-off-by: lichun
>> ---
>> ui/console.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/ui/console.c b/ui/console.c
>> index e8e5970..e07d2c3 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -270,10 +270,11 @@ void graphic_hw_update_done(QemuConsole *con)
>> void graphic_hw_update(QemuConsole *con)
>> {
>> bool async = false;
>> + con = con ? con : active_console;
>
>con should not be NULL at this point.
>
>Can you trigger a NULL pointer dereference here somehow?
run #./qemu-system-x86_64 -nodefaults test.img -vnc 0.0.0.0:0
Then connect with VNC client, It will cause QEMU CRASH.
>
>thanks,
> Gerd
>
[PULL 0/4] 9p queue for 5.2 (2020-11-06)
The following changes since commit e2766868d45d8c8f8991cfd133e6a0c14abfe577:
Merge remote-tracking branch
'remotes/kraxel/tags/fixes-20201104-pull-request' into staging (2020-11-04
22:13:02 +)
are available in the Git repository at:
https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201106
for you to fetch changes up to e6b99460b14469e0b83febc8d5a501947d1d5c7c:
hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen (2020-11-05
15:21:11 +0100)
9pfs: some fixes
* Fix meson build config for Xen.
* Code style fixes.
Philippe Mathieu-Daudé (1):
hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
Xinhao Zhang (3):
hw/9pfs : add spaces around operator
hw/9pfs : open brace '{' following struct go on the same line
hw/9pfs : add space before the open parenthesis '('
hw/9pfs/9p-local.c | 10 +-
hw/9pfs/9p.c| 16
hw/9pfs/9p.h| 9 +++--
hw/9pfs/Kconfig | 4
hw/9pfs/cofs.c | 2 +-
hw/9pfs/meson.build | 2 +-
6 files changed, 18 insertions(+), 25 deletions(-)
[PULL 2/4] hw/9pfs : open brace '{' following struct go on the same line
From: Xinhao Zhang
Fix code style. Open braces for struct should go on the same line.
Signed-off-by: Xinhao Zhang
Signed-off-by: Kai Deng
Reported-by: Euler Robot
Reviewed-by: Greg Kurz
Message-Id: <20201030043515.1030223-2-zhangxinh...@huawei.com>
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p.h | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 3dd1b50b1a..32df81f360 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -143,8 +143,7 @@ typedef struct {
*/
QEMU_BUILD_BUG_ON(sizeof(P9MsgHeader) != 7);
-struct V9fsPDU
-{
+struct V9fsPDU {
uint32_t size;
uint16_t tag;
uint8_t id;
@@ -270,8 +269,7 @@ union V9fsFidOpenState {
void *private;
};
-struct V9fsFidState
-{
+struct V9fsFidState {
int fid_type;
int32_t fid;
V9fsPath path;
@@ -338,8 +336,7 @@ typedef struct {
uint64_t path;
} QpfEntry;
-struct V9fsState
-{
+struct V9fsState {
QLIST_HEAD(, V9fsPDU) free_list;
QLIST_HEAD(, V9fsPDU) active_list;
V9fsFidState *fid_list;
--
2.20.1
[PULL 1/4] hw/9pfs : add spaces around operator
From: Xinhao Zhang
Fix code style. Operator needs spaces both sides.
Signed-off-by: Xinhao Zhang
Signed-off-by: Kai Deng
Reported-by: Euler Robot
Reviewed-by: Greg Kurz
Message-Id: <20201030043515.1030223-1-zhangxinh...@huawei.com>
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p-local.c | 10 +-
hw/9pfs/9p.c | 16
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3107637209..af52c1daac 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -162,13 +162,13 @@ static void local_mapped_file_attr(int dirfd, const char
*name,
memset(buf, 0, ATTR_MAX);
while (fgets(buf, ATTR_MAX, fp)) {
if (!strncmp(buf, "virtfs.uid", 10)) {
-stbuf->st_uid = atoi(buf+11);
+stbuf->st_uid = atoi(buf + 11);
} else if (!strncmp(buf, "virtfs.gid", 10)) {
-stbuf->st_gid = atoi(buf+11);
+stbuf->st_gid = atoi(buf + 11);
} else if (!strncmp(buf, "virtfs.mode", 11)) {
-stbuf->st_mode = atoi(buf+12);
+stbuf->st_mode = atoi(buf + 12);
} else if (!strncmp(buf, "virtfs.rdev", 11)) {
-stbuf->st_rdev = atoi(buf+12);
+stbuf->st_rdev = atoi(buf + 12);
}
memset(buf, 0, ATTR_MAX);
}
@@ -823,7 +823,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath
*dir_path, const char *name,
if (fd == -1) {
goto out;
}
-credp->fc_mode = credp->fc_mode|S_IFREG;
+credp->fc_mode = credp->fc_mode | S_IFREG;
if (fs_ctx->export_flags & V9FS_SM_MAPPED) {
/* Set cleint credentials in xattr */
err = local_set_xattrat(dirfd, name, credp);
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 741d222c3f..94df440fc7 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1091,7 +1091,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString
*extension)
}
}
-if (!(ret&~0777)) {
+if (!(ret & ~0777)) {
ret |= S_IFREG;
}
@@ -2776,7 +2776,7 @@ static void coroutine_fn v9fs_create(void *opaque)
v9fs_path_unlock(s);
} else {
err = v9fs_co_open2(pdu, fidp, &name, -1,
-omode_to_uflags(mode)|O_CREAT, perm, &stbuf);
+omode_to_uflags(mode) | O_CREAT, perm, &stbuf);
if (err < 0) {
goto out;
}
@@ -3428,7 +3428,7 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU *pdu,
struct statfs *stbuf)
* compute bsize factor based on host file system block size
* and client msize
*/
-bsize_factor = (s->msize - P9_IOHDRSZ)/stbuf->f_bsize;
+bsize_factor = (s->msize - P9_IOHDRSZ) / stbuf->f_bsize;
if (!bsize_factor) {
bsize_factor = 1;
}
@@ -3440,9 +3440,9 @@ static int v9fs_fill_statfs(V9fsState *s, V9fsPDU *pdu,
struct statfs *stbuf)
* adjust(divide) the number of blocks, free blocks and available
* blocks by bsize factor
*/
-f_blocks = stbuf->f_blocks/bsize_factor;
-f_bfree = stbuf->f_bfree/bsize_factor;
-f_bavail = stbuf->f_bavail/bsize_factor;
+f_blocks = stbuf->f_blocks / bsize_factor;
+f_bfree = stbuf->f_bfree / bsize_factor;
+f_bavail = stbuf->f_bavail / bsize_factor;
f_files = stbuf->f_files;
f_ffree = stbuf->f_ffree;
fsid_val = (unsigned int) stbuf->f_fsid.__val[0] |
@@ -4185,6 +4185,6 @@ static void __attribute__((__constructor__))
v9fs_set_fd_limit(void)
error_report("Failed to get the resource limit");
exit(1);
}
-open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur/3);
-open_fd_rc = rlim.rlim_cur/2;
+open_fd_hw = rlim.rlim_cur - MIN(400, rlim.rlim_cur / 3);
+open_fd_rc = rlim.rlim_cur / 2;
}
--
2.20.1
[PULL 4/4] hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen
From: Philippe Mathieu-Daudé
Commit b2c00bce54c ("meson: convert hw/9pfs, cleanup") introduced
CONFIG_9PFS (probably a wrong conflict resolution). This config is
not used anywhere. Backends depend on CONFIG_FSDEV_9P which itself
depends on CONFIG_VIRTFS.
Remove the invalid CONFIG_9PFS and use CONFIG_FSDEV_9P instead, to
fix the './configure --without-default-devices --enable-xen' build:
/usr/bin/ld: libcommon.fa.p/hw_xen_xen-legacy-backend.c.o: in function
`xen_be_register_common':
hw/xen/xen-legacy-backend.c:754: undefined reference to `xen_9pfs_ops'
/usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x8): undefined
reference to `local_ops'
/usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x20): undefined
reference to `synth_ops'
/usr/bin/ld: libcommon.fa.p/fsdev_qemu-fsdev.c.o:(.data.rel+0x38): undefined
reference to `proxy_ops'
collect2: error: ld returned 1 exit status
Fixes: b2c00bce54c ("meson: convert hw/9pfs, cleanup")
Suggested-by: Paolo Bonzini
Acked-by: Greg Kurz
Tested-by: Greg Kurz
Signed-off-by: Philippe Mathieu-Daudé
Acked-by: Christian Schoenebeck
Message-Id: <20201104115706.3101190-3-phi...@redhat.com>
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/Kconfig | 4
hw/9pfs/meson.build | 2 +-
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/hw/9pfs/Kconfig b/hw/9pfs/Kconfig
index d3ebd73730..3ae5749661 100644
--- a/hw/9pfs/Kconfig
+++ b/hw/9pfs/Kconfig
@@ -2,12 +2,8 @@ config FSDEV_9P
bool
depends on VIRTFS
-config 9PFS
-bool
-
config VIRTIO_9P
bool
default y
depends on VIRTFS && VIRTIO
select FSDEV_9P
-select 9PFS
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index cc09426212..99be5d9119 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -15,6 +15,6 @@ fs_ss.add(files(
'coxattr.c',
))
fs_ss.add(when: 'CONFIG_XEN', if_true: files('xen-9p-backend.c'))
-softmmu_ss.add_all(when: 'CONFIG_9PFS', if_true: fs_ss)
+softmmu_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-device.c'))
--
2.20.1
Re: [PULL 0/4] Linux user for 5.2 patches
On Thu, 5 Nov 2020 at 07:10, Laurent Vivier wrote: > > The following changes since commit 8680d6e36468f1ca00e2fe749bef50585d632401: > > Merge remote-tracking branch 'remotes/nvme/tags/pull-nvme-20201102' into st= > aging (2020-11-02 17:17:29 +) > > are available in the Git repository at: > > git://github.com/vivier/qemu.git tags/linux-user-for-5.2-pull-request > > for you to fetch changes up to 022625a8ade3005addb42700a145bae6a1653240: > > linux-user: Check copy_from_user() return value in vma_dump_size() (2020-11= > -04 22:28:05 +0100) > > > Coverity and compiler warning fixes > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
[PULL 3/4] hw/9pfs : add space before the open parenthesis '('
From: Xinhao Zhang
Fix code style. Space required before the open parenthesis '('.
Signed-off-by: Xinhao Zhang
Signed-off-by: Kai Deng
Reported-by: Euler Robot
Reviewed-by: Greg Kurz
Message-Id: <20201030043515.1030223-3-zhangxinh...@huawei.com>
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/cofs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index 55991916ec..0b321b456e 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -23,7 +23,7 @@ static ssize_t __readlink(V9fsState *s, V9fsPath *path,
V9fsString *buf)
ssize_t len, maxlen = PATH_MAX;
buf->data = g_malloc(PATH_MAX);
-for(;;) {
+for (;;) {
len = s->ops->readlink(&s->ctx, path, buf->data, maxlen);
if (len < 0) {
g_free(buf->data);
--
2.20.1
Re: [PATCH v2 00/44] Make qdev static property API usable by any QOM type
Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben:
> This series refactor the qdev property code so the static
> property system can be used by any QOM type. As an example, at
> the end of the series some properties in TYPE_MACHINE are
> converted to static properties to demonstrate the new API.
>
> Changes v1 -> v2
>
>
> * Rename functions and source files to call the new feature
> "field property" instead of "static property"
>
> * Change the API signature from an array-based interface similar
> to device_class_set_props() to a object_property_add()-like
> interface.
>
> This means instead of doing this:
>
> object_class_property_add_static_props(oc, my_array_of_props);
>
> properties are registered like this:
>
> object_class_property_add_field(oc, "property-name"
> PROP_XXX(MyState, my_field),
> prop_allow_set_always);
>
> where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL,
> etc.
In comparison, I really like the resulting code from the array based
interface in v1 better.
I think it's mostly for two reasons: First, the array is much more
compact and easier to read. And maybe even more importantly, you know
it's static data and only static data. The function based interface can
be mixed with other code or the parameter list can contain calls to
other functions with side effects, so there are a lot more opportunities
for surprises.
What I didn't like about the v1 interface is that there is still a
separate object_class_property_set_description() for each property, but
I think that could have been fixed by moving the description to the
definitions in the array, too.
On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote:
> On 29/10/20 23:02, Eduardo Habkost wrote:
> > +static Property machine_props[] = {
> > +DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
> > +DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
> > +DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
> > +DEFINE_PROP_STRING("dtb", MachineState, dtb),
> > +DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
> > +DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
> > +DEFINE_PROP_STRING("firmware", MachineState, firmware),
> > +DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
> > +DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
>
> While I think generalizing the _code_ for static properties is obviously
> a good idea, I am not sure about generalizing the interface for adding them.
>
> The reason is that we already have a place for adding properties in
> class_init, and having a second makes things "less local", so to speak.
As long as you have the function call to apply the properites array in
.class_init, it should be obvious enough what you're doing.
Of course, I think we should refrain from mixing both styles in a single
object, but generally speaking the array feels so much better that I
don't think we should reject it just because QOM only had a different
interface so far (and the property array is preexisting in qdev, too, so
we already have differences between objects - in fact, the majority of
objects is probably qdev today).
Kevin
[PATCH] migration/multifd: close TLS channel before socket finalize
Since we now support tls multifd, when we cancel migration, the TLS
sockets will be left as CLOSE-WAIT On Src which results in socket
leak.
Fix it by closing TLS channel before socket finalize.
Signed-off-by: Chuan Zheng
---
migration/multifd.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/migration/multifd.c b/migration/multifd.c
index 68b171f..832e475 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -523,6 +523,19 @@ static void multifd_send_terminate_threads(Error *err)
}
}
+static void multifd_tls_socket_close(QIOChannel *ioc, Error *err)
+{
+ if (ioc &&
+ object_dynamic_cast(OBJECT(ioc),
+ TYPE_QIO_CHANNEL_TLS)) {
+ /*
+ * TLS channel is special, we need close it before
+ * socket finalize.
+ */
+ qio_channel_close(ioc, &err);
+ }
+}
+
void multifd_save_cleanup(void)
{
int i;
@@ -542,6 +555,7 @@ void multifd_save_cleanup(void)
MultiFDSendParams *p = &multifd_send_state->params[i];
Error *local_err = NULL;
+multifd_tls_socket_close(p->c, NULL);
socket_send_channel_destroy(p->c);
p->c = NULL;
qemu_mutex_destroy(&p->mutex);
--
1.8.3.1
Re: [PATCH] migration/multifd: close TLS channel before socket finalize
Patchew URL: https://patchew.org/QEMU/1604657935-56394-1-git-send-email-zhengch...@huawei.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1604657935-56394-1-git-send-email-zhengch...@huawei.com Subject: [PATCH] migration/multifd: close TLS channel before socket finalize === 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/1604657935-56394-1-git-send-email-zhengch...@huawei.com -> patchew/1604657935-56394-1-git-send-email-zhengch...@huawei.com Switched to a new branch 'test' 4684928 migration/multifd: close TLS channel before socket finalize === OUTPUT BEGIN === ERROR: suspect code indent for conditional statements (5, 9) #25: FILE: migration/multifd.c:528: + if (ioc && [...] + /* total: 1 errors, 0 warnings, 26 lines checked Commit 468492886fac (migration/multifd: close TLS channel before socket finalize) 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/1604657935-56394-1-git-send-email-zhengch...@huawei.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [RFC PATCH 02/15] hw/riscv: migrate fdt field to generic MachineState
Bin Meng writes: > On Fri, Nov 6, 2020 at 1:57 AM Alex Bennée wrote: >> >> This is a mechanical change to make the fdt available through >> MachineState. >> >> Signed-off-by: Alex Bennée >> Reviewed-by: Alistair Francis >> Message-Id: <20201021170842.25762-3-alex.ben...@linaro.org> >> Signed-off-by: Alex Bennée >> --- >> include/hw/riscv/virt.h | 1 - >> hw/riscv/virt.c | 20 ++-- >> 2 files changed, 10 insertions(+), 11 deletions(-) > > What about the 'sifive_u' and 'spike' machines? If they support direct -kernel loading then we could certainly do the same for them. > > Regards, > Bin -- Alex Bennée
Re: [PATCH] target/microblaze: Fix possible array out of bounds in mmu_write()
On Tue, Nov 03, 2020 at 03:46:02PM +0800, AlexChen wrote:
> The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5].
> To avoid data access out of bounds, only if 'rn' is less than 3, we
> can print env->mmu.regs[rn]. In other cases, we can print
> env->mmu.regs[MMU_R_TLBX].
>
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
Reviewed-by: Edgar E. Iglesias
> ---
> target/microblaze/mmu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 1dbbb271c4..917ad6d69e 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn,
> uint32_t v)
> unsigned int i;
>
> qemu_log_mask(CPU_LOG_MMU,
> - "%s rn=%d=%x old=%x\n", __func__, rn, v,
> env->mmu.regs[rn]);
> + "%s rn=%d=%x old=%x\n", __func__, rn, v,
> + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]);
>
> if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) {
> qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
> --
> 2.19.1
Re: [RFC PATCH 12/15] stubs/xen-hw-stub: drop xenstore_store_pv_console_info stub
Philippe Mathieu-Daudé writes:
> On 11/5/20 6:51 PM, Alex Bennée wrote:
>> We should never build something that calls this without having it.
>
> "because ..."?
xen-all.c is only built when we have CONFIG_XEN which also gates the
only call-site in xen-console.c
>
> Reviewed-by: Philippe Mathieu-Daudé
>
>>
>> Signed-off-by: Alex Bennée
>> ---
>> stubs/xen-hw-stub.c | 4
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/stubs/xen-hw-stub.c b/stubs/xen-hw-stub.c
>> index 2ea8190921..15f3921a76 100644
>> --- a/stubs/xen-hw-stub.c
>> +++ b/stubs/xen-hw-stub.c
>> @@ -10,10 +10,6 @@
>> #include "hw/xen/xen.h"
>> #include "hw/xen/xen-x86.h"
>>
>> -void xenstore_store_pv_console_info(int i, Chardev *chr)
>> -{
>> -}
>> -
>> int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>> {
>> return -1;
>>
--
Alex Bennée
Re: [PULL 0/4] 9p queue for 5.2 (2020-11-06)
On Fri, 6 Nov 2020 at 09:36, Christian Schoenebeck wrote: > > The following changes since commit e2766868d45d8c8f8991cfd133e6a0c14abfe577: > > Merge remote-tracking branch > 'remotes/kraxel/tags/fixes-20201104-pull-request' into staging (2020-11-04 > 22:13:02 +) > > are available in the Git repository at: > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20201106 > > for you to fetch changes up to e6b99460b14469e0b83febc8d5a501947d1d5c7c: > > hw/9pfs: Fix Kconfig dependency problem between 9pfs and Xen (2020-11-05 > 15:21:11 +0100) > > > 9pfs: some fixes > > * Fix meson build config for Xen. > > * Code style fixes. Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
Re: Emulation for riscv
Palmer Dabbelt writes: > On Thu, 22 Oct 2020 17:56:38 PDT (-0700), alistai...@gmail.com wrote: >> On Thu, Oct 22, 2020 at 4:58 PM Moises Arreola wrote: >>> >>> Hello everyone, my name is Moses and I'm trying to set up a VM for a risc-v >>> processor, I'm using the Risc-V Getting Started Guide and on the final step >>> I'm getting an error while trying to launch the virtual machine using the >>> cmd: >> >> Hello, >> >> Please don't use the RISC-V Getting Started Guide. Pretty much all of >> the information there is out of date and wrong. Unfortunately we are >> unable to correct it. >> >> The QEMU wiki is a much better place for information: >> https://wiki.qemu.org/Documentation/Platforms/RISCV > > Ya, everything at riscv.org is useless. It's best to stick to the open source > documentation, as when that gets out of date we can at least fix it. Using a > distro helps a lot here, the wiki describes how to run a handful of popular > ones that were ported to RISC-V early but if your favorite isn't on the list > then it may have its own documentation somewhere else. Even better if you could submit some .rst pages for QEMU's git: docs/system/target-riscv.rst docs/system/riscv/virt.rst (and maybe the other models) then we could improve the user manual where RiscV is currently a little under-represented. A number of the systems have simple example command lines or explain the kernel support needed for the model. > >>> sudo qemu-system-riscv64 -nographic -machine virt \ >>> -kernel linux/arch/riscv/boot/Image -append "root=/dev/vda ro >>> console=ttyS0" \ >>> -drive file=busybox,format=raw,id=hd0 \ >>> -device virtio-blk-device,drive=hd0 >>> >>> But what I get in return is a message telling me that the file I gave >>> wasn't the right one, the actual output is: >>> >>> qemu-system-riscv64: -drive file=busybox,format=raw,id=hd0: A regular file >>> was expected by the 'file' driver, but something else was given >>> >>> And I checked the file busybox with de cmd "file" and got the following : >>> busybox: ELF 64-bit LSB executable, UCB RISC-V, version 1 (SYSV), >>> dynamically linked, interpreter /lib/ld-linux-riscv64-lp64d.so.1, for >>> GNU/Linux 4.15.0, stripped >> >> That looks like an ELF, which won't work when attached as a drive. >> >> How are you building this rootFS? >> >> Alistair >> >>> >>> So I was wondering if the error message was related to qemu. >>> Thanks in advance for answering any suggestions are welcome -- Alex Bennée
[PATCH v2] migration/multifd: close TLS channel before socket finalize
Since we now support tls multifd, when we cancel migration, the TLS
sockets will be left as CLOSE-WAIT On Src which results in socket
leak.
Fix it by closing TLS channel before socket finalize.
Signed-off-by: Chuan Zheng
---
migration/multifd.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/migration/multifd.c b/migration/multifd.c
index 68b171f..a6838dc 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -523,6 +523,19 @@ static void multifd_send_terminate_threads(Error *err)
}
}
+static void multifd_tls_socket_close(QIOChannel *ioc, Error *err)
+{
+if (ioc &&
+object_dynamic_cast(OBJECT(ioc),
+TYPE_QIO_CHANNEL_TLS)) {
+/*
+ * TLS channel is special, we need close it before
+ * socket finalize.
+ */
+qio_channel_close(ioc, &err);
+}
+}
+
void multifd_save_cleanup(void)
{
int i;
@@ -542,6 +555,7 @@ void multifd_save_cleanup(void)
MultiFDSendParams *p = &multifd_send_state->params[i];
Error *local_err = NULL;
+multifd_tls_socket_close(p->c, NULL);
socket_send_channel_destroy(p->c);
p->c = NULL;
qemu_mutex_destroy(&p->mutex);
--
1.8.3.1
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On Fri, 6 Nov 2020 at 06:15, Gan Qixin wrote: > > Modify the rule that limit the length of lines according to the following > ideas: > > --add a variable max_line_length to indicate the limit of line length and set > it to 100 by default > --when the line length exceeds max_line_length, output warning information > instead of error > --if/while/etc brace do not go on next line whether the line length exceeds > max_line_length or not > > Signed-off-by: Gan Qixin > --- > scripts/checkpatch.pl | 18 +- > 1 file changed, 5 insertions(+), 13 deletions(-) For the code changes Reviewed-by: Peter Maydell but we also need to update our coding style documentation to match. I'll send out a patch with some proposed text. Side note: the kernel version of this checkpatch change (kernel commit bdc48fa11e46f867) suppresses all line-length warnings for the "--file" use case. Do we care about that? thanks -- PMM
[PATCH] CODING_STYLE.rst: Be less strict about 80 character limit
Relax the wording about line lengths a little bit; this goes with the checkpatch changes to warn at 100 characters rather than 80. (Compare the Linux kernel commit bdc48fa11e46f8; our coding style is not theirs, but the rationale is good and applies to us too.) Signed-off-by: Peter Maydell --- CODING_STYLE.rst | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst index 8b13ef0669e..7bf4e39d487 100644 --- a/CODING_STYLE.rst +++ b/CODING_STYLE.rst @@ -85,8 +85,13 @@ Line width Lines should be 80 characters; try not to make them longer. Sometimes it is hard to do, especially when dealing with QEMU subsystems -that use long function or symbol names. Even in that case, do not make -lines much longer than 80 characters. +that use long function or symbol names. If wrapping the line at 80 columns +is obviously less readable and more awkward, prefer not to wrap it; better +to have an 85 character line than one which is awkwardly wrapped. + +Even in that case, try not to make lines much longer than 80 characters. +(The checkpatch script will warn at 100 characters, but this is intended +as a guard against obviously-overlength lines, not a target.) Rationale: -- 2.20.1
Re: [RFC PATCH 02/15] hw/riscv: migrate fdt field to generic MachineState
On Fri, Nov 6, 2020 at 6:21 PM Alex Bennée wrote: > > > Bin Meng writes: > > > On Fri, Nov 6, 2020 at 1:57 AM Alex Bennée wrote: > >> > >> This is a mechanical change to make the fdt available through > >> MachineState. > >> > >> Signed-off-by: Alex Bennée > >> Reviewed-by: Alistair Francis > >> Message-Id: <20201021170842.25762-3-alex.ben...@linaro.org> > >> Signed-off-by: Alex Bennée > >> --- > >> include/hw/riscv/virt.h | 1 - > >> hw/riscv/virt.c | 20 ++-- > >> 2 files changed, 10 insertions(+), 11 deletions(-) > > > > What about the 'sifive_u' and 'spike' machines? > > If they support direct -kernel loading then we could certainly do the > same for them. Yes, they do. Regards, Bin
Re: [PATCH v9 08/12] hw/block/nvme: Support Zoned Namespace Command Set
On Thu, Nov 05, 2020 at 11:53:38AM +0900, Dmitry Fomichev wrote:
> The emulation code has been changed to advertise NVM Command Set when
> "zoned" device property is not set (default) and Zoned Namespace
> Command Set otherwise.
>
> Define values and structures that are needed to support Zoned
> Namespace Command Set (NVMe TP 4053) in PCI NVMe controller emulator.
> Define trace events where needed in newly introduced code.
>
> In order to improve scalability, all open, closed and full zones
> are organized in separate linked lists. Consequently, almost all
> zone operations don't require scanning of the entire zone array
> (which potentially can be quite large) - it is only necessary to
> enumerate one or more zone lists.
>
> Handlers for three new NVMe commands introduced in Zoned Namespace
> Command Set specification are added, namely for Zone Management
> Receive, Zone Management Send and Zone Append.
>
> Device initialization code has been extended to create a proper
> configuration for zoned operation using device properties.
>
> Read/Write command handler is modified to only allow writes at the
> write pointer if the namespace is zoned. For Zone Append command,
> writes implicitly happen at the write pointer and the starting write
> pointer value is returned as the result of the command. Write Zeroes
> handler is modified to add zoned checks that are identical to those
> done as a part of Write flow.
>
> Subsequent commits in this series add ZDE support and checks for
> active and open zone limits.
>
> Signed-off-by: Niklas Cassel
> Signed-off-by: Hans Holmberg
> Signed-off-by: Ajay Joshi
> Signed-off-by: Chaitanya Kulkarni
> Signed-off-by: Matias Bjorling
> Signed-off-by: Aravind Ramesh
> Signed-off-by: Shin'ichiro Kawasaki
> Signed-off-by: Adam Manzanares
> Signed-off-by: Dmitry Fomichev
> Reviewed-by: Niklas Cassel
> ---
> hw/block/nvme-ns.h| 54 +++
> hw/block/nvme.h | 8 +
> hw/block/nvme-ns.c| 173
> hw/block/nvme.c | 971 +-
> hw/block/trace-events | 18 +-
> 5 files changed, 1209 insertions(+), 15 deletions(-)
>
(snip)
> +static uint16_t nvme_zone_mgmt_recv(NvmeCtrl *n, NvmeRequest *req)
> +{
> +NvmeCmd *cmd = (NvmeCmd *)&req->cmd;
> +NvmeNamespace *ns = req->ns;
> +/* cdw12 is zero-based number of dwords to return. Convert to bytes */
> +uint32_t data_size = (le32_to_cpu(cmd->cdw12) + 1) << 2;
> +uint32_t dw13 = le32_to_cpu(cmd->cdw13);
> +uint32_t zone_idx, zra, zrasf, partial;
> +uint64_t max_zones, nr_zones = 0;
> +uint16_t ret;
> +uint64_t slba;
> +NvmeZoneDescr *z;
> +NvmeZone *zs;
> +NvmeZoneReportHeader *header;
> +void *buf, *buf_p;
> +size_t zone_entry_sz;
> +
> +req->status = NVME_SUCCESS;
> +
> +ret = nvme_get_mgmt_zone_slba_idx(ns, cmd, &slba, &zone_idx);
> +if (ret) {
> +return ret;
> +}
> +
> +zra = dw13 & 0xff;
> +if (zra != NVME_ZONE_REPORT) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +zrasf = (dw13 >> 8) & 0xff;
> +if (zrasf > NVME_ZONE_REPORT_OFFLINE) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +if (data_size < sizeof(NvmeZoneReportHeader)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +ret = nvme_map_dptr(n, data_size, req);
nvme_map_dptr() call was not here in v8 patch set.
On v7 I commented that you were missing a call to nvme_check_mdts().
I think you still need to call nvme_check_mdts in order to verify
that data_size < mdts, no?
This function already has a call do nvme_dma(). nvme_dma() already
calls nvme_map_dptr().
If you use nvme_dma(), you cannot use nvme_map_dptr().
It will call nvme_map_addr() (which calls qemu_sglist_add()) on the
same buffer twice, causing the qsg->size to be twice what the user
sent in. Which will cause the:
if (unlikely(residual)) {
check in nvme_dma() to fail.
Looking at nvme_read()/nvme_write(), they use nvme_map_dptr()
(without any call to nvme_dma()), and then use dma_blk_read() or
dma_blk_write(). (and they both call nvme_check_mdts())
Kind regards,
Niklas
> +if (ret) {
> +return ret;
> +}
> +
> +partial = (dw13 >> 16) & 0x01;
> +
> +zone_entry_sz = sizeof(NvmeZoneDescr);
> +
> +max_zones = (data_size - sizeof(NvmeZoneReportHeader)) / zone_entry_sz;
> +buf = g_malloc0(data_size);
> +
> +header = (NvmeZoneReportHeader *)buf;
> +buf_p = buf + sizeof(NvmeZoneReportHeader);
> +
> +while (zone_idx < ns->num_zones && nr_zones < max_zones) {
> +zs = &ns->zone_array[zone_idx];
> +
> +if (!nvme_zone_matches_filter(zrasf, zs)) {
> +zone_idx++;
> +continue;
> +}
> +
> +z = (NvmeZoneDescr *)buf_p;
> +buf_p += sizeof(NvmeZoneDescr);
> +nr_zones++;
> +
> +z->zt = zs->d.zt;
> +z->zs = zs->d.zs;
> +z->zcap = cpu_to_le64(zs->d.zcap);
> +
[PATCH v2 0/7] block: permission update fix & refactor
Hi all! These series supersedes "Fix nested permission update" and includes one more fix (patch 01) and more improvements. I think patch 01 is good to have in 5.2, 02 is probably OK for 5.2 and the others are OK for next release. Still all may be taken to 5.2, up to block maintainers. Actually the series is a first part of my work announced in https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg08386.html to bring correct order to permission update (topological sort), update permission only on updated graph (and rollback graph changes), get rid of .active fields in block jobs. Supersedes: <20201031123502.4558-1-vsement...@virtuozzo.com> v2: all new except for 03:, which uses suggestions by Albert and more strict version of bdrv_replace_node. Vladimir Sementsov-Ogievskiy (7): block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache() block: add bdrv_replace_node_common() block: make bdrv_drop_intermediate() less wrong block: add bdrv_refresh_perms() helper block: bdrv_set_perm() drop redundant parameters. block: bdrv_child_set_perm() drop redundant parameters. block: drop tighten_restrictions block.c | 256 +++- 1 file changed, 105 insertions(+), 151 deletions(-) -- 2.21.3
[PATCH 0/2] Increase amount of data for monitor to read
The subject was discussed here: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html This series is a solution for the issue with QMP monitor buffered input. A little parser is introduced to throttle JSON commands read from the buffer so that QMP requests do not overwhelm the monitor input queue. A side effect raised in the test #247 was managed in the first patch. It may be considered as a workaround. Any sane fix suggested will be appreciated. Note: This series goes after the Vladimir's one: '[PATCH v3 00/25] backup performance: block_status + async"' To make the test #129 passed, the following patch should be applied first: '[PATCH v3 01/25] iotests: 129 don't check backup "busy"'. Andrey Shinkevich (2): iotests: add another bash sleep command to 247 monitor: increase amount of data for monitor to read chardev/char-fd.c | 64 +- chardev/char-socket.c | 54 +++--- chardev/char.c | 40 + include/chardev/char.h | 15 +++ monitor/monitor.c | 2 +- tests/qemu-iotests/247 | 2 ++ tests/qemu-iotests/247.out | 1 + 7 files changed, 161 insertions(+), 17 deletions(-) -- 1.8.3.1
[PATCH v2 2/7] block: add bdrv_replace_node_common()
Add new parameter to bdrv_replace_node(): auto_skip. With
auto_skip=false we'll have stricter behavior: update _all_ from
parents or fail. New behaviour will be used in the following commit in
block.c, so keep original function name as public interface.
Note: new error message is a bit funny in contrast with further
"Cannot" in case of frozen child, but we'd better keep some difference
to make it possible to distinguish one from another on failure. Still,
actually we'd better refactor should_update_child() call to distinguish
also different kinds of "should not". Let's do it later.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 19db7b7aeb..11c862d691 100644
--- a/block.c
+++ b/block.c
@@ -4563,8 +4563,16 @@ static bool should_update_child(BdrvChild *c,
BlockDriverState *to)
return ret;
}
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp)
+/*
+ * With auto_skip=true bdrv_replace_node_common skips updating from parents
+ * if it creates a parent-child relation loop or if parent is block-job.
+ *
+ * With auto_skip=false the error is returned if from has a parent which should
+ * not be updated.
+ */
+static void bdrv_replace_node_common(BlockDriverState *from,
+ BlockDriverState *to,
+ bool auto_skip, Error **errp)
{
BdrvChild *c, *next;
GSList *list = NULL, *p;
@@ -4583,7 +4591,12 @@ void bdrv_replace_node(BlockDriverState *from,
BlockDriverState *to,
QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
assert(c->bs == from);
if (!should_update_child(c, to)) {
-continue;
+if (auto_skip) {
+continue;
+}
+error_setg(errp, "Should not change '%s' link to '%s'",
+ c->name, from->node_name);
+goto out;
}
if (c->frozen) {
error_setg(errp, "Cannot change '%s' link to '%s'",
@@ -4623,6 +4636,12 @@ out:
bdrv_unref(from);
}
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp)
+{
+return bdrv_replace_node_common(from, to, true, errp);
+}
+
/*
* Add new bs contents at the top of an image chain while the chain is
* live, while keeping required fields on the top layer.
--
2.21.3
[PATCH 1/2] iotests: add another bash sleep command to 247
This patch paves the way for the one that follows. The following patch
makes the QMP monitor to read up to 4K from stdin at once. That results
in running the bash 'sleep' command before the _qemu_proc_exec() starts
in subshell. Another 'sleep' command with an unobtrusive 'query-status'
plays as a workaround.
Signed-off-by: Andrey Shinkevich
---
tests/qemu-iotests/247 | 2 ++
tests/qemu-iotests/247.out | 1 +
2 files changed, 3 insertions(+)
diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
index 87e37b3..7d316ec 100755
--- a/tests/qemu-iotests/247
+++ b/tests/qemu-iotests/247
@@ -59,6 +59,8 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
{"execute":"block-commit",
"arguments":{"device":"format-4", "top-node": "format-2",
"base-node":"format-0", "job-id":"job0"}}
EOF
+sleep 1
+echo '{"execute":"query-status"}'
if [ "${VALGRIND_QEMU}" == "y" ]; then
sleep 10
else
diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
index e909e83..13d9547 100644
--- a/tests/qemu-iotests/247.out
+++ b/tests/qemu-iotests/247.out
@@ -17,6 +17,7 @@ QMP_VERSION
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 134217728, "offset":
134217728, "speed": 0, "type": "commit"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+{"return": {"status": "running", "singlestep": false, "running": true}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
*** done
--
1.8.3.1
[PATCH v2 6/7] block: bdrv_child_set_perm() drop redundant parameters.
We must set the permission used for _check_. Assert that we have
backup and drop extra arguments.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/block.c b/block.c
index b61d20252f..b44db05d14 100644
--- a/block.c
+++ b/block.c
@@ -1897,7 +1897,7 @@ static int bdrv_child_check_perm(BdrvChild *c,
BlockReopenQueue *q,
GSList *ignore_children,
bool *tighten_restrictions, Error **errp);
static void bdrv_child_abort_perm_update(BdrvChild *c);
-static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
+static void bdrv_child_set_perm(BdrvChild *c);
typedef struct BlockReopenQueueEntry {
bool prepared;
@@ -2131,11 +2131,7 @@ static void bdrv_set_perm(BlockDriverState *bs)
/* Update all children */
QLIST_FOREACH(c, &bs->children, next) {
-uint64_t cur_perm, cur_shared;
-bdrv_child_perm(bs, c->bs, c, c->role, NULL,
-cumulative_perms, cumulative_shared_perms,
-&cur_perm, &cur_shared);
-bdrv_child_set_perm(c, cur_perm, cur_shared);
+bdrv_child_set_perm(c);
}
}
@@ -2298,13 +2294,10 @@ static int bdrv_child_check_perm(BdrvChild *c,
BlockReopenQueue *q,
return 0;
}
-static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
+static void bdrv_child_set_perm(BdrvChild *c)
{
c->has_backup_perm = false;
-c->perm = perm;
-c->shared_perm = shared;
-
bdrv_set_perm(c->bs);
}
@@ -2362,7 +2355,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm,
uint64_t shared,
return ret;
}
-bdrv_child_set_perm(c, perm, shared);
+bdrv_child_set_perm(c);
return 0;
}
--
2.21.3
[PATCH v2 3/7] block: make bdrv_drop_intermediate() less wrong
First, permission update loop tries to do iterations transactionally,
but the whole update is not transactional: nobody roll-back successful
loop iterations when some iteration fails.
Second, in the iteration we have nested permission update:
c->klass->update_filename may point to bdrv_child_cb_update_filename()
which calls bdrv_backing_update_filename(), which may do node reopen to
RW.
Permission update system is not prepared to nested updates, at least it
has intermediate permission-update state stored in BdrvChild
structures: has_backup_perm, backup_perm and backup_shared_perm.
So, let's first do bdrv_replace_node_common() (which is more
transactional than open-coded update in bdrv_drop_intermediate()) and
then call update_filename() in separate. We still do not rollback
changes in case of update_filename() failure but it's not much worse
than pre-patch behavior.
Note that bdrv_replace_node_common() does check for frozen children,
so corresponding check is dropped in bdrv_drop_intermediate().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 54 --
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index 11c862d691..77a3f8f1e2 100644
--- a/block.c
+++ b/block.c
@@ -4910,9 +4910,11 @@ int bdrv_drop_intermediate(BlockDriverState *top,
BlockDriverState *base,
{
BlockDriverState *explicit_top = top;
bool update_inherits_from;
-BdrvChild *c, *next;
+BdrvChild *c;
Error *local_err = NULL;
int ret = -EIO;
+g_autoptr(GSList) updated_children = NULL;
+GSList *p;
bdrv_ref(top);
bdrv_subtree_drained_begin(top);
@@ -4926,14 +4928,6 @@ int bdrv_drop_intermediate(BlockDriverState *top,
BlockDriverState *base,
goto exit;
}
-/* This function changes all links that point to top and makes
- * them point to base. Check that none of them is frozen. */
-QLIST_FOREACH(c, &top->parents, next_parent) {
-if (c->frozen) {
-goto exit;
-}
-}
-
/* If 'base' recursively inherits from 'top' then we should set
* base->inherits_from to top->inherits_from after 'top' and all
* other intermediate nodes have been dropped.
@@ -4950,36 +4944,36 @@ int bdrv_drop_intermediate(BlockDriverState *top,
BlockDriverState *base,
backing_file_str = base->filename;
}
-QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
-/* Check whether we are allowed to switch c from top to base */
-GSList *ignore_children = g_slist_prepend(NULL, c);
-ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
- ignore_children, NULL, &local_err);
-g_slist_free(ignore_children);
-if (ret < 0) {
-error_report_err(local_err);
-goto exit;
-}
+QLIST_FOREACH(c, &top->parents, next_parent) {
+updated_children = g_slist_prepend(updated_children, c);
+}
+
+bdrv_replace_node_common(top, base, false, &local_err);
+if (local_err) {
+error_report_err(local_err);
+goto exit;
+}
+
+for (p = updated_children; p; p = p->next) {
+c = p->data;
-/* If so, update the backing file path in the image file */
if (c->klass->update_filename) {
ret = c->klass->update_filename(c, base, backing_file_str,
&local_err);
if (ret < 0) {
-bdrv_abort_perm_update(base);
+/*
+ * TODO: Actually, we want to rollback all previous iterations
+ * of this loop, and (which is almost impossible) previous
+ * bdrv_replace_node()...
+ *
+ * Note, that c->klass->update_filename may lead to permission
+ * update, so it's a bad idea to call it inside permission
+ * update transaction of bdrv_replace_node.
+ */
error_report_err(local_err);
goto exit;
}
}
-
-/*
- * Do the actual switch in the in-memory graph.
- * Completes bdrv_check_update_perm() transaction internally.
- * c->frozen is false, we have checked that above.
- */
-bdrv_ref(base);
-bdrv_replace_child(c, base);
-bdrv_unref(top);
}
if (update_inherits_from) {
--
2.21.3
[PATCH v2 1/7] block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache()
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block.c b/block.c
index 56bacc9e9f..19db7b7aeb 100644
--- a/block.c
+++ b/block.c
@@ -5782,6 +5782,7 @@ int coroutine_fn
bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
if (ret < 0) {
+bdrv_abort_perm_update(bs);
bs->open_flags |= BDRV_O_INACTIVE;
return ret;
}
--
2.21.3
[PATCH v2 7/7] block: drop tighten_restrictions
The only users of this thing are:
1. bdrv_child_try_set_perm, to ignore failures on loosen restrictions
2. assertion in bdrv_replace_child
3. assertion in bdrv_inactivate_recurse
Assertions are not enough reason for overcomplication the permission
update system. So, look at bdrv_child_try_set_perm.
We are interested in tighten_restrictions only on failure. But on
failure this field is not reliable: we may fail in the middle of
permission update, some nodes are not touched and we don't know should
their permissions be tighten or not. So, we rely on the fact that if we
loose restrictions on some node (or BdrvChild), we'll not tighten
restriction in the whole subtree as part of this update (assertions 2
and 3 rely on this fact as well). And, if we rely on this fact anyway,
we can just check it on top, and don't pass additional pointer through
the whole recursive infrastructure.
Note also, that further patches will fix real bugs in permission update
system, so now is good time to simplify it, as a help for further
refactorings.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 88 +++--
1 file changed, 17 insertions(+), 71 deletions(-)
diff --git a/block.c b/block.c
index b44db05d14..8437d579e0 100644
--- a/block.c
+++ b/block.c
@@ -1894,8 +1894,7 @@ static int bdrv_fill_options(QDict **options, const char
*filename,
static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
uint64_t perm, uint64_t shared,
- GSList *ignore_children,
- bool *tighten_restrictions, Error **errp);
+ GSList *ignore_children, Error **errp);
static void bdrv_child_abort_perm_update(BdrvChild *c);
static void bdrv_child_set_perm(BdrvChild *c);
@@ -1968,43 +1967,18 @@ static void bdrv_child_perm(BlockDriverState *bs,
BlockDriverState *child_bs,
* permissions of all its parents. This involves checking whether all necessary
* permission changes to child nodes can be performed.
*
- * Will set *tighten_restrictions to true if and only if new permissions have
to
- * be taken or currently shared permissions are to be unshared. Otherwise,
- * errors are not fatal as long as the caller accepts that the restrictions
- * remain tighter than they need to be. The caller still has to abort the
- * transaction.
- * @tighten_restrictions cannot be used together with @q: When reopening, we
may
- * encounter fatal errors even though no restrictions are to be tightened. For
- * example, changing a node from RW to RO will fail if the WRITE permission is
- * to be kept.
- *
* A call to this function must always be followed by a call to bdrv_set_perm()
* or bdrv_abort_perm_update().
*/
static int bdrv_check_perm(BlockDriverState *bs, BlockReopenQueue *q,
uint64_t cumulative_perms,
uint64_t cumulative_shared_perms,
- GSList *ignore_children,
- bool *tighten_restrictions, Error **errp)
+ GSList *ignore_children, Error **errp)
{
BlockDriver *drv = bs->drv;
BdrvChild *c;
int ret;
-assert(!q || !tighten_restrictions);
-
-if (tighten_restrictions) {
-uint64_t current_perms, current_shared;
-uint64_t added_perms, removed_shared_perms;
-
-bdrv_get_cumulative_perm(bs, ¤t_perms, ¤t_shared);
-
-added_perms = cumulative_perms & ~current_perms;
-removed_shared_perms = current_shared & ~cumulative_shared_perms;
-
-*tighten_restrictions = added_perms || removed_shared_perms;
-}
-
/* Write permissions never work with read-only images */
if ((cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) &&
!bdrv_is_writable_after_reopen(bs, q))
@@ -2061,18 +2035,12 @@ static int bdrv_check_perm(BlockDriverState *bs,
BlockReopenQueue *q,
/* Check all children */
QLIST_FOREACH(c, &bs->children, next) {
uint64_t cur_perm, cur_shared;
-bool child_tighten_restr;
bdrv_child_perm(bs, c->bs, c, c->role, q,
cumulative_perms, cumulative_shared_perms,
&cur_perm, &cur_shared);
ret = bdrv_child_check_perm(c, q, cur_perm, cur_shared,
ignore_children,
-tighten_restrictions ? &child_tighten_restr
- : NULL,
errp);
-if (tighten_restrictions) {
-*tighten_restrictions |= child_tighten_restr;
-}
if (ret < 0) {
return ret;
}
@@ -2195,22 +2163,18 @@ char *bdrv_perm_names(uint64_t perm)
* set, the BdrvChild objects in this list are ignored in the calculations;
* this allows checking permission updates for an existing reference.
*
- * See bdrv_check_
[PATCH v2 5/7] block: bdrv_set_perm() drop redundant parameters.
We should never set permissions other than cumulative permissions of
parents. During bdrv_reopen_multiple() we _check_ for synthetic
permissions but when we do _set_ the graph is already updated.
Add an assertion to bdrv_reopen_multiple(), other cases are more
obvious.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index fc7633307f..b61d20252f 100644
--- a/block.c
+++ b/block.c
@@ -2106,9 +2106,9 @@ static void bdrv_abort_perm_update(BlockDriverState *bs)
}
}
-static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms,
- uint64_t cumulative_shared_perms)
+static void bdrv_set_perm(BlockDriverState *bs)
{
+uint64_t cumulative_perms, cumulative_shared_perms;
BlockDriver *drv = bs->drv;
BdrvChild *c;
@@ -2116,6 +2116,8 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t
cumulative_perms,
return;
}
+bdrv_get_cumulative_perm(bs, &cumulative_perms, &cumulative_shared_perms);
+
/* Update this node */
if (drv->bdrv_set_perm) {
drv->bdrv_set_perm(bs, cumulative_perms, cumulative_shared_perms);
@@ -2298,16 +2300,12 @@ static int bdrv_child_check_perm(BdrvChild *c,
BlockReopenQueue *q,
static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared)
{
-uint64_t cumulative_perms, cumulative_shared_perms;
-
c->has_backup_perm = false;
c->perm = perm;
c->shared_perm = shared;
-bdrv_get_cumulative_perm(c->bs, &cumulative_perms,
- &cumulative_shared_perms);
-bdrv_set_perm(c->bs, cumulative_perms, cumulative_shared_perms);
+bdrv_set_perm(c->bs);
}
static void bdrv_child_abort_perm_update(BdrvChild *c)
@@ -2333,7 +2331,7 @@ static int bdrv_refresh_perms(BlockDriverState *bs, bool
*tighten_restrictions,
bdrv_abort_perm_update(bs);
return ret;
}
-bdrv_set_perm(bs, perm, shared_perm);
+bdrv_set_perm(bs);
return 0;
}
@@ -2634,7 +2632,6 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
{
BlockDriverState *old_bs = child->bs;
-uint64_t perm, shared_perm;
/* Asserts that child->frozen == false */
bdrv_replace_child_noperm(child, new_bs);
@@ -2648,8 +2645,7 @@ static void bdrv_replace_child(BdrvChild *child,
BlockDriverState *new_bs)
* restrictions.
*/
if (new_bs) {
-bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm);
-bdrv_set_perm(new_bs, perm, shared_perm);
+bdrv_set_perm(new_bs);
}
if (old_bs) {
@@ -3867,7 +3863,13 @@ cleanup_perm:
}
if (ret == 0) {
-bdrv_set_perm(state->bs, state->perm, state->shared_perm);
+uint64_t perm, shared;
+
+bdrv_get_cumulative_perm(state->bs, &perm, &shared);
+assert(perm == state->perm);
+assert(shared == state->shared_perm);
+
+bdrv_set_perm(state->bs);
} else {
bdrv_abort_perm_update(state->bs);
if (state->replace_backing_bs && state->new_backing_bs) {
@@ -4637,8 +4639,7 @@ static void bdrv_replace_node_common(BlockDriverState
*from,
bdrv_unref(from);
}
-bdrv_get_cumulative_perm(to, &perm, &shared);
-bdrv_set_perm(to, perm, shared);
+bdrv_set_perm(to);
out:
g_slist_free(list);
--
2.21.3
[PATCH v2 4/7] block: add bdrv_refresh_perms() helper
Make separate function for common pattern.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 60 -
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index 77a3f8f1e2..fc7633307f 100644
--- a/block.c
+++ b/block.c
@@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
bdrv_abort_perm_update(c->bs);
}
+static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
+ Error **errp)
+{
+int ret;
+uint64_t perm, shared_perm;
+
+bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+if (ret < 0) {
+bdrv_abort_perm_update(bs);
+return ret;
+}
+bdrv_set_perm(bs, perm, shared_perm);
+
+return 0;
+}
+
int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
Error **errp)
{
@@ -2636,22 +2653,15 @@ static void bdrv_replace_child(BdrvChild *child,
BlockDriverState *new_bs)
}
if (old_bs) {
-/* Update permissions for old node. This is guaranteed to succeed
- * because we're just taking a parent away, so we're loosening
- * restrictions. */
bool tighten_restrictions;
-int ret;
-bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm);
-ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL,
- &tighten_restrictions, NULL);
+/*
+ * Update permissions for old node. We're just taking a parent away, so
+ * we're loosening restrictions. Errors of permission update are not
+ * fatal in this case, ignore them.
+ */
+bdrv_refresh_perms(old_bs, &tighten_restrictions, NULL);
assert(tighten_restrictions == false);
-if (ret < 0) {
-/* We only tried to loosen restrictions, so errors are not fatal */
-bdrv_abort_perm_update(old_bs);
-} else {
-bdrv_set_perm(old_bs, perm, shared_perm);
-}
/* When the parent requiring a non-default AioContext is removed, the
* node moves back to the main AioContext */
@@ -5760,7 +5770,6 @@ void bdrv_init_with_whitelist(void)
int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
{
BdrvChild *child, *parent;
-uint64_t perm, shared_perm;
Error *local_err = NULL;
int ret;
BdrvDirtyBitmap *bm;
@@ -5792,14 +5801,11 @@ int coroutine_fn
bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
*/
if (bs->open_flags & BDRV_O_INACTIVE) {
bs->open_flags &= ~BDRV_O_INACTIVE;
-bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+ret = bdrv_refresh_perms(bs, NULL, errp);
if (ret < 0) {
-bdrv_abort_perm_update(bs);
bs->open_flags |= BDRV_O_INACTIVE;
return ret;
}
-bdrv_set_perm(bs, perm, shared_perm);
if (bs->drv->bdrv_co_invalidate_cache) {
bs->drv->bdrv_co_invalidate_cache(bs, &local_err);
@@ -5875,7 +5881,6 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
{
BdrvChild *child, *parent;
bool tighten_restrictions;
-uint64_t perm, shared_perm;
int ret;
if (!bs->drv) {
@@ -5909,18 +5914,13 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
bs->open_flags |= BDRV_O_INACTIVE;
-/* Update permissions, they may differ for inactive nodes */
-bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
-ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
- &tighten_restrictions, NULL);
+/*
+ * Update permissions, they may differ for inactive nodes.
+ * We only tried to loosen restrictions, so errors are not fatal, ignore
+ * them.
+ */
+bdrv_refresh_perms(bs, &tighten_restrictions, NULL);
assert(tighten_restrictions == false);
-if (ret < 0) {
-/* We only tried to loosen restrictions, so errors are not fatal */
-bdrv_abort_perm_update(bs);
-} else {
-bdrv_set_perm(bs, perm, shared_perm);
-}
-
/* Recursively inactivate children */
QLIST_FOREACH(child, &bs->children, next) {
--
2.21.3
[PATCH 2/2] monitor: increase amount of data for monitor to read
QMP and HMP monitors read one byte at a time from the socket or stdin,
which is very inefficient. With 100+ VMs on the host, this results in
multiple extra system calls and CPU overuse.
This patch increases the amount of read data up to 4096 bytes that fits
the buffer size on the channel level.
Suggested-by: Denis V. Lunev
Signed-off-by: Andrey Shinkevich
---
chardev/char-fd.c | 64 +-
chardev/char-socket.c | 54 +++---
chardev/char.c | 40 +
include/chardev/char.h | 15 +++
monitor/monitor.c | 2 +-
tests/qemu-iotests/247.out | 2 +-
6 files changed, 159 insertions(+), 18 deletions(-)
diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2..6194fe6 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -33,6 +33,8 @@
#include "chardev/char-fd.h"
#include "chardev/char-io.h"
+#include "monitor/monitor-internal.h"
+
/* Called with chr_write_lock held. */
static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
{
@@ -41,7 +43,7 @@ static int fd_chr_write(Chardev *chr, const uint8_t *buf, int
len)
return io_channel_send(s->ioc_out, buf, len);
}
-static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean fd_chr_read_hmp(QIOChannel *chan, void *opaque)
{
Chardev *chr = CHARDEV(opaque);
FDChardev *s = FD_CHARDEV(opaque);
@@ -71,6 +73,66 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition
cond, void *opaque)
return TRUE;
}
+static gboolean fd_chr_read_qmp(QIOChannel *chan, void *opaque)
+{
+static JSONthrottle thl = {0};
+uint8_t *start;
+Chardev *chr = CHARDEV(opaque);
+FDChardev *s = FD_CHARDEV(opaque);
+int len, size, pos;
+ssize_t ret;
+
+if (!thl.load) {
+len = sizeof(thl.buf);
+if (len > s->max_size) {
+len = s->max_size;
+}
+if (len == 0) {
+return TRUE;
+}
+
+ret = qio_channel_read(
+chan, (gchar *)thl.buf, len, NULL);
+if (ret == 0) {
+remove_fd_in_watch(chr);
+qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+thl = (const JSONthrottle){0};
+return FALSE;
+}
+if (ret < 0) {
+return TRUE;
+}
+thl.load = ret;
+thl.cursor = 0;
+}
+
+size = thl.load;
+start = thl.buf + thl.cursor;
+pos = qemu_chr_end_position((const char *) start, size, &thl);
+if (pos >= 0) {
+size = pos + 1;
+}
+
+qemu_chr_be_write(chr, start, size);
+thl.cursor += size;
+thl.load -= size;
+
+return TRUE;
+}
+
+static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+CharBackend *be = chr->be;
+Monitor *mon = (Monitor *)be->opaque;
+
+if (monitor_is_qmp(mon)) {
+return fd_chr_read_qmp(chan, opaque);
+}
+
+return fd_chr_read_hmp(chan, opaque);
+}
+
static int fd_chr_read_poll(void *opaque)
{
Chardev *chr = CHARDEV(opaque);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8..8335e8c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -520,30 +520,54 @@ static void tcp_chr_disconnect(Chardev *chr)
static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
{
+static JSONthrottle thl = {0};
+uint8_t *start;
Chardev *chr = CHARDEV(opaque);
SocketChardev *s = SOCKET_CHARDEV(opaque);
-uint8_t buf[CHR_READ_BUF_LEN];
-int len, size;
+int len, size, pos;
if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
s->max_size <= 0) {
return TRUE;
}
-len = sizeof(buf);
-if (len > s->max_size) {
-len = s->max_size;
-}
-size = tcp_chr_recv(chr, (void *)buf, len);
-if (size == 0 || (size == -1 && errno != EAGAIN)) {
-/* connection closed */
-tcp_chr_disconnect(chr);
-} else if (size > 0) {
-if (s->do_telnetopt) {
-tcp_chr_process_IAC_bytes(chr, s, buf, &size);
+
+if (!thl.load) {
+len = sizeof(thl.buf);
+if (len > s->max_size) {
+len = s->max_size;
+}
+size = tcp_chr_recv(chr, (void *)thl.buf, len);
+if (size == 0 || (size == -1 && errno != EAGAIN)) {
+/* connection closed */
+tcp_chr_disconnect(chr);
+thl = (const JSONthrottle){0};
+return TRUE;
}
-if (size > 0) {
-qemu_chr_be_write(chr, buf, size);
+if (size < 0) {
+return TRUE;
}
+thl.load = size;
+thl.cursor = 0;
+}
+
+size = thl.load;
+start = thl.buf + thl.cursor;
+pos = qemu_chr_end_position((const char *) start, size, &thl);
+if (pos >= 0) {
+size = pos + 1;
+}
+len = size;
+
+if (s->do_telnetopt)
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity
On Thu, 5 Nov 2020 17:18:56 -0500 Daniele Buono wrote: > This patch adds supports for Control-Flow Integrity checks > on indirect function calls. > > Requires the use of clang, and link-time optimizations > > Changes in v3: > > - clang 11+ warnings are now handled directly at the source, > instead of disabling specific warnings for the whole code. > Some more work may be needed here to polish the patch, I > would kindly ask for a review from the corresponding > maintainers Process question :) Would you prefer to have this series merged in one go, or should maintainers pick the patches for their subsystem? > - Remove configure-time checks for toolchain compatibility > with LTO. > - the decorator to disable cfi checks on functions has > been renamed and moved to include/qemu/compiler.h > - configure-time checks for cfi support and dependencies > has been moved from configure to meson > > Link to v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html > Link to v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html > > Daniele Buono (9): > fuzz: Make fork_fuzz.ld compatible with LLVM's LLD > s390x: fix clang 11 warnings in cpu_models.c > hw/usb: reorder fields in UASStatus > s390x: Avoid variable size warning in ipl.h > scsi: fix overflow in scsi_disk_new_request_dump > configure,meson: add option to enable LTO > cfi: Initial support for cfi-icall in QEMU > check-block: enable iotests with cfi-icall > configure/meson: support Control-Flow Integrity > > accel/tcg/cpu-exec.c | 11 + > configure | 26 > hw/s390x/ipl.h| 4 +-- > hw/scsi/scsi-disk.c | 4 +++ > hw/usb/dev-uas.c | 2 +- > include/qemu/compiler.h | 12 + > meson.build | 46 +++ > meson_options.txt | 4 +++ > plugins/core.c| 37 > plugins/loader.c | 7 ++ > target/s390x/cpu_models.c | 8 +++--- > tcg/tci.c | 7 ++ > tests/check-block.sh | 18 -- > tests/qtest/fuzz/fork_fuzz.ld | 12 - > util/main-loop.c | 11 + > util/oslib-posix.c| 11 + > 16 files changed, 205 insertions(+), 15 deletions(-) >
Re: [PATCH v2 1/7] block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache()
On Fri 06 Nov 2020 01:42:35 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: Documents not in sphinx toctree
> > >> By running sphinx over the docs/ directory (like readthedocs.org > > >> presumably does), it finds a couple of rst documents that are not > > >> referenced: > > >> - cpu-hotplug.rst > > >> - microvm.rst > > >> - pr-manager.rst > > >> - virtio-net-failover.rst > > > > Given the current structure of the content in > > https://qemu.readthedocs.io/en/latest/, > > would adding this as a new bullet in "QEMU System Emulation User’s > > Guide" be the right thing to do? > > Adding which? > > For cpu-hotplug.rst: > I guess the system manual. The document has a bit of a > "tutorial" feel which doesn't entirely fit the rest of the > manuals. > > For microvm.rst: > docs/system/target-i386.rst should be split into > documentation for each of the machine models separately > (as a list of links to docs in docs/system/i386/, similar > to the structure of target-arm.rst and docs/system/arm/). > Then microvm.rst can go into docs/system/i386. > > For pr-manager.rst: > The parts that are documenting the qemu-pr-helper invocation > should turn into a docs/tools/ manpage for it. > The other parts should go in the system manual I guess. > > For virtio-net-failover.rst: > Should go under the "Network emulation" part of the system > manual, I think. Maybe existing memory emulation 'txt' files need to be converted into '.rst', and along with virtio-pmem.rst could be moved to a new section? Thanks, Pankaj
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
Peter Maydell writes: > On Fri, 6 Nov 2020 at 06:15, Gan Qixin wrote: >> >> Modify the rule that limit the length of lines according to the following >> ideas: >> >> --add a variable max_line_length to indicate the limit of line length and >> set it to 100 by default >> --when the line length exceeds max_line_length, output warning information >> instead of error >> --if/while/etc brace do not go on next line whether the line length exceeds >> max_line_length or not The commit message fails at explaining *why*. A controversial change like this should not be committed without proper rationale. >> Signed-off-by: Gan Qixin >> --- >> scripts/checkpatch.pl | 18 +- >> 1 file changed, 5 insertions(+), 13 deletions(-) > > For the code changes > Reviewed-by: Peter Maydell > > but we also need to update our coding style documentation > to match. I'll send out a patch with some proposed text. I disagree with the coding style change. The current "warn at 80, error at 90" is a compromise. It's the result of a lengthy argument. Why reopen it? > Side note: the kernel version of this checkpatch change > (kernel commit bdc48fa11e46f867) suppresses all line-length > warnings for the "--file" use case. Do we care about that? The kernel patch at least has a decent commit message.
Re: [PATCH] CODING_STYLE.rst: Be less strict about 80 character limit
Peter Maydell writes: > Relax the wording about line lengths a little bit; this goes with the > checkpatch changes to warn at 100 characters rather than 80. > > (Compare the Linux kernel commit bdc48fa11e46f8; our coding style is > not theirs, but the rationale is good and applies to us too.) > > Signed-off-by: Peter Maydell > --- > CODING_STYLE.rst | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst > index 8b13ef0669e..7bf4e39d487 100644 > --- a/CODING_STYLE.rst > +++ b/CODING_STYLE.rst > @@ -85,8 +85,13 @@ Line width > Lines should be 80 characters; try not to make them longer. > > Sometimes it is hard to do, especially when dealing with QEMU subsystems > -that use long function or symbol names. Even in that case, do not make > -lines much longer than 80 characters. > +that use long function or symbol names. If wrapping the line at 80 columns > +is obviously less readable and more awkward, prefer not to wrap it; better > +to have an 85 character line than one which is awkwardly wrapped. > + > +Even in that case, try not to make lines much longer than 80 characters. > +(The checkpatch script will warn at 100 characters, but this is intended > +as a guard against obviously-overlength lines, not a target.) > > Rationale: Alright, that's much more reasobale than I expected. Reviewed-by: Markus Armbruster One more thing that might be worth explaining: the width of the text matters, i.e. line length less indentation.
[PULL for-5.2 3/4] target/s390x: fix execution with icount
From: Pavel Dovgalyuk This patch adds some gen_io_start() calls to allow execution of s390x targets in icount mode with -smp 1. It enables deterministic timers and record/replay features. Suggested-by: Richard Henderson Signed-off-by: Pavel Dovgalyuk Reviewed-by: Richard Henderson Acked-by: David Hildenbrand Message-Id: <16041747.32240.17074484658979970129.stgit@pasha-ThinkPad-X280> Signed-off-by: Cornelia Huck --- target/s390x/insn-data.def | 70 +++--- target/s390x/translate.c | 15 2 files changed, 50 insertions(+), 35 deletions(-) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index d3bcdfd67b3c..b95bc98d357a 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -379,7 +379,7 @@ /* EXTRACT CPU ATTRIBUTE */ C(0xeb4c, ECAG,RSY_a, GIE, 0, a2, r1, 0, ecag, 0) /* EXTRACT CPU TIME */ -C(0xc801, ECTG,SSF, ECT, 0, 0, 0, 0, ectg, 0) +F(0xc801, ECTG,SSF, ECT, 0, 0, 0, 0, ectg, 0, IF_IO) /* EXTRACT FPC */ F(0xb38c, EFPC,RRE, Z, 0, 0, new, r1_32, efpc, 0, IF_BFP) /* EXTRACT PSW */ @@ -855,10 +855,10 @@ C(0xe32f, STRVG, RXY_a, Z, la2, r1_o, new, m1_64, rev64, 0) /* STORE CLOCK */ -C(0xb205, STCK,S, Z, la2, 0, new, m1_64, stck, 0) -C(0xb27c, STCKF, S, SCF, la2, 0, new, m1_64, stck, 0) +F(0xb205, STCK,S, Z, la2, 0, new, m1_64, stck, 0, IF_IO) +F(0xb27c, STCKF, S, SCF, la2, 0, new, m1_64, stck, 0, IF_IO) /* STORE CLOCK EXTENDED */ -C(0xb278, STCKE, S, Z, 0, a2, 0, 0, stcke, 0) +F(0xb278, STCKE, S, Z, 0, a2, 0, 0, stcke, 0, IF_IO) /* STORE FACILITY LIST EXTENDED */ C(0xb2b0, STFLE, S, SFLE, 0, a2, 0, 0, stfle, 0) @@ -1269,7 +1269,7 @@ E(0xb250, CSP, RRE, Z, r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL, IF_PRIV) E(0xb98a, CSPG,RRE, DAT_ENH, r1_o, ra2, r1_P, 0, csp, 0, MO_TEQ, IF_PRIV) /* DIAGNOSE (KVM hypercall) */ -F(0x8300, DIAG,RSI, Z, 0, 0, 0, 0, diag, 0, IF_PRIV) +F(0x8300, DIAG,RSI, Z, 0, 0, 0, 0, diag, 0, IF_PRIV | IF_IO) /* INSERT STORAGE KEY EXTENDED */ F(0xb229, ISKE,RRE, Z, 0, r2_o, new, r1_8, iske, 0, IF_PRIV) /* INVALIDATE DAT TABLE ENTRY */ @@ -1301,17 +1301,17 @@ /* RESET REFERENCE BIT EXTENDED */ F(0xb22a, RRBE,RRE, Z, 0, r2_o, 0, 0, rrbe, 0, IF_PRIV) /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */ -F(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0, IF_PRIV) +F(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO) /* SET ADDRESS SPACE CONTROL FAST */ F(0xb279, SACF,S, Z, 0, a2, 0, 0, sacf, 0, IF_PRIV) /* SET CLOCK */ -F(0xb204, SCK, S, Z, la2, 0, 0, 0, sck, 0, IF_PRIV) +F(0xb204, SCK, S, Z, la2, 0, 0, 0, sck, 0, IF_PRIV | IF_IO) /* SET CLOCK COMPARATOR */ -F(0xb206, SCKC,S, Z, 0, m2_64a, 0, 0, sckc, 0, IF_PRIV) +F(0xb206, SCKC,S, Z, 0, m2_64a, 0, 0, sckc, 0, IF_PRIV | IF_IO) /* SET CLOCK PROGRAMMABLE FIELD */ F(0x0107, SCKPF, E, Z, 0, 0, 0, 0, sckpf, 0, IF_PRIV) /* SET CPU TIMER */ -F(0xb208, SPT, S, Z, 0, m2_64a, 0, 0, spt, 0, IF_PRIV) +F(0xb208, SPT, S, Z, 0, m2_64a, 0, 0, spt, 0, IF_PRIV | IF_IO) /* SET PREFIX */ F(0xb210, SPX, S, Z, 0, m2_32ua, 0, 0, spx, 0, IF_PRIV) /* SET PSW KEY FROM ADDRESS */ @@ -1321,7 +1321,7 @@ /* SET SYSTEM MASK */ F(0x8000, SSM, S, Z, 0, m2_8u, 0, 0, ssm, 0, IF_PRIV) /* SIGNAL PROCESSOR */ -F(0xae00, SIGP,RS_a, Z, 0, a2, 0, 0, sigp, 0, IF_PRIV) +F(0xae00, SIGP,RS_a, Z, 0, a2, 0, 0, sigp, 0, IF_PRIV | IF_IO) /* STORE CLOCK COMPARATOR */ F(0xb207, STCKC, S, Z, la2, 0, new, m1_64a, stckc, 0, IF_PRIV) /* STORE CONTROL */ @@ -1332,7 +1332,7 @@ /* STORE CPU ID */ F(0xb202, STIDP, S, Z, la2, 0, new, m1_64a, stidp, 0, IF_PRIV) /* STORE CPU TIMER */ -F(0xb209, STPT,S, Z, la2, 0, new, m1_64a, stpt, 0, IF_PRIV) +F(0xb209, STPT,S, Z, la2, 0, new, m1_64a, stpt, 0, IF_PRIV | IF_IO) /* STORE FACILITY LIST */ F(0xb2b1, STFL,S, Z, 0, 0, 0, 0, stfl, 0, IF_PRIV) /* STORE PREFIX */ @@ -1352,35 +1352,35 @@ C(0xe501, TPROT, SSE, Z, la1, a2, 0, 0, tprot, 0) /* CCW I/O Instructions */ -F(0xb276, XSCH,S, Z, 0, 0, 0, 0, xsch, 0, IF_PRIV) -F(0xb230, CSCH,S, Z, 0, 0, 0, 0, csch, 0, IF_PRIV) -F(0xb231, HSCH,S, Z, 0, 0, 0, 0, hsch, 0, IF_PRIV) -F(0xb232, MSCH,S, Z, 0, insn, 0, 0, msch, 0, IF_PRIV) -F(0xb23b, RCHP,S, Z, 0, 0, 0, 0, rchp, 0, IF_PRIV) -F(0xb238, RSCH,S, Z, 0, 0, 0, 0, rsch, 0, IF_PRIV) -F(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0, IF_PRIV) -F(0xb23c, SCHM,S, Z, 0, insn, 0, 0, schm, 0, IF_PRIV) -F(0xb274, SIGA,S, Z, 0, 0, 0, 0, siga, 0, IF_PRIV) -F(0xb23a, ST
[PULL for-5.2 1/4] s390-bios: Skip writing iplb location to low core for ccw ipl
From: "Jason J. Herne"
The architecture states that the iplb location is only written to low
core for list directed ipl and not for traditional ccw ipl. If we don't
skip this then operating systems that load by reading into low core
memory may fail to start.
We should also not write the iplb pointer for network boot as it might
overwrite content that we got via network.
Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
Signed-off-by: Jason J. Herne
Signed-off-by: Christian Borntraeger
Acked-by: Thomas Huth
Message-Id: <20201030122823.347140-1-borntrae...@de.ibm.com>
Signed-off-by: Cornelia Huck
---
pc-bios/s390-ccw/main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 43c792cf9509..fc4bfaa45529 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -43,7 +43,9 @@ void write_subsystem_identification(void)
void write_iplb_location(void)
{
-lowcore->ptr_iplb = ptr2u32(&iplb);
+if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET)
{
+lowcore->ptr_iplb = ptr2u32(&iplb);
+}
}
unsigned int get_loadparm_index(void)
--
2.26.2
[PULL for-5.2 4/4] s390x: fix build for --without-default-devices
s390-pci-vfio.c calls into the vfio code, so we need it to be
built conditionally on vfio (which implies CONFIG_LINUX).
Fixes: cd7498d07fbb ("s390x/pci: Add routine to get the vfio dma available
count")
Reported-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Matthew Rosato
Message-Id: <20201103123237.718242-1-coh...@redhat.com>
Acked-by: Greg Kurz
Tested-by: Greg Kurz
Signed-off-by: Cornelia Huck
---
hw/s390x/meson.build | 2 +-
include/hw/s390x/s390-pci-vfio.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index f4663a835514..2a7818d94b94 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -27,7 +27,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
))
s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true:
files('s390-virtio-ccw.c'))
s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
-s390x_ss.add(when: 'CONFIG_LINUX', if_true: files('s390-pci-vfio.c'))
+s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
virtio_ss = ss.source_set()
virtio_ss.add(files('virtio-ccw.c'))
diff --git a/include/hw/s390x/s390-pci-vfio.h b/include/hw/s390x/s390-pci-vfio.h
index c7984905b3b7..ff708aef500f 100644
--- a/include/hw/s390x/s390-pci-vfio.h
+++ b/include/hw/s390x/s390-pci-vfio.h
@@ -13,8 +13,9 @@
#define HW_S390_PCI_VFIO_H
#include "hw/s390x/s390-pci-bus.h"
+#include CONFIG_DEVICES
-#ifdef CONFIG_LINUX
+#ifdef CONFIG_VFIO
bool s390_pci_update_dma_avail(int fd, unsigned int *avail);
S390PCIDMACount *s390_pci_start_dma_count(S390pciState *s,
S390PCIBusDevice *pbdev);
--
2.26.2
[PULL for-5.2 0/4] s390x fixes
The following changes since commit 3d6e32347a3b57dac7f469a07c5f520e69bd070a: Update version for v5.2.0-rc0 release (2020-11-03 21:11:57 +) are available in the Git repository at: https://github.com/cohuck/qemu tags/s390x-20201106 for you to fetch changes up to 77280d33bc9cfdbfb5b5d462259d644f5aefe9b3: s390x: fix build for --without-default-devices (2020-11-05 13:04:07 +0100) some s390x fixes, including a bios update Cornelia Huck (2): pc-bios/s390: update s390-ccw bios binaries s390x: fix build for --without-default-devices Jason J. Herne (1): s390-bios: Skip writing iplb location to low core for ccw ipl Pavel Dovgalyuk (1): target/s390x: fix execution with icount hw/s390x/meson.build | 2 +- include/hw/s390x/s390-pci-vfio.h | 3 +- pc-bios/s390-ccw.img | Bin 42608 -> 46704 bytes pc-bios/s390-ccw/main.c | 4 +- pc-bios/s390-netboot.img | Bin 67232 -> 71328 bytes target/s390x/insn-data.def | 70 +++ target/s390x/translate.c | 15 +++ 7 files changed, 56 insertions(+), 38 deletions(-) -- 2.26.2
Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test
Paolo Bonzini writes: > device-introspect-test uses HMP, so it should escape the device name > properly. Because of this, a few devices that had commas in their > names were escaping testing. > Signed-off-by: Paolo Bonzini $ git-grep '\.name *= *"[^"]*,' | cat hw/block/fdc.c:.name = "SUNW,fdtwo" Any others?
Re: [PATCH V2 1/2] plugins: Fix resource leak in connect_socket()
On 11/5/20 7:59 PM, AlexChen wrote:
> Close the fd when the connect() fails.
>
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
Your From: line ("AlexChen") is spelled differently than your S-o-b:
line ("Alex Chen"). While this is not fatal to the patch, it is
confusing, so you may want to update your git settings to produce mail
spelled in the same manner as the S-o-b.
Also, although you did manage to send a 0/2 letter, you did not thread
things:
0/2 Message-ID: <5fa4ae0b.1000...@huawei.com>
1/2 Message-ID: Message-ID: <5fa4ae11.6060...@huawei.com>, but no
In-Reply-To: or References: headers, which means it is a new top-level
thread. You may want to figure out why your mail setup is not
preserving threading.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PULL for-5.2 2/4] pc-bios/s390: update s390-ccw bios binaries
Contains "s390-bios: Skip writing iplb location to low core for ccw ipl".
Signed-off-by: Cornelia Huck
---
pc-bios/s390-ccw.img | Bin 42608 -> 46704 bytes
pc-bios/s390-netboot.img | Bin 67232 -> 71328 bytes
2 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/pc-bios/s390-ccw.img b/pc-bios/s390-ccw.img
index
5b57ea2837680ca6f6e98e7703b89818fd8fb316..4e4b3207c4626e5b9f04be7980d2b6a31538b0ee
100644
GIT binary patch
literal 46704
zcmeIbdt6mj`aizT0RahRb5RedxR0XZ1w7#0QqKWYoKlas!kkKkqml>&2h*B9Q88uH
zT#$*TX8P30q-~eqbflS5o06ucslGLll{%l9DeGie%%={?ahtzE}3t&T14n4LJUUUDCp-i%AGQ8_H*hKGem#k6!Bspo}=&^
zDeF-te|n^rQ?l4hMGT%J@N4w5&XDyzQ_LWL%#`4w-+o4UAIZ)=vcgOidZc~?D}H|4
zD${3sq!9)jX4?OnzBV(qNf_}@;*qE;`yjUD^Kr%G|Tw7dGC9LLsK&=x5=NV55
z{}pZtkJh1o!0T9m+p5+^KUjeIT7}5H
z9bBJ5y!^vEUC!W6r^9b8RQy7lq3g|4oT7Q8aBOpkVcHzftxjC0&CzwFo3~qn4}%8>
zXcxZhqRiMY3ROSzm$RUyUWl?H!aJ2Ym3l58^iS=#
zx^7S-IiLm~5!;81S4L{%b$y#EMw~tR+m?FKDhkwk5pZD0?SKdH%_P|v(=-BO)e5_o
zVvi=t>XNFn$4N}k>huQ)%A|)#mWJ39M$(Mnw!5(|3(`Ba4E+q!ddXiBZ@}0Zz}pbc
z`*L
z&TXR7dCdRu>uILWXQHFq9uK-D9bFFtDhJVVqyKG-pYY@GivTFe_$k%W7(bV4Y4&K~
zX_3%;T#&}lxG5`4DBjrzZnUPJA}Zma-c)E^MRy>6v#
z5AwBbPm7C0oYr5&1Mg04g0Q%r4nFM44?cWAD8euB*Wwh=hE{09cX@5M^NJ!d$3Dsp
z)VcI7&VRvcTS9QHzPM=;Q!}US@pkZ4p$fTx)XlA?mWeMxlfDZ4?9f&P1t1(oI(TD_
z%jFcV(MzTr5Gv+3v~b7%5zwh#F_U{fu1x}U>x9a)*mzuwIO>ZVfzr*G_Kr3_ZVmdr&=v5lMj><2;
zDWD{76On*N6soFlJCr}TRb@|fC$(&QH@HFaqtioDLQq}3;cbf|T4R83!Yuf|?cMem
z;(dnUV@V6WgIBODptJ(r0hQ-S7Z|1ZhhNl!Ai`BlB*Z+d_=`sL&^q2
zAALsiMR&+{W{UxTrru|U1KzXb&Y3_^4S3R&y8*jt^!y0wp}*T!5p>A)<^Nba{aK95
z5j+(D7DOcVN;?TI@miL=9nmU+r;y8U6v3~M&I%~)lSIAE4xQGS3(0Drw<
zcJ?Pqe!N>taf*gVz%>_w$A{=&VI|YhY8Pm>ilul@Eupg%l&-Q;N|SQoILJ
zyyq+_z6?@KGyRCyd;_gn>|r<}!FzyJ!Ur2m{=`M3lRjRj-6Qn)l+`%3!MK6
z<#Yee+`9*D$D-{kT>lD}?c;v0b6YQ$|DN;Z+^>b(+{U}7
z%6J_*imf;F)J=iBdaKxeyHms^&kHQChaF3uw|5w8lE`guU9pN_sALMenSX{qE>2Yv8n
z650~5a~=>#p!$&PJ_gx+3>@;9mTvJtQXhj4aWvqE6ehO~<@VRq&yXB5;wh|k2e0*N
znhz)ovzniH?XyN*j970BlGj>3AI!Rhe@s7Jd|SY0#LyALqY
zdWZ6QSwkKAy_TXv?z^af87I(+4?&X)G3;Sp?T=bOxkLABw_BN$N327uwG3ej_7{Kw
z&&{_hBCaVbUQbTM+&Xp%g!@Fy{{-OKU7-4`pc7epcv*m-V5${^0oQJ{CAhGYp;B!>
z`HK3b+HKH19r|wrYkd90cA72ip_H}Ii+6#e;%U5OyN#hpJ7^o+gouhQ8#Q{eeNc|J
zQ*1^*jLJ|BG|I<7imz@IzDVeWb-X^8p>IZ`G&|}$Y
z@z{qEwGx@SF(L{1F~E);V{}o?9vb_f&`M*pbiIir^`bWjrr6DtpK}A}D=F{%BT3{}
z_n}uR_(;LnF^2vFAArBdVyB6A2ciFa=YxYgX|$nDSbtq-Cy##_k6B9`4V(Zz=3>e9
zc9V-^{*Ji>^xQFUH9Vh~!{o<*cmp6AM^YBfbHr~SR-$G+&sBk3Vq$`=iprlY(Z<0l
zc`b`W)@iK%73gUjIs_8;7}>Co-qcE^j!jEJU)Dau$;ZQ9wYHmtz<%ff2fA!e9R>U*
z1NX$O;HFxZ6ru`vP;zaY7>fSDJ(UF3kj2E9McPL2C+)!P==*}0Oi;d`@|gLn-vZKC-?qS}?|LvhaTB7qLKlL|CZx@Y%5KU#0AT{rD&J$Ya`dpvfH~hV_OH
zZ37JJjfC~t$<)68k7@Uyb)5Mjm0fs{%dY48-*CQ$@^jf!IsG8FVGGi~l-qc?O)m21*8@4wXxndhK>JMz7#fB1&vQN&w#f%RM*C-mar@!i{%XklF>W`S+I@LK
z`n_HIDwz6}!0V42Eq*W3`x}M725BSG%}DP+`L#%I3i$jtNR2lv6TIOS
zRW#?YM}dz9&RvO}!*#56cbHn2Fp~^^_isUYt#}Z+Wte!NK=ohOP3zX8Hb(3KJ|0Hf
zABD88#Q1fg88YEzmGBISgk9F_IV?S@e=V?_V@OXDBq1Jq#u7Z|AWzh3Ffg8iG1g%Q
zvagVBlxEBQYfA?>?+D{<731x4&i|P6D>%QI^NTsp`_?ZDIM2H0%UsSc;5HY*ALlq6
z#UXgil-p*0N7ZQ%&O%4rQqi>
zZgm*BoekNTuNDy9g{S8ma*U^6;OP(C>P@1+7uR>~Z{~~!{xi7lJsvTW{P@7ln2or1
zLT{QsP)*vDTHC6K0+u+&{0~8cwSjtHsg#7&T`3JH*$6wm7AeC!bR^4VI`vI`3)tKs
z>Y>kQm&3cFf;GO9f?6M2`k~n21}VNqZJ$NkWzg%8C$xV_q{Ew7Tu|x5uA3|qboA5E
zg78V9fnOjl3D^Onm0?twOMV9!1$;!WNZ}lL6q9(}fwl_Tr;^Q!k{G`xtc0!gh0lhS
z+{?Wx!%BY1B{znZu>Zg8O`||HdI&fJEJeFd}
z_x99uU~QMEg@$Rs?_fYeeq9xQZzes2XINQayO!xw3(4P#-cajpHZ`7Zd710jjeVw7aGOKeb`g2o_zLHPp&LBHL^_wPrcO`m-K
z-bvtz;T?Q}d8!helCxWM@_Vkn9Bfp9{aq!=`*Am3-#VVzh_PVo>#t|p>
zX%+_gzlB|BTB)Cm-_sWUPp5wD_Wci8#!4M6T>)
zbe;=R(V>6T7~w;V6NA^oe{QFEt{4Qq7|jq}jujEMGHkF%IBaVrc&r0#g|s@vujbL*
zq!lmx6JS;;M)zJKMGv3Jc1FWn`-hY}F$#Ny1tN`BeBzbFr)^bQoh67_CeSQ2!;zCb
zKJOcO3KiDDL%qUUn_P+lq4>3n1-iTevEd6
zVfqCkCUYCNVVxKYT1-XsgFXCNLwL-48il_f`g9DyPRG%o!Ml0S60uOg+b7BY|w*}ljtB6eTApdFGLTa0u
zQ<&^mhbFqG_?>R#wp)EEsJ&JDxuAIWGvp1!8s9BPrc;Z=ZEic-+5yQ@K$S0kN|1%1
zAss@{c=e~-w$cpH0OU7W0%Nfp=rWudXw2cSx`z1GfYS#q+&@AQw)Dd3fZ3}*WJ^j-
z#&}C!G7kliXu5{}8{^geyiACitC$upiNR2QZbRI}sfJ
znK_?kjo3ir&14Pwue;iyL&Si>L`1h!Pl?ndip#ol{LaE0tZ5cv8A#oz<(0gy{RAFd
z7Zwa!27VQ=ht@Qk*R)pqIjHZ|pX9mVM`PZJa);5DEgWJ1v@0wM##)3G@_s2v+S8e)
zoC;wb!oB0T_W(rTFT#jFGdU>mLvXCkz@(LD9jk21zpzqgGqxEfy
zxPZo+@r8}|$kq?gY7fsah$P})tnX$(Ya>6H%BQkzw6=0D;0q9OtZgPde_ii5#pOku
z@8mqzLFI;*$ofWli8Cg0OiF1gV-UwDyw`CEKK
zymQpneCYb%&fre8nkBWmBNU@0E%MG=kSMkyNpybd6+!k#h~Js+Q7hn&TJbazX$#A?SSOzL|DtenczDm$C90chbWgFToy#TAg9ZO}bx}
z=^UT@Y6_1?=OAX!35=!Nd5+PL=SX<18!9eeKaDVV)2o{7_Bsh&fVN(hoQX@8&tH5Q8z^YEO8}jFQHNU?PTnXhqQ7u
z5_|A{kZtJW76n+$hA)6w5;6XII>=
zPXCml!N~~VtA+OXM9LM~U*9FaGx{?=^
zSTo%g7>eKBxfb6*@T7(J6?~>CGHerUL$&c@ApA`76MyGad@-6WRUnOZsy@*md^l5Y
z70s$hZ5}DqZ7R+x0sm(K`Ap%@h9P%4uY3(up10sy~VZKH?U!VTjWKm
ztH$eV*t~r?%7Y1-DotAI9li99j%>s=#7~h(sETQ#++lbrVE(
zK*T5fyu(Gedl=Vs#YR
z$^qrn2GONWHJbw5tHQu}B39_z<}+|UPJ2Y}1lBBr9)OpVLcGm#mIOFr;1^jUhml_t
za5_Q%ynI#UlCMcSu<817NiUOD;qi)Tw0~KnrNo~C^|ArgFY$|Lpvcws#-D;mXNmYg
z!c_}?s5SY)7S0c!{tXg#4{$LU=}-eNX#`Wmi>^^V;mUKe#W(=0Ipd)fI}lrl!TJ7p
z{R@MW=nlp&$WI0*5kGh6?*k6vix$RX=)7n84S?4{a9wb>++zonTF5i*9f2B_BE|`O
zuew7|pNa2
[Question] Fuzz: No rule to make target 'i386-softmmu/fuzz'
Hi, I am a newbie to QEMU and trying to build the virtual-device fuzzer according to qemu/docs/devel/fuzzing.txt, which says: --- Configure with (substitute the clang binaries with the version you installed). ... CC=clang-8 CXX=clang++-8 /path/to/configure --enable-fuzzing \ --enable-sanitizers Fuzz targets are built similarly to system/softmmu: make i386-softmmu/fuzz This builds ./i386-softmmu/qemu-fuzz-i386 --- But when I did this on my Ubuntu 20.04 x86-64 with qemu-5.2.0-rc0 release code, the make complained it could not find the target: --- root@iZj6canc2b2vgdozetp9foZ:~/qemu# CC=clang-10 CXX=clang++-10 ./configure --enable-fuzzing --enable-sanitizers > configure.log root@iZj6canc2b2vgdozetp9foZ:~/qemu# make i386-softmmu/fuzz changing dir to build for make "i386-softmmu/fuzz"... make[1]: Entering directory '/root/qemu/build' /usr/bin/ninja build.ninja && touch build.ninja.stamp ninja: no work to do. /usr/bin/python3 -B /root/qemu/meson/meson.py introspect --targets -- tests --benchmarks | /usr/bin/python3 -B scripts/mtest2make.py > Makefile.mtest make[1]: *** No rule to make target 'i386-softmmu/fuzz'. Stop. make[1]: Leaving directory '/root/qemu/build' make: *** [GNUmakefile:11: i386-softmmu/fuzz] Error 2 --- Did I missed something or misunderstood the instructions? Thanks. Using './build' as the directory for build output cross containers no NOTE: guest cross-compilers enabled: cc cc The Meson build system Version: 0.55.3 Source dir: /root/qemu Build dir: /root/qemu/build Build type: native build Project name: qemu Project version: 5.1.90 C compiler for the host machine: clang-10 (clang 10.0.0-4ubuntu1 "clang version 10.0.0-4ubuntu1 ") C linker for the host machine: clang-10 ld.bfd 2.34 Host machine cpu family: x86_64 Host machine cpu: x86_64 ../meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases. Program sh found: YES Program python3 found: YES (/usr/bin/python3) ../meson.build:99: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=undefined". ../meson.build:99: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=address". ../meson.build:99: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=fuzzer-no-link". ../meson.build:101: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=undefined". ../meson.build:101: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=address". ../meson.build:103: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=undefined". ../meson.build:103: WARNING: Consider using the built-in option for sanitizers instead of using "-fsanitize=address". C++ compiler for the host machine: clang++-10 (clang 10.0.0-4ubuntu1 "clang version 10.0.0-4ubuntu1 ") C++ linker for the host machine: clang++-10 ld.bfd 2.34 Program cgcc found: NO Library m found: YES Library util found: YES Run-time dependency appleframeworks found: NO (tried framework) Found pkg-config: /usr/bin/pkg-config (0.29.1) Run-time dependency pixman-1 found: YES 0.38.4 Library aio found: NO Run-time dependency zlib found: YES 1.2.11 Run-time dependency xkbcommon found: NO (tried pkgconfig) Library rt found: YES Did not find CMake 'cmake' Found CMake: NO Run-time dependency libudev found: NO (tried pkgconfig and cmake) Library mpathpersist found: NO Run-time dependency ncursesw found: YES 6.2.20200212 sdl2-config found: NO Run-time dependency sdl2 found: NO (tried pkgconfig and config-tool) Run-time dependency libpng found: NO (tried pkgconfig) Has header "jpeglib.h" : NO Has header "sasl/sasl.h" : NO Run-time dependency u2f-emu found: NO (tried pkgconfig) Run-time dependency libkeyutils found: NO (tried pkgconfig) Checking for function "gettid" : YES Program scripts/minikconf.py found: YES Configuring aarch64-softmmu-config-target.h using configuration Configuring aarch64-softmmu-config-devices.mak with command Reading depfile: /root/qemu/build/meson-private/aarch64-softmmu-config-devices.mak.d Configuring aarch64-softmmu-config-devices.h using configuration Configuring alpha-softmmu-config-target.h using configuration Configuring alpha-softmmu-config-devices.mak with command Reading depfile: /root/qemu/build/meson-private/alpha-softmmu-config-devices.mak.d Configuring alpha-softmmu-config-devices.h using configuration Configuring arm-softmmu-config-target.h using configuration Configuring arm-softmmu-config-devices.mak with command Reading depfile: /root/qemu/build/meson-private/arm-softmmu-config-devices.mak.d Configuring arm-softmmu-config-devices.h using configuration Configuring avr-softmmu-config-target.h using configuration Configuring avr-softmmu-config-devices.mak with command Reading depfile: /root/qemu/build/meson-private/avr-softmmu-config-devices.mak.d
Re: [Question] Fuzz: No rule to make target 'i386-softmmu/fuzz'
On 201106 2104, liqiuhao727 wrote: > Hi, > > I am a newbie to QEMU and trying to build the virtual-device fuzzer > according to qemu/docs/devel/fuzzing.txt, which says: > > --- > Configure with (substitute the clang binaries with the version you > installed). > ... > CC=clang-8 CXX=clang++-8 /path/to/configure --enable-fuzzing \ > --enable-sanitizers > Fuzz targets are built similarly to system/softmmu: Ah, these instructions went out of date when QEMU switched to meson. I'll send a patch to update them. > make i386-softmmu/fuzz > This builds ./i386-softmmu/qemu-fuzz-i386 This should be: make qemu-fuzz-i386 It looks like you are running these commands from the root qemu directory, so the resulting binary should be ./build/qemu-fuzz-i386 There are a couple fixes to the fuzzers that should be applied soon, so it might be a good idea to grab updated sources soon. They are part of this pull-req: https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg01142.html -Alex > --- > > But when I did this on my Ubuntu 20.04 x86-64 with qemu-5.2.0-rc0 > release code, the make complained it could not find the target: > > --- > root@iZj6canc2b2vgdozetp9foZ:~/qemu# CC=clang-10 CXX=clang++-10 > ./configure --enable-fuzzing --enable-sanitizers > configure.log > root@iZj6canc2b2vgdozetp9foZ:~/qemu# make i386-softmmu/fuzz > changing dir to build for make "i386-softmmu/fuzz"... > make[1]: Entering directory '/root/qemu/build' > /usr/bin/ninja build.ninja && touch build.ninja.stamp > ninja: no work to do. > /usr/bin/python3 -B /root/qemu/meson/meson.py introspect --targets -- > tests --benchmarks | /usr/bin/python3 -B scripts/mtest2make.py > > Makefile.mtest > make[1]: *** No rule to make target 'i386-softmmu/fuzz'. Stop. > make[1]: Leaving directory '/root/qemu/build' > make: *** [GNUmakefile:11: i386-softmmu/fuzz] Error 2 > --- > > Did I missed something or misunderstood the instructions? > Thanks.
Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test
On 06/11/20 14:15, Markus Armbruster wrote: Paolo Bonzini writes: device-introspect-test uses HMP, so it should escape the device name properly. Because of this, a few devices that had commas in their names were escaping testing. Signed-off-by: Paolo Bonzini $ git-grep '\.name *= *"[^"]*,' | cat hw/block/fdc.c:.name = "SUNW,fdtwo" Any others? Not that I know, but this is a bug anyway. :) Paolo
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity
Hi Cornelia, I don't have a real preference either way. So if it is acceptable to have the clang11+ patches separated and handled by the maintainers for the proper subsystem, I'd say whatever the maintainers prefer. In my opinion, the patches for clang11+ support may be merged separately. I'm saying this because, from my tests, the only feature that needs clang11+ to compile with Control-Flow Integrity is fuzzing. However, the main way we're fuzzing QEMU is through OSSfuzz, and I don't think their infrastructure is using a compiler that new, so we wouldn't be able to enable it anyway. (Alex can chip in to confirm this) On the other hand, if someone is looking for temporary support in-house, they can just add -Wno-[...] as extra-cflags until the additional patches land. (Assuming CFI lands before the clang11+ patches). Regards, Daniele On 11/6/2020 7:47 AM, Cornelia Huck wrote: On Thu, 5 Nov 2020 17:18:56 -0500 Daniele Buono wrote: This patch adds supports for Control-Flow Integrity checks on indirect function calls. Requires the use of clang, and link-time optimizations Changes in v3: - clang 11+ warnings are now handled directly at the source, instead of disabling specific warnings for the whole code. Some more work may be needed here to polish the patch, I would kindly ask for a review from the corresponding maintainers Process question :) Would you prefer to have this series merged in one go, or should maintainers pick the patches for their subsystem? - Remove configure-time checks for toolchain compatibility with LTO. - the decorator to disable cfi checks on functions has been renamed and moved to include/qemu/compiler.h - configure-time checks for cfi support and dependencies has been moved from configure to meson Link to v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html Link to v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html Daniele Buono (9): fuzz: Make fork_fuzz.ld compatible with LLVM's LLD s390x: fix clang 11 warnings in cpu_models.c hw/usb: reorder fields in UASStatus s390x: Avoid variable size warning in ipl.h scsi: fix overflow in scsi_disk_new_request_dump configure,meson: add option to enable LTO cfi: Initial support for cfi-icall in QEMU check-block: enable iotests with cfi-icall configure/meson: support Control-Flow Integrity accel/tcg/cpu-exec.c | 11 + configure | 26 hw/s390x/ipl.h| 4 +-- hw/scsi/scsi-disk.c | 4 +++ hw/usb/dev-uas.c | 2 +- include/qemu/compiler.h | 12 + meson.build | 46 +++ meson_options.txt | 4 +++ plugins/core.c| 37 plugins/loader.c | 7 ++ target/s390x/cpu_models.c | 8 +++--- tcg/tci.c | 7 ++ tests/check-block.sh | 18 -- tests/qtest/fuzz/fork_fuzz.ld | 12 - util/main-loop.c | 11 + util/oslib-posix.c| 11 + 16 files changed, 205 insertions(+), 15 deletions(-)
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On Fri, 6 Nov 2020 at 13:07, Markus Armbruster wrote: > The current "warn at 80, error at 90" is a compromise. It's the result > of a lengthy argument. Why reopen it? There was some previous discussion under this thread: https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html which I think is what prompted this patch. thanks -- PMM
Re: [PATCH] physmem: improve ram size error messages
Ping
> Ram size mismatch condition logs below message.
>
>"Length mismatch: pc.ram: 0x8000 in != 0x18000: Invalid argument"
>
> This patch improves the readability of error messages.
> Removed the superflous "in" and changed "Length" to "Size".
>
> Signed-off-by: Pankaj Gupta
> Reported-by: Li Zhang
> ---
> softmmu/physmem.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index e319fb2a1e..8da184f4a6 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1756,15 +1756,15 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t
> newsize, Error **errp)
>
> if (!(block->flags & RAM_RESIZEABLE)) {
> error_setg_errno(errp, EINVAL,
> - "Length mismatch: %s: 0x" RAM_ADDR_FMT
> - " in != 0x" RAM_ADDR_FMT, block->idstr,
> + "Size mismatch: %s: 0x" RAM_ADDR_FMT
> + " != 0x" RAM_ADDR_FMT, block->idstr,
> newsize, block->used_length);
> return -EINVAL;
> }
>
> if (block->max_length < newsize) {
> error_setg_errno(errp, EINVAL,
> - "Length too large: %s: 0x" RAM_ADDR_FMT
> + "Size too large: %s: 0x" RAM_ADDR_FMT
> " > 0x" RAM_ADDR_FMT, block->idstr,
> newsize, block->max_length);
> return -EINVAL;
> --
> 2.20.1
>
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On 11/6/20 2:39 PM, Peter Maydell wrote: > On Fri, 6 Nov 2020 at 13:07, Markus Armbruster wrote: >> The current "warn at 80, error at 90" is a compromise. It's the result >> of a lengthy argument. Why reopen it? > > There was some previous discussion under this thread: > https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html > > which I think is what prompted this patch. Can we keep the error please? Maybe 132 is the next display logical limit once we increased the warning from 80 to 100. I understand hardware evolved, we have larger displays with better resolution and can fit more characters in a line. I am a bit wary however functions become heavier (more code into a single function). Maybe this checkpatch change should go with a another one warning when a function has more than 80 lines, excluding comments? (Even 80 is too much for my taste). Regards, Phil.
[PATCH 1/1] Change the order of g_free(info) and tracepoint
Fixes Coverity issue:
CID 1436126: Memory - illegal accesses (USE_AFTER_FREE)
Fixes: a9e271ec9b36 ("vfio: Add migration region initialization and finalize
function")
Signed-off-by: Kirti Wankhede
---
hw/vfio/migration.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 3ce285ea395d..55261562d4f3 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -897,8 +897,8 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
goto add_blocker;
}
-g_free(info);
trace_vfio_migration_probe(vbasedev->name, info->index);
+g_free(info);
return 0;
add_blocker:
--
2.7.0
Re: [PATCH] target/microblaze: Fix possible array out of bounds in mmu_write()
On 11/3/20 8:46 AM, AlexChen wrote:
> The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5].
> To avoid data access out of bounds, only if 'rn' is less than 3, we
> can print env->mmu.regs[rn]. In other cases, we can print
> env->mmu.regs[MMU_R_TLBX].
>
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
> ---
> target/microblaze/mmu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c
> index 1dbbb271c4..917ad6d69e 100644
> --- a/target/microblaze/mmu.c
> +++ b/target/microblaze/mmu.c
> @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn,
> uint32_t v)
> unsigned int i;
>
> qemu_log_mask(CPU_LOG_MMU,
> - "%s rn=%d=%x old=%x\n", __func__, rn, v,
> env->mmu.regs[rn]);
> + "%s rn=%d=%x old=%x\n", __func__, rn, v,
> + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]);
Nack. If rn >= ARRAY_SIZE(env->mmu.regs), then don't displays it.
Else it is confuse to see a value unrelated to the MMU index used...
>
> if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) {
> qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
>
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé wrote: > Can we keep the error please? Maybe 132 is the next display logical > limit once we increased the warning from 80 to 100. > > I understand hardware evolved, we have larger displays with better > resolution and can fit more characters in a line. > I am a bit wary however functions become heavier (more code into > a single function). Maybe this checkpatch change should go with > a another one warning when a function has more than 80 lines, > excluding comments? (Even 80 is too much for my taste). Personally I just don't think checkpatch should be nudging people into folding 85-character lines, especially when there are multiple very similar lines in a row and only one would get folded, eg the prototypes in target/arm/helper.h -- some of these just edge beyond 80 characters and I think wrapping them is clearly worse for readability. If we don't want people sending us "style fix" patches which wrap >80 char lines (which I think we do not) then we shouldn't have checkpatch complain about them, because if it does then that's what we get. thanks -- PMM
Re: [PATCH] qtest: Fix bad printf format specifiers
On 11/6/20 7:33 AM, Markus Armbruster wrote:
> Thomas Huth writes:
>
>> On 05/11/2020 06.14, AlexChen wrote:
>>> On 2020/11/4 18:44, Thomas Huth wrote:
On 04/11/2020 11.23, AlexChen wrote:
> We should use printf format specifier "%u" instead of "%d" for
> argument of type "unsigned int".
>
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
> ---
> tests/qtest/arm-cpu-features.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/arm-cpu-features.c
> b/tests/qtest/arm-cpu-features.c
> index d20094d5a7..bc681a95d5 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -536,7 +536,7 @@ static void test_query_cpu_model_expansion_kvm(const
> void *data)
> if (kvm_supports_sve) {
> g_assert(vls != 0);
> max_vq = 64 - __builtin_clzll(vls);
> -sprintf(max_name, "sve%d", max_vq * 128);
> +sprintf(max_name, "sve%u", max_vq * 128);
>
> /* Enabling a supported length is of course fine. */
> assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name);
> @@ -556,7 +556,7 @@ static void test_query_cpu_model_expansion_kvm(const
> void *data)
> * unless all larger, supported vector lengths are also
> * disabled.
> */
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
> error = g_strdup_printf("cannot disable %s", name);
> assert_error(qts, "host", error,
> "{ %s: true, %s: false }",
> @@ -569,7 +569,7 @@ static void test_query_cpu_model_expansion_kvm(const
> void *data)
> * we need at least one vector length enabled.
> */
> vq = __builtin_ffsll(vls);
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
> error = g_strdup_printf("cannot disable %s", name);
> assert_error(qts, "host", error, "{ %s: false }", name);
> g_free(error);
> @@ -581,7 +581,7 @@ static void test_query_cpu_model_expansion_kvm(const
> void *data)
> }
> }
> if (vq <= SVE_MAX_VQ) {
> -sprintf(name, "sve%d", vq * 128);
> +sprintf(name, "sve%u", vq * 128);
> error = g_strdup_printf("cannot enable %s", name);
> assert_error(qts, "host", error, "{ %s: true }", name);
> g_free(error);
>
max_vq and vq are both "uint32_t" and not "unsigned int" ... so if you want
to fix this really really correctly, please use PRIu32 from inttypes.h
instead.
>>>
>>> Hi Thomas,
>>> Thanks for your review.
>>> According to the definition of the macro PRIu32(# define PRIu32
>>> "u"),
>>> using PRIu32 works the same as using %u to print, and using PRIu32 to print
>>> is relatively rare in QEMU(%u 720, PRIu32 only 120). Can we continue to use
>>> %u to
>>> print max_vq and vq in this patch.
>>> Of course, this is just my small small suggestion. If you think it is
>>> better to use
>>> PRIu32 for printing, I will send patch V2.
>>
>> Well, %u happens to work since "int" is 32-bit with all current compilers
>> that we support.
>
> Yes, it works.
>
>> But if there is ever a compiler where the size of int is
>> different, you'll get a compiler warning here again.
>
> No, we won't.
>
> If we ever use a compiler where int is narrower than 32 bits, then the
> type of the argument is actually uint32_t[1]. We can forget about this
> case, because "int narrower than 32 bits" is not going to fly with our
> code base.
>
> If we ever use a compiler where int is wider than 32 bits, then the type
> of the argument is *not* uint32_t[2]. PRIu32 will work anyway, because
> it will actually retrieve an unsigned int argument, *not* an uint32_t
> argument[3].
>
> In other words "%" PRIu32 is just a less legible alias for "%u" in all
> cases that matter.
Can we add a checkpatch rule to avoid using 'PRI[dux]32' format,
so it is clear for everyone?
>
> And that's why I would simply use "%u".
>
>> So if we now fix this
>> up, then let's do it really right and use PRIu32, please.
>>
>> Thomas
>
>
> [1] Because promotion does nothing either argument, and the usual
> arithmetic conversions convert to uint32_t. See my first reply.
>
> [2] Because uint32_t gets promoted to unsigned int. See my first reply.
>
> [3] Because variable arguments undergo default argument promotion (§
> 6.5.2.2 Function calls), which promotes uint32_t to unsigned int.
>
>
Re: [PATCH v1 1/1] hw/intc/ibex_plic: Clear the claim register when read
On 11/6/20 3:32 AM, Alistair Francis wrote: > After claiming the interrupt by reading the claim register we want to > clear the register to make sure the interrupt doesn't appear at the next > read. > > This matches the documentation for the claim register as it clears the > pending bit (which we already do): > https://docs.opentitan.org/hw/ip/rv_plic/doc/index.html "When an interrupt is claimed by a target the relevant bit of IP is cleared." Correct. Reviewed-by: Philippe Mathieu-Daudé > > This also matches the current hardware. > > Signed-off-by: Alistair Francis > --- > hw/intc/ibex_plic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c > index f49fa67c91..235e6b88ff 100644 > --- a/hw/intc/ibex_plic.c > +++ b/hw/intc/ibex_plic.c > @@ -139,6 +139,9 @@ static uint64_t ibex_plic_read(void *opaque, hwaddr addr, > /* Return the current claimed interrupt */ > ret = s->claim; > > +/* Clear the claimed interrupt */ > +s->claim = 0x; > + > /* Update the interrupt status after the claim */ > ibex_plic_update(s); > } >
Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus
On 11/5/20 11:18 PM, Daniele Buono wrote:
> The UASStatus data structure has a variable sized field inside of type uas_iu,
> that however is not placed at the end of the data structure.
>
> This placement triggers a warning with clang 11, and while not a bug right
> now,
> (the status is never a uas_iu_command, which is the variable-sized case),
> it could become one in the future.
The problem is uas_iu_command::add_cdb, indeed.
>
> ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable
> sized type 'uas_iu' not at the end of a struct or class is a GNU extension
> [-Werror,-Wgnu-variable-sized-type-not-at-end]
If possible remove the "../qemu-base/" as it does not provide
any useful information.
> uas_iustatus;
> ^
> 1 error generated.
>
> Fix this by moving uas_iu at the end of the struct
Your patch silents the warning, but the problem is the same.
It would be safer/cleaner to make 'status' a pointer on the heap IMO.
>
> Signed-off-by: Daniele Buono
> ---
> hw/usb/dev-uas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index cec071d96c..5ef3f4fec9 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -154,9 +154,9 @@ struct UASRequest {
>
> struct UASStatus {
> uint32_t stream;
> -uas_iustatus;
> uint32_t length;
> QTAILQ_ENTRY(UASStatus) next;
> +uas_iustatus;
> };
>
> /* - */
>
Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump
On 11/5/20 11:19 PM, Daniele Buono wrote:
> scsi_disk_new_request_dump is used to dump the content of a scsi request
> for tracing. It does that by decoding the command to get the size of the
> command buffer, and then printing the content of such buffer on a string.
>
> When using gcc with link-time optimizations, it warns that the argument of
> malloc may be too large.
>
> In function 'scsi_disk_new_request_dump',
> inlined from 'scsi_new_request' at
> ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2588:9:
> ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2562:17: warning: argument 1 value
> '18446744073709551612' exceeds maximum object size 9223372036854775807
> [-Walloc-size-larger-than=]
> line_buffer = g_malloc(len * 5 + 1);
> ^
> ../qemu-cfi-v3/hw/scsi/scsi-disk.c: In function 'scsi_new_request':
> /usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation
> function 'g_malloc' declared here
> gpointer g_malloc (gsize n_bytes) G_GNUC_MALLOC
> G_GNUC_ALLOC_SIZE(1);
>
> len is a signed integer filled up by scsi_cdb_length which can return -1
> if it can't decode the command. In this case, g_malloc would probably fail.
> However, an unknown command here is a possibility, and since this is used for
> tracing, we should try to print the command anyway, for debugging purposes.
>
> Since knowing the size of the command in the buffer is impossible (could not
> decode the command), only print the header by setting len=1 if scsi_cdb_length
> returned -1
>
> Signed-off-by: Daniele Buono
> ---
> If we had a way to know the (maximum) size of the buffer, we could
> alternatively dump the whole buffer, instead of dumping only the
> first byte. Not sure if this can be done, nor if it is considered
> a better option.
>
> We could also produce an error instead/in addition to just dumping
> the buffer, if the command cannot be decoded.
>
> hw/scsi/scsi-disk.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e859534eaf..d70dfdd9dc 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -2559,6 +2559,10 @@ static void scsi_disk_new_request_dump(uint32_t lun,
> uint32_t tag, uint8_t *buf)
> int len = scsi_cdb_length(buf);
> char *line_buffer, *p;
>
> +if (len < 0) {
> +len = 1;
> +}
> +
> line_buffer = g_malloc(len * 5 + 1);
>
> for (i = 0, p = line_buffer; i < len; i++) {
>
I think scsi_cdb_length() should always return >=1,
and scsi_req_parse_cdb() return if len <= 1.
Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump
On 11/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
> On 11/5/20 11:19 PM, Daniele Buono wrote:
>> scsi_disk_new_request_dump is used to dump the content of a scsi request
>> for tracing. It does that by decoding the command to get the size of the
>> command buffer, and then printing the content of such buffer on a string.
>>
>> When using gcc with link-time optimizations, it warns that the argument of
>> malloc may be too large.
>>
>> In function 'scsi_disk_new_request_dump',
>> inlined from 'scsi_new_request' at
>> ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2588:9:
>> ../qemu-cfi-v3/hw/scsi/scsi-disk.c:2562:17: warning: argument 1 value
>> '18446744073709551612' exceeds maximum object size 9223372036854775807
>> [-Walloc-size-larger-than=]
>> line_buffer = g_malloc(len * 5 + 1);
>> ^
>> ../qemu-cfi-v3/hw/scsi/scsi-disk.c: In function 'scsi_new_request':
>> /usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation
>> function 'g_malloc' declared here
>> gpointer g_malloc (gsize n_bytes) G_GNUC_MALLOC
>> G_GNUC_ALLOC_SIZE(1);
>>
>> len is a signed integer filled up by scsi_cdb_length which can return -1
>> if it can't decode the command. In this case, g_malloc would probably fail.
>> However, an unknown command here is a possibility, and since this is used for
>> tracing, we should try to print the command anyway, for debugging purposes.
>>
>> Since knowing the size of the command in the buffer is impossible (could not
>> decode the command), only print the header by setting len=1 if
>> scsi_cdb_length
>> returned -1
>>
>> Signed-off-by: Daniele Buono
>> ---
>> If we had a way to know the (maximum) size of the buffer, we could
>> alternatively dump the whole buffer, instead of dumping only the
>> first byte. Not sure if this can be done, nor if it is considered
>> a better option.
>>
>> We could also produce an error instead/in addition to just dumping
>> the buffer, if the command cannot be decoded.
>>
>> hw/scsi/scsi-disk.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index e859534eaf..d70dfdd9dc 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -2559,6 +2559,10 @@ static void scsi_disk_new_request_dump(uint32_t lun,
>> uint32_t tag, uint8_t *buf)
>> int len = scsi_cdb_length(buf);
>> char *line_buffer, *p;
>>
>> +if (len < 0) {
>> +len = 1;
>> +}
>> +
>> line_buffer = g_malloc(len * 5 + 1);
>>
>> for (i = 0, p = line_buffer; i < len; i++) {
>>
>
> I think scsi_cdb_length() should always return >=1,
> and scsi_req_parse_cdb() return if len <= 1.
Looking at how this works, scsi_req_new() shouldn't take
only a pointer to buffer without knowing its size...
We should add a buflen argument and propagate it.
Then we can check if scsi_cdb_length() <= buflen,
and dump buflen if unknown opcode.
Regards,
Phil.
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On 11/6/20 3:16 PM, Peter Maydell wrote: > On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé wrote: >> Can we keep the error please? Maybe 132 is the next display logical >> limit once we increased the warning from 80 to 100. >> >> I understand hardware evolved, we have larger displays with better >> resolution and can fit more characters in a line. >> I am a bit wary however functions become heavier (more code into >> a single function). Maybe this checkpatch change should go with >> a another one warning when a function has more than 80 lines, >> excluding comments? (Even 80 is too much for my taste). > > Personally I just don't think checkpatch should be nudging people > into folding 85-character lines, especially when there are > multiple very similar lines in a row and only one would get > folded, eg the prototypes in target/arm/helper.h -- some of > these just edge beyond 80 characters and I think wrapping them > is clearly worse for readability. If we don't want people > sending us "style fix" patches which wrap >80 char lines > (which I think we do not) then we shouldn't have checkpatch > complain about them, because if it does then that's what we get. I think I was not clear. I am not arguing against changing the *length* limit of a line (although I'd still keep one, as I don't think we want lines with 500 characters). I'm suggesting an orthogonal change, restricting the number of lines in a function :) > > thanks > -- PMM >
Re: [PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD
On 201105 1718, Daniele Buono wrote:
> LLVM's linker, LLD, supports the keyword "INSERT AFTER", starting with
> version 11.
> However, when multiple sections are defined in the same "INSERT AFTER",
> they are added in a reversed order, compared to BFD's LD.
>
> This patch makes fork_fuzz.ld generic enough to work with both linkers.
> Each section now has its own "INSERT AFTER" keyword, so proper ordering is
> defined between the sections added.
>
Hi Daniele,
Good to know that LLVM now has support for "INSERT AFTER" :)
I compared the resulting symbols between __FUZZ_COUNTERS_{START,END}
(after linking with BFD) before/after this patch, and they look good. I
also ran a test-build with OSS-Fuzz container and confirmed that the
resulting binary also had proper symbols.
Reviewed-by: Alexander Bulekov
Tested-by: Alexander Bulekov
Thanks
> Signed-off-by: Daniele Buono
> ---
> tests/qtest/fuzz/fork_fuzz.ld | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
> index bfb667ed06..cfb88b7fdb 100644
> --- a/tests/qtest/fuzz/fork_fuzz.ld
> +++ b/tests/qtest/fuzz/fork_fuzz.ld
> @@ -16,6 +16,11 @@ SECTIONS
>/* Lowest stack counter */
>*(__sancov_lowest_stack);
>}
> +}
> +INSERT AFTER .data;
> +
> +SECTIONS
> +{
>.data.fuzz_ordered :
>{
>/*
> @@ -34,6 +39,11 @@ SECTIONS
> */
> *(.bss._ZN6fuzzer3TPCE);
>}
> +}
> +INSERT AFTER .data.fuzz_start;
> +
> +SECTIONS
> +{
>.data.fuzz_end : ALIGN(4K)
>{
>__FUZZ_COUNTERS_END = .;
> @@ -43,4 +53,4 @@ SECTIONS
> * Don't overwrite the SECTIONS in the default linker script. Instead insert
> the
> * above into the default script
> */
> -INSERT AFTER .data;
> +INSERT AFTER .data.fuzz_ordered;
> --
> 2.17.1
>
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity
On 201106 0835, Daniele Buono wrote: > Hi Cornelia, > > I don't have a real preference either way. > > So if it is acceptable to have the clang11+ patches separated and > handled by the maintainers for the proper subsystem, I'd say whatever > the maintainers prefer. > > In my opinion, the patches for clang11+ support may be merged > separately. > > I'm saying this because, from my tests, the only feature that needs > clang11+ to compile with Control-Flow Integrity is fuzzing. > However, the main way we're fuzzing QEMU is through OSSfuzz, and I don't > think their infrastructure is using a compiler that new, so we wouldn't > be able to enable it anyway. (Alex can chip in to confirm this) I think oss-fuzz is using a bleeding edge version of Clang, so that might not be a problem. Here is the oss-fuzz build-log from earlier today: https://oss-fuzz-build-logs.storage.googleapis.com/log-1747e14f-6b87-43e0-96aa-07ea159e7eb2.txt ... Step #4: C compiler for the host machine: clang (clang 12.0.0 "clang version 12.0.0 (https://github.com/llvm/llvm-project.git c9f69ee7f94cfefc373c3c6cae08e51b11e6d3c2)") Step #4: C linker for the host machine: clang ld.bfd 2.26.1 Step #4: Host machine cpu family: x86_64 ... I'm not sure what the status of LTO/LLD support is on oss-fuzz/libfuzzer. There are some sparse mentions of lld/lto in the repo: https://github.com/google/oss-fuzz/issues/933 https://github.com/google/oss-fuzz/pull/3597 I haven't found any projects actively using lld on oss-fuzz, but I might not be grepping hard enough. I personally haven't tried building the fuzzers with LTO yet, but it seems like a good idea. I'll try it out. -Alex > On the other hand, if someone is looking for temporary support in-house, > they can just add -Wno-[...] as extra-cflags until the additional > patches land. (Assuming CFI lands before the clang11+ patches). > > Regards, > Daniele > > On 11/6/2020 7:47 AM, Cornelia Huck wrote: > > On Thu, 5 Nov 2020 17:18:56 -0500 > > Daniele Buono wrote: > > > > > This patch adds supports for Control-Flow Integrity checks > > > on indirect function calls. > > > > > > Requires the use of clang, and link-time optimizations > > > > > > Changes in v3: > > > > > > - clang 11+ warnings are now handled directly at the source, > > > instead of disabling specific warnings for the whole code. > > > Some more work may be needed here to polish the patch, I > > > would kindly ask for a review from the corresponding > > > maintainers > > > > Process question :) > > > > Would you prefer to have this series merged in one go, or should > > maintainers pick the patches for their subsystem? > > > > > - Remove configure-time checks for toolchain compatibility > > > with LTO. > > > - the decorator to disable cfi checks on functions has > > > been renamed and moved to include/qemu/compiler.h > > > - configure-time checks for cfi support and dependencies > > > has been moved from configure to meson > > > > > > Link to v2: > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html > > > Link to v1: > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html > > > > > > Daniele Buono (9): > > >fuzz: Make fork_fuzz.ld compatible with LLVM's LLD > > >s390x: fix clang 11 warnings in cpu_models.c > > >hw/usb: reorder fields in UASStatus > > >s390x: Avoid variable size warning in ipl.h > > >scsi: fix overflow in scsi_disk_new_request_dump > > >configure,meson: add option to enable LTO > > >cfi: Initial support for cfi-icall in QEMU > > >check-block: enable iotests with cfi-icall > > >configure/meson: support Control-Flow Integrity > > > > > > accel/tcg/cpu-exec.c | 11 + > > > configure | 26 > > > hw/s390x/ipl.h| 4 +-- > > > hw/scsi/scsi-disk.c | 4 +++ > > > hw/usb/dev-uas.c | 2 +- > > > include/qemu/compiler.h | 12 + > > > meson.build | 46 +++ > > > meson_options.txt | 4 +++ > > > plugins/core.c| 37 > > > plugins/loader.c | 7 ++ > > > target/s390x/cpu_models.c | 8 +++--- > > > tcg/tci.c | 7 ++ > > > tests/check-block.sh | 18 -- > > > tests/qtest/fuzz/fork_fuzz.ld | 12 - > > > util/main-loop.c | 11 + > > > util/oslib-posix.c| 11 + > > > 16 files changed, 205 insertions(+), 15 deletions(-) > > > > > > >
Re: [PULL for-5.2 0/4] s390x fixes
On Fri, 6 Nov 2020 at 13:13, Cornelia Huck wrote: > > The following changes since commit 3d6e32347a3b57dac7f469a07c5f520e69bd070a: > > Update version for v5.2.0-rc0 release (2020-11-03 21:11:57 +) > > are available in the Git repository at: > > https://github.com/cohuck/qemu tags/s390x-20201106 > > for you to fetch changes up to 77280d33bc9cfdbfb5b5d462259d644f5aefe9b3: > > s390x: fix build for --without-default-devices (2020-11-05 13:04:07 +0100) > > > some s390x fixes, including a bios update > > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2 for any user-visible changes. -- PMM
Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value
Paolo Bonzini writes:
> Right now, help options are parsed normally and then checked
> specially in opt_validate. but only if coming from
> qemu_opts_parse or qemu_opts_parse_noisily.
> Move the check from opt_validate to the common workhorses
> of qemu_opts_parse and qemu_opts_parse_noisily, opts_do_parse
> and get_opt_name_value.
>
> As a result, opts_parse and opts_do_parse do not return an error anymore
> when help is requested---just like qemu_opts_parse_noisily.
>
> This will come in handy in the next patch, which will
> raise a warning for "-object memory-backend-ram,share"
> ("flag" option with no =on/=off part) but not for
> "-object memory-backend-ram,help".
>
> Signed-off-by: Paolo Bonzini
> ---
> util/qemu-option.c | 40
> 1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index acefbc23fa..61fc96f9dd 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -504,17 +504,13 @@ static QemuOpt *opt_create(QemuOpts *opts, const char
> *name, char *value,
> return opt;
> }
>
> -static bool opt_validate(QemuOpt *opt, bool *help_wanted,
> - Error **errp)
> +static bool opt_validate(QemuOpt *opt, Error **errp)
> {
> const QemuOptDesc *desc;
>
> desc = find_desc_by_name(opt->opts->list->desc, opt->name);
> if (!desc && !opts_accepts_any(opt->opts)) {
> error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> -if (help_wanted && is_help_option(opt->name)) {
> -*help_wanted = true;
> -}
> return false;
> }
Two callers, one passes null (trivial: no change), one non-null (more
interesting).
Behavior before the patch is rather peculiar:
* The caller needs to opt into the help syntax by passing non-null
@help_wanted.
* A request for help is recognized only when the option name is not
recognized. Two cases:
- When @opts accepts anything, we ignore cries for help.
- Else, we recognize it only when there is no option named "help".
* A help request is an ordinary option parameter (possibly sugared)
where the parameter name is a "help option" (i.e. "help" or "?"), and
the value doesn't matter.
Examples:
- "help=..."
- "help" (sugar for "help=on")
- "nohelp" (sugar for "help=off")
- "?=..."
- "?" (sugar for "?=on")
- "no?" (sugar for "?=off")
* A request for help is treated as an error: we set @errp and return
false.
>
> @@ -531,7 +527,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const
> char *value,
> {
> QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
>
> -if (!opt_validate(opt, NULL, errp)) {
> +if (!opt_validate(opt, errp)) {
> qemu_opt_del(opt);
> return false;
This is the trivial caller.
> }
> @@ -767,16 +763,18 @@ void qemu_opts_print(QemuOpts *opts, const char
> *separator)
>
> static const char *get_opt_name_value(const char *params,
>const char *firstname,
> + bool *help_wanted,
>char **name, char **value)
> {
> -const char *p, *pe, *pc;
> -
> -pe = strchr(params, '=');
> -pc = strchr(params, ',');
> +const char *p;
> +size_t len;
>
> -if (!pe || (pc && pc < pe)) {
> +len = strcspn(params, "=,");
> +if (params[len] != '=') {
> /* found "foo,more" */
> -if (firstname) {
> +if (help_wanted && starts_with_help_option(params) == len) {
> +*help_wanted = true;
> +} else if (firstname) {
> /* implicitly named first option */
> *name = g_strdup(firstname);
> p = get_opt_value(params, value);
This function parses one parameter from @params into @name, @value, and
returns a pointer to the next parameter, or else to the terminating
'\0'.
Funny: it cannot fail. QemuOpts is an indiscriminate omnivore ;)
The patch does two separate things:
1. It streamlines how we look ahead to '=', ',' or '\0'.
Three cases: '=' comes first, '-' comes first, '\0' comes first.
Before the patch: !pe || (pc && pc < pe) means there is no '=', or
else there is ',' and it's before '='. In other words, '=' does not
come first.
After the patch: params[len] != '=' where len = strcspn(params, "=,")
means '=' does not come first.
Okay, but make it a separate patch, please.
The ,more in both comments is slightly misleading. Observation, not
demand.
2. Optional parsing of help (opt in by passing non-null @help_wanted).
I wonder why this is opt-in. I trust that'll become clear further
down.
If @params starts with "help option", and it's followed by ',' or
'\0', set *help_wanted instead of *name and *value. Okay.
The function needed a written contract before, and now it needs it
more. Observation, not demand.
> @@ -814,7 +812,10 @@ st
Re: [PATCH v2 3/7] block: make bdrv_drop_intermediate() less wrong
On Fri 06 Nov 2020 01:42:37 PM CET, Vladimir Sementsov-Ogievskiy wrote: > First, permission update loop tries to do iterations transactionally, > but the whole update is not transactional: nobody roll-back successful > loop iterations when some iteration fails. > > Second, in the iteration we have nested permission update: > c->klass->update_filename may point to bdrv_child_cb_update_filename() > which calls bdrv_backing_update_filename(), which may do node reopen to > RW. > > Permission update system is not prepared to nested updates, at least it > has intermediate permission-update state stored in BdrvChild > structures: has_backup_perm, backup_perm and backup_shared_perm. > > So, let's first do bdrv_replace_node_common() (which is more > transactional than open-coded update in bdrv_drop_intermediate()) and > then call update_filename() in separate. We still do not rollback > changes in case of update_filename() failure but it's not much worse > than pre-patch behavior. > > Note that bdrv_replace_node_common() does check for frozen children, > so corresponding check is dropped in bdrv_drop_intermediate(). > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 4/7] block: add bdrv_refresh_perms() helper
On Fri 06 Nov 2020 01:42:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Make separate function for common pattern.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block.c | 60 -
> 1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/block.c b/block.c
> index 77a3f8f1e2..fc7633307f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
> bdrv_abort_perm_update(c->bs);
> }
>
> +static int bdrv_refresh_perms(BlockDriverState *bs, bool
> *tighten_restrictions,
> + Error **errp)
> +{
> +int ret;
> +uint64_t perm, shared_perm;
> +
> +bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
> +ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
> errp);
Aren't you supposed to pass tighten_restrictions here ?
Berto
[PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling
The handling of the FPU state in sparc64_get_context() and
sparc64_set_context() is not the same as what the kernel actually
does: we unconditionally read and write the FP registers and the
FSR, GSR and FPRS, but the kernel logic is more complicated:
* in get_context the kernel has code for saving FPU registers,
but it is hidden inside an "if (fenab) condition and the
fenab flag is always set to 0 (inside an "#if 1" which has
been in the kernel for over 15 years). So the effect is that
the FPU state part is always written as zeroes.
* in set_context the kernel looks at the fenab field in the
structure from the guest, and only restores the state if
it is set; it also looks at the structure's FPRS to see
whether either the upper or lower or both halves of the
register file have valid data.
Bring our implementations into line with the kernel:
* in get_context:
- clear the entire target_ucontext at the top of the
function (as the kernel does)
- then don't write the FPU state, so those fields remain zero
- this fixes Coverity issue CID 1432305 by deleting the code
it was complaining about
* in set_context:
- check the fenab and the fpsr to decide which parts of
the FPU data to restore, if any
- instead of setting the FPU registers by doing two
32-bit loads and filling in the .upper and .lower parts
of the CPU_Double union separately, just do a 64-bit
load of the whole register at once. This fixes Coverity
issue CID 1432303 because we now access the dregs[] part
of the mcfpu_fregs union rather than the sregs[] part
(which is not large enough to actually cover the whole of
the data, so we were accessing off the end of sregs[])
We change both functions in a single commit to avoid potentially
breaking bisection.
Signed-off-by: Peter Maydell
---
target/sparc/cpu.h| 4 ++-
linux-user/sparc/signal.c | 74 +++
2 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index b9369398f24..277254732b9 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -156,7 +156,9 @@ enum {
#define PS_IE(1<<1)
#define PS_AG(1<<0) /* v9, zero on UA2007 */
-#define FPRS_FEF (1<<2)
+#define FPRS_DL (1 << 0)
+#define FPRS_DU (1 << 1)
+#define FPRS_FEF (1 << 2)
#define HS_PRIV (1<<2)
#endif
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index d12adc8e6ff..e661a769cb1 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -402,8 +402,10 @@ void sparc64_set_context(CPUSPARCState *env)
abi_ulong ucp_addr;
struct target_ucontext *ucp;
target_mc_gregset_t *grp;
+target_mc_fpu_t *fpup;
abi_ulong pc, npc, tstate;
unsigned int i;
+unsigned char fenab;
ucp_addr = env->regwptr[WREG_O0];
if (!lock_user_struct(VERIFY_READ, ucp, ucp_addr, 1)) {
@@ -467,26 +469,42 @@ void sparc64_set_context(CPUSPARCState *env)
__get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp));
__get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7));
-/* FIXME this does not match how the kernel handles the FPU in
- * its sparc64_set_context implementation. In particular the FPU
- * is only restored if fenab is non-zero in:
- * __get_user(fenab, &(ucp->tuc_mcontext.mc_fpregs.mcfpu_enab));
- */
-__get_user(env->fprs, &(ucp->tuc_mcontext.mc_fpregs.mcfpu_fprs));
-{
-uint32_t *src = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
-for (i = 0; i < 64; i++, src++) {
-if (i & 1) {
-__get_user(env->fpr[i/2].l.lower, src);
-} else {
-__get_user(env->fpr[i/2].l.upper, src);
+fpup = &ucp->tuc_mcontext.mc_fpregs;
+
+__get_user(fenab, &(fpup->mcfpu_enab));
+if (fenab) {
+abi_ulong fprs;
+
+/*
+ * We use the FPRS from the guest only in deciding whether
+ * to restore the upper, lower, or both banks of the FPU regs.
+ * The kernel here writes the FPU register data into the
+ * process's current_thread_info state and unconditionally
+ * clears FPRS and TSTATE_PEF: this disables the FPU so that the
+ * next FPU-disabled trap will copy the data out of
+ * current_thread_info and into the real FPU registers.
+ * QEMU doesn't need to handle lazy-FPU-state-restoring like that,
+ * so we always load the data directly into the FPU registers
+ * and leave FPRS and TSTATE_PEF alone (so the FPU stays enabled).
+ * Note that because we (and the kernel) always write zeroes for
+ * the fenab and fprs in sparc64_get_context() none of this code
+ * will execute unless the guest manually constructed or changed
+ * the context structure.
+ */
+__get_user(fprs, &(fpup->mcfpu_fprs));
+if (fprs & FPRS_DL) {
+for (i =
[PATCH v2 0/4] linux/sparc: more get/set_context fixes
Based-on: 20201105212314.9628-1-peter.mayd...@linaro.org
("[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs")
This series fixes a few more issues with our sparc linux-user
sparc64_get_context() and sparc64_set_context() implementation:
* we weren't handling FPU regs correctly, and also the way
we coded the handling triggered Coverity warnings
* some stray pointless error checks
* we shouldn't restore %g7 in set_context
* we weren't saving and restoring tstate correctly
My main aim here was to deal with the Coverity errors, but
the rest are things I noticed while I was working on the
code or which had fixme comments, and I figured I'd fix
them while the code was fresh in my mind.
thanks
-- PMM
Peter Maydell (4):
linux-user/sparc: Correct sparc64_get/set_context() FPU handling
linux-user/sparc: Remove unneeded checks of 'err' from
sparc64_get_context()
linux-user/sparc: Don't restore %g7 in sparc64_set_context()
linux-user/sparc: Handle tstate in sparc64_get/set_context()
target/sparc/cpu.h | 28 +---
linux-user/sparc/signal.c | 87 -
target/sparc/int64_helper.c | 5 +--
3 files changed, 71 insertions(+), 49 deletions(-)
--
2.20.1
[PATCH v2 4/4] linux-user/sparc: Handle tstate in sparc64_get/set_context()
Correctly implement save/restore of the tstate field in
sparc64_get_context() and sparc64_set_context():
* Don't use the CWP value from the guest in set_context
* Construct and save a tstate value rather than leaving
it as zero in get_context
To do this we factor out the "calculate TSTATE value from CPU state"
code from sparc_cpu_do_interrupt() into its own sparc64_tstate()
function; that in turn requires us to move some of the function
prototypes out from inside a CPU_NO_IO_DEFS ifdef guard.
Signed-off-by: Peter Maydell
---
target/sparc/cpu.h | 24
linux-user/sparc/signal.c | 7 +++
target/sparc/int64_helper.c | 5 +
3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 277254732b9..4b2290650be 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -608,10 +608,6 @@ target_ulong cpu_get_psr(CPUSPARCState *env1);
void cpu_put_psr(CPUSPARCState *env1, target_ulong val);
void cpu_put_psr_raw(CPUSPARCState *env1, target_ulong val);
#ifdef TARGET_SPARC64
-target_ulong cpu_get_ccr(CPUSPARCState *env1);
-void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
-target_ulong cpu_get_cwp64(CPUSPARCState *env1);
-void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
void cpu_change_pstate(CPUSPARCState *env1, uint32_t new_pstate);
void cpu_gl_switch_gregs(CPUSPARCState *env, uint32_t new_gl);
#endif
@@ -829,4 +825,24 @@ static inline bool tb_am_enabled(int tb_flags)
#endif
}
+#ifdef TARGET_SPARC64
+/* win_helper.c */
+target_ulong cpu_get_ccr(CPUSPARCState *env1);
+void cpu_put_ccr(CPUSPARCState *env1, target_ulong val);
+target_ulong cpu_get_cwp64(CPUSPARCState *env1);
+void cpu_put_cwp64(CPUSPARCState *env1, int cwp);
+
+static inline uint64_t sparc64_tstate(CPUSPARCState *env)
+{
+uint64_t tstate = (cpu_get_ccr(env) << 32) |
+((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
+cpu_get_cwp64(env);
+
+if (env->def.features & CPU_FEATURE_GL) {
+tstate |= (env->gl & 7ULL) << 40;
+}
+return tstate;
+}
+#endif
+
#endif
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index ed32c7abd17..a6c7c7664a2 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -438,9 +438,9 @@ void sparc64_set_context(CPUSPARCState *env)
env->npc = npc;
__get_user(env->y, &((*grp)[SPARC_MC_Y]));
__get_user(tstate, &((*grp)[SPARC_MC_TSTATE]));
+/* Honour TSTATE_ASI, TSTATE_ICC and TSTATE_XCC only */
env->asi = (tstate >> 24) & 0xff;
-cpu_put_ccr(env, tstate >> 32);
-cpu_put_cwp64(env, tstate & 0x1f);
+cpu_put_ccr(env, (tstate >> 32) & 0xff);
__get_user(env->gregs[1], (&(*grp)[SPARC_MC_G1]));
__get_user(env->gregs[2], (&(*grp)[SPARC_MC_G2]));
__get_user(env->gregs[3], (&(*grp)[SPARC_MC_G3]));
@@ -557,8 +557,7 @@ void sparc64_get_context(CPUSPARCState *env)
}
}
-/* XXX: tstate must be saved properly */
-//__put_user(env->tstate, &((*grp)[SPARC_MC_TSTATE]));
+__put_user(sparc64_tstate(env), &((*grp)[SPARC_MC_TSTATE]));
__put_user(env->pc, &((*grp)[SPARC_MC_PC]));
__put_user(env->npc, &((*grp)[SPARC_MC_NPC]));
__put_user(env->y, &((*grp)[SPARC_MC_Y]));
diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
index f3e7f32de61..735668f5006 100644
--- a/target/sparc/int64_helper.c
+++ b/target/sparc/int64_helper.c
@@ -131,9 +131,7 @@ void sparc_cpu_do_interrupt(CPUState *cs)
}
tsptr = cpu_tsptr(env);
-tsptr->tstate = (cpu_get_ccr(env) << 32) |
-((env->asi & 0xff) << 24) | ((env->pstate & 0xf3f) << 8) |
-cpu_get_cwp64(env);
+tsptr->tstate = sparc64_tstate(env);
tsptr->tpc = env->pc;
tsptr->tnpc = env->npc;
tsptr->tt = intno;
@@ -148,7 +146,6 @@ void sparc_cpu_do_interrupt(CPUState *cs)
}
if (env->def.features & CPU_FEATURE_GL) {
-tsptr->tstate |= (env->gl & 7ULL) << 40;
cpu_gl_switch_gregs(env, env->gl + 1);
env->gl++;
}
--
2.20.1
[PATCH v2 2/4] linux-user/sparc: Remove unneeded checks of 'err' from sparc64_get_context()
Unlike the kernel macros, our __get_user() and __put_user() do not
return a failure code. Kernel code typically has a style of
err |= __get_user(...); err |= __get_user(...);
and then checking err at the end. In sparc64_get_context() our
version of the code dropped the accumulating into err but left the
"if (err) goto do_sigsegv" checks, which will never be taken. Delete
unnecessary if()s.
Signed-off-by: Peter Maydell
---
linux-user/sparc/signal.c | 4
1 file changed, 4 deletions(-)
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index e661a769cb1..43dcd137f51 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -555,8 +555,6 @@ void sparc64_get_context(CPUSPARCState *env)
for (i = 0; i < TARGET_NSIG_WORDS; i++, dst++, src++) {
__put_user(*src, dst);
}
-if (err)
-goto do_sigsegv;
}
/* XXX: tstate must be saved properly */
@@ -598,8 +596,6 @@ void sparc64_get_context(CPUSPARCState *env)
* hidden behind an "if (fenab)" where fenab is always 0).
*/
-if (err)
-goto do_sigsegv;
unlock_user_struct(ucp, ucp_addr, 1);
return;
do_sigsegv:
--
2.20.1
[PATCH v2 3/4] linux-user/sparc: Don't restore %g7 in sparc64_set_context()
The kernel does not restore the g7 register in sparc64_set_context(); neither should we. (We still save it in sparc64_get_context().) Signed-off-by: Peter Maydell --- linux-user/sparc/signal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c index 43dcd137f51..ed32c7abd17 100644 --- a/linux-user/sparc/signal.c +++ b/linux-user/sparc/signal.c @@ -447,7 +447,7 @@ void sparc64_set_context(CPUSPARCState *env) __get_user(env->gregs[4], (&(*grp)[SPARC_MC_G4])); __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5])); __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6])); -__get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7])); +/* Skip g7 as that's the thread register in userspace */ /* * Note that unlike the kernel, we didn't need to mess with the -- 2.20.1
Re: [PATCH v2 0/4] linux/sparc: more get/set_context fixes
On Fri, 6 Nov 2020 at 15:27, Peter Maydell wrote:
>
> Based-on: 20201105212314.9628-1-peter.mayd...@linaro.org
> ("[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs")
>
> This series fixes a few more issues with our sparc linux-user
> sparc64_get_context() and sparc64_set_context() implementation:
> * we weren't handling FPU regs correctly, and also the way
>we coded the handling triggered Coverity warnings
> * some stray pointless error checks
> * we shouldn't restore %g7 in set_context
> * we weren't saving and restoring tstate correctly
The 'v2' in the subject tag is wrong, incidentally; this is
the first version of this series :-)
-- PMM
Re: [PATCH v2 2/7] block: add bdrv_replace_node_common()
On Fri 06 Nov 2020 01:42:36 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Add new parameter to bdrv_replace_node(): auto_skip. With > auto_skip=false we'll have stricter behavior: update _all_ from > parents or fail. New behaviour will be used in the following commit in > block.c, so keep original function name as public interface. > > Note: new error message is a bit funny in contrast with further > "Cannot" in case of frozen child, but we'd better keep some difference > to make it possible to distinguish one from another on failure. Still, > actually we'd better refactor should_update_child() call to distinguish > also different kinds of "should not". Let's do it later. > > Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Berto
Re: [PATCH 0/2] Increase amount of data for monitor to read
Please exclude this address when reply: jc...@redhat.com Andrey
Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test
Paolo Bonzini writes: > On 06/11/20 14:15, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> device-introspect-test uses HMP, so it should escape the device name >>> properly. Because of this, a few devices that had commas in their >>> names were escaping testing. >>> Signed-off-by: Paolo Bonzini >> >> $ git-grep '\.name *= *"[^"]*,' | cat >> hw/block/fdc.c:.name = "SUNW,fdtwo" >> Any others? > > Not that I know, but this is a bug anyway. :) Yes, but "a few devices" made me curious.
Re: Migrating to the gitlab issue tracker
On 11/04/20 18:19, Daniel P. Berrangé wrote: > This just sounds like fairly niche requirements for which directly > subscribing to the project issue tracker will satisfy 99% of the time. OK. Laszlo
Re: [PATCH] qtest: Fix bad printf format specifiers
Philippe Mathieu-Daudé writes: > On 11/6/20 7:33 AM, Markus Armbruster wrote: [...] >> In other words "%" PRIu32 is just a less legible alias for "%u" in all >> cases that matter. > > Can we add a checkpatch rule to avoid using 'PRI[dux]32' format, > so it is clear for everyone? I guess we could, but *I* can't: -ENOTIME, sorry.
Re: [PATCH v2 00/44] Make qdev static property API usable by any QOM type
On Fri, Nov 06, 2020 at 10:45:11AM +0100, Kevin Wolf wrote:
> Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben:
> > This series refactor the qdev property code so the static
> > property system can be used by any QOM type. As an example, at
> > the end of the series some properties in TYPE_MACHINE are
> > converted to static properties to demonstrate the new API.
> >
> > Changes v1 -> v2
> >
> >
> > * Rename functions and source files to call the new feature
> > "field property" instead of "static property"
> >
> > * Change the API signature from an array-based interface similar
> > to device_class_set_props() to a object_property_add()-like
> > interface.
> >
> > This means instead of doing this:
> >
> > object_class_property_add_static_props(oc, my_array_of_props);
> >
> > properties are registered like this:
> >
> > object_class_property_add_field(oc, "property-name"
> > PROP_XXX(MyState, my_field),
> > prop_allow_set_always);
> >
> > where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL,
> > etc.
>
> In comparison, I really like the resulting code from the array based
> interface in v1 better.
>
> I think it's mostly for two reasons: First, the array is much more
> compact and easier to read. And maybe even more importantly, you know
> it's static data and only static data. The function based interface can
> be mixed with other code or the parameter list can contain calls to
> other functions with side effects, so there are a lot more opportunities
> for surprises.
This is a really good point, and I strongly agree with it.
Letting code do funny tricks at runtime is one of the reasons QOM
properties became hard to introspect.
>
> What I didn't like about the v1 interface is that there is still a
> separate object_class_property_set_description() for each property, but
> I think that could have been fixed by moving the description to the
> definitions in the array, too.
This would be very easy to implement.
>
> On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote:
> > On 29/10/20 23:02, Eduardo Habkost wrote:
> > > +static Property machine_props[] = {
> > > +DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
> > > +DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
> > > +DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
> > > +DEFINE_PROP_STRING("dtb", MachineState, dtb),
> > > +DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
> > > +DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
> > > +DEFINE_PROP_STRING("firmware", MachineState, firmware),
> > > +DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
> > > +DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> >
> > While I think generalizing the _code_ for static properties is obviously
> > a good idea, I am not sure about generalizing the interface for adding them.
> >
> > The reason is that we already have a place for adding properties in
> > class_init, and having a second makes things "less local", so to speak.
>
> As long as you have the function call to apply the properites array in
> .class_init, it should be obvious enough what you're doing.
>
> Of course, I think we should refrain from mixing both styles in a single
> object, but generally speaking the array feels so much better that I
> don't think we should reject it just because QOM only had a different
> interface so far (and the property array is preexisting in qdev, too, so
> we already have differences between objects - in fact, the majority of
> objects is probably qdev today).
This is also a strong argument. The QEMU code base has ~500
matches for "object*_property_add*" calls, and ~2100 for
"DEFINE_PROP*".
Converting qdev arrays to object_class_property_add_*() calls
would be a huge effort with no gains. The end result would be
two different APIs for registering class field properties
coexisting for a long time, and people still confused on what's
the preferred API.
--
Eduardo
Re: [PATCH 1/1] Change the order of g_free(info) and tracepoint
On Friday, 2020-11-06 at 19:09:24 +0530, Kirti Wankhede wrote:
> Fixes Coverity issue:
> CID 1436126: Memory - illegal accesses (USE_AFTER_FREE)
>
> Fixes: a9e271ec9b36 ("vfio: Add migration region initialization and finalize
> function")
>
> Signed-off-by: Kirti Wankhede
Maybe "fix use after free in vfio_migration_probe" as a summary?
Reviewed-by: David Edmondson
> ---
> hw/vfio/migration.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 3ce285ea395d..55261562d4f3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -897,8 +897,8 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error
> **errp)
> goto add_blocker;
> }
>
> -g_free(info);
> trace_vfio_migration_probe(vbasedev->name, info->index);
> +g_free(info);
> return 0;
>
> add_blocker:
> --
> 2.7.0
dme.
--
I think I waited too long, I'm moving into the dollhouse.
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
Peter Maydell writes: > On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé wrote: >> Can we keep the error please? Maybe 132 is the next display logical >> limit once we increased the warning from 80 to 100. >> >> I understand hardware evolved, we have larger displays with better >> resolution and can fit more characters in a line. [...] > > Personally I just don't think checkpatch should be nudging people > into folding 85-character lines, especially when there are > multiple very similar lines in a row and only one would get > folded, eg the prototypes in target/arm/helper.h -- some of > these just edge beyond 80 characters and I think wrapping them > is clearly worse for readability. The warning's intent is "are you sure this line is better not broken?" The problem is people treating it as an order that absolves them from using good judgement instead. I propose to fix it by phrasing the warning more clearly. Instead of WARNING: line over 80 characters we could say WARNING: line over 80 characters Please examine the line, and use your judgement to decide whether it should be broken. > If we don't want people > sending us "style fix" patches which wrap >80 char lines > (which I think we do not) then we shouldn't have checkpatch > complain about them, because if it does then that's what we get. I think that's throwing out the baby with the bathwater. checkpatch's purpose is not guiding inexperienced developers to style issues they can fix. It is relieving maintainers of the tedium of catching and explaining certain kinds of issues patches frequently have. Neutering checks that have led inexperienced developers to post less than useful patches may well relieve maintainers of having to reject such patches. But it comes a price: maintainers and contributors lose automated help with checking useful patches. I consider that a bad trade. We may want to discourage inexperienced contributors from sending us style fix patches. Fixing style takes good taste, which develops only with experience. Moreover, fixing up style builds only little experience. At best, it exercises "configure; make check" and the patch submission process and running "make check"). There are better ways to get your feet wet.
Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code
On Fri, 6 Nov 2020 at 16:08, Markus Armbruster wrote: > Peter Maydell writes: > > Personally I just don't think checkpatch should be nudging people > > into folding 85-character lines, especially when there are > > multiple very similar lines in a row and only one would get > > folded, eg the prototypes in target/arm/helper.h -- some of > > these just edge beyond 80 characters and I think wrapping them > > is clearly worse for readability. > > The warning's intent is "are you sure this line is better not broken?" > The problem is people treating it as an order that absolves them from > using good judgement instead. > > I propose to fix it by phrasing the warning more clearly. Instead of > > WARNING: line over 80 characters > > we could say > > WARNING: line over 80 characters > Please examine the line, and use your judgement to decide whether > it should be broken. I would suggest that for a line over 80 characters and less than 85 characters, the answer is going to be "better not broken" a pretty high percentage of the time; that is, the warning has too many false positives, and we should tune it to have fewer. And the lure of "produce no warnings" is a strong one, so we should be at least cautious about what our tooling is nudging us into doing. thanks -- PMM
Re: [PATCH 1/1] Change the order of g_free(info) and tracepoint
Kirti Wankhede writes:
> Fixes Coverity issue:
> CID 1436126: Memory - illegal accesses (USE_AFTER_FREE)
>
> Fixes: a9e271ec9b36 ("vfio: Add migration region initialization and finalize
> function")
>
> Signed-off-by: Kirti Wankhede
Reviewed-by: Alex Bennée
--
Alex Bennée
Re: [PATCH] CODING_STYLE.rst: Be less strict about 80 character limit
On Fri, Nov 06, 2020 at 11:29:40AM +, Peter Maydell wrote: > Relax the wording about line lengths a little bit; this goes with the > checkpatch changes to warn at 100 characters rather than 80. > > (Compare the Linux kernel commit bdc48fa11e46f8; our coding style is > not theirs, but the rationale is good and applies to us too.) > > Signed-off-by: Peter Maydell Reviewed-by: Michael S. Tsirkin > --- > CODING_STYLE.rst | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst > index 8b13ef0669e..7bf4e39d487 100644 > --- a/CODING_STYLE.rst > +++ b/CODING_STYLE.rst > @@ -85,8 +85,13 @@ Line width > Lines should be 80 characters; try not to make them longer. > > Sometimes it is hard to do, especially when dealing with QEMU subsystems > -that use long function or symbol names. Even in that case, do not make > -lines much longer than 80 characters. > +that use long function or symbol names. If wrapping the line at 80 columns > +is obviously less readable and more awkward, prefer not to wrap it; better > +to have an 85 character line than one which is awkwardly wrapped. > + > +Even in that case, try not to make lines much longer than 80 characters. > +(The checkpatch script will warn at 100 characters, but this is intended > +as a guard against obviously-overlength lines, not a target.) > > Rationale: > > -- > 2.20.1
Re: [PATCH 2/2] qemu-option: warn for short-form boolean options
Paolo Bonzini writes:
> Options such as "server" or "nowait", that are commonly found in -chardev,
> are sugar for "server=on" and "wait=off". This is quite surprising and
> also does not have any notion of typing attached. It is even possible to
> do "-device e1000,noid" and get a device with "id=off".
>
> Deprecate all this, except for -chardev and -spice where it is in
> wide use.
I consider this a misuse of deprecation, to be frank. If something is
known to be unused, we just remove it. Deprecation is precisely for
things that are used. I'm with Daniel here: let's deprecate this sugar
everywhere.
Wide use may justify extending the deprecation grace period.
> Signed-off-by: Paolo Bonzini
> ---
> chardev/char.c | 1 +
> docs/system/deprecated.rst | 7 +++
> include/qemu/option.h | 1 +
> tests/test-qemu-opts.c | 1 +
> ui/spice-core.c| 1 +
> util/qemu-option.c | 21 ++---
> 6 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 78553125d3..108da615df 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -829,6 +829,7 @@ Chardev *qemu_chr_find(const char *name)
>
> QemuOptsList qemu_chardev_opts = {
> .name = "chardev",
> +.allow_flag_options = true, /* server, nowait, etc. */
> .implied_opt_name = "backend",
> .head = QTAILQ_HEAD_INITIALIZER(qemu_chardev_opts.head),
> .desc = {
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 32a0e620db..0e7edf7e56 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,13 @@ Drives with interface types other than ``if=none`` are
> for onboard
> devices. It is possible to use drives the board doesn't pick up with
> -device. This usage is now deprecated. Use ``if=none`` instead.
>
> +Short-form boolean options (since 5.2)
> +''
> +
> +Boolean options such as ``share=on``/``share=off`` can be written
> +in short form as ``share`` and ``noshare``. This is deprecated
> +for all command-line options except ``-chardev` and ``-spice``, for
> +which the short form was in wide use.
Unlike the commit message, the text here misleads readers into assuming
the sugar applies only to boolean options.
>
> QEMU Machine Protocol (QMP) commands
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ac69352e0e..046ac06a1f 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -65,6 +65,7 @@ struct QemuOptsList {
> const char *name;
> const char *implied_opt_name;
> bool merge_lists; /* Merge multiple uses of option into a single list?
> */
> +bool allow_flag_options; /* Whether to warn for short-form boolean
> options */
I disagree with having this option. I'm reviewing it anyway.
Staying under the 80 characters limit is really, really easy here:
bool allow_flag_options;/* Warn on short-form boolean options? */
I had to read ahead to figure out that false means warn, true means
accept silently. The comment is confusing because "whether to warn"
suggests "warn if true", which is wrong.
Clearer (I think), and even shorter:
bool allow_bool_sugar; /* Is boolean option sugar okay? */
> QTAILQ_HEAD(, QemuOpts) head;
> QemuOptDesc desc[];
> };
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 297ffe79dd..a74c475773 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -77,6 +77,7 @@ static QemuOptsList opts_list_02 = {
> static QemuOptsList opts_list_03 = {
> .name = "opts_list_03",
> .implied_opt_name = "implied",
> +.allow_flag_options = true,
> .head = QTAILQ_HEAD_INITIALIZER(opts_list_03.head),
> .desc = {
> /* no elements => accept any params */
Can you point me to the tests this hunk affects?
Do we have test coverage for boolean sugar with both values of
allow_flag_options?
If no, why is that okay?
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index eea52f5389..08f834fa41 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -397,6 +397,7 @@ static SpiceChannelList *qmp_query_spice_channels(void)
>
> static QemuOptsList qemu_spice_opts = {
> .name = "spice",
> +.allow_flag_options = true, /* disable-agent-file-xfer, sasl, unix, etc.
> */
> .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
> .merge_lists = true,
> .desc = {
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 61fc96f9dd..858860377b 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -763,10 +763,12 @@ void qemu_opts_print(QemuOpts *opts, const char
> *separator)
>
> static const char *get_opt_name_value(const char *params,
>const char *firstname,
> + bool warn_on_flag,
Two callers pass false,
Re: [PATCH] migration/dirtyrate: simplify inlcudes in dirtyrate.c
* Zheng Chuan (zhengch...@huawei.com) wrote: > Kindly ping for not forgetting this trivial fix:) Yes but it's too late for the merge window, so it'll happen on the next one, no rush! Dave > On 2020/10/30 22:09, Mark Kanda wrote: > > On 10/29/2020 10:58 PM, Chuan Zheng wrote: > >> Remove redundant blank line which is left by Commit 662770af7c6e8c, > >> also take this opportunity to remove redundant includes in dirtyrate.c. > >> > >> Signed-off-by: Chuan Zheng > > > > Reviewed-by: Mark Kanda > > > >> --- > >> Â migration/dirtyrate.c | 5 - > >> Â 1 file changed, 5 deletions(-) > >> > >> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c > >> index 8f728d2..ccb9814 100644 > >> --- a/migration/dirtyrate.c > >> +++ b/migration/dirtyrate.c > >> @@ -11,17 +11,12 @@ > >> Â Â */ > >> Â Â #include "qemu/osdep.h" > >> - > >> Â #include > >> Â #include "qapi/error.h" > >> Â #include "cpu.h" > >> -#include "qemu/config-file.h" > >> -#include "exec/memory.h" > >> Â #include "exec/ramblock.h" > >> -#include "exec/target_page.h" > >> Â #include "qemu/rcu_queue.h" > >> Â #include "qapi/qapi-commands-migration.h" > >> -#include "migration.h" > >> Â #include "ram.h" > >> Â #include "trace.h" > >> Â #include "dirtyrate.h" > >> > > -- > Regards. > Chuan > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value
One more thought...
Markus Armbruster writes:
> Paolo Bonzini writes:
[...]
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
[...]
>> @@ -767,16 +763,18 @@ void qemu_opts_print(QemuOpts *opts, const char
>> *separator)
>>
>> static const char *get_opt_name_value(const char *params,
>>const char *firstname,
>> + bool *help_wanted,
>>char **name, char **value)
>> {
>> -const char *p, *pe, *pc;
>> -
>> -pe = strchr(params, '=');
>> -pc = strchr(params, ',');
>> +const char *p;
>> +size_t len;
>>
>> -if (!pe || (pc && pc < pe)) {
>> +len = strcspn(params, "=,");
>> +if (params[len] != '=') {
>> /* found "foo,more" */
>> -if (firstname) {
>> +if (help_wanted && starts_with_help_option(params) == len) {
>> +*help_wanted = true;
>> +} else if (firstname) {
>> /* implicitly named first option */
>> *name = g_strdup(firstname);
>> p = get_opt_value(params, value);
>
> This function parses one parameter from @params into @name, @value, and
> returns a pointer to the next parameter, or else to the terminating
> '\0'.
Like opt_validate() before, this sets *help_wanted only to true.
Callers must pass a pointer to false. Perhaps having it set
*help_wanted always could simplify things overall. Up to you.
[...]
Re: [PATCH 1/1] Change the order of g_free(info) and tracepoint
On 11/6/20 4:59 PM, David Edmondson wrote:
> On Friday, 2020-11-06 at 19:09:24 +0530, Kirti Wankhede wrote:
>
>> Fixes Coverity issue:
>> CID 1436126: Memory - illegal accesses (USE_AFTER_FREE)
>>
>> Fixes: a9e271ec9b36 ("vfio: Add migration region initialization and finalize
>> function")
>>
>> Signed-off-by: Kirti Wankhede
>
> Maybe "fix use after free in vfio_migration_probe" as a summary?
Yes please :)
Reviewed-by: Philippe Mathieu-Daudé
>
> Reviewed-by: David Edmondson
>
>> ---
>> hw/vfio/migration.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 3ce285ea395d..55261562d4f3 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -897,8 +897,8 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error
>> **errp)
>> goto add_blocker;
>> }
>>
>> -g_free(info);
>> trace_vfio_migration_probe(vbasedev->name, info->index);
>> +g_free(info);
>> return 0;
>>
>> add_blocker:
>> --
>> 2.7.0
>
> dme.
>
[PATCH-for-6.0 0/2] hw/scsi/scsi-disk: QOM style change
Some QOM style changes in TYPE_SCSI_DISK to follow the rest of the codebase style. No logical change. Philippe Mathieu-Daudé (2): hw/scsi/scsi-disk: Rename type as TYPE_SCSI_DISK hw/scsi/scsi-disk: Use SCSI_DISK_GET_CLASS() macro hw/scsi/scsi-disk.c | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.26.2
[PATCH-for-6.0 2/2] hw/scsi/scsi-disk: Use SCSI_DISK_GET_CLASS() macro
Use the SCSI_DISK_GET_CLASS() macro to match the rest of
the codebase.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/scsi-disk.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index d2b9cb28da1..deb51ec8e7d 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -338,7 +338,7 @@ static void scsi_read_complete(void *opaque, int ret)
static void scsi_do_read(SCSIDiskReq *r, int ret)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
+SCSIDiskClass *sdc = SCSI_DISK_GET_CLASS(s);
assert (r->req.aiocb == NULL);
if (scsi_disk_req_check_error(r, ret, false)) {
@@ -438,7 +438,7 @@ static bool scsi_handle_rw_error(SCSIDiskReq *r, int error,
bool acct_failed)
{
bool is_read = (r->req.cmd.mode == SCSI_XFER_FROM_DEV);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
+SCSIDiskClass *sdc = SCSI_DISK_GET_CLASS(s);
BlockErrorAction action = blk_get_error_action(s->qdev.conf.blk,
is_read, error);
@@ -538,7 +538,7 @@ static void scsi_write_data(SCSIRequest *req)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
+SCSIDiskClass *sdc = SCSI_DISK_GET_CLASS(s);
/* No data transfer may already be in progress */
assert(r->req.aiocb == NULL);
@@ -2149,7 +2149,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req,
uint8_t *buf)
{
SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
-SCSIDiskClass *sdc = (SCSIDiskClass *) object_get_class(OBJECT(s));
+SCSIDiskClass *sdc = SCSI_DISK_GET_CLASS(s);
uint32_t len;
uint8_t command;
--
2.26.2
[PATCH-for-6.0 1/2] hw/scsi/scsi-disk: Rename type as TYPE_SCSI_DISK
Rename TYPE_SCSI_DISK without the '_BASE' suffix to match
the other abstract types in the codebase.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/scsi-disk.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e859534eaf3..d2b9cb28da1 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -53,9 +53,9 @@
#define DEFAULT_MAX_UNMAP_SIZE (1 * GiB)
#define DEFAULT_MAX_IO_SIZE INT_MAX /* 2 GB - 1 block */
-#define TYPE_SCSI_DISK_BASE "scsi-disk-base"
+#define TYPE_SCSI_DISK "scsi-disk-base"
-OBJECT_DECLARE_TYPE(SCSIDiskState, SCSIDiskClass, SCSI_DISK_BASE)
+OBJECT_DECLARE_TYPE(SCSIDiskState, SCSIDiskClass, SCSI_DISK)
struct SCSIDiskClass {
SCSIDeviceClass parent_class;
@@ -2956,7 +2956,7 @@ BlockAIOCB *scsi_dma_writev(int64_t offset, QEMUIOVector
*iov,
static void scsi_disk_base_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
-SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass);
+SCSIDiskClass *sdc = SCSI_DISK_CLASS(klass);
dc->fw_name = "disk";
dc->reset = scsi_disk_reset;
@@ -2966,7 +2966,7 @@ static void scsi_disk_base_class_initfn(ObjectClass
*klass, void *data)
}
static const TypeInfo scsi_disk_base_info = {
-.name = TYPE_SCSI_DISK_BASE,
+.name = TYPE_SCSI_DISK,
.parent= TYPE_SCSI_DEVICE,
.class_init= scsi_disk_base_class_initfn,
.instance_size = sizeof(SCSIDiskState),
@@ -3036,7 +3036,7 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void
*data)
static const TypeInfo scsi_hd_info = {
.name = "scsi-hd",
-.parent= TYPE_SCSI_DISK_BASE,
+.parent= TYPE_SCSI_DISK,
.class_init= scsi_hd_class_initfn,
};
@@ -3067,7 +3067,7 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void
*data)
static const TypeInfo scsi_cd_info = {
.name = "scsi-cd",
-.parent= TYPE_SCSI_DISK_BASE,
+.parent= TYPE_SCSI_DISK,
.class_init= scsi_cd_class_initfn,
};
@@ -3090,7 +3090,7 @@ static void scsi_block_class_initfn(ObjectClass *klass,
void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
-SCSIDiskClass *sdc = SCSI_DISK_BASE_CLASS(klass);
+SCSIDiskClass *sdc = SCSI_DISK_CLASS(klass);
sc->realize = scsi_block_realize;
sc->alloc_req= scsi_block_new_request;
@@ -3106,7 +3106,7 @@ static void scsi_block_class_initfn(ObjectClass *klass,
void *data)
static const TypeInfo scsi_block_info = {
.name = "scsi-block",
-.parent= TYPE_SCSI_DISK_BASE,
+.parent= TYPE_SCSI_DISK,
.class_init= scsi_block_class_initfn,
};
#endif
@@ -3146,7 +3146,7 @@ static void scsi_disk_class_initfn(ObjectClass *klass,
void *data)
static const TypeInfo scsi_disk_info = {
.name = "scsi-disk",
-.parent= TYPE_SCSI_DISK_BASE,
+.parent= TYPE_SCSI_DISK,
.class_init= scsi_disk_class_initfn,
};
--
2.26.2
Re: [PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling
On 11/6/20 7:27 AM, Peter Maydell wrote:
> +if (fprs & FPRS_DU) {
> +for (i = 16; i < 31; i++) {
32.
Otherwise,
Reviewed-by: Richard Henderson
r~
Re: Question on UEFI ACPI tables setup and probing on arm64
On 11/05/20 05:30, Ying Fang wrote:
> I see it in Qemu the *loader_start* is fixed at 1 GiB on the
> physical address space which points to the DRAM base. In ArmVirtQemu.dsc
> PcdDeviceTreeInitialBaseAddress is set 0x4000 with correspondence.
>
> Here I also see the discussion about DRAM base for ArmVirtQemu.
> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03127.html
>
> I am still not sure how UEFI knows that it is running on a ArmVirtQemu
> machine type.
It doesn't know. It remains a convention.
This part is not auto-detected; the constants in QEMU and edk2 are
independently open-coded, their values were synchronized by human effort
initially.
The user or the management layer have to make sure they boot a UEFI
firmware binary on the machine type that is compatible with the machine
type.
There is some meta-data to help with that:
> Does UEFI derive it from the fdt *compatible* property ?
Please see the schema "docs/interop/firmware.json" in the QEMU tree; in
particular the @FirmwareTarget element.
For an actual example: QEMU bundles some edk2 firmware binaries (purely
as a convenience, not for production), and those are accompanied by
matching descriptor files. See
"pc-bios/descriptors/60-edk2-aarch64.json". (It is a template that's
fixed up during QEMU installation, but that's tangential here.)
"targets": [
{
"architecture": "aarch64",
"machines": [
"virt-*"
]
}
],
Thanks
Laszlo
Re: [PATCH v2 3/4] linux-user/sparc: Don't restore %g7 in sparc64_set_context()
On 11/6/20 7:27 AM, Peter Maydell wrote: > The kernel does not restore the g7 register in sparc64_set_context(); > neither should we. (We still save it in sparc64_get_context().) > > Signed-off-by: Peter Maydell > --- > linux-user/sparc/signal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 2/4] linux-user/sparc: Remove unneeded checks of 'err' from sparc64_get_context()
On 11/6/20 7:27 AM, Peter Maydell wrote: > Unlike the kernel macros, our __get_user() and __put_user() do not > return a failure code. Kernel code typically has a style of > err |= __get_user(...); err |= __get_user(...); > and then checking err at the end. In sparc64_get_context() our > version of the code dropped the accumulating into err but left the > "if (err) goto do_sigsegv" checks, which will never be taken. Delete > unnecessary if()s. > > Signed-off-by: Peter Maydell > --- > linux-user/sparc/signal.c | 4 > 1 file changed, 4 deletions(-) Reviewed-by: Richard Henderson r~
[PATCH for-5.2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer
The ctucan device has 4 CAN bus cores, each of which has a set of 20
32-bit registers for writing the transmitted data. The registers are
however not contiguous; each core's buffers is 0x100 bytes after
the last.
We got the checks on the address wrong in the ctucan_mem_write()
function:
* the first "is addr in range at all" check allowed
addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
byte off the end of the range
* the decode of addresses into core-number plus offset in the
tx buffer for that core failed to check that the offset was
in range, so the guest could write off the end of the
tx_buffer[] array
* the decode had an explicit check for whether the core-number
was out of range, which is actually impossible given the
CTUCAN_CORE_MEM_SIZE check and the number of cores.
Fix the top level check, check the offset, and turn the check
on the core-number into an assertion.
Fixes: Coverity CID 1432874
Signed-off-by: Peter Maydell
---
hw/net/can/ctucan_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index d20835cd7e9..ea09bf71a0c 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
uint64_t val,
DPRINTF("write 0x%02llx addr 0x%02x\n",
(unsigned long long)val, (unsigned int)addr);
-if (addr > CTUCAN_CORE_MEM_SIZE) {
+if (addr >= CTUCAN_CORE_MEM_SIZE) {
return;
}
@@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
uint64_t val,
addr -= CTU_CAN_FD_TXTB1_DATA_1;
buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
addr %= CTUCAN_CORE_TXBUFF_SPAN;
-if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
+assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
+if (addr < sizeof(s->tx_buffer[buff_num].data)) {
uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr);
*bufp = cpu_to_le32(val);
}
--
2.20.1
[PATCH for-5.2 4/4] hw/net/ctucan_core: Use stl_le_p to write to tx_buffers
Instead of casting an address within a uint8_t array to a
uint32_t*, use stl_le_p(). This handles possibly misaligned
addresses which would otherwise crash on some hosts.
Signed-off-by: Peter Maydell
---
hw/net/can/ctucan_core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index f2ce978e5ec..e66526efa83 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -305,8 +305,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
uint64_t val,
addr %= CTUCAN_CORE_TXBUFF_SPAN;
assert(buff_num < CTUCAN_CORE_TXBUF_NUM);
if (addr < sizeof(s->tx_buffer[buff_num].data)) {
-uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr);
-*bufp = cpu_to_le32(val);
+stl_le_p(s->tx_buffer[buff_num].data + addr, val);
}
} else {
switch (addr & ~3) {
--
2.20.1
[PATCH for-5.2 2/4] hw/net/can/ctucan: Avoid unused value in ctucan_send_ready_buffers()
Coverity points out that in ctucan_send_ready_buffers() we
set buff_st_mask = 0xf << (i * 4) inside the loop, but then
we never use it before overwriting it later.
The only thing we use the mask for is as part of the code that is
inserting the new buff_st field into tx_status. That is more
comprehensibly written using deposit32(), so do that and drop the
mask variable entirely.
We also update the buff_st local variable at multiple points
during this function, but nothing can ever see these
intermediate values, so just drop those, write the final
TXT_TOK as a fixed constant value, and collapse the only
remaining set/use of buff_st down into an extract32().
Fixes: Coverity CID 1432869
Signed-off-by: Peter Maydell
---
hw/net/can/ctucan_core.c | 15 +++
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
index ea09bf71a0c..f2ce978e5ec 100644
--- a/hw/net/can/ctucan_core.c
+++ b/hw/net/can/ctucan_core.c
@@ -240,8 +240,6 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
uint8_t *pf;
int buff2tx_idx;
uint32_t tx_prio_max;
-unsigned int buff_st;
-uint32_t buff_st_mask;
if (!s->mode_settings.s.ena) {
return;
@@ -256,10 +254,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
uint32_t prio;
-buff_st_mask = 0xf << (i * 4);
-buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;
-
-if (buff_st != TXT_RDY) {
+if (extract32(s->tx_status.u32, i * 4, 4) != TXT_RDY) {
continue;
}
prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
@@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
if (buff2tx_idx == -1) {
break;
}
-buff_st_mask = 0xf << (buff2tx_idx * 4);
-buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
int_stat.u32 = 0;
-buff_st = TXT_RDY;
pf = s->tx_buffer[buff2tx_idx].data;
ctucan_buff2frame(pf, &frame);
s->status.s.idle = 0;
@@ -283,12 +275,11 @@ static void ctucan_send_ready_buffers(CtuCanCoreState *s)
s->status.s.idle = 1;
s->status.s.txs = 0;
s->tx_fr_ctr.s.tx_fr_ctr_val++;
-buff_st = TXT_TOK;
int_stat.s.txi = 1;
int_stat.s.txbhci = 1;
s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
-s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
-(buff_st << (buff2tx_idx * 4));
+s->tx_status.u32 = deposit32(s->tx_status.u32,
+ buff2tx_idx * 4, 4, TXT_TOK);
} while (1);
}
--
2.20.1
Re: [PATCH v2 1/4] linux-user/sparc: Correct sparc64_get/set_context() FPU handling
On Fri, 6 Nov 2020 at 17:09, Richard Henderson
wrote:
>
> On 11/6/20 7:27 AM, Peter Maydell wrote:
> > +if (fprs & FPRS_DU) {
> > +for (i = 16; i < 31; i++) {
>
> 32.
Derp. Lucky this code basically never gets run, eh ? :-)
-- PMM
