RE: [PATCH 0/8] Fix memory leak of some device state in migration
Thank you for you review. I will modify patches according to your opinion.
Jinhao Gao
-Original Message-
From: Michael S. Tsirkin [mailto:m...@redhat.com]
Sent: 2020年12月27日 21:20
To: gaojinhao
Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Marc-André Lureau
; Stefan Berger ;
Jason Wang ; David Gibson ;
Greg Kurz ; Juan Quintela ; Dr . David
Alan Gilbert ; Wanghaibin (D)
; zhukeqian
Subject: Re: [PATCH 0/8] Fix memory leak of some device state in migration
On Sat, Dec 26, 2020 at 06:33:39PM +0800, g00517791 wrote:
> From: Jinhao Gao
>
> For some device state having some fields of VMS_ALLOC flag, they don't
> free memory allocated for the fields in vmstate_save_state and vmstate
> _load_state. We add funcs or sentences of free memory before
> allocation of memory or after load of memory to avoid memory leak.
>
Isn't there a way to handle it centrally?
IIUC the issue is repeated loads in case a load fails, right?
So can't we do something along the lines of:
Signed-off-by: Michael S. Tsirkin
diff --git a/migration/vmstate.c b/migration/vmstate.c index
e9d2aef66b..873f76739f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const
VMStateField *field,
gsize size = vmstate_size(opaque, field);
size *= vmstate_n_elems(opaque, field);
if (size) {
+g_free(*(void **)ptr);
*(void **)ptr = g_malloc(size);
}
}
--
MST
RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
Hi David,
Firstly, thank you for you review. And then for your review, I worry that a
memory leak will occur if QEMU exits after saves vmsd. So, we free it in
post_save func.
Jinhao Gao
-Original Message-
From: David Gibson [mailto:da...@gibson.dropbear.id.au]
Sent: 2020-12-28 14:58
To: gaojinhao
Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Marc-André Lureau
; Stefan Berger ;
Michael S . Tsirkin ; Jason Wang ; Greg
Kurz ; Juan Quintela ; Dr . David Alan
Gilbert ; Wanghaibin (D) ;
zhukeqian
Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> From: Jinhao Gao
>
> When VM migrate VMState of spapr_pci, the field(msi_devs) of spapr_pci
> having a flag of VMS_ALLOC need to allocate memory. If the src doesn't
> free memory of msi_devs in SaveStateEntry of spapr_pci after QEMUFile
> save VMState of spapr_pci, it may result in memory leak of msi_devs.
> We add the post_save func to free memory, which prevents memory leak.
>
> Signed-off-by: Jinhao Gao
Not really a memory leak, since it will get freed on the next pre_save. But,
we might as well free it earlier if we can ,so
Acked-by: David Gibson
> ---
> hw/ppc/spapr_pci.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index
> 76d7c91e9c..1b2b940606 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
> return 0;
> }
>
> +static int spapr_pci_post_save(void *opaque) {
> +SpaprPhbState *sphb = opaque;
> +
> +g_free(sphb->msi_devs);
> +sphb->msi_devs = NULL;
> +sphb->msi_devs_num = 0;
> +return 0;
> +}
> +
> static int spapr_pci_post_load(void *opaque, int version_id) {
> SpaprPhbState *sphb = opaque;
> @@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> .version_id = 2,
> .minimum_version_id = 2,
> .pre_save = spapr_pci_pre_save,
> +.post_save = spapr_pci_post_save,
> .post_load = spapr_pci_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[PATCH 0/2] bsd-user, FreeBSD update
>From 10b13162949debdbbd8394bc1047511d1a900176 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Mon, 28 Dec 2020 08:10:43 + Subject: [PATCH 0/2] *** SUBJECT HERE *** bsd-user, FreeBSD update. David Carlier (2): bsd-user, updating the FreeBSD's syscall list, based on the 11.x bsd-user, Adding more strace support for a handful of syscalls. bsd-user/freebsd/strace.list | 12 bsd-user/freebsd/syscall_nr.h | 25 ++--- 2 files changed, 34 insertions(+), 3 deletions(-) -- 2.30.0.rc2
[PATCH 2/2] bsd-user, Adding more strace support for a handful of syscalls.
---
bsd-user/freebsd/strace.list | 12
1 file changed, 12 insertions(+)
diff --git a/bsd-user/freebsd/strace.list b/bsd-user/freebsd/strace.list
index 2800a2d4eb..136d2c42d7 100644
--- a/bsd-user/freebsd/strace.list
+++ b/bsd-user/freebsd/strace.list
@@ -38,6 +38,13 @@
{ TARGET_FREEBSD_NR_adjtime, "adjtime", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_bind, "bind", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_break, "break", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_enter, "cap_enter", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_fcntls_get, "cap_fcntls_get", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_fcntls_limit, "cap_fcntls_limit", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_getmode, "cap_getmode", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_ioctls_get, "cap_ioctls_get", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_ioctls_limit, "cap_ioctls_limit", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cap_rights_limit, "cap_rights_limit", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_chdir, "chdir", "%s(\"%s\")", NULL, NULL },
{ TARGET_FREEBSD_NR_chflags, "chflags", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_chmod, "chmod", "%s(\"%s\",%#o)", NULL, NULL },
@@ -48,6 +55,8 @@
{ TARGET_FREEBSD_NR_clock_settime, "clock_settime", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_close, "close", "%s(%d)", NULL, NULL },
{ TARGET_FREEBSD_NR_connect, "connect", "%s(%d,%#x,%d)", NULL, NULL },
+{ TARGET_FREEBSD_NR_cpuset_getdomain, "cpuset_getdomain", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_cpuset_setdomain, "cpuset_setdomain", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_dup, "dup", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_dup2, "dup2", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_eaccess, "eaccess", "%s(\"%s\",%#x)", NULL, NULL },
@@ -148,6 +157,8 @@
{ TARGET_FREEBSD_NR_pathconf, "pathconf", "%s(\"%s\", %d)", NULL, NULL },
{ TARGET_FREEBSD_NR_pipe, "pipe", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_poll, "poll", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_posix_fallocate, "posix_fallocate", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_posix_openpt, "posix_openpt", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_pread, "pread", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_preadv, "preadv", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_profil, "profil", NULL, NULL, NULL },
@@ -227,5 +238,6 @@
{ TARGET_FREEBSD_NR_utimes, "utimes", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_vfork, "vfork", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_wait4, "wait4", NULL, NULL, NULL },
+{ TARGET_FREEBSD_NR_wait4, "wait6", NULL, NULL, NULL },
{ TARGET_FREEBSD_NR_write, "write", "%s(%d,%#x,%d)", NULL, NULL },
{ TARGET_FREEBSD_NR_writev, "writev", "%s(%d,%p,%#x)", NULL, NULL },
--
2.30.0.rc2
[PATCH 1/2] bsd-user, updating the FreeBSD's syscall list, based on the 11.x
--- bsd-user/freebsd/syscall_nr.h | 25 ++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/bsd-user/freebsd/syscall_nr.h b/bsd-user/freebsd/syscall_nr.h index d849024792..14d2465858 100644 --- a/bsd-user/freebsd/syscall_nr.h +++ b/bsd-user/freebsd/syscall_nr.h @@ -1,8 +1,8 @@ /* * System call numbers. * - * created from FreeBSD: releng/9.1/sys/kern/syscalls.master 229723 - * 2012-01-06 19:29:16Z jhb + * created from FreeBSD: stable/11/sys/kern/syscalls.master a61bf07 + * 2019-04-28 emaste */ #define TARGET_FREEBSD_NR_syscall 0 @@ -447,4 +447,23 @@ #define TARGET_FREEBSD_NR_rctl_remove_rule 529 #define TARGET_FREEBSD_NR_posix_fallocate 530 #define TARGET_FREEBSD_NR_posix_fadvise 531 -#define TARGET_FREEBSD_NR_MAXSYSCALL532 +#define TARGET_FREEBSD_NR_wait6 532 +#define TARGET_FREEBSD_NR_cap_rights_limit 533 +#define TARGET_FREEBSD_NR_cap_ioctls_limit 534 +#define TARGET_FREEBSD_NR_cap_ioctls_get 535 +#define TARGET_FREEBSD_NR_cap_fcntls_limit 536 +#define TARGET_FREEBSD_NR_cap_fcntls_get 537 +#define TARGET_FREEBSD_NR_bindat 538 +#define TARGET_FREEBSD_NR_connectat 539 +#define TARGET_FREEBSD_NR_chflagsat 540 +#define TARGET_FREEBSD_NR_acceptat 541 +#define TARGET_FREEBSD_NR_pipe2 542 +#define TARGET_FREEBSD_NR_aio_mlock 543 +#define TARGET_FREEBSD_NR_procctl 544 +#define TARGET_FREEBSD_NR_ppoll 545 +#define TARGET_FREEBSD_NR_futimens 546 +#define TARGET_FREEBSD_NR_utimensat 547 +#define TARGET_FREEBSD_NR_numa_getaffinity 548 +#define TARGET_FREEBSD_NR_numa_setaffinity 549 +#define TARGET_FREEBSD_NR_fdatasync 550 +#define TARGET_FREEBSD_NR_MAXSYSCALL551 -- 2.30.0.rc2
Re: [PATCH 6/6] spapr: Model DR connectors as simple objects
On Fri, Dec 18, 2020 at 11:34:00AM +0100, Greg Kurz wrote:
> Modeling DR connectors as individual devices raises some
> concerns, as already discussed a year ago in this thread:
>
> https://patchew.org/QEMU/20191017205953.13122-1-chel...@linux.vnet.ibm.com/
>
> First, high maxmem settings creates too many DRC devices.
> This causes scalability issues. It severely increase boot
> time because the multiple traversals of the DRC list that
> are performed during machine setup are quadratic operations.
> This is directly related to the fact that DRCs are modeled
> as individual devices and added to the composition tree.
>
> Second, DR connectors are really an internal concept of
> PAPR. They aren't something that the user or management
> layer can manipulate in any way. We already don't allow
> their creation with device_add by clearing user_creatable.
>
> DR connectors don't even need to be modeled as actual
> devices since they don't sit in a bus. They just need
> to be associated to an 'owner' object and to have the
> equivalent of realize/unrealize functions.
>
> Downgrade them to be simple objects. Convert the existing
> realize() and unrealize() to be methods of the DR connector
> base class. Also have the base class to inherit from the
> vmstate_if interface directly. The get_id() hook simply
> returns NULL, just as device_vmstate_if_get_id() does for
> devices that don't sit in a bus. The DR connector is no
> longer made a child object. This means that it must be
> explicitely freed when no longer needed. This is only
> required for PHBs and PCI bridges actually : have them to
> free the DRC with spapr_dr_connector_free() instead of
> object_unparent().
>
> No longer add the DRCs to the QOM composition tree. Track
> them with a glib hash table using the global DRC index as
> the key instead. This makes traversal a linear operation.
I have some reservations about this one. The main thing is that
attaching migration state to something that's not a device seems a bit
odd to me. AFAICT exactly one other non-device implements
TYPE_VMSTATE_IF, and what it does isn't very clear to me.
As I might have mentioned to you I had a different idea for how to
address this problem: still use a TYPE_DEVICE, but have it manage a
whole array of DRCs as one unit, rather than just a single one.
Specifically I was thinking:
* one array per PCI bus (DRCs for each function on the bus)
* one array for each block of memory (so one for base memory, one for
each DIMM)
* one array for all the cpus
* one array for all the PHBs
It has some disadvantages compared to your scheme: it still leaves
(less) devices which can't be user managed, which is a bit ugly. On
the other hand, each of those arrays can reasonably be dense, so we
can use direct indexing rather than a hash table, which is a bit
nicer.
Thoughts?
>
> Signed-off-by: Greg Kurz
> ---
> include/hw/ppc/spapr_drc.h | 8 +-
> hw/ppc/spapr_drc.c | 166 ++---
> hw/ppc/spapr_pci.c | 2 +-
> 3 files changed, 69 insertions(+), 107 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 8982927d5c24..a26aa8b9d4c3 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -170,7 +170,7 @@ typedef enum {
>
> typedef struct SpaprDrc {
> /*< private >*/
> -DeviceState parent;
> +Object parent;
>
> uint32_t id;
> Object *owner;
> @@ -193,7 +193,7 @@ struct SpaprMachineState;
>
> typedef struct SpaprDrcClass {
> /*< private >*/
> -DeviceClass parent;
> +ObjectClass parent;
> SpaprDrcState empty_state;
> SpaprDrcState ready_state;
>
> @@ -209,6 +209,9 @@ typedef struct SpaprDrcClass {
>
> int (*dt_populate)(SpaprDrc *drc, struct SpaprMachineState *spapr,
> void *fdt, int *fdt_start_offset, Error **errp);
> +
> +void (*realize)(SpaprDrc *drc);
> +void (*unrealize)(SpaprDrc *drc);
> } SpaprDrcClass;
>
> typedef struct SpaprDrcPhysical {
> @@ -232,6 +235,7 @@ SpaprDrcType spapr_drc_type(SpaprDrc *drc);
>
> SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> uint32_t id);
> +void spapr_dr_connector_free(SpaprDrc *drc);
> SpaprDrc *spapr_drc_by_index(uint32_t index);
> SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
> int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t
> drc_type_mask);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8571d5bafe4e..e26763f8b5a4 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,7 +27,6 @@
> #include "sysemu/reset.h"
> #include "trace.h"
>
> -#define DRC_CONTAINER_PATH "/dr-connector"
> #define DRC_INDEX_TYPE_SHIFT 28
> #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
>
> @@ -503,65 +502,56 @@ static const VMStateDescription vmstate_spapr_drc = {
> }
> };
>
> -static void drc_realize(Device
Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
On Mon, Dec 28, 2020 at 08:10:31AM +, gaojinhao wrote:
> Hi David,
> Firstly, thank you for you review. And then for your review, I worry
> that a memory leak will occur if QEMU exits after saves vmsd. So, we
> free it in post_save func.
If qemu exits, all its memory will be freed, so we don't care.
>
> Jinhao Gao
>
> -Original Message-
> From: David Gibson [mailto:da...@gibson.dropbear.id.au]
> Sent: 2020-12-28 14:58
> To: gaojinhao
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Marc-André Lureau
> ; Stefan Berger ;
> Michael S . Tsirkin ; Jason Wang ; Greg
> Kurz ; Juan Quintela ; Dr . David Alan
> Gilbert ; Wanghaibin (D) ;
> zhukeqian
> Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
>
> On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> > From: Jinhao Gao
> >
> > When VM migrate VMState of spapr_pci, the field(msi_devs) of spapr_pci
> > having a flag of VMS_ALLOC need to allocate memory. If the src doesn't
> > free memory of msi_devs in SaveStateEntry of spapr_pci after QEMUFile
> > save VMState of spapr_pci, it may result in memory leak of msi_devs.
> > We add the post_save func to free memory, which prevents memory leak.
> >
> > Signed-off-by: Jinhao Gao
>
> Not really a memory leak, since it will get freed on the next pre_save. But,
> we might as well free it earlier if we can ,so
>
> Acked-by: David Gibson
>
> > ---
> > hw/ppc/spapr_pci.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index
> > 76d7c91e9c..1b2b940606 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
> > return 0;
> > }
> >
> > +static int spapr_pci_post_save(void *opaque) {
> > +SpaprPhbState *sphb = opaque;
> > +
> > +g_free(sphb->msi_devs);
> > +sphb->msi_devs = NULL;
> > +sphb->msi_devs_num = 0;
> > +return 0;
> > +}
> > +
> > static int spapr_pci_post_load(void *opaque, int version_id) {
> > SpaprPhbState *sphb = opaque;
> > @@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> > .version_id = 2,
> > .minimum_version_id = 2,
> > .pre_save = spapr_pci_pre_save,
> > +.post_save = spapr_pci_post_save,
> > .post_load = spapr_pci_post_load,
> > .fields = (VMStateField[]) {
> > VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[PATCH v2 0/3] Fix memory leak of some device state in migration
From: Jinhao Gao For some device state having some fields of VMS_ALLOC flag, they don't free memory allocated for the fields in vmstate_save_state and vmstate_load_state. We add funcs or sentences of free memory before and after VM saves or loads device state to avoid memory leak. v2 - Drop patch1-3,6-8 of v1 - Address Michael's comment (free memory before load vmsd centrally) - Add David's Acked-by and Michael's Signed-off-by Jinhao Gao (3): spapr_pci: Fix memory leak of vmstate_spapr_pci savevm: Fix memory leak of vmstate_configuration vmstate: Fix memory leak in vmstate_handle_alloc() hw/ppc/spapr_pci.c | 11 +++ migration/savevm.c | 31 +++ migration/vmstate.c | 1 + 3 files changed, 39 insertions(+), 4 deletions(-) -- 2.23.0
[PATCH v2 2/3] savevm: Fix memory leak of vmstate_configuration
From: Jinhao Gao
When VM migrate VMState of configuration, the fields(name and capabilities)
of configuration having a flag of VMS_ALLOC need to allocate memory. If the
src doesn't free memory of capabilities in SaveState after save VMState of
configuration, or the dst doesn't free memory of name and capabilities in post
load of configuration, it may result in memory leak of name and capabilities.
We free memory in configuration_post_save and configuration_post_load func,
which prevents memory leak.
Signed-off-by: Jinhao Gao
---
migration/savevm.c | 31 +++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 5f937a2762..13f1a5dab7 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -314,6 +314,16 @@ static int configuration_pre_save(void *opaque)
return 0;
}
+static int configuration_post_save(void *opaque)
+{
+SaveState *state = opaque;
+
+g_free(state->capabilities);
+state->capabilities = NULL;
+state->caps_count = 0;
+return 0;
+}
+
static int configuration_pre_load(void *opaque)
{
SaveState *state = opaque;
@@ -364,24 +374,36 @@ static int configuration_post_load(void *opaque, int
version_id)
{
SaveState *state = opaque;
const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+int ret = 0;
if (strncmp(state->name, current_name, state->len) != 0) {
error_report("Machine type received is '%.*s' and local is '%s'",
(int) state->len, state->name, current_name);
-return -EINVAL;
+ret = -EINVAL;
+goto out;
}
if (state->target_page_bits != qemu_target_page_bits()) {
error_report("Received TARGET_PAGE_BITS is %d but local is %d",
state->target_page_bits, qemu_target_page_bits());
-return -EINVAL;
+ret = -EINVAL;
+goto out;
}
if (!configuration_validate_capabilities(state)) {
-return -EINVAL;
+ret = -EINVAL;
+goto out;
}
-return 0;
+out:
+g_free((void *)state->name);
+state->name = NULL;
+state->len = 0;
+g_free(state->capabilities);
+state->capabilities = NULL;
+state->caps_count = 0;
+
+return ret;
}
static int get_capability(QEMUFile *f, void *pv, size_t size,
@@ -515,6 +537,7 @@ static const VMStateDescription vmstate_configuration = {
.pre_load = configuration_pre_load,
.post_load = configuration_post_load,
.pre_save = configuration_pre_save,
+.post_save = configuration_post_save,
.fields = (VMStateField[]) {
VMSTATE_UINT32(len, SaveState),
VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
--
2.23.0
[PATCH v2 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()
From: Jinhao Gao
Some memory allocated for fields having a flag of VMS_ALLOC in SaveState
may not free before VM load vmsd in migration. So we pre-free memory before
allocation in vmstate_handle_alloc() to avoid memleaks.
Signed-off-by: Jinhao Gao
Signed-off-by: Michael S. Tsirkin
---
migration/vmstate.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e9d2aef66b..873f76739f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const
VMStateField *field,
gsize size = vmstate_size(opaque, field);
size *= vmstate_n_elems(opaque, field);
if (size) {
+g_free(*(void **)ptr);
*(void **)ptr = g_malloc(size);
}
}
--
2.23.0
[PATCH v2 1/3] spapr_pci: Fix memory leak of vmstate_spapr_pci
From: Jinhao Gao
When VM migrate VMState of spapr_pci, the field(msi_devs) of spapr_pci
having a flag of VMS_ALLOC need to allocate memory. If the src doesn't free
memory of msi_devs in SaveStateEntry of spapr_pci after QEMUFile save
VMState of spapr_pci, it may result in memory leak of msi_devs. We add the
post_save func to free memory, which prevents memory leak.
Signed-off-by: Jinhao Gao
Acked-by: David Gibson
---
hw/ppc/spapr_pci.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 76d7c91e9c..1b2b940606 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
return 0;
}
+static int spapr_pci_post_save(void *opaque)
+{
+SpaprPhbState *sphb = opaque;
+
+g_free(sphb->msi_devs);
+sphb->msi_devs = NULL;
+sphb->msi_devs_num = 0;
+return 0;
+}
+
static int spapr_pci_post_load(void *opaque, int version_id)
{
SpaprPhbState *sphb = opaque;
@@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
.version_id = 2,
.minimum_version_id = 2,
.pre_save = spapr_pci_pre_save,
+.post_save = spapr_pci_post_save,
.post_load = spapr_pci_post_load,
.fields = (VMStateField[]) {
VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
--
2.23.0
Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
I am not convinced, however. Specifically, attaching this to the DRC
doesn't make sense to me. We're adding exactly one DRC related async
hcall, and I can't really see much call for another one. We could
have other async hcalls - indeed we already have one for HPT resizing
- but attaching this to DRCs doesn't help for those.
>
> > hw/ppc/spapr_drc.c | 149
> >
> > include/hw/ppc/spapr_drc.h | 25 +++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > +Error *err = NULL;
> > +uint64_t token;
> > +SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > +state = g_malloc0(sizeof(*state));
> > +state->pending = true;
> > +
> > +qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > +if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > +error_report_err(err);
> > +g_free(state);
> > +qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +return 0;
> > +}
> > +
> > +if (!token) /* Token should be non-zero */
> > +goto retry;
> > +
> > +if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > +QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > +if (tmp->continue_token == token) {
> > +/* If the token already in use, get a new one */
> > +goto retry;
> > +}
> > +}
> > +}
> > +
> > +state->continue_token = token;
> > +QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > +qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > +return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > +int response = -1;
> > +SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > +/*
> > + * state is freed only after this thread finishes(after
> > pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > +response = state->func(state->data);
> > +
> > +state->hcall_ret = response;
> > +state->pending = 0;
> > +
> > +return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is
> > supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled
> there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void
> > *data)
> > +{
> > +SpaprDrcDeviceAsyncHCallState *state;
> > +
> > +qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > +if (state->continue_token == token) {
> > +state->func = func;
> > +state->data = data;
> > +qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation cou
RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
Thank you for you reply, I understand.
Jinhao Gao
-Original Message-
From: David Gibson [mailto:da...@gibson.dropbear.id.au]
Sent: 2020年12月28日 16:30
To: gaojinhao
Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Marc-André Lureau
; Stefan Berger ;
Michael S . Tsirkin ; Jason Wang ; Greg
Kurz ; Juan Quintela ; Dr . David Alan
Gilbert ; Wanghaibin (D) ;
zhukeqian
Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
On Mon, Dec 28, 2020 at 08:10:31AM +, gaojinhao wrote:
> Hi David,
> Firstly, thank you for you review. And then for your review, I worry
> that a memory leak will occur if QEMU exits after saves vmsd. So, we
> free it in post_save func.
If qemu exits, all its memory will be freed, so we don't care.
>
> Jinhao Gao
>
> -Original Message-
> From: David Gibson [mailto:da...@gibson.dropbear.id.au]
> Sent: 2020-12-28 14:58
> To: gaojinhao
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Marc-André Lureau
> ; Stefan Berger
> ; Michael S . Tsirkin ;
> Jason Wang ; Greg Kurz ; Juan
> Quintela ; Dr . David Alan Gilbert
> ; Wanghaibin (D) ;
> zhukeqian
> Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of
> vmstate_spapr_pci
>
> On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> > From: Jinhao Gao
> >
> > When VM migrate VMState of spapr_pci, the field(msi_devs) of
> > spapr_pci having a flag of VMS_ALLOC need to allocate memory. If the
> > src doesn't free memory of msi_devs in SaveStateEntry of spapr_pci
> > after QEMUFile save VMState of spapr_pci, it may result in memory leak of
> > msi_devs.
> > We add the post_save func to free memory, which prevents memory leak.
> >
> > Signed-off-by: Jinhao Gao
>
> Not really a memory leak, since it will get freed on the next
> pre_save. But, we might as well free it earlier if we can ,so
>
> Acked-by: David Gibson
>
> > ---
> > hw/ppc/spapr_pci.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index
> > 76d7c91e9c..1b2b940606 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
> > return 0;
> > }
> >
> > +static int spapr_pci_post_save(void *opaque) {
> > +SpaprPhbState *sphb = opaque;
> > +
> > +g_free(sphb->msi_devs);
> > +sphb->msi_devs = NULL;
> > +sphb->msi_devs_num = 0;
> > +return 0;
> > +}
> > +
> > static int spapr_pci_post_load(void *opaque, int version_id) {
> > SpaprPhbState *sphb = opaque;
> > @@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> > .version_id = 2,
> > .minimum_version_id = 2,
> > .pre_save = spapr_pci_pre_save,
> > +.post_save = spapr_pci_post_save,
> > .post_load = spapr_pci_post_load,
> > .fields = (VMStateField[]) {
> > VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH 1/4] qobject: open brace '{' following struct go on the same line
On 12/28/20 8:11 AM, Zhang Han wrote:
> Put open brace '{' on the same line of struct.
>
> Signed-off-by: Zhang Han
> ---
> qobject/json-parser.c | 3 +--
> qobject/qjson.c | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 4/4] qobject: braces {} are necessary for all arms of this statement
On 12/28/20 8:11 AM, Zhang Han wrote:
> Add braces {} for arms of if/for statement
>
> Signed-off-by: Zhang Han
> ---
> qobject/qdict.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 03/10] vt82c686b: Rename VT82C686B to VT82C686B_ISA
On 12/28/20 3:08 AM, BALATON Zoltan via wrote: > This is really the ISA bridge part so name the type accordingly. > > Signed-off-by: BALATON Zoltan > --- > hw/isa/vt82c686.c | 20 ++-- > 1 file changed, 10 insertions(+), 10 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 10/10] vt82c686: Remove unneeded includes and defines
On 12/28/20 3:08 AM, BALATON Zoltan via wrote: > These are not used or not needed. > > Signed-off-by: BALATON Zoltan > --- > v2: Added back a few that we get indirectly but keep it explicit > > hw/isa/vt82c686.c | 5 - > 1 file changed, 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 1/3] spapr_pci: Fix memory leak of vmstate_spapr_pci
On Mon, Dec 28, 2020 at 05:00:51PM +0800, g00517791 wrote:
> From: Jinhao Gao
>
> When VM migrate VMState of spapr_pci, the field(msi_devs) of spapr_pci
> having a flag of VMS_ALLOC need to allocate memory. If the src doesn't free
> memory of msi_devs in SaveStateEntry of spapr_pci after QEMUFile save
> VMState of spapr_pci, it may result in memory leak of msi_devs. We add the
> post_save func to free memory, which prevents memory leak.
>
> Signed-off-by: Jinhao Gao
> Acked-by: David Gibson
Reviewed-by: Michael S. Tsirkin
> ---
> hw/ppc/spapr_pci.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 76d7c91e9c..1b2b940606 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
> return 0;
> }
>
> +static int spapr_pci_post_save(void *opaque)
> +{
> +SpaprPhbState *sphb = opaque;
> +
> +g_free(sphb->msi_devs);
> +sphb->msi_devs = NULL;
> +sphb->msi_devs_num = 0;
> +return 0;
> +}
> +
> static int spapr_pci_post_load(void *opaque, int version_id)
> {
> SpaprPhbState *sphb = opaque;
> @@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> .version_id = 2,
> .minimum_version_id = 2,
> .pre_save = spapr_pci_pre_save,
> +.post_save = spapr_pci_post_save,
> .post_load = spapr_pci_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
> --
> 2.23.0
Re: [PATCH v2 2/3] savevm: Fix memory leak of vmstate_configuration
On Mon, Dec 28, 2020 at 05:00:52PM +0800, g00517791 wrote:
> From: Jinhao Gao
>
> When VM migrate VMState of configuration, the fields(name and capabilities)
> of configuration having a flag of VMS_ALLOC need to allocate memory. If the
> src doesn't free memory of capabilities in SaveState after save VMState of
> configuration, or the dst doesn't free memory of name and capabilities in post
> load of configuration, it may result in memory leak of name and capabilities.
> We free memory in configuration_post_save and configuration_post_load func,
> which prevents memory leak.
>
> Signed-off-by: Jinhao Gao
Reviewed-by: Michael S. Tsirkin
> ---
> migration/savevm.c | 31 +++
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5f937a2762..13f1a5dab7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -314,6 +314,16 @@ static int configuration_pre_save(void *opaque)
> return 0;
> }
>
> +static int configuration_post_save(void *opaque)
> +{
> +SaveState *state = opaque;
> +
> +g_free(state->capabilities);
> +state->capabilities = NULL;
> +state->caps_count = 0;
> +return 0;
> +}
> +
> static int configuration_pre_load(void *opaque)
> {
> SaveState *state = opaque;
> @@ -364,24 +374,36 @@ static int configuration_post_load(void *opaque, int
> version_id)
> {
> SaveState *state = opaque;
> const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> +int ret = 0;
>
> if (strncmp(state->name, current_name, state->len) != 0) {
> error_report("Machine type received is '%.*s' and local is '%s'",
> (int) state->len, state->name, current_name);
> -return -EINVAL;
> +ret = -EINVAL;
> +goto out;
> }
>
> if (state->target_page_bits != qemu_target_page_bits()) {
> error_report("Received TARGET_PAGE_BITS is %d but local is %d",
> state->target_page_bits, qemu_target_page_bits());
> -return -EINVAL;
> +ret = -EINVAL;
> +goto out;
> }
>
> if (!configuration_validate_capabilities(state)) {
> -return -EINVAL;
> +ret = -EINVAL;
> +goto out;
> }
>
> -return 0;
> +out:
> +g_free((void *)state->name);
> +state->name = NULL;
> +state->len = 0;
> +g_free(state->capabilities);
> +state->capabilities = NULL;
> +state->caps_count = 0;
> +
> +return ret;
> }
>
> static int get_capability(QEMUFile *f, void *pv, size_t size,
> @@ -515,6 +537,7 @@ static const VMStateDescription vmstate_configuration = {
> .pre_load = configuration_pre_load,
> .post_load = configuration_post_load,
> .pre_save = configuration_pre_save,
> +.post_save = configuration_post_save,
> .fields = (VMStateField[]) {
> VMSTATE_UINT32(len, SaveState),
> VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
> --
> 2.23.0
Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support
在 2020/12/28 上午6:13, BALATON Zoltan 写道:
From: Guenter Roeck
The IDE legacy mode emulation has been removed in commit 4ea98d317eb
("ide/via: Implement and use native PCI IDE mode") but some Linux
kernels (probably including def_config) require legacy mode on the
Fuloong2e so only emulating native mode did not turn out feasible.
Add property to via-ide model to make the mode configurable, and set
legacy mode for Fuloong2e.
Signed-off-by: Guenter Roeck
[balaton: Use bit in flags for property, add comment for missing BAR4]
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Guenter Roeck
Reviewed-by: Jiaxun Yang
---
v2: Reworded commit message
Re: [PATCH v12 1/7] Introduce yank feature
On Tue, 22 Dec 2020 12:00:29 +0400
Marc-André Lureau wrote:
> On Sun, Dec 13, 2020 at 3:48 PM Lukas Straub wrote:
>
> > The yank feature allows to recover from hanging qemu by "yanking"
> > at various parts. Other qemu systems can register themselves and
> > multiple yank functions. Then all yank functions for selected
> > instances can be called by the 'yank' out-of-band qmp command.
> > Available instances can be queried by a 'query-yank' oob command.
> >
> > Signed-off-by: Lukas Straub
> > Acked-by: Stefan Hajnoczi
> > Reviewed-by: Markus Armbruster
> > ---
> > MAINTAINERS | 7 ++
> > include/qemu/yank.h | 95 +++
> > qapi/meson.build | 1 +
> > qapi/qapi-schema.json | 1 +
> > qapi/yank.json| 119 +++
> > util/meson.build | 1 +
> > util/yank.c | 216 ++
> > 7 files changed, 440 insertions(+)
> > create mode 100644 include/qemu/yank.h
> > create mode 100644 qapi/yank.json
> > create mode 100644 util/yank.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d48a4e8a8b..5d7e3c0e4b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2705,6 +2705,13 @@ F: util/uuid.c
> > F: include/qemu/uuid.h
> > F: tests/test-uuid.c
> >
> > +Yank feature
> > +M: Lukas Straub
> > +S: Odd fixes
> > +F: util/yank.c
> > +F: include/qemu/yank.h
> > +F: qapi/yank.json
> > +
> > COLO Framework
> > M: zhanghailiang
> > S: Maintained
> > diff --git a/include/qemu/yank.h b/include/qemu/yank.h
> > new file mode 100644
> > index 00..96f5b2626f
> > --- /dev/null
> > +++ b/include/qemu/yank.h
> > @@ -0,0 +1,95 @@
> > +/*
> > + * QEMU yank feature
> > + *
> > + * Copyright (c) Lukas Straub
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef YANK_H
> > +#define YANK_H
> > +
> > +#include "qapi/qapi-types-yank.h"
> > +
> > +typedef void (YankFn)(void *opaque);
> > +
> > +/**
> > + * yank_register_instance: Register a new instance.
> > + *
> > + * This registers a new instance for yanking. Must be called before any
> > yank
> > + * function is registered for this instance.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance: The instance.
> > + * @errp: Error object.
> > + */
> > +void yank_register_instance(const YankInstance *instance, Error **errp);
> > +
> >
>
> It's a good idea to return a success boolean. (see include/qapi/error.h)
Changed for the next version.
> +/**
> > + * yank_unregister_instance: Unregister a instance.
> > + *
> > + * This unregisters a instance. Must be called only after every yank
> > function
> > + * of the instance has been unregistered.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance: The instance.
> > + */
> > +void yank_unregister_instance(const YankInstance *instance);
> > +
> > +/**
> > + * yank_register_function: Register a yank function
> > + *
> > + * This registers a yank function. All limitations of qmp oob commands
> > apply
> > + * to the yank function as well. See docs/devel/qapi-code-gen.txt under
> > + * "An OOB-capable command handler must satisfy the following conditions".
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance: The instance.
> > + * @func: The yank function.
> > + * @opaque: Will be passed to the yank function.
> > + */
> > +void yank_register_function(const YankInstance *instance,
> > +YankFn *func,
> > +void *opaque);
> > +
> > +/**
> > + * yank_unregister_function: Unregister a yank function
> > + *
> > + * This unregisters a yank function.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance: The instance.
> > + * @func: func that was passed to yank_register_function.
> > + * @opaque: opaque that was passed to yank_register_function.
> > + */
> > +void yank_unregister_function(const YankInstance *instance,
> > + YankFn *func,
> > + void *opaque);
> > +
> > +/**
> > + * yank_generic_iochannel: Generic yank function for iochannel
> > + *
> > + * This is a generic yank function which will call qio_channel_shutdown
> > on the
> > + * provided QIOChannel.
> > + *
> > + * @opaque: QIOChannel to shutdown
> > + */
> > +void yank_generic_iochannel(void *opaque);
> > +
> > +#define BLOCKDEV_YANK_INSTANCE(the_node_name) (&(YankInstance) { \
> > +.type = YANK_INSTANCE_TYPE_BLOCK_NODE, \
> > +.u.block_node.node_name = (the_node_name) })
> > +
> > +#define CHARDEV_YANK_INSTANCE(the_id) (&(YankInstance) { \
> > +.type = YANK_INSTANCE_TYPE_CHARDEV, \
> > +.u.chardev.id = (the_id) })
> > +
> > +#define MIGRATION_YANK_INSTANCE (&(YankInstance) { \
> > +.type = YANK_INSTANCE_TYPE_MIGRATION })
> > +
> > +#endif
> > diff --git a/qapi/meson.build b/qapi/meson.build
> > i
[Question] VNC CA certificate update live
Hi all:The VNC of QEMU suppots TLS encryption. The client & server can use arbitrary certificates from CA certificates the running VM loaded(user can use new certificates immediately), but if the CA certificate is changed to a new one，the running VM still use the old CA. Is it reasonable to provide an API(e.g.QMP) to replace the CA certificate for running VM live？Any security problem?Regards,Zihao
[PATCH v13 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu
Hello Everyone, So here is v13. Changes: v13: -Address Marc-André Lureau comments: -make yank_register_instance return bool -rename yank_compare_instances to yank_instance_equal -remove breaks -use g_str_equal instead of strcmp -use g_new0 instead of g_slice_new -use QEMU_LOCK_GUARD instead of qemu_mutex_lock/unlock v12: -rebase onto master -minor change to migration (removal of "defer" branch in qemu_start_incoming_migration) -add Reviewed-by tags v11: -squashed MAINTAINERS update into patch 1 -move qmp doc of yank before misc -add title for qmp docs -change "Since:" to 6.0 -add Reviewed-by tags v10: -moved from qapi/misc.json to qapi/yank.json -rename 'blockdev' -> 'block-node' -document difference betwen migration yank instance and migrate_cancel -better document return values of yank command -better document yank_lock -minor style and spelling fixes v9: -rebase onto master -implemented new qmp api as proposed by Markus v8: -add Reviewed-by and Acked-by tags -rebase onto master -minor change to migration -convert to meson -change "Since:" to 5.2 -varios code style fixes (Markus Armbruster) -point to oob restrictions in comment to yank_register_function (Markus Armbruster) -improve qmp documentation (Markus Armbruster) -document oob suitability of qio_channel and io_shutdown (Markus Armbruster) v7: -yank_register_instance now returns error via Error **errp instead of aborting -dropped "chardev/char.c: Check for duplicate id before creating chardev" v6: -add Reviewed-by and Acked-by tags -rebase on master -lots of changes in nbd due to rebase -only take maintainership of util/yank.c and include/qemu/yank.h (Daniel P. Berrangé) -fix a crash discovered by the newly added chardev test -fix the test itself v5: -move yank.c to util/ -move yank.h to include/qemu/ -add license to yank.h -use const char* -nbd: use atomic_store_release and atomic_load_aqcuire -io-channel: ensure thread-safety and document it -add myself as maintainer for yank v4: -fix build errors... v3: -don't touch softmmu/vl.c, use __contructor__ attribute instead (Paolo Bonzini) -fix build errors -rewrite migration patch so it actually passes all tests v2: -don't touch io/ code anymore -always register yank functions -'yank' now takes a list of instances to yank -'query-yank' returns a list of yankable instances Overview: Hello Everyone, In many cases, if qemu has a network connection (qmp, migration, chardev, etc.) to some other server and that server dies or hangs, qemu hangs too. These patches introduce the new 'yank' out-of-band qmp command to recover from these kinds of hangs. The different subsystems register callbacks which get executed with the yank command. For example the callback can shutdown() a socket. This is intended for the colo use-case, but it can be used for other things too of course. Regards, Lukas Straub Lukas Straub (7): Introduce yank feature block/nbd.c: Add yank feature chardev/char-socket.c: Add yank feature migration: Add yank feature io/channel-tls.c: make qio_channel_tls_shutdown thread-safe io: Document qmp oob suitability of qio_channel_shutdown and io_shutdown tests/test-char.c: Wait for the chardev to connect in char_socket_client_dupid_test MAINTAINERS | 7 ++ block/nbd.c | 153 +++-- chardev/char-socket.c | 34 ++ include/io/channel.h | 5 +- include/qemu/yank.h | 97 io/channel-tls.c | 6 +- migration/channel.c | 13 +++ migration/migration.c | 22 migration/multifd.c | 10 ++ migration/qemu-file-channel.c | 7 ++ migration/savevm.c| 5 + qapi/meson.build | 1 + qapi/qapi-schema.json | 1 + qapi/yank.json| 119 tests/test-char.c | 1 + util/meson.build | 1 + util/yank.c | 206 ++ 17 files changed, 624 insertions(+), 64 deletions(-) create mode 100644 include/qemu/yank.h create mode 100644 qapi/yank.json create mode 100644 util/yank.c -- 2.29.2 pgpf0vsY9MAsU.pgp Description: OpenPGP digital signature
[PATCH v13 2/7] block/nbd.c: Add yank feature
Register a yank function which shuts down the socket and sets
s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
error occured.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
block/nbd.c | 153 +++-
1 file changed, 92 insertions(+), 61 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 42536702b6..0f8d17db6a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,6 +35,7 @@
#include "qemu/option.h"
#include "qemu/cutils.h"
#include "qemu/main-loop.h"
+#include "qemu/atomic.h"
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qmp/qstring.h"
@@ -44,6 +45,8 @@
#include "block/nbd.h"
#include "block/block_int.h"
+#include "qemu/yank.h"
+
#define EN_OPTSTR ":exportname="
#define MAX_NBD_REQUESTS16
@@ -141,14 +144,13 @@ typedef struct BDRVNBDState {
NBDConnectThread *connect_thread;
} BDRVNBDState;
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
- Error **errp);
-static QIOChannelSocket *nbd_co_establish_connection(BlockDriverState *bs,
- Error **errp);
+static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
+Error **errp);
+static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
bool detach);
-static int nbd_client_handshake(BlockDriverState *bs, QIOChannelSocket *sioc,
-Error **errp);
+static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
+static void nbd_yank(void *opaque);
static void nbd_clear_bdrvstate(BDRVNBDState *s)
{
@@ -166,12 +168,12 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
if (ret == -EIO) {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
NBD_CLIENT_CONNECTING_NOWAIT;
}
} else {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
s->state = NBD_CLIENT_QUIT;
@@ -204,7 +206,7 @@ static void reconnect_delay_timer_cb(void *opaque)
{
BDRVNBDState *s = opaque;
-if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) {
s->state = NBD_CLIENT_CONNECTING_NOWAIT;
while (qemu_co_enter_next(&s->free_sema, NULL)) {
/* Resume all queued requests */
@@ -216,7 +218,7 @@ static void reconnect_delay_timer_cb(void *opaque)
static void reconnect_delay_timer_init(BDRVNBDState *s, uint64_t
expire_time_ns)
{
-if (s->state != NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) != NBD_CLIENT_CONNECTING_WAIT) {
return;
}
@@ -261,7 +263,7 @@ static void nbd_client_attach_aio_context(BlockDriverState
*bs,
* s->connection_co is either yielded from nbd_receive_reply or from
* nbd_co_reconnect_loop()
*/
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
}
@@ -287,7 +289,7 @@ static void coroutine_fn
nbd_client_co_drain_begin(BlockDriverState *bs)
reconnect_delay_timer_del(s);
-if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) {
s->state = NBD_CLIENT_CONNECTING_NOWAIT;
qemu_co_queue_restart_all(&s->free_sema);
}
@@ -338,13 +340,14 @@ static void nbd_teardown_connection(BlockDriverState *bs)
static bool nbd_client_connecting(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT ||
-s->state == NBD_CLIENT_CONNECTING_NOWAIT;
+NBDClientState state = qatomic_load_acquire(&s->state);
+return state == NBD_CLIENT_CONNECTING_WAIT ||
+state == NBD_CLIENT_CONNECTING_NOWAIT;
}
static bool nbd_client_connecting_wait(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT;
+return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
}
static void connect_bh(void *opaque)
@@ -424,12 +427,12 @@ static void *connect_thread_func(void *opaque)
return NULL;
}
-static QIOChannelSocket *coroutine_fn
+static int coroutine_fn
nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
{
+int ret;
QemuThread thread;
BDRVNBDState *s = bs->opaque;
-QIOChannelSocket *res;
NBDConnectThread *thr = s->connect_thread
[PATCH v13 3/7] chardev/char-socket.c: Add yank feature
Register a yank function to shutdown the socket on yank.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
---
chardev/char-socket.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8dd0..8a707d766c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -34,6 +34,7 @@
#include "qapi/error.h"
#include "qapi/clone-visitor.h"
#include "qapi/qapi-visit-sockets.h"
+#include "qemu/yank.h"
#include "chardev/char-io.h"
#include "qom/object.h"
@@ -70,6 +71,7 @@ struct SocketChardev {
size_t read_msgfds_num;
int *write_msgfds;
size_t write_msgfds_num;
+bool registered_yank;
SocketAddress *addr;
bool is_listen;
@@ -415,6 +417,12 @@ static void tcp_chr_free_connection(Chardev *chr)
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
+if (s->state == TCP_CHARDEV_STATE_CONNECTING
+|| s->state == TCP_CHARDEV_STATE_CONNECTED) {
+yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(s->sioc));
+}
object_unref(OBJECT(s->sioc));
s->sioc = NULL;
object_unref(OBJECT(s->ioc));
@@ -932,6 +940,9 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
}
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
ret = tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
return ret;
@@ -946,6 +957,9 @@ static void tcp_chr_accept(QIONetListener *listener,
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, cioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(cioc));
tcp_chr_new_client(chr, cioc);
}
@@ -961,6 +975,9 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error
**errp)
object_unref(OBJECT(sioc));
return -1;
}
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
return 0;
@@ -976,6 +993,9 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_net_listener_wait_client(s->listener);
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
}
@@ -1086,6 +1106,9 @@ static void char_socket_finalize(Object *obj)
object_unref(OBJECT(s->tls_creds));
}
g_free(s->tls_authz);
+if (s->registered_yank) {
+yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+}
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
@@ -1101,6 +1124,9 @@ static void qemu_chr_socket_connected(QIOTask *task, void
*opaque)
if (qio_task_propagate_error(task, &err)) {
tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
+yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
check_report_connect_error(chr, err);
goto cleanup;
}
@@ -1134,6 +1160,9 @@ static void tcp_chr_connect_client_async(Chardev *chr)
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_channel_socket_new();
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
/*
* Normally code would use the qio_channel_socket_connect_async
* method which uses a QIOTask + qio_task_set_error internally
@@ -1376,6 +1405,11 @@ static void qmp_chardev_open_socket(Chardev *chr,
qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
}
+if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+return;
+}
+s->registered_yank = true;
+
/* be isn't opened until we get a connection */
*be_opened = false;
--
2.29.2
pgp046WYCM5rR.pgp
Description: OpenPGP digital signature
[PATCH v13 4/7] migration: Add yank feature
Register yank functions on sockets to shut them down.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Acked-by: Dr. David Alan Gilbert
---
migration/channel.c | 13 +
migration/migration.c | 22 ++
migration/multifd.c | 10 ++
migration/qemu-file-channel.c | 7 +++
migration/savevm.c| 5 +
5 files changed, 57 insertions(+)
diff --git a/migration/channel.c b/migration/channel.c
index 8a783baa0b..35fe234e9c 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -18,6 +18,8 @@
#include "trace.h"
#include "qapi/error.h"
#include "io/channel-tls.h"
+#include "io/channel-socket.h"
+#include "qemu/yank.h"
/**
* @migration_channel_process_incoming - Create new incoming migration channel
@@ -35,6 +37,11 @@ void migration_channel_process_incoming(QIOChannel *ioc)
trace_migration_set_incoming_channel(
ioc, object_get_typename(OBJECT(ioc)));
+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function(MIGRATION_YANK_INSTANCE, yank_generic_iochannel,
+ QIO_CHANNEL(ioc));
+}
+
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
!object_dynamic_cast(OBJECT(ioc),
@@ -67,6 +74,12 @@ void migration_channel_connect(MigrationState *s,
ioc, object_get_typename(OBJECT(ioc)), hostname, error);
if (!error) {
+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function(MIGRATION_YANK_INSTANCE,
+ yank_generic_iochannel,
+ QIO_CHANNEL(ioc));
+}
+
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
!object_dynamic_cast(OBJECT(ioc),
diff --git a/migration/migration.c b/migration/migration.c
index e0dbde4091..92f7cb70b2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -56,6 +56,7 @@
#include "net/announce.h"
#include "qemu/queue.h"
#include "multifd.h"
+#include "qemu/yank.h"
#ifdef CONFIG_VFIO
#include "hw/vfio/vfio-common.h"
@@ -254,6 +255,8 @@ void migration_incoming_state_destroy(void)
qapi_free_SocketAddressList(mis->socket_address_list);
mis->socket_address_list = NULL;
}
+
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
static void migrate_generate_event(int new_state)
@@ -418,6 +421,10 @@ static void qemu_start_incoming_migration(const char *uri,
Error **errp)
{
const char *p = NULL;
+if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+return;
+}
+
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
if (strstart(uri, "tcp:", &p) ||
strstart(uri, "unix:", NULL) ||
@@ -432,6 +439,7 @@ static void qemu_start_incoming_migration(const char *uri,
Error **errp)
} else if (strstart(uri, "fd:", &p)) {
fd_start_incoming_migration(p, errp);
} else {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
error_setg(errp, "unknown migration protocol: %s", uri);
}
}
@@ -1737,6 +1745,7 @@ static void migrate_fd_cleanup(MigrationState *s)
}
notifier_list_notify(&migration_state_notifiers, s);
block_cleanup_parameters(s);
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
static void migrate_fd_cleanup_schedule(MigrationState *s)
@@ -2011,6 +2020,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
* only re-setup the migration stream and poke existing migration
* to continue using that newly established channel.
*/
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
qemu_start_incoming_migration(uri, errp);
}
@@ -2148,6 +2158,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
return;
}
+if (!(has_resume && resume)) {
+if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+return;
+}
+}
+
if (strstart(uri, "tcp:", &p) ||
strstart(uri, "unix:", NULL) ||
strstart(uri, "vsock:", NULL)) {
@@ -2161,6 +2177,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
} else if (strstart(uri, "fd:", &p)) {
fd_start_outgoing_migration(s, p, &local_err);
} else {
+if (!(has_resume && resume)) {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+}
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
"a valid migration protocol");
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
@@ -2170,6 +2189,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
if (local_err) {
+if (!(has_resume && resume)) {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+}
migrate_fd_error(s, local_err);
error_propagate(errp, local_err);
return;
diff --git a/migration/multifd.c b/migration/multifd
[PATCH v13 1/7] Introduce yank feature
The yank feature allows to recover from hanging qemu by "yanking"
at various parts. Other qemu systems can register themselves and
multiple yank functions. Then all yank functions for selected
instances can be called by the 'yank' out-of-band qmp command.
Available instances can be queried by a 'query-yank' oob command.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Markus Armbruster
---
MAINTAINERS | 7 ++
include/qemu/yank.h | 97
qapi/meson.build | 1 +
qapi/qapi-schema.json | 1 +
qapi/yank.json| 119
util/meson.build | 1 +
util/yank.c | 206 ++
7 files changed, 432 insertions(+)
create mode 100644 include/qemu/yank.h
create mode 100644 qapi/yank.json
create mode 100644 util/yank.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e7c8f0488..f465a4045a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2716,6 +2716,13 @@ F: util/uuid.c
F: include/qemu/uuid.h
F: tests/test-uuid.c
+Yank feature
+M: Lukas Straub
+S: Odd fixes
+F: util/yank.c
+F: include/qemu/yank.h
+F: qapi/yank.json
+
COLO Framework
M: zhanghailiang
S: Maintained
diff --git a/include/qemu/yank.h b/include/qemu/yank.h
new file mode 100644
index 00..5b93c70cbf
--- /dev/null
+++ b/include/qemu/yank.h
@@ -0,0 +1,97 @@
+/*
+ * QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef YANK_H
+#define YANK_H
+
+#include "qapi/qapi-types-yank.h"
+
+typedef void (YankFn)(void *opaque);
+
+/**
+ * yank_register_instance: Register a new instance.
+ *
+ * This registers a new instance for yanking. Must be called before any yank
+ * function is registered for this instance.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @errp: Error object.
+ *
+ * Returns true on success or false if an error occured.
+ */
+bool yank_register_instance(const YankInstance *instance, Error **errp);
+
+/**
+ * yank_unregister_instance: Unregister a instance.
+ *
+ * This unregisters a instance. Must be called only after every yank function
+ * of the instance has been unregistered.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ */
+void yank_unregister_instance(const YankInstance *instance);
+
+/**
+ * yank_register_function: Register a yank function
+ *
+ * This registers a yank function. All limitations of qmp oob commands apply
+ * to the yank function as well. See docs/devel/qapi-code-gen.txt under
+ * "An OOB-capable command handler must satisfy the following conditions".
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @func: The yank function.
+ * @opaque: Will be passed to the yank function.
+ */
+void yank_register_function(const YankInstance *instance,
+YankFn *func,
+void *opaque);
+
+/**
+ * yank_unregister_function: Unregister a yank function
+ *
+ * This unregisters a yank function.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @func: func that was passed to yank_register_function.
+ * @opaque: opaque that was passed to yank_register_function.
+ */
+void yank_unregister_function(const YankInstance *instance,
+ YankFn *func,
+ void *opaque);
+
+/**
+ * yank_generic_iochannel: Generic yank function for iochannel
+ *
+ * This is a generic yank function which will call qio_channel_shutdown on the
+ * provided QIOChannel.
+ *
+ * @opaque: QIOChannel to shutdown
+ */
+void yank_generic_iochannel(void *opaque);
+
+#define BLOCKDEV_YANK_INSTANCE(the_node_name) (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_BLOCK_NODE, \
+.u.block_node.node_name = (the_node_name) })
+
+#define CHARDEV_YANK_INSTANCE(the_id) (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_CHARDEV, \
+.u.chardev.id = (the_id) })
+
+#define MIGRATION_YANK_INSTANCE (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_MIGRATION })
+
+#endif
diff --git a/qapi/meson.build b/qapi/meson.build
index 0e98146f1f..ab68e7900e 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -47,6 +47,7 @@ qapi_all_modules = [
'trace',
'transaction',
'ui',
+ 'yank',
]
qapi_storage_daemon_modules = [
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 0b444b76d2..3441c9a9ae 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -86,6 +86,7 @@
{ 'include': 'machine.json' }
{ 'include': 'machine-target.json' }
{ 'include': 'replay.json' }
+{ 'include': 'yank.json' }
{ 'include': 'misc.json' }
{ 'include': 'misc-target.json' }
{ 'include': 'audio.json' }
diff --git a/qapi/yank.json b/qapi/yank.json
new file mode 100644
index 00..167a775594
--- /dev/null
+++ b/qapi/yank.json
@@ -0,0 +1,119 @@
+# -*- Mode: Pyt
[PATCH v13 6/7] io: Document qmp oob suitability of qio_channel_shutdown and io_shutdown
Migration and yank code assume that qio_channel_shutdown is thread
-safe and can be called from qmp oob handler. Document this after
checking the code.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Daniel P. Berrangé
---
include/io/channel.h | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/io/channel.h b/include/io/channel.h
index 4d6fe45f63..ab9ea77959 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -92,7 +92,8 @@ struct QIOChannel {
* provide additional optional features.
*
* Consult the corresponding public API docs for a description
- * of the semantics of each callback
+ * of the semantics of each callback. io_shutdown in particular
+ * must be thread-safe, terminate quickly and must not block.
*/
struct QIOChannelClass {
ObjectClass parent;
@@ -510,6 +511,8 @@ int qio_channel_close(QIOChannel *ioc,
* QIO_CHANNEL_FEATURE_SHUTDOWN prior to calling
* this method.
*
+ * This function is thread-safe, terminates quickly and does not block.
+ *
* Returns: 0 on success, -1 on error
*/
int qio_channel_shutdown(QIOChannel *ioc,
--
2.29.2
pgpj1NGKALLNT.pgp
Description: OpenPGP digital signature
[PATCH v13 5/7] io/channel-tls.c: make qio_channel_tls_shutdown thread-safe
Make qio_channel_tls_shutdown thread-safe by using atomics when
accessing tioc->shutdown.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Daniel P. Berrangé
---
io/channel-tls.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 388f019977..2ae1b92fc0 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -23,6 +23,7 @@
#include "qemu/module.h"
#include "io/channel-tls.h"
#include "trace.h"
+#include "qemu/atomic.h"
static ssize_t qio_channel_tls_write_handler(const char *buf,
@@ -277,7 +278,8 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
return QIO_CHANNEL_ERR_BLOCK;
}
} else if (errno == ECONNABORTED &&
- (tioc->shutdown & QIO_CHANNEL_SHUTDOWN_READ)) {
+ (qatomic_load_acquire(&tioc->shutdown) &
+QIO_CHANNEL_SHUTDOWN_READ)) {
return 0;
}
@@ -361,7 +363,7 @@ static int qio_channel_tls_shutdown(QIOChannel *ioc,
{
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
-tioc->shutdown |= how;
+qatomic_or(&tioc->shutdown, how);
return qio_channel_shutdown(tioc->master, how, errp);
}
--
2.29.2
pgp2PeUh5Ljll.pgp
Description: OpenPGP digital signature
[PATCH v13 7/7] tests/test-char.c: Wait for the chardev to connect in char_socket_client_dupid_test
A connecting chardev object has an additional reference by the connecting thread, so if the chardev is still connecting by the end of the test, then the chardev object won't be freed. This in turn means that the yank instance won't be unregistered and when running the next test-case yank_register_instance will abort, because the yank instance is already/still registered. Signed-off-by: Lukas Straub Reviewed-by: Daniel P. Berrangé --- tests/test-char.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-char.c b/tests/test-char.c index 953e0d1c1f..41a76410d8 100644 --- a/tests/test-char.c +++ b/tests/test-char.c @@ -937,6 +937,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque) g_assert_nonnull(opts); chr1 = qemu_chr_new_from_opts(opts, NULL, &error_abort); g_assert_nonnull(chr1); +qemu_chr_wait_connected(chr1, &error_abort); chr2 = qemu_chr_new_from_opts(opts, NULL, &local_err); g_assert_null(chr2); -- 2.29.2 pgpXI7iao5yGZ.pgp Description: OpenPGP digital signature
[PATCH v14 1/7] Introduce yank feature
The yank feature allows to recover from hanging qemu by "yanking"
at various parts. Other qemu systems can register themselves and
multiple yank functions. Then all yank functions for selected
instances can be called by the 'yank' out-of-band qmp command.
Available instances can be queried by a 'query-yank' oob command.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Markus Armbruster
---
MAINTAINERS | 7 ++
include/qemu/yank.h | 97
qapi/meson.build | 1 +
qapi/qapi-schema.json | 1 +
qapi/yank.json| 119
util/meson.build | 1 +
util/yank.c | 207 ++
7 files changed, 433 insertions(+)
create mode 100644 include/qemu/yank.h
create mode 100644 qapi/yank.json
create mode 100644 util/yank.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e7c8f0488..f465a4045a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2716,6 +2716,13 @@ F: util/uuid.c
F: include/qemu/uuid.h
F: tests/test-uuid.c
+Yank feature
+M: Lukas Straub
+S: Odd fixes
+F: util/yank.c
+F: include/qemu/yank.h
+F: qapi/yank.json
+
COLO Framework
M: zhanghailiang
S: Maintained
diff --git a/include/qemu/yank.h b/include/qemu/yank.h
new file mode 100644
index 00..5b93c70cbf
--- /dev/null
+++ b/include/qemu/yank.h
@@ -0,0 +1,97 @@
+/*
+ * QEMU yank feature
+ *
+ * Copyright (c) Lukas Straub
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef YANK_H
+#define YANK_H
+
+#include "qapi/qapi-types-yank.h"
+
+typedef void (YankFn)(void *opaque);
+
+/**
+ * yank_register_instance: Register a new instance.
+ *
+ * This registers a new instance for yanking. Must be called before any yank
+ * function is registered for this instance.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @errp: Error object.
+ *
+ * Returns true on success or false if an error occured.
+ */
+bool yank_register_instance(const YankInstance *instance, Error **errp);
+
+/**
+ * yank_unregister_instance: Unregister a instance.
+ *
+ * This unregisters a instance. Must be called only after every yank function
+ * of the instance has been unregistered.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ */
+void yank_unregister_instance(const YankInstance *instance);
+
+/**
+ * yank_register_function: Register a yank function
+ *
+ * This registers a yank function. All limitations of qmp oob commands apply
+ * to the yank function as well. See docs/devel/qapi-code-gen.txt under
+ * "An OOB-capable command handler must satisfy the following conditions".
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @func: The yank function.
+ * @opaque: Will be passed to the yank function.
+ */
+void yank_register_function(const YankInstance *instance,
+YankFn *func,
+void *opaque);
+
+/**
+ * yank_unregister_function: Unregister a yank function
+ *
+ * This unregisters a yank function.
+ *
+ * This function is thread-safe.
+ *
+ * @instance: The instance.
+ * @func: func that was passed to yank_register_function.
+ * @opaque: opaque that was passed to yank_register_function.
+ */
+void yank_unregister_function(const YankInstance *instance,
+ YankFn *func,
+ void *opaque);
+
+/**
+ * yank_generic_iochannel: Generic yank function for iochannel
+ *
+ * This is a generic yank function which will call qio_channel_shutdown on the
+ * provided QIOChannel.
+ *
+ * @opaque: QIOChannel to shutdown
+ */
+void yank_generic_iochannel(void *opaque);
+
+#define BLOCKDEV_YANK_INSTANCE(the_node_name) (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_BLOCK_NODE, \
+.u.block_node.node_name = (the_node_name) })
+
+#define CHARDEV_YANK_INSTANCE(the_id) (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_CHARDEV, \
+.u.chardev.id = (the_id) })
+
+#define MIGRATION_YANK_INSTANCE (&(YankInstance) { \
+.type = YANK_INSTANCE_TYPE_MIGRATION })
+
+#endif
diff --git a/qapi/meson.build b/qapi/meson.build
index 0e98146f1f..ab68e7900e 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -47,6 +47,7 @@ qapi_all_modules = [
'trace',
'transaction',
'ui',
+ 'yank',
]
qapi_storage_daemon_modules = [
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 0b444b76d2..3441c9a9ae 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -86,6 +86,7 @@
{ 'include': 'machine.json' }
{ 'include': 'machine-target.json' }
{ 'include': 'replay.json' }
+{ 'include': 'yank.json' }
{ 'include': 'misc.json' }
{ 'include': 'misc-target.json' }
{ 'include': 'audio.json' }
diff --git a/qapi/yank.json b/qapi/yank.json
new file mode 100644
index 00..167a775594
--- /dev/null
+++ b/qapi/yank.json
@@ -0,0 +1,119 @@
+# -*- Mode: Pyt
[PATCH v14 2/7] block/nbd.c: Add yank feature
Register a yank function which shuts down the socket and sets
s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
error occured.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
---
block/nbd.c | 153 +++-
1 file changed, 92 insertions(+), 61 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 42536702b6..0f8d17db6a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,6 +35,7 @@
#include "qemu/option.h"
#include "qemu/cutils.h"
#include "qemu/main-loop.h"
+#include "qemu/atomic.h"
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qmp/qstring.h"
@@ -44,6 +45,8 @@
#include "block/nbd.h"
#include "block/block_int.h"
+#include "qemu/yank.h"
+
#define EN_OPTSTR ":exportname="
#define MAX_NBD_REQUESTS16
@@ -141,14 +144,13 @@ typedef struct BDRVNBDState {
NBDConnectThread *connect_thread;
} BDRVNBDState;
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
- Error **errp);
-static QIOChannelSocket *nbd_co_establish_connection(BlockDriverState *bs,
- Error **errp);
+static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
+Error **errp);
+static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
bool detach);
-static int nbd_client_handshake(BlockDriverState *bs, QIOChannelSocket *sioc,
-Error **errp);
+static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
+static void nbd_yank(void *opaque);
static void nbd_clear_bdrvstate(BDRVNBDState *s)
{
@@ -166,12 +168,12 @@ static void nbd_clear_bdrvstate(BDRVNBDState *s)
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
if (ret == -EIO) {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
NBD_CLIENT_CONNECTING_NOWAIT;
}
} else {
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
s->state = NBD_CLIENT_QUIT;
@@ -204,7 +206,7 @@ static void reconnect_delay_timer_cb(void *opaque)
{
BDRVNBDState *s = opaque;
-if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) {
s->state = NBD_CLIENT_CONNECTING_NOWAIT;
while (qemu_co_enter_next(&s->free_sema, NULL)) {
/* Resume all queued requests */
@@ -216,7 +218,7 @@ static void reconnect_delay_timer_cb(void *opaque)
static void reconnect_delay_timer_init(BDRVNBDState *s, uint64_t
expire_time_ns)
{
-if (s->state != NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) != NBD_CLIENT_CONNECTING_WAIT) {
return;
}
@@ -261,7 +263,7 @@ static void nbd_client_attach_aio_context(BlockDriverState
*bs,
* s->connection_co is either yielded from nbd_receive_reply or from
* nbd_co_reconnect_loop()
*/
-if (s->state == NBD_CLIENT_CONNECTED) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED) {
qio_channel_attach_aio_context(QIO_CHANNEL(s->ioc), new_context);
}
@@ -287,7 +289,7 @@ static void coroutine_fn
nbd_client_co_drain_begin(BlockDriverState *bs)
reconnect_delay_timer_del(s);
-if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+if (qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT) {
s->state = NBD_CLIENT_CONNECTING_NOWAIT;
qemu_co_queue_restart_all(&s->free_sema);
}
@@ -338,13 +340,14 @@ static void nbd_teardown_connection(BlockDriverState *bs)
static bool nbd_client_connecting(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT ||
-s->state == NBD_CLIENT_CONNECTING_NOWAIT;
+NBDClientState state = qatomic_load_acquire(&s->state);
+return state == NBD_CLIENT_CONNECTING_WAIT ||
+state == NBD_CLIENT_CONNECTING_NOWAIT;
}
static bool nbd_client_connecting_wait(BDRVNBDState *s)
{
-return s->state == NBD_CLIENT_CONNECTING_WAIT;
+return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTING_WAIT;
}
static void connect_bh(void *opaque)
@@ -424,12 +427,12 @@ static void *connect_thread_func(void *opaque)
return NULL;
}
-static QIOChannelSocket *coroutine_fn
+static int coroutine_fn
nbd_co_establish_connection(BlockDriverState *bs, Error **errp)
{
+int ret;
QemuThread thread;
BDRVNBDState *s = bs->opaque;
-QIOChannelSocket *res;
NBDConnectThread *thr = s->connect_thread
[PATCH v14 4/7] migration: Add yank feature
Register yank functions on sockets to shut them down.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Acked-by: Dr. David Alan Gilbert
---
migration/channel.c | 13 +
migration/migration.c | 22 ++
migration/multifd.c | 10 ++
migration/qemu-file-channel.c | 7 +++
migration/savevm.c| 5 +
5 files changed, 57 insertions(+)
diff --git a/migration/channel.c b/migration/channel.c
index 8a783baa0b..35fe234e9c 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -18,6 +18,8 @@
#include "trace.h"
#include "qapi/error.h"
#include "io/channel-tls.h"
+#include "io/channel-socket.h"
+#include "qemu/yank.h"
/**
* @migration_channel_process_incoming - Create new incoming migration channel
@@ -35,6 +37,11 @@ void migration_channel_process_incoming(QIOChannel *ioc)
trace_migration_set_incoming_channel(
ioc, object_get_typename(OBJECT(ioc)));
+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function(MIGRATION_YANK_INSTANCE, yank_generic_iochannel,
+ QIO_CHANNEL(ioc));
+}
+
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
!object_dynamic_cast(OBJECT(ioc),
@@ -67,6 +74,12 @@ void migration_channel_connect(MigrationState *s,
ioc, object_get_typename(OBJECT(ioc)), hostname, error);
if (!error) {
+if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
+yank_register_function(MIGRATION_YANK_INSTANCE,
+ yank_generic_iochannel,
+ QIO_CHANNEL(ioc));
+}
+
if (s->parameters.tls_creds &&
*s->parameters.tls_creds &&
!object_dynamic_cast(OBJECT(ioc),
diff --git a/migration/migration.c b/migration/migration.c
index e0dbde4091..92f7cb70b2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -56,6 +56,7 @@
#include "net/announce.h"
#include "qemu/queue.h"
#include "multifd.h"
+#include "qemu/yank.h"
#ifdef CONFIG_VFIO
#include "hw/vfio/vfio-common.h"
@@ -254,6 +255,8 @@ void migration_incoming_state_destroy(void)
qapi_free_SocketAddressList(mis->socket_address_list);
mis->socket_address_list = NULL;
}
+
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
static void migrate_generate_event(int new_state)
@@ -418,6 +421,10 @@ static void qemu_start_incoming_migration(const char *uri,
Error **errp)
{
const char *p = NULL;
+if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+return;
+}
+
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
if (strstart(uri, "tcp:", &p) ||
strstart(uri, "unix:", NULL) ||
@@ -432,6 +439,7 @@ static void qemu_start_incoming_migration(const char *uri,
Error **errp)
} else if (strstart(uri, "fd:", &p)) {
fd_start_incoming_migration(p, errp);
} else {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
error_setg(errp, "unknown migration protocol: %s", uri);
}
}
@@ -1737,6 +1745,7 @@ static void migrate_fd_cleanup(MigrationState *s)
}
notifier_list_notify(&migration_state_notifiers, s);
block_cleanup_parameters(s);
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
static void migrate_fd_cleanup_schedule(MigrationState *s)
@@ -2011,6 +2020,7 @@ void qmp_migrate_recover(const char *uri, Error **errp)
* only re-setup the migration stream and poke existing migration
* to continue using that newly established channel.
*/
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
qemu_start_incoming_migration(uri, errp);
}
@@ -2148,6 +2158,12 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
return;
}
+if (!(has_resume && resume)) {
+if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+return;
+}
+}
+
if (strstart(uri, "tcp:", &p) ||
strstart(uri, "unix:", NULL) ||
strstart(uri, "vsock:", NULL)) {
@@ -2161,6 +2177,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
} else if (strstart(uri, "fd:", &p)) {
fd_start_outgoing_migration(s, p, &local_err);
} else {
+if (!(has_resume && resume)) {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+}
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
"a valid migration protocol");
migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
@@ -2170,6 +2189,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
if (local_err) {
+if (!(has_resume && resume)) {
+yank_unregister_instance(MIGRATION_YANK_INSTANCE);
+}
migrate_fd_error(s, local_err);
error_propagate(errp, local_err);
return;
diff --git a/migration/multifd.c b/migration/multifd
[PATCH v14 3/7] chardev/char-socket.c: Add yank feature
Register a yank function to shutdown the socket on yank.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
---
chardev/char-socket.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8dd0..8a707d766c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -34,6 +34,7 @@
#include "qapi/error.h"
#include "qapi/clone-visitor.h"
#include "qapi/qapi-visit-sockets.h"
+#include "qemu/yank.h"
#include "chardev/char-io.h"
#include "qom/object.h"
@@ -70,6 +71,7 @@ struct SocketChardev {
size_t read_msgfds_num;
int *write_msgfds;
size_t write_msgfds_num;
+bool registered_yank;
SocketAddress *addr;
bool is_listen;
@@ -415,6 +417,12 @@ static void tcp_chr_free_connection(Chardev *chr)
tcp_set_msgfds(chr, NULL, 0);
remove_fd_in_watch(chr);
+if (s->state == TCP_CHARDEV_STATE_CONNECTING
+|| s->state == TCP_CHARDEV_STATE_CONNECTED) {
+yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(s->sioc));
+}
object_unref(OBJECT(s->sioc));
s->sioc = NULL;
object_unref(OBJECT(s->ioc));
@@ -932,6 +940,9 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
}
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
ret = tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
return ret;
@@ -946,6 +957,9 @@ static void tcp_chr_accept(QIONetListener *listener,
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, cioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(cioc));
tcp_chr_new_client(chr, cioc);
}
@@ -961,6 +975,9 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error
**errp)
object_unref(OBJECT(sioc));
return -1;
}
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
return 0;
@@ -976,6 +993,9 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_net_listener_wait_client(s->listener);
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
tcp_chr_new_client(chr, sioc);
object_unref(OBJECT(sioc));
}
@@ -1086,6 +1106,9 @@ static void char_socket_finalize(Object *obj)
object_unref(OBJECT(s->tls_creds));
}
g_free(s->tls_authz);
+if (s->registered_yank) {
+yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+}
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
@@ -1101,6 +1124,9 @@ static void qemu_chr_socket_connected(QIOTask *task, void
*opaque)
if (qio_task_propagate_error(task, &err)) {
tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
+yank_unregister_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
check_report_connect_error(chr, err);
goto cleanup;
}
@@ -1134,6 +1160,9 @@ static void tcp_chr_connect_client_async(Chardev *chr)
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
sioc = qio_channel_socket_new();
tcp_chr_set_client_ioc_name(chr, sioc);
+yank_register_function(CHARDEV_YANK_INSTANCE(chr->label),
+ yank_generic_iochannel,
+ QIO_CHANNEL(sioc));
/*
* Normally code would use the qio_channel_socket_connect_async
* method which uses a QIOTask + qio_task_set_error internally
@@ -1376,6 +1405,11 @@ static void qmp_chardev_open_socket(Chardev *chr,
qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
}
+if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+return;
+}
+s->registered_yank = true;
+
/* be isn't opened until we get a connection */
*be_opened = false;
--
2.29.2
pgpv2XqGH6yVR.pgp
Description: OpenPGP digital signature
[PATCH v14 0/7] Introduce 'yank' oob qmp command to recover from hanging qemu
Hello Everyone, So here is v14. Changes: v14: -fix checkpatch.pl warning v13: -Address Marc-André Lureau comments: -make yank_register_instance return bool -rename yank_compare_instances to yank_instance_equal -remove breaks -use g_str_equal instead of strcmp -use g_new0 instead of g_slice_new -use QEMU_LOCK_GUARD instead of qemu_mutex_lock/unlock v12: -rebase onto master -minor change to migration (removal of "defer" branch in qemu_start_incoming_migration) -add Reviewed-by tags v11: -squashed MAINTAINERS update into patch 1 -move qmp doc of yank before misc -add title for qmp docs -change "Since:" to 6.0 -add Reviewed-by tags v10: -moved from qapi/misc.json to qapi/yank.json -rename 'blockdev' -> 'block-node' -document difference betwen migration yank instance and migrate_cancel -better document return values of yank command -better document yank_lock -minor style and spelling fixes v9: -rebase onto master -implemented new qmp api as proposed by Markus v8: -add Reviewed-by and Acked-by tags -rebase onto master -minor change to migration -convert to meson -change "Since:" to 5.2 -varios code style fixes (Markus Armbruster) -point to oob restrictions in comment to yank_register_function (Markus Armbruster) -improve qmp documentation (Markus Armbruster) -document oob suitability of qio_channel and io_shutdown (Markus Armbruster) v7: -yank_register_instance now returns error via Error **errp instead of aborting -dropped "chardev/char.c: Check for duplicate id before creating chardev" v6: -add Reviewed-by and Acked-by tags -rebase on master -lots of changes in nbd due to rebase -only take maintainership of util/yank.c and include/qemu/yank.h (Daniel P. Berrangé) -fix a crash discovered by the newly added chardev test -fix the test itself v5: -move yank.c to util/ -move yank.h to include/qemu/ -add license to yank.h -use const char* -nbd: use atomic_store_release and atomic_load_aqcuire -io-channel: ensure thread-safety and document it -add myself as maintainer for yank v4: -fix build errors... v3: -don't touch softmmu/vl.c, use __contructor__ attribute instead (Paolo Bonzini) -fix build errors -rewrite migration patch so it actually passes all tests v2: -don't touch io/ code anymore -always register yank functions -'yank' now takes a list of instances to yank -'query-yank' returns a list of yankable instances Overview: Hello Everyone, In many cases, if qemu has a network connection (qmp, migration, chardev, etc.) to some other server and that server dies or hangs, qemu hangs too. These patches introduce the new 'yank' out-of-band qmp command to recover from these kinds of hangs. The different subsystems register callbacks which get executed with the yank command. For example the callback can shutdown() a socket. This is intended for the colo use-case, but it can be used for other things too of course. Regards, Lukas Straub Lukas Straub (7): Introduce yank feature block/nbd.c: Add yank feature chardev/char-socket.c: Add yank feature migration: Add yank feature io/channel-tls.c: make qio_channel_tls_shutdown thread-safe io: Document qmp oob suitability of qio_channel_shutdown and io_shutdown tests/test-char.c: Wait for the chardev to connect in char_socket_client_dupid_test MAINTAINERS | 7 ++ block/nbd.c | 153 +++-- chardev/char-socket.c | 34 ++ include/io/channel.h | 5 +- include/qemu/yank.h | 97 io/channel-tls.c | 6 +- migration/channel.c | 13 +++ migration/migration.c | 22 migration/multifd.c | 10 ++ migration/qemu-file-channel.c | 7 ++ migration/savevm.c| 5 + qapi/meson.build | 1 + qapi/qapi-schema.json | 1 + qapi/yank.json| 119 +++ tests/test-char.c | 1 + util/meson.build | 1 + util/yank.c | 207 ++ 17 files changed, 625 insertions(+), 64 deletions(-) create mode 100644 include/qemu/yank.h create mode 100644 qapi/yank.json create mode 100644 util/yank.c -- 2.29.2 pgpvsHdgtMt6z.pgp Description: OpenPGP digital signature
[PATCH v14 7/7] tests/test-char.c: Wait for the chardev to connect in char_socket_client_dupid_test
A connecting chardev object has an additional reference by the connecting thread, so if the chardev is still connecting by the end of the test, then the chardev object won't be freed. This in turn means that the yank instance won't be unregistered and when running the next test-case yank_register_instance will abort, because the yank instance is already/still registered. Signed-off-by: Lukas Straub Reviewed-by: Daniel P. Berrangé --- tests/test-char.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-char.c b/tests/test-char.c index 953e0d1c1f..41a76410d8 100644 --- a/tests/test-char.c +++ b/tests/test-char.c @@ -937,6 +937,7 @@ static void char_socket_client_dupid_test(gconstpointer opaque) g_assert_nonnull(opts); chr1 = qemu_chr_new_from_opts(opts, NULL, &error_abort); g_assert_nonnull(chr1); +qemu_chr_wait_connected(chr1, &error_abort); chr2 = qemu_chr_new_from_opts(opts, NULL, &local_err); g_assert_null(chr2); -- 2.29.2 pgp20PxoeFU0o.pgp Description: OpenPGP digital signature
[PATCH v14 5/7] io/channel-tls.c: make qio_channel_tls_shutdown thread-safe
Make qio_channel_tls_shutdown thread-safe by using atomics when
accessing tioc->shutdown.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Daniel P. Berrangé
---
io/channel-tls.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 388f019977..2ae1b92fc0 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -23,6 +23,7 @@
#include "qemu/module.h"
#include "io/channel-tls.h"
#include "trace.h"
+#include "qemu/atomic.h"
static ssize_t qio_channel_tls_write_handler(const char *buf,
@@ -277,7 +278,8 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
return QIO_CHANNEL_ERR_BLOCK;
}
} else if (errno == ECONNABORTED &&
- (tioc->shutdown & QIO_CHANNEL_SHUTDOWN_READ)) {
+ (qatomic_load_acquire(&tioc->shutdown) &
+QIO_CHANNEL_SHUTDOWN_READ)) {
return 0;
}
@@ -361,7 +363,7 @@ static int qio_channel_tls_shutdown(QIOChannel *ioc,
{
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
-tioc->shutdown |= how;
+qatomic_or(&tioc->shutdown, how);
return qio_channel_shutdown(tioc->master, how, errp);
}
--
2.29.2
pgpHbozxbN5If.pgp
Description: OpenPGP digital signature
[PATCH v14 6/7] io: Document qmp oob suitability of qio_channel_shutdown and io_shutdown
Migration and yank code assume that qio_channel_shutdown is thread
-safe and can be called from qmp oob handler. Document this after
checking the code.
Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi
Reviewed-by: Daniel P. Berrangé
---
include/io/channel.h | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/io/channel.h b/include/io/channel.h
index 4d6fe45f63..ab9ea77959 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -92,7 +92,8 @@ struct QIOChannel {
* provide additional optional features.
*
* Consult the corresponding public API docs for a description
- * of the semantics of each callback
+ * of the semantics of each callback. io_shutdown in particular
+ * must be thread-safe, terminate quickly and must not block.
*/
struct QIOChannelClass {
ObjectClass parent;
@@ -510,6 +511,8 @@ int qio_channel_close(QIOChannel *ioc,
* QIO_CHANNEL_FEATURE_SHUTDOWN prior to calling
* this method.
*
+ * This function is thread-safe, terminates quickly and does not block.
+ *
* Returns: 0 on success, -1 on error
*/
int qio_channel_shutdown(QIOChannel *ioc,
--
2.29.2
pgp0WZzoC_GZj.pgp
Description: OpenPGP digital signature
[PATCH] meson: fix Cocoa option in summary
From: Chris Hofstaedtler
Regression introduced in f9332757898.
Signed-off-by: Chris Hofstaedtler
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index e864cdd155..9c152a85bd 100644
--- a/meson.build
+++ b/meson.build
@@ -2112,7 +2112,7 @@ summary_info += {'strip binaries':get_option('strip')}
summary_info += {'profiler': config_host.has_key('CONFIG_PROFILER')}
summary_info += {'static build': config_host.has_key('CONFIG_STATIC')}
if targetos == 'darwin'
- summary_info += {'Cocoa support': config_host.has_key('CONFIG_COCOA')}
+ summary_info += {'Cocoa support': config_host_data.get('CONFIG_COCOA',
false)}
endif
# TODO: add back version
summary_info += {'SDL support': sdl.found()}
--
2.29.2
[PATCH] meson: fix ncurses detection on macOS
Without this, meson fails with "curses package not usable"
when using ncurses 6.2. Apparently the wide functions
(addwstr, etc) are hidden behind the extra define, and
meson does not define it at that detection stage.
Signed-off-by: Chris Hofstaedtler
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 9c152a85bd..7b9d92c14a 100644
--- a/meson.build
+++ b/meson.build
@@ -510,7 +510,7 @@ if have_system and not get_option('curses').disabled()
endforeach
msg = get_option('curses').enabled() ? 'curses library not found' : ''
if curses.found()
-if cc.links(curses_test, dependencies: [curses])
+if cc.links(curses_test, args: '-DNCURSES_WIDECHAR', dependencies:
[curses])
curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
dependencies: [curses])
else
msg = 'curses package not usable'
--
2.29.2
Re: [PATCH 0/2] bsd-user, FreeBSD update
On Mon, Dec 28, 2020 at 1:15 AM David CARLIER wrote: > From 10b13162949debdbbd8394bc1047511d1a900176 Mon Sep 17 00:00:00 2001 > From: David Carlier > Date: Mon, 28 Dec 2020 08:10:43 + > Subject: [PATCH 0/2] *** SUBJECT HERE *** > > bsd-user, FreeBSD update. > > David Carlier (2): > bsd-user, updating the FreeBSD's syscall list, based on the 11.x > bsd-user, Adding more strace support for a handful of syscalls. > > bsd-user/freebsd/strace.list | 12 > bsd-user/freebsd/syscall_nr.h | 25 ++--- > 2 files changed, 34 insertions(+), 3 deletions(-) > Have you seen my patches in this area? Are you familiar with the bsd-user efforts we've been doing at https://github.com/qemu-bsd-user/qemu-bsd-user We have about 300 patches in the queue and the more that others change things, the harder it is to get them in. They are a twisty maze of conflicts early in the series and some not-updated API calls dealing with the evolution of the qemu cpu model as well. I posted this series two weeks ago: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05528.html This part https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05530.html updates the system call numbers to the latest FreeBSD 13 numbers. Perhaps you could help in these efforts? They have been going on since around Qemu 1.0 and we were bad about getting them upstreamed early, and so are paying the price now. We use the code in the above repo to build about 40k packages for a couple of different architectures. Warner > -- > 2.30.0.rc2 > >
Re: [PATCH 0/2] bsd-user, FreeBSD update
Oh good to know I understand better why syscalls not updated for so long. To upstream I would suggest not to push straight all these changes in one shot and focus on FreeBSD at first, much less to review and so on. Regards. On Mon, 28 Dec 2020 at 16:21, Warner Losh wrote: > > > > On Mon, Dec 28, 2020 at 1:15 AM David CARLIER wrote: >> >> From 10b13162949debdbbd8394bc1047511d1a900176 Mon Sep 17 00:00:00 2001 >> From: David Carlier >> Date: Mon, 28 Dec 2020 08:10:43 + >> Subject: [PATCH 0/2] *** SUBJECT HERE *** >> >> bsd-user, FreeBSD update. >> >> David Carlier (2): >> bsd-user, updating the FreeBSD's syscall list, based on the 11.x >> bsd-user, Adding more strace support for a handful of syscalls. >> >> bsd-user/freebsd/strace.list | 12 >> bsd-user/freebsd/syscall_nr.h | 25 ++--- >> 2 files changed, 34 insertions(+), 3 deletions(-) > > > Have you seen my patches in this area? Are you familiar with the bsd-user > efforts we've been doing at https://github.com/qemu-bsd-user/qemu-bsd-user We > have about 300 patches in the queue and the more that others change things, > the harder it is to get them in. They are a twisty maze of conflicts early in > the series and some not-updated API calls dealing with the evolution of the > qemu cpu model as well. > > I posted this series two weeks ago: > https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05528.html > > This part https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05530.html > updates the system call numbers to the latest FreeBSD 13 numbers. > > Perhaps you could help in these efforts? They have been going on since around > Qemu 1.0 and we were bad about getting them upstreamed early, and so are > paying the price now. We use the code in the above repo to build about 40k > packages for a couple of different architectures. > > Warner > >> >> -- >> 2.30.0.rc2 >>
Re: [PATCH 0/2] bsd-user, FreeBSD update
Yes. I've picked 4 changes to make sure that I've got the size and groupings of patches right for this project. I've heard nothing back on them, so I'll try again after the first of the year. I'd thought about just removing it all and pushing up the current state, but I think even that might be unreviewable. Warner On Mon, Dec 28, 2020 at 9:39 AM David CARLIER wrote: > Oh good to know I understand better why syscalls not updated for so long. > > To upstream I would suggest not to push straight all these changes in > one shot and focus on FreeBSD at first, much less to review and so on. > > Regards. > > On Mon, 28 Dec 2020 at 16:21, Warner Losh wrote: > > > > > > > > On Mon, Dec 28, 2020 at 1:15 AM David CARLIER > wrote: > >> > >> From 10b13162949debdbbd8394bc1047511d1a900176 Mon Sep 17 00:00:00 2001 > >> From: David Carlier > >> Date: Mon, 28 Dec 2020 08:10:43 + > >> Subject: [PATCH 0/2] *** SUBJECT HERE *** > >> > >> bsd-user, FreeBSD update. > >> > >> David Carlier (2): > >> bsd-user, updating the FreeBSD's syscall list, based on the 11.x > >> bsd-user, Adding more strace support for a handful of syscalls. > >> > >> bsd-user/freebsd/strace.list | 12 > >> bsd-user/freebsd/syscall_nr.h | 25 ++--- > >> 2 files changed, 34 insertions(+), 3 deletions(-) > > > > > > Have you seen my patches in this area? Are you familiar with the > bsd-user efforts we've been doing at > https://github.com/qemu-bsd-user/qemu-bsd-user We have about 300 patches > in the queue and the more that others change things, the harder it is to > get them in. They are a twisty maze of conflicts early in the series and > some not-updated API calls dealing with the evolution of the qemu cpu model > as well. > > > > I posted this series two weeks ago: > https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05528.html > > > > This part > https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg05530.html > updates the system call numbers to the latest FreeBSD 13 numbers. > > > > Perhaps you could help in these efforts? They have been going on since > around Qemu 1.0 and we were bad about getting them upstreamed early, and so > are paying the price now. We use the code in the above repo to build about > 40k packages for a couple of different architectures. > > > > Warner > > > >> > >> -- > >> 2.30.0.rc2 > >> >
Re: [PATCH] meson: fix Cocoa option in summary
Hi Chris,
On 12/28/20 4:09 PM, Chris Hofstaedtler wrote:
> From: Chris Hofstaedtler
>
> Regression introduced in f9332757898.
Isn't it commit b4e312e953b? If so you could add:
Fixes: b4e312e953b ("configure: move cocoa option to Meson")
> Signed-off-by: Chris Hofstaedtler
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index e864cdd155..9c152a85bd 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2112,7 +2112,7 @@ summary_info += {'strip binaries':
> get_option('strip')}
> summary_info += {'profiler': config_host.has_key('CONFIG_PROFILER')}
> summary_info += {'static build': config_host.has_key('CONFIG_STATIC')}
> if targetos == 'darwin'
> - summary_info += {'Cocoa support': config_host.has_key('CONFIG_COCOA')}
> + summary_info += {'Cocoa support': config_host_data.get('CONFIG_COCOA',
> false)}
> endif
> # TODO: add back version
> summary_info += {'SDL support': sdl.found()}
>
Re: [PATCH] meson: fix ncurses detection on macOS
On 12/28/20 4:16 PM, Chris Hofstaedtler wrote:
> Without this, meson fails with "curses package not usable"
> when using ncurses 6.2. Apparently the wide functions
> (addwstr, etc) are hidden behind the extra define, and
> meson does not define it at that detection stage.
Seems reasonable, but still Cc'ing more developers.
>
> Signed-off-by: Chris Hofstaedtler
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 9c152a85bd..7b9d92c14a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -510,7 +510,7 @@ if have_system and not get_option('curses').disabled()
>endforeach
>msg = get_option('curses').enabled() ? 'curses library not found' : ''
>if curses.found()
> -if cc.links(curses_test, dependencies: [curses])
> +if cc.links(curses_test, args: '-DNCURSES_WIDECHAR', dependencies:
> [curses])
>curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
> dependencies: [curses])
> else
>msg = 'curses package not usable'
>
Re: [PATCH] meson: fix ncurses detection on macOS
Philippe Mathieu-Daudé, le lun. 28 déc. 2020 18:20:13 +0100, a ecrit:
> On 12/28/20 4:16 PM, Chris Hofstaedtler wrote:
> > Without this, meson fails with "curses package not usable"
> > when using ncurses 6.2. Apparently the wide functions
> > (addwstr, etc) are hidden behind the extra define, and
> > meson does not define it at that detection stage.
>
> Seems reasonable, but still Cc'ing more developers.
That looks sensible indeed.
> > Signed-off-by: Chris Hofstaedtler
> > ---
> > meson.build | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 9c152a85bd..7b9d92c14a 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -510,7 +510,7 @@ if have_system and not get_option('curses').disabled()
> >endforeach
> >msg = get_option('curses').enabled() ? 'curses library not found' : ''
> >if curses.found()
> > -if cc.links(curses_test, dependencies: [curses])
> > +if cc.links(curses_test, args: '-DNCURSES_WIDECHAR', dependencies:
> > [curses])
> >curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
> > dependencies: [curses])
> > else
> >msg = 'curses package not usable'
Re: [PATCH v3] gitlab-ci.yml: Add openSUSE Leap 15.2 for gitlab CI/CD
Hi, On 12/24/20 5:59 AM, Cho, Yu-Chen wrote: Add build-system-opensuse jobs and opensuse-leap.docker dockerfile. Use openSUSE Leap 15.2 container image in the gitlab-CI. Signed-off-by: Cho, Yu-Chen --- v3: Drop the "acceptance-system-opensuse" job part of the patch for now to get at least the basic compile-coverage v2: Drop some package from dockerfile to make docker image more light. v1: Add build-system-opensuse jobs and opensuse-leap.docker dockerfile. Use openSUSE Leap 15.2 container image in the gitlab-CI. --- .gitlab-ci.d/containers.yml | 5 ++ .gitlab-ci.yml| 20 +++ tests/docker/dockerfiles/opensuse-leap.docker | 54 +++ 3 files changed, 79 insertions(+) create mode 100644 tests/docker/dockerfiles/opensuse-leap.docker On Gitlab CI this new docker file has no issues: https://gitlab.com/wainersm/qemu/-/jobs/934243313 One test won't execute due to lack of hostname program: https://gitlab.com/wainersm/qemu/-/jobs/934243313#L3698 Using it locally has some issues though. I can build the image as ... $ make docker-image-opensuse-leap ... but I cannot run the test-build script as ... $ make docker-test-build@opensuse-leap .. and the reason is that it misses the tar program which is used to untar the QEMU sources inside the container. Ensuring that tar is installed wasn't enough either, I had to adjust the path to python (/usr/bin/python3.8 doesn't exist). So I did change: diff --git a/tests/docker/dockerfiles/opensuse-leap.docker b/tests/docker/dockerfiles/opensuse-leap.docker index 8b0d915bff..0e64893e4a 100644 --- a/tests/docker/dockerfiles/opensuse-leap.docker +++ b/tests/docker/dockerfiles/opensuse-leap.docker @@ -43,12 +43,13 @@ ENV PACKAGES \ libspice-server-devel \ systemd-devel \ systemtap-sdt-devel \ + tar \ usbredir-devel \ virglrenderer-devel \ xen-devel \ vte-devel \ zlib-devel -ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3.8 +ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3.6 RUN zypper update -y && zypper --non-interactive install -y $PACKAGES RUN rpm -q $PACKAGES | sort > /packages.txt diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml index 892ca8d838..910754a699 100644 --- a/.gitlab-ci.d/containers.yml +++ b/.gitlab-ci.d/containers.yml @@ -246,3 +246,8 @@ amd64-ubuntu-container: <<: *container_job_definition variables: NAME: ubuntu + +amd64-opensuse-leap-container: + <<: *container_job_definition + variables: +NAME: opensuse-leap diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 98bff03b47..a1df981c9a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -195,6 +195,26 @@ acceptance-system-centos: MAKE_CHECK_ARGS: check-acceptance <<: *acceptance_definition +build-system-opensuse: + <<: *native_build_job_definition + variables: +IMAGE: opensuse-leap +TARGETS: s390x-softmmu x86_64-softmmu aarch64-softmmu +MAKE_CHECK_ARGS: check-build + artifacts: +expire_in: 2 days +paths: + - build + +check-system-opensuse: + <<: *native_test_job_definition + needs: +- job: build-system-opensuse + artifacts: true + variables: +IMAGE: opensuse-leap +MAKE_CHECK_ARGS: check + build-disabled: <<: *native_build_job_definition variables: diff --git a/tests/docker/dockerfiles/opensuse-leap.docker b/tests/docker/dockerfiles/opensuse-leap.docker new file mode 100644 index 00..8b0d915bff --- /dev/null +++ b/tests/docker/dockerfiles/opensuse-leap.docker @@ -0,0 +1,54 @@ +FROM opensuse/leap:15.2 + +# Please keep this list sorted alphabetically The list of packages below isn't sorted. Thanks for contributing this! - Wainer +ENV PACKAGES \ +bc \ +brlapi-devel \ +bzip2 \ +cyrus-sasl-devel \ +gcc \ +gcc-c++ \ +mkisofs \ +gettext-runtime \ +git \ +glib2-devel \ +glusterfs-devel \ +libgnutls-devel \ +gtk3-devel \ +libaio-devel \ +libattr-devel \ +libcap-ng-devel \ +libepoxy-devel \ +libfdt-devel \ +libiscsi-devel \ +libjpeg8-devel \ +libpmem-devel \ +libpng16-devel \ +librbd-devel \ +libseccomp-devel \ +libssh-devel \ +lzo-devel \ +make \ +libSDL2_image-devel \ +ncurses-devel \ +ninja \ +libnuma-devel \ +perl \ +libpixman-1-0-devel \ +python3-base \ +python3-virtualenv \ +rdma-core-devel \ +libSDL2-devel \ +snappy-devel \ +libspice-server-devel \ +systemd-devel \ +systemtap-sdt-devel \ +usbredir-devel \ +virglrenderer-devel \ +xen-devel \ +vte-devel \ +zlib-devel +ENV QEMU_CONFIGURE_OPTS --python=/usr/bin/python3.8 + +RUN zypper update -y && zypper --non-interactive install -y $PACKAGES +RUN rpm -q $PACKAGES | sort > /packages.txt
Question: How to change backing file ?
Hello everyone, in a class project, i would like to change the backing file of the current image opened with a particular path file. I try differents functions i saw in the source code - bdrv_change_backing_file - bdrv_open - bdrv_open_child but no one work... from segmentation fault error to bdrv_attach_backing passing through parent->blocking_error, i don't know how to achieve what i want. -- *Kevin Nguetchouang.*
Re: Problems with irq mapping in qemu v5.2
On 24/12/2020 08:11, BALATON Zoltan via wrote: On Wed, 23 Dec 2020, Guenter Roeck wrote: On Thu, Dec 24, 2020 at 02:34:07AM +0100, BALATON Zoltan wrote: [ ... ] If we need legacy mode then we may be able to emulate that by setting BARs to legacy ports ignoring what values are written to them if legacy mode config is set (which may be what the real chip does) and we already have IRQs hard wired to legacy values so that would give us legacy and half-native mode which is enough for both fuloong2e and pegasos2 but I'm not sure how can we fix BARs in QEMU because that's also handled by generic PCI code which I also don't want to break. The code below works for booting Linux while at the same time not affecting any other emulation. I don't claim it to be a perfect fix, and overloading the existing property is a bit hackish, but it does work. Yes, maybe combining it with my original patch 1 to change secondary to flags to make it a bit cleaner would work for me. Then we would either only emulate legacy or half-native mode which is sufficient for these two machines we have. If Mark or others do not object it this time, I can update my patch and resubmit with this one to fix this issue, otherwise let's wait what idea do they have because I hate to spend time with something only to be discarded again. I think we don't need more complete emulation of this chip than this for now but if somebody wants to attempt that I don't mind as long as it does not break pegasos2. I had a play with your patches this afternoon, and spent some time performing some experiments and also reading various PCI bus master specifications and datasheets: this helped me understand a lot more about the theory of IRQ routing and compatible vs. legacy mode. From reading all the documentation (including the VIA and other datasheets) I cannot find any reference to a half-native mode which makes me think something else is wrong here. At the simplest level it could simply be that the VIA doesn't tri-state its legacy IRQ lines whilst the device is in native mode (the SI controller has an option for this), or it could indicate there is a PCI IRQ routing problem somewhere else that hasn't been picked up yet. All of the datasheets suggest that legacy vs. native mode is selected by setting the correct bits in PCI_CLASS_PROG, and Linux reads this byte and configures itself to use legacy or native mode accordingly. Since the current default for the VIA is 0x8a then it should default to legacy mode, but we're immediately hitting some issues here: I've summarised my notes below for those interested. 1) PCI bus reset loses the default BAR addresses The first problem we find is that the initialisation of the PCI bus erases the default BAR addresses: that's to say lines 133-137 in hw/ide/via.c will in effect do nothing: 133 pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x01f0); 134 pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x03f4); 135 pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0170); 136 pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0374); 137 pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0xcc01); /* BMIBA: 20-23h */ The lifecycle of the VIA IDE device goes like this: init() -> realize() -> reset() but then the PCI bus reset in pci_do_device_reset() immediately wipes the BAR addresses. This is why the legacy IDE ports currently don't appear at startup. Note I do see that other devices do try this e.g. gt64120_pci_realize() so it's an easy mistake to make. 2) -kernel doesn't initialise the VIA device If you take a look at the PMON source it is possible to see that the firmware explicitly sets the PCI_CLASS_PROG to compatibility mode and disables the native PCI interrupt (https://github.com/loongson-community/pmon-2ef/blob/master/sys/dev/pci/vt82c686.c#L82). Since Linux reads this byte on startup then this is why the kernel switches to compatibility mode by default. However the point here is that booting a kernel directly without firmware means the VIA IDE device isn't initialised as it would be in real life, and that's why there are attempts to pre-configure the device accordingly in via_ide_realize()/via_ide_reset(). 3) QEMU doesn't (easily) enable a BAR to be disabled The ideal situation would be for QEMU's VIA IDE device to check PCI_CLASS_PROG and configure itself dynamically: with PCI_CLASS_PROG set for legacy mode by default, the device can disable its BARs until they are explicitly enabled. According to the PCI bus master specification the recommended behaviour for a device in compatible mode is to ignore all writes to the BARs, and for all BAR reads to return 0. This fits nicely with Guenter's finding that the BMDMA BAR should not return a value in order for Linux to boot correctly in legacy mode. Unfortunately there is no existing functionality for this in QEMU which means you would have to do this manually by overriding the PCI conf
Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode
On 27/12/2020 22:13, BALATON Zoltan wrote:
We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for further flags if needed in
the future. This patch changes the "secondary" field to "flags" and
change CMD646 and its users accordingly and define a new flag for
forcing legacy mode that will be used by via-ide for now.
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
---
v2: Fixed typo in commit message
hw/ide/cmd646.c | 4 ++--
hw/sparc64/sun4u.c | 2 +-
include/hw/ide/pci.h | 7 ++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c254631485..7a96016116 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error
**errp)
pci_conf[PCI_CLASS_PROG] = 0x8f;
pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
Doesn't the existing comment above cause checkpatch to fail?
-if (d->secondary) {
+if (d->flags & BIT(PCI_IDE_SECONDARY)) {
/* XXX: if not enabled, really disable the seconday IDE controller */
pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
}
@@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
}
static Property cmd646_ide_properties[] = {
-DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0fa13a7330..c46baa9f48 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
}
pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
-qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
+qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
pci_ide_create_devs(pci_dev);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..75d1a32f6d 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -42,6 +42,11 @@ typedef struct BMDMAState {
#define TYPE_PCI_IDE "pci-ide"
OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
+enum {
+PCI_IDE_SECONDARY, /* used only for cmd646 */
+PCI_IDE_LEGACY_MODE
+};
+
struct PCIIDEState {
/*< private >*/
PCIDevice parent_obj;
@@ -49,7 +54,7 @@ struct PCIIDEState {
IDEBus bus[2];
BMDMAState bmdma[2];
-uint32_t secondary; /* used only for cmd646 */
+uint32_t flags;
MemoryRegion bmdma_bar;
MemoryRegion cmd_bar[2];
MemoryRegion data_bar[2];
Other than that I think this looks okay.
ATB,
Mark.
[Bug 1908551] Re: aarch64 SVE emulation breaks strnlen and strrchr
** Changed in: qemu
Status: New => Confirmed
** Changed in: qemu
Assignee: (unassigned) => Richard Henderson (rth)
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1908551
Title:
aarch64 SVE emulation breaks strnlen and strrchr
Status in QEMU:
Confirmed
Bug description:
arm optimized-routines have sve string functions with test code.
the test worked up until recently: with qemu-5.2.0 i see
$ qemu-aarch64 build/bin/test/strnlen
PASS strnlen
PASS __strnlen_aarch64
__strnlen_aarch64_sve (0x490fa0, 32) len 32 returned 64, expected 32
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80"
__strnlen_aarch64_sve (0x490fa0, 32) len 33 returned 64, expected 32
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80a"
__strnlen_aarch64_sve (0x490fa0, 33) len 33 returned 64, expected 33
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80a"
__strnlen_aarch64_sve (0x490fa0, 32) len 34 returned 64, expected 32
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80ab"
__strnlen_aarch64_sve (0x490fa0, 33) len 34 returned 64, expected 33
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80ab"
__strnlen_aarch64_sve (0x490fa0, 34) len 34 returned 64, expected 34
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80ab"
__strnlen_aarch64_sve (0x490fa0, 32) len 35 returned 64, expected 32
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80a\x00c"
__strnlen_aarch64_sve (0x490fa0, 33) len 35 returned 64, expected 33
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80ab\x00"
__strnlen_aarch64_sve (0x490fa0, 34) len 35 returned 64, expected 34
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80abc"
__strnlen_aarch64_sve (0x490fa0, 35) len 35 returned 64, expected 35
input: "abcdefghijklmnopqrstuvwxyz\{|}~\x7f\x80abc"
FAIL __strnlen_aarch64_sve
however the test passes with
qemu-aarch64 -cpu max,sve-max-vq=2
there should be nothing vector length specific in the code.
i haven't debugged it further, to reproduce the issue clone
https://github.com/ARM-software/optimized-routines
and run 'make build/bin/test/strnlen' with a config.mk like
SUBS = string
ARCH = aarch64
CROSS_COMPILE = aarch64-none-linux-gnu-
CC = $(CROSS_COMPILE)gcc
CFLAGS = -std=c99 -pipe -O3
CFLAGS += -march=armv8.2-a+sve
EMULATOR = qemu-aarch64
(native compilation works too, and you can run 'make check' to
run all string tests) this will build a static linked executable
into build/bin/test. if you want a smaller test case edit
string/test/strnlen.c
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1908551/+subscriptions
Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support
On 27/12/2020 22:13, BALATON Zoltan via wrote:
From: Guenter Roeck
The IDE legacy mode emulation has been removed in commit 4ea98d317eb
("ide/via: Implement and use native PCI IDE mode") but some Linux
kernels (probably including def_config) require legacy mode on the
Fuloong2e so only emulating native mode did not turn out feasible.
Add property to via-ide model to make the mode configurable, and set
legacy mode for Fuloong2e.
Signed-off-by: Guenter Roeck
[balaton: Use bit in flags for property, add comment for missing BAR4]
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Guenter Roeck
---
v2: Reworded commit message
hw/ide/via.c| 19 +--
hw/mips/fuloong2e.c | 4 +++-
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b33..7d54d7e829 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
#include "sysemu/dma.h"
@@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
&d->bus[1], "via-ide1-cmd", 4);
pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
-bmdma_setup_bar(d);
-pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
+/* Missing BAR4 will make Linux driver fall back to legacy PIO mode */
+bmdma_setup_bar(d);
+pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+}
Since the default value of the legacy mode parameter is false, then this means the
device assumes native mode by default. Therefore PCI_CLASS_PROG should be set to 0x8f
unless legacy mode is being used, in which case it should be 0x8a.
qdev_init_gpio_in(ds, via_ide_set_irq, 2);
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
+ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
+i ? 0x376 : 0x3f6);
+}
You could actually remove the if() here: PCI configuration always leaves a gap at the
lower end of IO address space to avoid clashes with legacy ports. Therefore if a
guest decides to switch to native mode and configure the BAR, it will never clash
with the default legacy IDE ports. This has the advantage of minimising the parts
protected by PCI_IDE_LEGACY_MODE whilst also providing the legacy ports if someone
can devise a method to dynamically switch between compatible and native modes later.
ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
@@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
}
}
+static Property via_ide_properties[] = {
+DEFINE_PROP_BIT("legacy_mode", PCIIDEState, flags, PCI_IDE_LEGACY_MODE,
+false),
The convention for new qdev/QOM properties is that they should use hyphens instead of
underscores (see the comment underneath object_property_try_add() at
https://qemu.readthedocs.io/en/latest/devel/qom.html).
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void via_ide_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -223,6 +237,7 @@ static void via_ide_class_init(ObjectClass *klass, void
*data)
k->device_id = PCI_DEVICE_ID_VIA_IDE;
k->revision = 0x06;
k->class_id = PCI_CLASS_STORAGE_IDE;
+device_class_set_props(dc, via_ide_properties);
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
}
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 45c596f4fe..f0733e87b7 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -253,7 +253,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
/* Super I/O */
isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
+qdev_prop_set_bit(&dev->qdev, "legacy_mode", true);
+pci_realize_and_unref(dev, pci_bus, &error_fatal);
pci_ide_create_devs(dev);
pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
ATB,
Mark.
Re: [PATCH] meson: fix ncurses detection on macOS
On Mon, Dec 28, 2020 at 11:51 PM Chris Hofstaedtler
wrote:
>
> Without this, meson fails with "curses package not usable"
> when using ncurses 6.2. Apparently the wide functions
> (addwstr, etc) are hidden behind the extra define, and
> meson does not define it at that detection stage.
>
> Signed-off-by: Chris Hofstaedtler
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 9c152a85bd..7b9d92c14a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -510,7 +510,7 @@ if have_system and not get_option('curses').disabled()
>endforeach
>msg = get_option('curses').enabled() ? 'curses library not found' : ''
>if curses.found()
> -if cc.links(curses_test, dependencies: [curses])
> +if cc.links(curses_test, args: '-DNCURSES_WIDECHAR', dependencies:
[curses])
>curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
dependencies: [curses])
> else
>msg = 'curses package not usable'
> --
> 2.29.2
>
>
Better to share curses_compile_args
--- a/meson.build
+++ b/meson.build
@@ -504,16 +504,16 @@ if have_system and not get_option('curses').disabled()
endif
endforeach
msg = get_option('curses').enabled() ? 'curses library not found' : ''
+ curses_compile_args = ['-DNCURSES_WIDECHAR']
if curses.found()
-if cc.links(curses_test, dependencies: [curses])
- curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
dependencies: [curses])
+if cc.links(curses_test, args: curses_compile_args, dependencies:
[curses])
+ curses = declare_dependency(compile_args: curses_compile_args,
dependencies: [curses])
else
msg = 'curses package not usable'
curses = not_found
endif
endif
if not curses.found()
-curses_compile_args = ['-DNCURSES_WIDECHAR']
has_curses_h = cc.has_header('curses.h', args: curses_compile_args)
if targetos != 'windows' and not has_curses_h
message('Trying with /usr/include/ncursesw')
--
2.29.2.windows.3
--
此致
礼
罗勇刚
Yours
sincerely,
Yonggang Luo
Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode
On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
On 27/12/2020 22:13, BALATON Zoltan wrote:
We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for further flags if needed in
the future. This patch changes the "secondary" field to "flags" and
change CMD646 and its users accordingly and define a new flag for
forcing legacy mode that will be used by via-ide for now.
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
---
v2: Fixed typo in commit message
hw/ide/cmd646.c | 4 ++--
hw/sparc64/sun4u.c | 2 +-
include/hw/ide/pci.h | 7 ++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c254631485..7a96016116 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev,
Error **errp)
pci_conf[PCI_CLASS_PROG] = 0x8f;
pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
Doesn't the existing comment above cause checkpatch to fail?
It didn't (nether when I ran it nor in patchew) but I can fix it if I send
a new version.
Regards,
BALATON Zoltan
-if (d->secondary) {
+if (d->flags & BIT(PCI_IDE_SECONDARY)) {
/* XXX: if not enabled, really disable the seconday IDE
controller */
pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
}
@@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
}
static Property cmd646_ide_properties[] = {
-DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY,
false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0fa13a7330..c46baa9f48 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion
*address_space_mem,
}
pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
-qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
+qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
pci_ide_create_devs(pci_dev);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..75d1a32f6d 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -42,6 +42,11 @@ typedef struct BMDMAState {
#define TYPE_PCI_IDE "pci-ide"
OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
+enum {
+PCI_IDE_SECONDARY, /* used only for cmd646 */
+PCI_IDE_LEGACY_MODE
+};
+
struct PCIIDEState {
/*< private >*/
PCIDevice parent_obj;
@@ -49,7 +54,7 @@ struct PCIIDEState {
IDEBus bus[2];
BMDMAState bmdma[2];
-uint32_t secondary; /* used only for cmd646 */
+uint32_t flags;
MemoryRegion bmdma_bar;
MemoryRegion cmd_bar[2];
MemoryRegion data_bar[2];
Other than that I think this looks okay.
ATB,
Mark.
RFC: Start-time Device Addition for BMC devices
Hi; Currently, devices for a BMC are specified in the board init method for that machine, see: - https://github.com/qemu/qemu/blob/b785d25e91718a660546a6550f64b3c543af7754/hw/arm/aspeed.c#L414 This requires listing all i2c devices, and setting some properties. QMP can be used to set the properties for those devices at run-time already. We're looking at developing many BMC boards, but for most boards, the i2c devices that correspond to PCIe devices vary by configuration -- which PCI cards are plugged into which slots. We'd like to not hard-code those i2c devices and have variations of the same device board. For instance: board-bmc + two cards (slot 1, slot 2) board-bmc + one card (slot 3) The slot to i2c bus mapping is beyond the scope of this, because that isn't something that technically changes*. To solve this problem, we'd like to add an optional device json configuration parameter. Using QMP (which I believe supports adding devices of this type in the schema), the file would be parsed after the board_init method and additional devices then loaded. Are there any immediate problems with this kind of approach, or possibly it's already supported in some way? Thanks, Patrick *The i2c bus numbering can be altered, but typically a BMC's device-tree uses aliases to ensure consistency of i2c-bus numbering between configurations.
Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support
On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
On 27/12/2020 22:13, BALATON Zoltan via wrote:
From: Guenter Roeck
The IDE legacy mode emulation has been removed in commit 4ea98d317eb
("ide/via: Implement and use native PCI IDE mode") but some Linux
kernels (probably including def_config) require legacy mode on the
Fuloong2e so only emulating native mode did not turn out feasible.
Add property to via-ide model to make the mode configurable, and set
legacy mode for Fuloong2e.
Signed-off-by: Guenter Roeck
[balaton: Use bit in flags for property, add comment for missing BAR4]
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Guenter Roeck
---
v2: Reworded commit message
hw/ide/via.c| 19 +--
hw/mips/fuloong2e.c | 4 +++-
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b33..7d54d7e829 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
#include "sysemu/dma.h"
@@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error
**errp)
&d->bus[1], "via-ide1-cmd", 4);
pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
-bmdma_setup_bar(d);
-pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
+/* Missing BAR4 will make Linux driver fall back to legacy PIO
mode */
+bmdma_setup_bar(d);
+pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO,
&d->bmdma_bar);
+}
Since the default value of the legacy mode parameter is false, then this
means the device assumes native mode by default. Therefore PCI_CLASS_PROG
should be set to 0x8f unless legacy mode is being used, in which case it
should be 0x8a.
I think this casued problems before because if it's not set to 0x8a
(legacy) at start then guests may assume it's already switched to native
mode by firmware and won't program the BARs and it will not work. This
way, even if it looks odd all guests I've tested work so I don't want to
touch this, because I don't want to test all guests again.
Keep in mind we're not fully emulating this device so not every
combination that may work on real hardware work in this model. We really
either only emulate "half-native" mode (i.e. native mode with ISA IRQs)
that's needed for pegasos2 guests or now again only emulate legacy mode if
property is set for Linux on fuloong2e. (My original patch emulated
half-native and native mode instead of legacy thinking that Linux on
fuloong2e would adapt.) All other combinations, including switching
between these two is expected to not work which is due to QEMU limitations
as you've also discovered now. I think this is still an improvement over
only emulating legacy mode before even if it does not yet fully model the
chip and I've also cleaned up PCI IDE emulation during implementing this
so that code can be shared between via-ide, sii3112 and CMD646 without
much duplication.
qdev_init_gpio_in(ds, via_ide_set_irq, 2);
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
+ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
+i ? 0x376 : 0x3f6);
+}
You could actually remove the if() here: PCI configuration always leaves a
gap at the lower end of IO address space to avoid clashes with legacy ports.
Therefore if a guest decides to switch to native mode and configure the BAR,
it will never clash with the default legacy IDE ports. This has the advantage
of minimising the parts protected by PCI_IDE_LEGACY_MODE whilst also
providing the legacy ports if someone can devise a method to dynamically
switch between compatible and native modes later.
I think leaving the legacy ports enabled is a bad idea for at least two
reasons: 1) It may clash with other io ports on other machines, e.g. I'm
not sure on PPC where firmware or OS does not expect to see legacy ISA
ports won't map some io BAR of a PCI card there. 2) If this is left
enabled there would be two ports poking the same registers so if that does
not cause a problem by itself, writing to one accidentally (like when
something is mapped over it) could cause corruption of IDE state so I
think it's much better to protect this than later trying to debug such
problems. (You can't get rid of the flag without implementing mode
switching that needs rewrite of ISA and PCI emulation in QEMU so just get
over it.)
ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
@@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
}
}
+static Property via_ide_properties[] = {
+D
Re: Problems with irq mapping in qemu v5.2
On Mon, 28 Dec 2020, Mark Cave-Ayland wrote: On 24/12/2020 08:11, BALATON Zoltan via wrote: On Wed, 23 Dec 2020, Guenter Roeck wrote: On Thu, Dec 24, 2020 at 02:34:07AM +0100, BALATON Zoltan wrote: [ ... ] If we need legacy mode then we may be able to emulate that by setting BARs to legacy ports ignoring what values are written to them if legacy mode config is set (which may be what the real chip does) and we already have IRQs hard wired to legacy values so that would give us legacy and half-native mode which is enough for both fuloong2e and pegasos2 but I'm not sure how can we fix BARs in QEMU because that's also handled by generic PCI code which I also don't want to break. The code below works for booting Linux while at the same time not affecting any other emulation. I don't claim it to be a perfect fix, and overloading the existing property is a bit hackish, but it does work. Yes, maybe combining it with my original patch 1 to change secondary to flags to make it a bit cleaner would work for me. Then we would either only emulate legacy or half-native mode which is sufficient for these two machines we have. If Mark or others do not object it this time, I can update my patch and resubmit with this one to fix this issue, otherwise let's wait what idea do they have because I hate to spend time with something only to be discarded again. I think we don't need more complete emulation of this chip than this for now but if somebody wants to attempt that I don't mind as long as it does not break pegasos2. I had a play with your patches this afternoon, and spent some time performing some experiments and also reading various PCI bus master specifications and datasheets: this helped me understand a lot more about the theory of IRQ routing and compatible vs. legacy mode. From reading all the documentation (including the VIA and other datasheets) I cannot find any reference to a half-native mode which makes me think The half-native mode is my simpler term for Linux's "non 100% native mode". This may not exist in hardware but exists as a concept in some Linux (and maybe other) drivers so emulating it just means we do what these drivers expect to work correctly. How this maps to hardware and what interactions are there with firmware may be interesting but I'm not interested to find out as long as all guests we care about work because adding more complexity just for the sake of correctly modeling hardware seems like a waste of time in this case. Thanks for taking the time to find and document these though, it may be useful if someone wants to clean this up further. I'm satisfied with getting it in good enough shape for fuloong2e and pegasos2 to boot the guests we want, because I'd rather spend time on other, more interesting stuff such as writing replacement firmware for pegasos2 to avoid needing an undistributable ROM, implementing missing sound support, improving ati-vga or getting the Mac ROM work with g3beige, and also FPU emulation on PPC (and these are just the QEMU related stuff, I can think of others too). All of those seem time better spent than beating this via-ide model further now just for the sake of perfection without any gain, because guests will not work better even after spending more time with this. That's why I call it a waste of time. I know you prefer perfect patches but as they say "Perfect is the enemy of good." (I could think of better use of your time too such as finishing your screamer patches or improving OpenBIOS or your original sparc interest but that's for you to decide what you do.) I also try to improve these models and add missing stuff as needed but my goal is not perfection because I don't have that much time, just reaching good enough. It can always be improved later (or corrected if it turns out to be needed as in this case) but if we always hold back until getting it perfect we wont get anywhere. If your level of perfection was a requirement in QEMU a lot of devices would not be there as they could not get in in the first place which means other people cannot improve it as there's nothing there to start with. So I think something that is good enough is at least a good start towards perfection. We can argue what level is good enough. I think if it makes guests work which seems to be the general approach in QEMU as a lot of devices don't actually model real hardware correctly but just so that guests run with it. Of course we should make it clean and follow hardware where possible but a lot of models don't do that (maybe actually very few are anywhere near perfect). something else is wrong here. At the simplest level it could simply be that the VIA doesn't tri-state its legacy IRQ lines whilst the device is in native mode (the SI controller has an option for this), or it could indicate there is a PCI IRQ routing problem somewhere else that hasn't been picked up yet. All of the datasheets suggest that l
[PATCH v3 0/2] Fix via-ide for fuloong2e
v3 with review comments from Mark addressed BALATON Zoltan (1): ide: Make room for flags in PCIIDEState and add one for legacy mode Guenter Roeck (1): via-ide: Fix fuloong2e support hw/ide/cmd646.c | 6 +++--- hw/ide/via.c | 19 +-- hw/mips/fuloong2e.c | 4 +++- hw/sparc64/sun4u.c | 2 +- include/hw/ide/pci.h | 7 ++- 5 files changed, 30 insertions(+), 8 deletions(-) -- 2.21.3
[PATCH v3 2/2] via-ide: Fix fuloong2e support
From: Guenter Roeck
The IDE legacy mode emulation has been removed in commit 4ea98d317eb
("ide/via: Implement and use native PCI IDE mode") but some Linux
kernels (probably including def_config) require legacy mode on the
Fuloong2e so only emulating native mode did not turn out feasible.
Add property to via-ide model to make the mode configurable, and set
legacy mode for Fuloong2e.
Signed-off-by: Guenter Roeck
[balaton: Use bit in flags for property, add comment for missing BAR4]
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Guenter Roeck
Reviewed-by: Jiaxun Yang
---
v3: Use dash in property name
v2: Reworded commit message
hw/ide/via.c| 19 +--
hw/mips/fuloong2e.c | 4 +++-
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b33..2d935b910f 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "qemu/module.h"
#include "sysemu/dma.h"
@@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
&d->bus[1], "via-ide1-cmd", 4);
pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
-bmdma_setup_bar(d);
-pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
+/* Missing BAR4 will make Linux driver fall back to legacy PIO mode */
+bmdma_setup_bar(d);
+pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+}
qdev_init_gpio_in(ds, via_ide_set_irq, 2);
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
+ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
+i ? 0x376 : 0x3f6);
+}
ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
bmdma_init(&d->bus[i], &d->bmdma[i], d);
@@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
}
}
+static Property via_ide_properties[] = {
+DEFINE_PROP_BIT("legacy-mode", PCIIDEState, flags, PCI_IDE_LEGACY_MODE,
+false),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void via_ide_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -223,6 +237,7 @@ static void via_ide_class_init(ObjectClass *klass, void
*data)
k->device_id = PCI_DEVICE_ID_VIA_IDE;
k->revision = 0x06;
k->class_id = PCI_CLASS_STORAGE_IDE;
+device_class_set_props(dc, via_ide_properties);
set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
}
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 45c596f4fe..d334fde389 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -253,7 +253,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int
slot, qemu_irq intc,
/* Super I/O */
isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
-dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
+qdev_prop_set_bit(&dev->qdev, "legacy-mode", true);
+pci_realize_and_unref(dev, pci_bus, &error_fatal);
pci_ide_create_devs(dev);
pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
--
2.21.3
[PATCH v3 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode
We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for further flags if needed in
the future. This patch changes the "secondary" field to "flags" and
change CMD646 and its users accordingly and define a new flag for
forcing legacy mode that will be used by via-ide for now.
Signed-off-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
---
v3: Convert // comment in context
v2: Fixed typo in commit message
hw/ide/cmd646.c | 6 +++---
hw/sparc64/sun4u.c | 2 +-
include/hw/ide/pci.h | 7 ++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c254631485..cfea7fca06 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -255,8 +255,8 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error
**errp)
pci_conf[PCI_CLASS_PROG] = 0x8f;
-pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
-if (d->secondary) {
+pci_conf[CNTRL] = CNTRL_EN_CH0; /* enable IDE0 */
+if (d->flags & BIT(PCI_IDE_SECONDARY)) {
/* XXX: if not enabled, really disable the seconday IDE controller */
pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
}
@@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
}
static Property cmd646_ide_properties[] = {
-DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0fa13a7330..c46baa9f48 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
}
pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
-qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
+qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
pci_ide_create_devs(pci_dev);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..75d1a32f6d 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -42,6 +42,11 @@ typedef struct BMDMAState {
#define TYPE_PCI_IDE "pci-ide"
OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
+enum {
+PCI_IDE_SECONDARY, /* used only for cmd646 */
+PCI_IDE_LEGACY_MODE
+};
+
struct PCIIDEState {
/*< private >*/
PCIDevice parent_obj;
@@ -49,7 +54,7 @@ struct PCIIDEState {
IDEBus bus[2];
BMDMAState bmdma[2];
-uint32_t secondary; /* used only for cmd646 */
+uint32_t flags;
MemoryRegion bmdma_bar;
MemoryRegion cmd_bar[2];
MemoryRegion data_bar[2];
--
2.21.3
[PATCH v3 0/7] fuzz: improve crash case minimization
Extend and refine the crash case minimization process. Test input: Bug 1909261 full_reproducer 6500 QTest instructions (write mostly) Refined (-M1 minimization level) vs. Original version: real 38m31.942s <-- real 532m57.192s user 28m18.188s <-- user 89m0.536s sys 12m42.239s <-- sys 50m33.074s 2558 instructions <-- 2846 instructions Test Enviroment: i7-8550U, 16GB LPDDR3, SSD Ubuntu 20.04.1 5.4.0-58-generic x86_64 Python 3.8.5 v2 --> v3: Fix: checkpatch.pl errors v1 --> v2: New: [PATCH v2 1/7] New: [PATCH v2 2/7] New: [PATCH v2 4/7] New: [PATCH v2 6/7] New: [PATCH v2 7/7] Fix: [PATCH 2/4] split using binary approach Fix: [PATCH 3/4] typo in comments Discard: [PATCH 1/4] the hardcoded regex match for crash detection Discard: [PATCH 4/4] the delaying minimizer Thanks for the suggestions from: Alexander Bulekov Qiuhao Li (7): fuzz: accelerate non-crash detection fuzz: double the IOs to remove for every loop fuzz: split write operand using binary approach fuzz: loop the remove minimizer and refactoring fuzz: add minimization options fuzz: set bits in operand of write/out to zero fuzz: heuristic split write based on past IOs scripts/oss-fuzz/minimize_qtest_trace.py | 260 ++- 1 file changed, 212 insertions(+), 48 deletions(-) -- 2.25.1
[PATCH v3 1/7] fuzz: accelerate non-crash detection
We spend much time waiting for the timeout program during the minimization
process until it passes a time limit. This patch hacks the CLOSED (indicates
the redirection file closed) notification in QTest's output if it doesn't
crash.
Test with quadrupled trace input at:
https://bugs.launchpad.net/qemu/+bug/1890333/comments/1
Original version:
real 1m37.246s
user 0m13.069s
sys 0m8.399s
Refined version:
real 0m45.904s
user 0m16.874s
sys 0m10.042s
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 5e405a0d5f..aa69c7963e 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -29,30 +29,46 @@ whether the crash occred. Optionally, manually set a string
that idenitifes the
crash by setting CRASH_TOKEN=
""".format((sys.argv[0])))
+deduplication_note = """\n\
+Note: While trimming the input, sometimes the mutated trace triggers a
different
+crash output but indicates the same bug. Under this situation, our minimizer is
+incapable of recognizing and stopped from removing it. In the future, we may
+use a more sophisticated crash case deduplication method.
+\n"""
+
def check_if_trace_crashes(trace, path):
-global CRASH_TOKEN
with open(path, "w") as tracefile:
tracefile.write("".join(trace))
-rc = subprocess.Popen("timeout -s 9 {timeout}s {qemu_path} {qemu_args}
2>&1\
+proc = subprocess.Popen("timeout {timeout}s {qemu_path} {qemu_args} 2>&1\
< {trace_path}".format(timeout=TIMEOUT,
qemu_path=QEMU_PATH,
qemu_args=QEMU_ARGS,
trace_path=path),
shell=True,
stdin=subprocess.PIPE,
- stdout=subprocess.PIPE)
-stdo = rc.communicate()[0]
-output = stdo.decode('unicode_escape')
-if rc.returncode == 137:# Timed Out
-return False
-if len(output.splitlines()) < 2:
-return False
-
+ stdout=subprocess.PIPE,
+ encoding="utf-8")
+global CRASH_TOKEN
if CRASH_TOKEN is None:
-CRASH_TOKEN = output.splitlines()[-2]
+try:
+outs, _ = proc.communicate(timeout=5)
+CRASH_TOKEN = outs.splitlines()[-2]
+except subprocess.TimeoutExpired:
+print("subprocess.TimeoutExpired")
+return False
+print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
+global deduplication_note
+print(deduplication_note)
+return True
-return CRASH_TOKEN in output
+for line in iter(proc.stdout.readline, b''):
+if "CLOSED" in line:
+return False
+if CRASH_TOKEN in line:
+return True
+
+return False
def minimize_trace(inpath, outpath):
@@ -66,7 +82,6 @@ def minimize_trace(inpath, outpath):
print("Crashed in {} seconds".format(end-start))
TIMEOUT = (end-start)*5
print("Setting the timeout for {} seconds".format(TIMEOUT))
-print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
i = 0
newtrace = trace[:]
--
2.25.1
[PATCH v3 2/7] fuzz: double the IOs to remove for every loop
Instead of removing IO instructions one by one, we can try deleting multiple
instructions at once. According to the locality of reference, we double the
number of instructions to remove for the next round and recover it to one
once we fail.
This patch is usually significant for large input.
Test with quadrupled trace input at:
https://bugs.launchpad.net/qemu/+bug/1890333/comments/1
Patched 1/6 version:
real 0m45.904s
user 0m16.874s
sys 0m10.042s
Refined version:
real 0m11.412s
user 0m6.888s
sys 0m3.325s
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 33 +++-
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index aa69c7963e..0b665ae657 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -85,19 +85,28 @@ def minimize_trace(inpath, outpath):
i = 0
newtrace = trace[:]
-# For each line
+remove_step = 1
while i < len(newtrace):
-# 1.) Try to remove it completely and reproduce the crash. If it works,
-# we're done.
-prior = newtrace[i]
-print("Trying to remove {}".format(newtrace[i]))
-# Try to remove the line completely
-newtrace[i] = ""
+# 1.) Try to remove lines completely and reproduce the crash.
+# If it works, we're done.
+if (i+remove_step) >= len(newtrace):
+remove_step = 1
+prior = newtrace[i:i+remove_step]
+for j in range(i, i+remove_step):
+newtrace[j] = ""
+print("Removing {lines} ...".format(lines=prior))
if check_if_trace_crashes(newtrace, outpath):
-i += 1
+i += remove_step
+# Double the number of lines to remove for next round
+remove_step *= 2
continue
-newtrace[i] = prior
-
+# Failed to remove multiple IOs, fast recovery
+if remove_step > 1:
+for j in range(i, i+remove_step):
+newtrace[j] = prior[j-i]
+remove_step = 1
+continue
+newtrace[i] = prior[0] # remove_step = 1
# 2.) Try to replace write{bwlq} commands with a write addr, len
# command. Since this can require swapping endianness, try both LE and
# BE options. We do this, so we can "trim" the writes in (3)
@@ -118,7 +127,7 @@ def minimize_trace(inpath, outpath):
if(check_if_trace_crashes(newtrace, outpath)):
break
else:
-newtrace[i] = prior
+newtrace[i] = prior[0]
# 3.) If it is a qtest write command: write addr len data, try to split
# it into two separate write commands. If splitting the write down the
@@ -151,7 +160,7 @@ def minimize_trace(inpath, outpath):
if check_if_trace_crashes(newtrace, outpath):
i -= 1
else:
-newtrace[i] = prior
+newtrace[i] = prior[0]
del newtrace[i+1]
i += 1
check_if_trace_crashes(newtrace, outpath)
--
2.25.1
[PATCH v3 5/7] fuzz: add minimization options
-M1: loop around the remove minimizer
-M2: try setting bits in operand of write/out to zero
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 30
1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 70ac0c5366..4273ee7505 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -16,6 +16,10 @@ QEMU_PATH = None
TIMEOUT = 5
CRASH_TOKEN = None
+# Minimization levels
+M1 = False # loop around the remove minimizer
+M2 = False # try setting bits in operand of write/out to zero
+
write_suffix_lookup = {"b": (1, "B"),
"w": (2, "H"),
"l": (4, "L"),
@@ -23,10 +27,19 @@ write_suffix_lookup = {"b": (1, "B"),
def usage():
sys.exit("""\
-Usage: QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} input_trace output_trace
+Usage:
+
+QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} [Options] input_trace
output_trace
+
By default, will try to use the second-to-last line in the output to identify
whether the crash occred. Optionally, manually set a string that idenitifes the
crash by setting CRASH_TOKEN=
+
+Options:
+
+-M1: enable a loop around the remove minimizer, which may help decrease some
+ timing dependant instructions. Off by default.
+-M2: try setting bits in operand of write/out to zero. Off by default.
""".format((sys.argv[0])))
deduplication_note = """\n\
@@ -213,24 +226,31 @@ def minimize_trace(inpath, outpath):
print("Setting the timeout for {} seconds".format(TIMEOUT))
newtrace = trace[:]
-
+global M1, M2
# remove minimizer
old_len = len(newtrace) + 1
while(old_len > len(newtrace)):
old_len = len(newtrace)
+print("trace lenth = ", old_len)
remove_minimizer(newtrace, outpath)
+if not M1 and not M2:
+break
newtrace = list(filter(lambda s: s != "", newtrace))
assert(check_if_trace_crashes(newtrace, outpath))
# set zero minimizer
-set_zero_minimizer(newtrace, outpath)
+if M2:
+set_zero_minimizer(newtrace, outpath)
assert(check_if_trace_crashes(newtrace, outpath))
if __name__ == '__main__':
if len(sys.argv) < 3:
usage()
-
+if "-M1" in sys.argv:
+M1 = True
+if "-M2" in sys.argv:
+M2 = True
QEMU_PATH = os.getenv("QEMU_PATH")
QEMU_ARGS = os.getenv("QEMU_ARGS")
if QEMU_PATH is None or QEMU_ARGS is None:
@@ -239,4 +259,4 @@ if __name__ == '__main__':
# QEMU_ARGS += " -accel qtest"
CRASH_TOKEN = os.getenv("CRASH_TOKEN")
QEMU_ARGS += " -qtest stdio -monitor none -serial none "
-minimize_trace(sys.argv[1], sys.argv[2])
+minimize_trace(sys.argv[-2], sys.argv[-1])
--
2.25.1
[PATCH v3 3/7] fuzz: split write operand using binary approach
Currently, we split the write commands' data from the middle. If it does not
work, try to move the pivot left by one byte and retry until there is no
space.
But, this method has two flaws:
1. It may fail to trim all unnecessary bytes on the right side.
For example, there is an IO write command:
write addr uuuu
u is the unnecessary byte for the crash. Unlike ram write commands, in most
case, a split IO write won't trigger the same crash, So if we split from the
middle, we will get:
write addr uu (will be removed in next round)
write addr uu
For uu, since split it from the middle and retry to the leftmost byte
won't get the same crash, we will be stopped from removing the last two
bytes.
2. The algorithm complexity is O(n) since we move the pivot byte by byte.
To solve the first issue, we can try a symmetrical position on the right if
we fail on the left. As for the second issue, instead moving by one byte, we
can approach the boundary exponentially, achieving O(log(n)).
Give an example:
uu len=6
+
|
+
xxx,xuu 6/2=3 fail
+
+--+-+
||
++
xx,xxuu 6/2^2=1 fail u,u 6-1=5 success
+ +
+--++ |
| |+-+ u removed
+ +
xx,xxu 5/2=2 fail ,u 6-2=4 success
+
|
+---+ u removed
In some rare case, this algorithm will fail to trim all unnecessary bytes:
xuxx
-xuxx Fail
-xuxx Fail
xuxx- Fail
...
I think the trade-off is worth it.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 29
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 0b665ae657..1a26bf5b93 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -94,7 +94,7 @@ def minimize_trace(inpath, outpath):
prior = newtrace[i:i+remove_step]
for j in range(i, i+remove_step):
newtrace[j] = ""
-print("Removing {lines} ...".format(lines=prior))
+print("Removing {lines} ...\n".format(lines=prior))
if check_if_trace_crashes(newtrace, outpath):
i += remove_step
# Double the number of lines to remove for next round
@@ -107,9 +107,11 @@ def minimize_trace(inpath, outpath):
remove_step = 1
continue
newtrace[i] = prior[0] # remove_step = 1
+
# 2.) Try to replace write{bwlq} commands with a write addr, len
# command. Since this can require swapping endianness, try both LE and
# BE options. We do this, so we can "trim" the writes in (3)
+
if (newtrace[i].startswith("write") and not
newtrace[i].startswith("write ")):
suffix = newtrace[i].split()[0][-1]
@@ -130,11 +132,15 @@ def minimize_trace(inpath, outpath):
newtrace[i] = prior[0]
# 3.) If it is a qtest write command: write addr len data, try to split
-# it into two separate write commands. If splitting the write down the
-# middle does not work, try to move the pivot "left" and retry, until
-# there is no space left. The idea is to prune unneccessary bytes from
-# long writes, while accommodating arbitrary MemoryRegion access sizes
-# and alignments.
+# it into two separate write commands. If splitting the data operand
+# from length/2^n bytes to the left does not work, try to move the
pivot
+# to the right side, then add one to n, until length/2^n == 0. The idea
+# is to prune unneccessary bytes from long writes, while accommodating
+# arbitrary MemoryRegion access sizes and alignments.
+
+# This algorithm will fail under some rare situations.
+# e.g., xuxx (u is the unnecessary byte)
+
if newtrace[i].startswith("write "):
addr = int(newtrace[i].split()[1], 16)
length = int(newtrace[i].split()[2], 16)
@@ -143,6 +149,7 @@ def minimize_trace(inpath, outpath):
leftlength = int(length/2)
rightlength = length - leftlength
newtrace.insert(i+1, "")
+power = 1
while leftlength > 0:
newtrace[i] = "write {addr} {size} 0x{data}\n".format(
addr=hex(addr),
@@ -154,9 +161,13 @@ def minimize_trace(inpath, outpath):
data=data[leftlength*2:])
if check_if_trac
[PATCH v3 4/7] fuzz: loop the remove minimizer and refactoring
Now we use a one-time scan and remove strategy in the remval minimizer,
which is not suitable for timing dependent instructions.
For example, instruction A will indicate an address where the config
chunk locates, and instruction B will make the configuration active. If
we have the following instruction sequence:
...
A1
B1
A2
B2
...
A2 and B2 are the actual instructions that trigger the bug.
If we scan from top to bottom, after we remove A1, the behavior of B1
might be unknowable, including not to crash the program. But we will
successfully remove B1 later cause A2 and B2 will crash the process
anyway:
...
A1
A2
B2
...
Now one more trimming will remove A1.
In the perfect case, we would need to be able to remove A and B (or C!) at
the same time. But for now, let's just add a loop around the minimizer.
Since we only remove instructions, this iterative algorithm is converging.
Tested with Bug 1908062.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 80 +++-
1 file changed, 65 insertions(+), 15 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 1a26bf5b93..70ac0c5366 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -71,21 +71,9 @@ def check_if_trace_crashes(trace, path):
return False
-def minimize_trace(inpath, outpath):
-global TIMEOUT
-with open(inpath) as f:
-trace = f.readlines()
-start = time.time()
-if not check_if_trace_crashes(trace, outpath):
-sys.exit("The input qtest trace didn't cause a crash...")
-end = time.time()
-print("Crashed in {} seconds".format(end-start))
-TIMEOUT = (end-start)*5
-print("Setting the timeout for {} seconds".format(TIMEOUT))
-
-i = 0
-newtrace = trace[:]
+def remove_minimizer(newtrace, outpath):
remove_step = 1
+i = 0
while i < len(newtrace):
# 1.) Try to remove lines completely and reproduce the crash.
# If it works, we're done.
@@ -174,7 +162,69 @@ def minimize_trace(inpath, outpath):
newtrace[i] = prior[0]
del newtrace[i+1]
i += 1
-check_if_trace_crashes(newtrace, outpath)
+
+
+def set_zero_minimizer(newtrace, outpath):
+# try setting bits in operands of out/write to zero
+i = 0
+while i < len(newtrace):
+if (not newtrace[i].startswith("write ") and not
+ newtrace[i].startswith("out")):
+ i += 1
+ continue
+# write ADDR SIZE DATA
+# outx ADDR VALUE
+print("\nzero setting bits: {}".format(newtrace[i]))
+
+prefix = " ".join(newtrace[i].split()[:-1])
+data = newtrace[i].split()[-1]
+data_bin = bin(int(data, 16))
+data_bin_list = list(data_bin)
+
+for j in range(2, len(data_bin_list)):
+prior = newtrace[i]
+if (data_bin_list[j] == '1'):
+data_bin_list[j] = '0'
+data_try = hex(int("".join(data_bin_list), 2))
+# It seems qtest only accepts padded hex-values.
+if len(data_try) % 2 == 1:
+data_try = data_try[:2] + "0" + data_try[2:-1]
+
+newtrace[i] = "{prefix} {data_try}\n".format(
+prefix=prefix,
+data_try=data_try)
+
+if not check_if_trace_crashes(newtrace, outpath):
+data_bin_list[j] = '1'
+newtrace[i] = prior
+i += 1
+
+
+def minimize_trace(inpath, outpath):
+global TIMEOUT
+with open(inpath) as f:
+trace = f.readlines()
+start = time.time()
+if not check_if_trace_crashes(trace, outpath):
+sys.exit("The input qtest trace didn't cause a crash...")
+end = time.time()
+print("Crashed in {} seconds".format(end-start))
+TIMEOUT = (end-start)*5
+print("Setting the timeout for {} seconds".format(TIMEOUT))
+
+newtrace = trace[:]
+
+# remove minimizer
+old_len = len(newtrace) + 1
+while(old_len > len(newtrace)):
+old_len = len(newtrace)
+remove_minimizer(newtrace, outpath)
+newtrace = list(filter(lambda s: s != "", newtrace))
+assert(check_if_trace_crashes(newtrace, outpath))
+
+# set zero minimizer
+set_zero_minimizer(newtrace, outpath)
+assert(check_if_trace_crashes(newtrace, outpath))
if __name__ == '__main__':
--
2.25.1
[PATCH v3 7/7] fuzz: heuristic split write based on past IOs
If previous write commands write the same length of data with the same step,
we view it as a hint.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 55
1 file changed, 55 insertions(+)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 050b9f2195..5a8e90a604 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -83,6 +83,42 @@ def check_if_trace_crashes(trace, path):
return False
+# If previous write commands write the same length of data at the same
+# interval, we view it as a hint.
+def split_write_hint(newtrace, i):
+HINT_LEN = 3 # > 2
+if i <=(HINT_LEN-1):
+return None
+
+#find previous continuous write traces
+k = 0
+l = i-1
+writes = []
+while (k != HINT_LEN and l >= 0):
+if newtrace[l].startswith("write "):
+writes.append(newtrace[l])
+k += 1
+l -= 1
+elif newtrace[l] == "":
+l -= 1
+else:
+return None
+if k != HINT_LEN:
+return None
+
+length = int(writes[0].split()[2], 16)
+for j in range(1, HINT_LEN):
+if length != int(writes[j].split()[2], 16):
+return None
+
+step = int(writes[0].split()[1], 16) - int(writes[1].split()[1], 16)
+for j in range(1, HINT_LEN-1):
+if step != int(writes[j].split()[1], 16) - \
+int(writes[j+1].split()[1], 16):
+return None
+
+return (int(writes[0].split()[1], 16)+step, length)
+
def remove_minimizer(newtrace, outpath):
remove_step = 1
@@ -147,6 +183,25 @@ def remove_minimizer(newtrace, outpath):
length = int(newtrace[i].split()[2], 16)
data = newtrace[i].split()[3][2:]
if length > 1:
+
+# Can we get a hint from previous writes?
+hint = split_write_hint(newtrace, i)
+if hint is not None:
+hint_addr = hint[0]
+hint_len = hint[1]
+if hint_addr >= addr and hint_addr+hint_len <= addr+length:
+newtrace[i] = "write {addr} {size} 0x{data}\n".format(
+addr=hex(hint_addr),
+size=hex(hint_len),
+data=data[(hint_addr-addr)*2:\
+(hint_addr-addr)*2+hint_len*2])
+if check_if_trace_crashes(newtrace, outpath):
+# next round
+i += 1
+continue
+newtrace[i] = prior[0]
+
+# Try splitting it using a binary approach
leftlength = int(length/2)
rightlength = length - leftlength
newtrace.insert(i+1, "")
--
2.25.1
[PATCH v3 6/7] fuzz: set bits in operand of write/out to zero
Simplifying the crash cases by opportunistically setting bits in operands of out/write to zero may help to debug, since usually bit one means turn on or trigger a function while zero is the default turn-off setting. Tested Bug 1908062. Signed-off-by: Qiuhao Li --- scripts/oss-fuzz/minimize_qtest_trace.py | 4 1 file changed, 4 insertions(+) diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py b/scripts/oss-fuzz/minimize_qtest_trace.py index 4273ee7505..050b9f2195 100755 --- a/scripts/oss-fuzz/minimize_qtest_trace.py +++ b/scripts/oss-fuzz/minimize_qtest_trace.py @@ -243,6 +243,10 @@ def minimize_trace(inpath, outpath): set_zero_minimizer(newtrace, outpath) assert(check_if_trace_crashes(newtrace, outpath)) +# set zero minimizer +set_zero_minimizer(newtrace, outpath) +assert(check_if_trace_crashes(newtrace, outpath)) + if __name__ == '__main__': if len(sys.argv) < 3: -- 2.25.1
RE: [PATCH 1/3] qapi/net: Add new QMP command for COLO passthrough
> -Original Message-
> From: Jason Wang
> Sent: Monday, December 28, 2020 3:11 PM
> To: Zhang, Chen ; qemu-dev de...@nongnu.org>; Eric Blake ; Dr. David Alan
> Gilbert ; Markus Armbruster
> Cc: Zhang Chen
> Subject: Re: [PATCH 1/3] qapi/net: Add new QMP command for COLO
> passthrough
>
>
> On 2020/12/28 上午8:38, Zhang, Chen wrote:
> >
> >> -Original Message-
> >> From: Jason Wang
> >> Sent: Friday, December 25, 2020 2:20 PM
> >> To: Zhang, Chen ; qemu-dev >> de...@nongnu.org>; Eric Blake ; Dr. David Alan
> >> Gilbert ; Markus Armbruster
>
> >> Cc: Zhang Chen
> >> Subject: Re: [PATCH 1/3] qapi/net: Add new QMP command for COLO
> >> passthrough
> >>
> >>
> >> On 2020/12/24 上午9:09, Zhang Chen wrote:
> >>> From: Zhang Chen
> >>>
> >>> Since the real user scenario does not need to monitor all traffic.
> >>> Add colo-passthrough-add and colo-passthrough-del to maintain a COLO
> >>> network passthrough list.
> >>>
> >>> Signed-off-by: Zhang Chen
> >>> ---
> >>>net/net.c | 12
> >>>qapi/net.json | 46
> >> ++
> >>>2 files changed, 58 insertions(+)
> >>>
> >>> diff --git a/net/net.c b/net/net.c
> >>> index e1035f21d1..eac7a92618 100644
> >>> --- a/net/net.c
> >>> +++ b/net/net.c
> >>> @@ -1151,6 +1151,18 @@ void qmp_netdev_del(const char *id, Error
> >> **errp)
> >>>qemu_del_net_client(nc);
> >>>}
> >>>
> >>> +void qmp_colo_passthrough_add(const char *prot, const uint32_t port,
> >>> + Error **errp) {
> >>> +/* Setup passthrough connection */ }
> >>> +
> >>> +void qmp_colo_passthrough_del(const char *prot, const uint32_t port,
> >>> + Error **errp) {
> >>> +/* Delete passthrough connection */ }
> >>> +
> >>>static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
> >>>{
> >>>char *str;
> >>> diff --git a/qapi/net.json b/qapi/net.json index
> >>> c31748c87f..466c29714e 100644
> >>> --- a/qapi/net.json
> >>> +++ b/qapi/net.json
> >>> @@ -714,3 +714,49 @@
> >>>##
> >>>{ 'event': 'FAILOVER_NEGOTIATED',
> >>> 'data': {'device-id': 'str'} }
> >>> +
> >>> +##
> >>> +# @colo-passthrough-add:
> >>> +#
> >>> +# Add passthrough entry according to customer's needs in COLO-
> compare.
> >>> +#
> >>> +# @protocol: COLO passthrough just support TCP and UDP.
> >>> +#
> >>> +# @port: TCP or UDP port number.
> >>> +#
> >>> +# Returns: Nothing on success
> >>> +#
> >>> +# Since: 5.3
> >>> +#
> >>> +# Example:
> >>> +#
> >>> +# -> { "execute": "colo-passthrough-add",
> >>> +# "arguments": { "protocol": "tcp", "port": 3389 } }
> >>> +# <- { "return": {} }
> >>> +#
> >>> +##
> >>> +{ 'command': 'colo-passthrough-add',
> >>> + 'data': {'protocol': 'str', 'port': 'uint32'} }
> >>
> >> Do we plan to support 4-tuple (src ip,src port, dst ip, dst port) in
> >> the future? If yes, let's add them now.
> >>
> >> And do we plan to support wildcard here?
> > I think just using the port is enough for COLO compare.
> > Because in this case, users need passthrough some guest services are
> distinguished by static ports.
> > And for support 4-tuple and wildcard are a good question, do you think
> > we should add some passthrough Mechanism for all Qemu net filter? If yes,
> we should support that in the future.
>
>
> I think we can start form COLO. To avoid QMP compatibility issues, I would
> like to add the n tuple and wildcard support now.
OK, I will do this job in next version.
For the QMP compatibility issues, please give me a demo of what we want to see,
Like some existing commands.
Thanks
Chen
>
> Thanks
>
>
> >
> > Thanks
> > Chen
> >
> >> Thanks
> >>
> >>
> >>> +
> >>> +##
> >>> +# @colo-passthrough-del:
> >>> +#
> >>> +# Delete passthrough entry according to customer's needs in COLO-
> >> compare.
> >>> +#
> >>> +# @protocol: COLO passthrough just support TCP and UDP.
> >>> +#
> >>> +# @port: TCP or UDP port number.
> >>> +#
> >>> +# Returns: Nothing on success
> >>> +#
> >>> +# Since: 5.3
> >>> +#
> >>> +# Example:
> >>> +#
> >>> +# -> { "execute": "colo-passthrough-del",
> >>> +# "arguments": { "protocol": "tcp", "port": 3389 } }
> >>> +# <- { "return": {} }
> >>> +#
> >>> +##
> >>> +{ 'command': 'colo-passthrough-del',
> >>> + 'data': {'protocol': 'str', 'port': 'uint32'} }
Bug in Bonito? (mips/fuloong2e)
Hello, While continuing with part two of my vt82c686b clean ups I've tried to implement SMBus IO base configuration in the vt82c686b-pm part that I've already done for vt8231 for pegasos2 and it should be the same for 686B. (In short, writing address to pm config 0x90 sets base address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is what the firmware does first and it would allow removing hard coded 0xeee1 value and the property to set it and then I could reuse the same PM part in VT8231. I have code to implement this and it works with pegasos2 but fails with fuloong2e and pmon. I've debugged it that write to 0xd2 comes out as 0xd0 after some mapping in bonito: bonito_spciconf_write: bonito_spciconf_write 0490 size 4 val eee1 bonito_sbridge_pciaddr: cfgaddr 10490 pciaddr 2c90 busno 0 devno 5 funno 4 regno 144 pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1 via_pm_write addr 0x90 val 0xeee1 len 0x4 bonito_spciconf_write: bonito_spciconf_write 04d2 size 2 val 1 bonito_sbridge_pciaddr: cfgaddr 104d2 pciaddr 2cd0 busno 0 devno 5 funno 4 regno 208 pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1 via_pm_write addr 0xd0 val 0x1 len 0x2 Note the first write to 0x90 is correct but the second ends up at 0xd0 instead of 0xd2 after bonito_sbridge_pciaddr(). This is somewhere here: https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci-host/bonito.c;h=a99eced06574f999f3f1b999576ae09d5f4b06ca;hb=HEAD#l446 Any idea what this is trying to do and how to fix it? Regards, BALATON Zoltan
Re: [PATCH] gdb: riscv: Add target description
On Thu, Dec 24, 2020 at 1:09 AM Sylvain Pelissier
wrote:
>
> Target description is not currently implemented in RISC-V architecture. Thus
> GDB won't set it properly when attached. The patch implements the target
> description response.
>
> Signed-off-by: Sylvain Pelissier
> ---
> target/riscv/cpu.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 254cd83f8b..489d66038c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -556,6 +556,15 @@ static Property riscv_cpu_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +static gchar *riscv_gdb_arch_name(CPUState *cs)
> +{
> +#ifdef TARGET_RISCV64
Use riscv_cpu_is_32bit() instead of #ifdefs
> +return g_strdup("riscv:rv64");
> +#else
> +return g_strdup("riscv:rv32");
> +#endif
> +}
> +
> static void riscv_cpu_class_init(ObjectClass *c, void *data)
> {
> RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -591,6 +600,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void
> *data)
> /* For now, mark unmigratable: */
> cc->vmsd = &vmstate_riscv_cpu;
> #endif
> +cc->gdb_arch_name = riscv_gdb_arch_name;
> #ifdef CONFIG_TCG
> cc->tcg_initialize = riscv_translate_init;
> cc->tlb_fill = riscv_cpu_tlb_fill;
> --
Regards,
Bin
[PATCH v4 0/7] fuzz: improve crash case minimization
Extend and refine the crash case minimization process. Test input: Bug 1909261 full_reproducer 6500 QTest instructions (write mostly) Refined (-M1 minimization level) vs. Original version: real 38m31.942s <-- real 532m57.192s user 28m18.188s <-- user 89m0.536s sys 12m42.239s <-- sys 50m33.074s 2558 instructions <-- 2846 instructions Test Enviroment: i7-8550U, 16GB LPDDR3, SSD Ubuntu 20.04.1 5.4.0-58-generic x86_64 Python 3.8.5 v4: Fix: messy diff in [PATCH v3 4/7] v3: Fix: checkpatch.pl errors v2: New: [PATCH v2 1/7] New: [PATCH v2 2/7] New: [PATCH v2 4/7] New: [PATCH v2 6/7] New: [PATCH v2 7/7] Fix: [PATCH 2/4] split using binary approach Fix: [PATCH 3/4] typo in comments Discard: [PATCH 1/4] the hardcoded regex match for crash detection Discard: [PATCH 4/4] the delaying minimizer Thanks for the suggestions from: Alexander Bulekov Qiuhao Li (7): fuzz: accelerate non-crash detection fuzz: double the IOs to remove for every loop fuzz: split write operand using binary approach fuzz: loop the remove minimizer and refactoring fuzz: set bits in operand of write/out to zero fuzz: add minimization options fuzz: heuristic split write based on past IOs scripts/oss-fuzz/minimize_qtest_trace.py | 257 ++- 1 file changed, 209 insertions(+), 48 deletions(-) -- 2.25.1
[PATCH v4 1/7] fuzz: accelerate non-crash detection
We spend much time waiting for the timeout program during the minimization
process until it passes a time limit. This patch hacks the CLOSED (indicates
the redirection file closed) notification in QTest's output if it doesn't
crash.
Test with quadrupled trace input at:
https://bugs.launchpad.net/qemu/+bug/1890333/comments/1
Original version:
real 1m37.246s
user 0m13.069s
sys 0m8.399s
Refined version:
real 0m45.904s
user 0m16.874s
sys 0m10.042s
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 41
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 5e405a0d5f..aa69c7963e 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -29,30 +29,46 @@ whether the crash occred. Optionally, manually set a string
that idenitifes the
crash by setting CRASH_TOKEN=
""".format((sys.argv[0])))
+deduplication_note = """\n\
+Note: While trimming the input, sometimes the mutated trace triggers a
different
+crash output but indicates the same bug. Under this situation, our minimizer is
+incapable of recognizing and stopped from removing it. In the future, we may
+use a more sophisticated crash case deduplication method.
+\n"""
+
def check_if_trace_crashes(trace, path):
-global CRASH_TOKEN
with open(path, "w") as tracefile:
tracefile.write("".join(trace))
-rc = subprocess.Popen("timeout -s 9 {timeout}s {qemu_path} {qemu_args}
2>&1\
+proc = subprocess.Popen("timeout {timeout}s {qemu_path} {qemu_args} 2>&1\
< {trace_path}".format(timeout=TIMEOUT,
qemu_path=QEMU_PATH,
qemu_args=QEMU_ARGS,
trace_path=path),
shell=True,
stdin=subprocess.PIPE,
- stdout=subprocess.PIPE)
-stdo = rc.communicate()[0]
-output = stdo.decode('unicode_escape')
-if rc.returncode == 137:# Timed Out
-return False
-if len(output.splitlines()) < 2:
-return False
-
+ stdout=subprocess.PIPE,
+ encoding="utf-8")
+global CRASH_TOKEN
if CRASH_TOKEN is None:
-CRASH_TOKEN = output.splitlines()[-2]
+try:
+outs, _ = proc.communicate(timeout=5)
+CRASH_TOKEN = outs.splitlines()[-2]
+except subprocess.TimeoutExpired:
+print("subprocess.TimeoutExpired")
+return False
+print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
+global deduplication_note
+print(deduplication_note)
+return True
-return CRASH_TOKEN in output
+for line in iter(proc.stdout.readline, b''):
+if "CLOSED" in line:
+return False
+if CRASH_TOKEN in line:
+return True
+
+return False
def minimize_trace(inpath, outpath):
@@ -66,7 +82,6 @@ def minimize_trace(inpath, outpath):
print("Crashed in {} seconds".format(end-start))
TIMEOUT = (end-start)*5
print("Setting the timeout for {} seconds".format(TIMEOUT))
-print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
i = 0
newtrace = trace[:]
--
2.25.1
[PATCH v4 2/7] fuzz: double the IOs to remove for every loop
Instead of removing IO instructions one by one, we can try deleting multiple
instructions at once. According to the locality of reference, we double the
number of instructions to remove for the next round and recover it to one
once we fail.
This patch is usually significant for large input.
Test with quadrupled trace input at:
https://bugs.launchpad.net/qemu/+bug/1890333/comments/1
Patched 1/6 version:
real 0m45.904s
user 0m16.874s
sys 0m10.042s
Refined version:
real 0m11.412s
user 0m6.888s
sys 0m3.325s
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 33 +++-
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index aa69c7963e..0b665ae657 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -85,19 +85,28 @@ def minimize_trace(inpath, outpath):
i = 0
newtrace = trace[:]
-# For each line
+remove_step = 1
while i < len(newtrace):
-# 1.) Try to remove it completely and reproduce the crash. If it works,
-# we're done.
-prior = newtrace[i]
-print("Trying to remove {}".format(newtrace[i]))
-# Try to remove the line completely
-newtrace[i] = ""
+# 1.) Try to remove lines completely and reproduce the crash.
+# If it works, we're done.
+if (i+remove_step) >= len(newtrace):
+remove_step = 1
+prior = newtrace[i:i+remove_step]
+for j in range(i, i+remove_step):
+newtrace[j] = ""
+print("Removing {lines} ...".format(lines=prior))
if check_if_trace_crashes(newtrace, outpath):
-i += 1
+i += remove_step
+# Double the number of lines to remove for next round
+remove_step *= 2
continue
-newtrace[i] = prior
-
+# Failed to remove multiple IOs, fast recovery
+if remove_step > 1:
+for j in range(i, i+remove_step):
+newtrace[j] = prior[j-i]
+remove_step = 1
+continue
+newtrace[i] = prior[0] # remove_step = 1
# 2.) Try to replace write{bwlq} commands with a write addr, len
# command. Since this can require swapping endianness, try both LE and
# BE options. We do this, so we can "trim" the writes in (3)
@@ -118,7 +127,7 @@ def minimize_trace(inpath, outpath):
if(check_if_trace_crashes(newtrace, outpath)):
break
else:
-newtrace[i] = prior
+newtrace[i] = prior[0]
# 3.) If it is a qtest write command: write addr len data, try to split
# it into two separate write commands. If splitting the write down the
@@ -151,7 +160,7 @@ def minimize_trace(inpath, outpath):
if check_if_trace_crashes(newtrace, outpath):
i -= 1
else:
-newtrace[i] = prior
+newtrace[i] = prior[0]
del newtrace[i+1]
i += 1
check_if_trace_crashes(newtrace, outpath)
--
2.25.1
[PATCH v4 7/7] fuzz: heuristic split write based on past IOs
If previous write commands write the same length of data with the same step,
we view it as a hint.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 56
1 file changed, 56 insertions(+)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index a681984076..6cbf2b0419 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -85,6 +85,43 @@ def check_if_trace_crashes(trace, path):
return False
+# If previous write commands write the same length of data at the same
+# interval, we view it as a hint.
+def split_write_hint(newtrace, i):
+HINT_LEN = 3 # > 2
+if i <=(HINT_LEN-1):
+return None
+
+#find previous continuous write traces
+k = 0
+l = i-1
+writes = []
+while (k != HINT_LEN and l >= 0):
+if newtrace[l].startswith("write "):
+writes.append(newtrace[l])
+k += 1
+l -= 1
+elif newtrace[l] == "":
+l -= 1
+else:
+return None
+if k != HINT_LEN:
+return None
+
+length = int(writes[0].split()[2], 16)
+for j in range(1, HINT_LEN):
+if length != int(writes[j].split()[2], 16):
+return None
+
+step = int(writes[0].split()[1], 16) - int(writes[1].split()[1], 16)
+for j in range(1, HINT_LEN-1):
+if step != int(writes[j].split()[1], 16) - \
+int(writes[j+1].split()[1], 16):
+return None
+
+return (int(writes[0].split()[1], 16)+step, length)
+
+
def remove_minimizer(newtrace, outpath):
remove_step = 1
i = 0
@@ -148,6 +185,25 @@ def remove_minimizer(newtrace, outpath):
length = int(newtrace[i].split()[2], 16)
data = newtrace[i].split()[3][2:]
if length > 1:
+
+# Can we get a hint from previous writes?
+hint = split_write_hint(newtrace, i)
+if hint is not None:
+hint_addr = hint[0]
+hint_len = hint[1]
+if hint_addr >= addr and hint_addr+hint_len <= addr+length:
+newtrace[i] = "write {addr} {size} 0x{data}\n".format(
+addr=hex(hint_addr),
+size=hex(hint_len),
+data=data[(hint_addr-addr)*2:\
+(hint_addr-addr)*2+hint_len*2])
+if check_if_trace_crashes(newtrace, outpath):
+# next round
+i += 1
+continue
+newtrace[i] = prior[0]
+
+# Try splitting it using a binary approach
leftlength = int(length/2)
rightlength = length - leftlength
newtrace.insert(i+1, "")
--
2.25.1
[PATCH v4 4/7] fuzz: loop the remove minimizer and refactoring
Now we use a one-time scan and remove strategy in the remval minimizer,
which is not suitable for timing dependent instructions.
For example, instruction A will indicate an address where the config
chunk locates, and instruction B will make the configuration active. If
we have the following instruction sequence:
...
A1
B1
A2
B2
...
A2 and B2 are the actual instructions that trigger the bug.
If we scan from top to bottom, after we remove A1, the behavior of B1
might be unknowable, including not to crash the program. But we will
successfully remove B1 later cause A2 and B2 will crash the process
anyway:
...
A1
A2
B2
...
Now one more trimming will remove A1.
In the perfect case, we would need to be able to remove A and B (or C!) at
the same time. But for now, let's just add a loop around the minimizer.
Since we only remove instructions, this iterative algorithm is converging.
Tested with Bug 1908062.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 41 +++-
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 1a26bf5b93..378a7ccec6 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -71,21 +71,9 @@ def check_if_trace_crashes(trace, path):
return False
-def minimize_trace(inpath, outpath):
-global TIMEOUT
-with open(inpath) as f:
-trace = f.readlines()
-start = time.time()
-if not check_if_trace_crashes(trace, outpath):
-sys.exit("The input qtest trace didn't cause a crash...")
-end = time.time()
-print("Crashed in {} seconds".format(end-start))
-TIMEOUT = (end-start)*5
-print("Setting the timeout for {} seconds".format(TIMEOUT))
-
-i = 0
-newtrace = trace[:]
+def remove_minimizer(newtrace, outpath):
remove_step = 1
+i = 0
while i < len(newtrace):
# 1.) Try to remove lines completely and reproduce the crash.
# If it works, we're done.
@@ -174,7 +162,30 @@ def minimize_trace(inpath, outpath):
newtrace[i] = prior[0]
del newtrace[i+1]
i += 1
-check_if_trace_crashes(newtrace, outpath)
+
+
+def minimize_trace(inpath, outpath):
+global TIMEOUT
+with open(inpath) as f:
+trace = f.readlines()
+start = time.time()
+if not check_if_trace_crashes(trace, outpath):
+sys.exit("The input qtest trace didn't cause a crash...")
+end = time.time()
+print("Crashed in {} seconds".format(end-start))
+TIMEOUT = (end-start)*5
+print("Setting the timeout for {} seconds".format(TIMEOUT))
+
+newtrace = trace[:]
+
+# remove minimizer
+old_len = len(newtrace) + 1
+while(old_len > len(newtrace)):
+old_len = len(newtrace)
+remove_minimizer(newtrace, outpath)
+newtrace = list(filter(lambda s: s != "", newtrace))
+
+assert(check_if_trace_crashes(newtrace, outpath))
if __name__ == '__main__':
--
2.25.1
[PATCH v4 3/7] fuzz: split write operand using binary approach
Currently, we split the write commands' data from the middle. If it does not
work, try to move the pivot left by one byte and retry until there is no
space.
But, this method has two flaws:
1. It may fail to trim all unnecessary bytes on the right side.
For example, there is an IO write command:
write addr uuuu
u is the unnecessary byte for the crash. Unlike ram write commands, in most
case, a split IO write won't trigger the same crash, So if we split from the
middle, we will get:
write addr uu (will be removed in next round)
write addr uu
For uu, since split it from the middle and retry to the leftmost byte
won't get the same crash, we will be stopped from removing the last two
bytes.
2. The algorithm complexity is O(n) since we move the pivot byte by byte.
To solve the first issue, we can try a symmetrical position on the right if
we fail on the left. As for the second issue, instead moving by one byte, we
can approach the boundary exponentially, achieving O(log(n)).
Give an example:
uu len=6
+
|
+
xxx,xuu 6/2=3 fail
+
+--+-+
||
++
xx,xxuu 6/2^2=1 fail u,u 6-1=5 success
+ +
+--++ |
| |+-+ u removed
+ +
xx,xxu 5/2=2 fail ,u 6-2=4 success
+
|
+---+ u removed
In some rare case, this algorithm will fail to trim all unnecessary bytes:
xuxx
-xuxx Fail
-xuxx Fail
xuxx- Fail
...
I think the trade-off is worth it.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 29
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 0b665ae657..1a26bf5b93 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -94,7 +94,7 @@ def minimize_trace(inpath, outpath):
prior = newtrace[i:i+remove_step]
for j in range(i, i+remove_step):
newtrace[j] = ""
-print("Removing {lines} ...".format(lines=prior))
+print("Removing {lines} ...\n".format(lines=prior))
if check_if_trace_crashes(newtrace, outpath):
i += remove_step
# Double the number of lines to remove for next round
@@ -107,9 +107,11 @@ def minimize_trace(inpath, outpath):
remove_step = 1
continue
newtrace[i] = prior[0] # remove_step = 1
+
# 2.) Try to replace write{bwlq} commands with a write addr, len
# command. Since this can require swapping endianness, try both LE and
# BE options. We do this, so we can "trim" the writes in (3)
+
if (newtrace[i].startswith("write") and not
newtrace[i].startswith("write ")):
suffix = newtrace[i].split()[0][-1]
@@ -130,11 +132,15 @@ def minimize_trace(inpath, outpath):
newtrace[i] = prior[0]
# 3.) If it is a qtest write command: write addr len data, try to split
-# it into two separate write commands. If splitting the write down the
-# middle does not work, try to move the pivot "left" and retry, until
-# there is no space left. The idea is to prune unneccessary bytes from
-# long writes, while accommodating arbitrary MemoryRegion access sizes
-# and alignments.
+# it into two separate write commands. If splitting the data operand
+# from length/2^n bytes to the left does not work, try to move the
pivot
+# to the right side, then add one to n, until length/2^n == 0. The idea
+# is to prune unneccessary bytes from long writes, while accommodating
+# arbitrary MemoryRegion access sizes and alignments.
+
+# This algorithm will fail under some rare situations.
+# e.g., xuxx (u is the unnecessary byte)
+
if newtrace[i].startswith("write "):
addr = int(newtrace[i].split()[1], 16)
length = int(newtrace[i].split()[2], 16)
@@ -143,6 +149,7 @@ def minimize_trace(inpath, outpath):
leftlength = int(length/2)
rightlength = length - leftlength
newtrace.insert(i+1, "")
+power = 1
while leftlength > 0:
newtrace[i] = "write {addr} {size} 0x{data}\n".format(
addr=hex(addr),
@@ -154,9 +161,13 @@ def minimize_trace(inpath, outpath):
data=data[leftlength*2:])
if check_if_trac
[PATCH v4 5/7] fuzz: set bits in operand of write/out to zero
Simplifying the crash cases by opportunistically setting bits in operands of
out/write to zero may help to debug, since usually bit one means turn on or
trigger a function while zero is the default turn-off setting.
Tested Bug 1908062.
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 39
1 file changed, 39 insertions(+)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 378a7ccec6..70ac0c5366 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -164,6 +164,42 @@ def remove_minimizer(newtrace, outpath):
i += 1
+def set_zero_minimizer(newtrace, outpath):
+# try setting bits in operands of out/write to zero
+i = 0
+while i < len(newtrace):
+if (not newtrace[i].startswith("write ") and not
+ newtrace[i].startswith("out")):
+ i += 1
+ continue
+# write ADDR SIZE DATA
+# outx ADDR VALUE
+print("\nzero setting bits: {}".format(newtrace[i]))
+
+prefix = " ".join(newtrace[i].split()[:-1])
+data = newtrace[i].split()[-1]
+data_bin = bin(int(data, 16))
+data_bin_list = list(data_bin)
+
+for j in range(2, len(data_bin_list)):
+prior = newtrace[i]
+if (data_bin_list[j] == '1'):
+data_bin_list[j] = '0'
+data_try = hex(int("".join(data_bin_list), 2))
+# It seems qtest only accepts padded hex-values.
+if len(data_try) % 2 == 1:
+data_try = data_try[:2] + "0" + data_try[2:-1]
+
+newtrace[i] = "{prefix} {data_try}\n".format(
+prefix=prefix,
+data_try=data_try)
+
+if not check_if_trace_crashes(newtrace, outpath):
+data_bin_list[j] = '1'
+newtrace[i] = prior
+i += 1
+
+
def minimize_trace(inpath, outpath):
global TIMEOUT
with open(inpath) as f:
@@ -184,7 +220,10 @@ def minimize_trace(inpath, outpath):
old_len = len(newtrace)
remove_minimizer(newtrace, outpath)
newtrace = list(filter(lambda s: s != "", newtrace))
+assert(check_if_trace_crashes(newtrace, outpath))
+# set zero minimizer
+set_zero_minimizer(newtrace, outpath)
assert(check_if_trace_crashes(newtrace, outpath))
--
2.25.1
[PATCH v4 6/7] fuzz: add minimization options
-M1: loop around the remove minimizer
-M2: try setting bits in operand of write/out to zero
Signed-off-by: Qiuhao Li
---
scripts/oss-fuzz/minimize_qtest_trace.py | 32 +++-
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py
b/scripts/oss-fuzz/minimize_qtest_trace.py
index 70ac0c5366..a681984076 100755
--- a/scripts/oss-fuzz/minimize_qtest_trace.py
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -16,6 +16,10 @@ QEMU_PATH = None
TIMEOUT = 5
CRASH_TOKEN = None
+# Minimization levels
+M1 = False # loop around the remove minimizer
+M2 = False # try setting bits in operand of write/out to zero
+
write_suffix_lookup = {"b": (1, "B"),
"w": (2, "H"),
"l": (4, "L"),
@@ -23,10 +27,20 @@ write_suffix_lookup = {"b": (1, "B"),
def usage():
sys.exit("""\
-Usage: QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} input_trace output_trace
+Usage:
+
+QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} [Options] input_trace
output_trace
+
By default, will try to use the second-to-last line in the output to identify
whether the crash occred. Optionally, manually set a string that idenitifes the
crash by setting CRASH_TOKEN=
+
+Options:
+
+-M1: enable a loop around the remove minimizer, which may help decrease some
+ timing dependant instructions. Off by default.
+-M2: try setting bits in operand of write/out to zero. Off by default.
+
""".format((sys.argv[0])))
deduplication_note = """\n\
@@ -213,24 +227,30 @@ def minimize_trace(inpath, outpath):
print("Setting the timeout for {} seconds".format(TIMEOUT))
newtrace = trace[:]
-
+global M1, M2
# remove minimizer
old_len = len(newtrace) + 1
while(old_len > len(newtrace)):
old_len = len(newtrace)
+print("trace lenth = ", old_len)
remove_minimizer(newtrace, outpath)
+if not M1 and not M2:
+break
newtrace = list(filter(lambda s: s != "", newtrace))
assert(check_if_trace_crashes(newtrace, outpath))
-# set zero minimizer
-set_zero_minimizer(newtrace, outpath)
+if M2:
+set_zero_minimizer(newtrace, outpath)
assert(check_if_trace_crashes(newtrace, outpath))
if __name__ == '__main__':
if len(sys.argv) < 3:
usage()
-
+if "-M1" in sys.argv:
+M1 = True
+if "-M2" in sys.argv:
+M2 = True
QEMU_PATH = os.getenv("QEMU_PATH")
QEMU_ARGS = os.getenv("QEMU_ARGS")
if QEMU_PATH is None or QEMU_ARGS is None:
@@ -239,4 +259,4 @@ if __name__ == '__main__':
# QEMU_ARGS += " -accel qtest"
CRASH_TOKEN = os.getenv("CRASH_TOKEN")
QEMU_ARGS += " -qtest stdio -monitor none -serial none "
-minimize_trace(sys.argv[1], sys.argv[2])
+minimize_trace(sys.argv[-2], sys.argv[-1])
--
2.25.1
Re: [PATCH] RISC-V: Place DTB at 3GB boundary instead of 4GB
Hi Atish, On Wed, Dec 23, 2020 at 9:20 AM Bin Meng wrote: > > Hi Atish, > > On Wed, Dec 23, 2020 at 3:59 AM Atish Patra wrote: > > > > On Tue, 2020-12-22 at 13:35 +0800, Bin Meng wrote: > > > Hi Atish, > > > > > > On Sat, Dec 19, 2020 at 3:46 AM Atish Patra > > > wrote: > > > > > > > > On Fri, 2020-12-18 at 16:42 +0800, Bin Meng wrote: > > > > > Hi Atish, > > > > > > > > > > On Fri, Dec 18, 2020 at 4:00 PM Atish Patra > > > > > wrote: > > > > > > > > > > > > On Fri, 2020-12-18 at 15:33 +0800, Bin Meng wrote: > > > > > > > Hi Atish, > > > > > > > > > > > > > > On Fri, Dec 18, 2020 at 3:27 PM Atish Patra < > > > > > > > atish.pa...@wdc.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > On Fri, 2020-12-18 at 15:21 +0800, Bin Meng wrote: > > > > > > > > > Hi Atish, > > > > > > > > > > > > > > > > > > On Fri, Dec 18, 2020 at 5:48 AM Atish Patra > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > Currently, we place the DTB at 2MB from 4GB or end of > > > > > > > > > > DRAM > > > > > > > > > > which > > > > > > > > > > ever is > > > > > > > > > > lesser. However, Linux kernel can address only 1GB of > > > > > > > > > > memory > > > > > > > > > > for > > > > > > > > > > RV32. > > > > > > > > > > Thus, it can not map anything beyond 3GB (assuming 2GB > > > > > > > > > > is > > > > > > > > > > the > > > > > > > > > > starting address). > > > > > > > > > > As a result, it can not process DT and panic if opensbi > > > > > > > > > > dynamic > > > > > > > > > > firmware > > > > > > > > > > is used. > > > > > > > > > > > > > > > > > > > > Fix this by placing the DTB at 2MB from 3GB or end of > > > > > > > > > > DRAM > > > > > > > > > > whichever is lower. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Atish Patra > > > > > > > > > > --- > > > > > > > > > > hw/riscv/boot.c | 4 ++-- > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > With this patch, 32-bit sifive_u still does not boot > > > > > > > > > kernel > > > > > > > > > with > > > > > > > > > the > > > > > > > > > following patch applied on 5.10: > > > > > > > > > > > > > > > > > > https://patchwork.kernel.org/project/linux-riscv/patch/20201217074855.1948743-1-atish.pa...@wdc.com/ > > > > > > > > > > > > > > > > > > Command I used: > > > > > > > > > $ qemu-system-riscv32 -nographic -M sifive_u -m 1G -smp 5 > > > > > > > > > - > > > > > > > > > kernel > > > > > > > > > arch/riscv/boot/Image > > > > > > > > > > > > > > > > > > 32-bit virt cannot boot the same kernel image with memory > > > > > > > > > set > > > > > > > > > to > > > > > > > > > 2G > > > > > > > > > either: > > > > > > > > > $ qemu-system-riscv32 -nographic -M virt -m 2G -smp 4 - > > > > > > > > > kernel > > > > > > > > > arch/riscv/boot/Image > > > > > > > > > > > > > > > > > > > > > > > > > Hi Bin, > > > > > > > > As mentioned in the email on the linux mailing list, this > > > > > > > > patch > > > > > > > > only > > > > > > > > solves 2GB problem. sifive_u problem is solved by > > > > > > > > Alistair's > > > > > > > > patch[1]. > > > > > > > > > > > > > > > > He is planning to send the PR soon. The issue with sifive_u > > > > > > > > boot > > > > > > > > was it > > > > > > > > was failing the 32 bit test earlier resulting a 2MB aligned > > > > > > > > address > > > > > > > > instead of 4MB. > > > > > > > > > > > > > > Ah, I see. However my testing shows that virt with 2G still > > > > > > > does > > > > > > > not > > > > > > > boot with this patch. > > > > > > > > > > > > > > > > > > > Strange. I verified again with following combination with -bios > > > > > > and > > > > > > without bios parameter. > > > > > > > > > > > > 1. virt 32/64 with 1GB/2GB memory > > > > > > 2. sifive_u 32/64 bit with 1GB/2GB memory (Alistair's patch > > > > > > included) > > > > > > > > > > > > Can you share the boot log along with the head commit of Qemu > > > > > > and > > > > > > commandline ? I am using 5.10 kernel with my kernel fix. > > > > > > > > > > > > > > > > I was using Alistair's QEMU repo for testing and 5.10 kernel with > > > > > your > > > > > kernel fix: > > > > > > > > > > $ git checkout -b testing pull-riscv-to-apply-20201217-1 > > > > > $ apply this patch > > > > > $ mkdir build;cd build;../configure > > > > > --target-list=riscv64-softmmu,riscv32-softmmu;make -j > > > > > > > > > > $ ./qemu-system-riscv32 -nographic -M virt -m 2G -smp 4 -kernel > > > > > ~/work/git/linux/arch/riscv/boot/Image > > > > > > > > > > OpenSBI v0.8 > > > > >_ _ > > > > > / __ \ / | _ \_ _| > > > > > | | | |_ __ ___ _ __ | (___ | |_) || | > > > > > | | | | '_ \ / _ \ '_ \ \___ \| _ < | | > > > > > | |__| | |_) | __/ | | |) | |_) || |_ > > > > > \/| .__/ \___|_| |_|_/|/_| > > > > > | | > > > > > |_| > > > > > > > > > > Platform Name : riscv-virtio,qemu > > > > > Platform Features
Re: Bug in Bonito? (mips/fuloong2e)
在 2020/12/29 上午11:26, BALATON Zoltan 写道: Hello, While continuing with part two of my vt82c686b clean ups I've tried to implement SMBus IO base configuration in the vt82c686b-pm part that I've already done for vt8231 for pegasos2 and it should be the same for 686B. (In short, writing address to pm config 0x90 sets base address of smbus regs and bit 0 of 0xd2 enables/disables it.) This is what the firmware does first and it would allow removing hard coded 0xeee1 value and the property to set it and then I could reuse the same PM part in VT8231. [...] Any idea what this is trying to do and how to fix it? It's trying to translate Bonito style PCI config space r/w to standard PCI config space R/W. A quick galance told me change BONITO_PCICONF_REG_MASK to 0xff may help. Thanks. - Jiaxun Regards, BALATON Zoltan
