[PATCH v3 4/7] vmbus: vmbus implementation

[Jon Doron]

Add the VMBus infrastructure -- bus, devices, root bridge, vmbus state
machine, vmbus channel interactions, etc.

VMBus is a collection of technologies.  At its lowest layer, it's a message
passing and signaling mechanism, allowing efficient passing of messages to and
from guest VMs.  A layer higher, it's a mechanism for defining channels of
communication, where each channel is tagged with a type (which implies a
protocol) and a instance ID.  A layer higher than that, it's a bus driver,
serving as the basis of device enumeration within a VM, where a channel can
optionally be exposed as a paravirtual device.  When a server-side (paravirtual
back-end) component wishes to offer a channel to a guest VM, it does so by
specifying a channel type, a mode, and an instance ID.  VMBus then exposes this
in the guest.

More information about VMBus can be found in the file
vmbuskernelmodeclientlibapi.h in Microsoft's WDK.

TODO:
 - split into smaller palatable pieces
 - more comments
 - check and handle corner cases

Kudos to Evgeny Yakovlev (formerly eyakov...@virtuozzo.com) and Andrey
Smetatin (formerly asmeta...@virtuozzo.com) for research and
prototyping.

Signed-off-by: Roman Kagan 
Signed-off-by: Maciej S. Szmigiero 
Signed-off-by: Jon Doron 
---
 Makefile.objs|1 +
 hw/hyperv/Kconfig|5 +
 hw/hyperv/Makefile.objs  |1 +
 hw/hyperv/trace-events   |   18 +
 hw/hyperv/vmbus.c| 2672 ++
 include/hw/hyperv/vmbus-bridge.h |   32 +
 include/hw/hyperv/vmbus.h|  227 +++
 7 files changed, 2956 insertions(+)
 create mode 100644 hw/hyperv/trace-events
 create mode 100644 hw/hyperv/vmbus.c
 create mode 100644 include/hw/hyperv/vmbus-bridge.h
 create mode 100644 include/hw/hyperv/vmbus.h

diff --git a/Makefile.objs b/Makefile.objs
index a7c967633a..1ef80ce58f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -151,6 +151,7 @@ trace-events-subdirs += hw/block/dataplane
 trace-events-subdirs += hw/char
 trace-events-subdirs += hw/dma
 trace-events-subdirs += hw/hppa
+trace-events-subdirs += hw/hyperv
 trace-events-subdirs += hw/i2c
 trace-events-subdirs += hw/i386
 trace-events-subdirs += hw/i386/xen
diff --git a/hw/hyperv/Kconfig b/hw/hyperv/Kconfig
index a1fa8ff9be..3fbfe41c9e 100644
--- a/hw/hyperv/Kconfig
+++ b/hw/hyperv/Kconfig
@@ -6,3 +6,8 @@ config HYPERV_TESTDEV
 bool
 default y if TEST_DEVICES
 depends on HYPERV
+
+config VMBUS
+bool
+default y
+depends on HYPERV
diff --git a/hw/hyperv/Makefile.objs b/hw/hyperv/Makefile.objs
index edaca2f763..5b614e040c 100644
--- a/hw/hyperv/Makefile.objs
+++ b/hw/hyperv/Makefile.objs
@@ -1,2 +1,3 @@
 obj-y += hyperv.o
 obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
+obj-$(CONFIG_VMBUS) += vmbus.o
diff --git a/hw/hyperv/trace-events b/hw/hyperv/trace-events
new file mode 100644
index 00..ba5bd62d61
--- /dev/null
+++ b/hw/hyperv/trace-events
@@ -0,0 +1,18 @@
+# vmbus
+vmbus_recv_message(uint32_t type, uint32_t size) "type %d size %d"
+vmbus_signal_event(void) ""
+vmbus_channel_notify_guest(uint32_t chan_id) "channel #%d"
+vmbus_post_msg(uint32_t type, uint32_t size) "type %d size %d"
+vmbus_msg_cb(int status) "message status %d"
+vmbus_process_incoming_message(uint32_t message_type) "type %d"
+vmbus_initiate_contact(uint16_t major, uint16_t minor, uint32_t vcpu, uint64_t 
monitor_page1, uint64_t monitor_page2, uint64_t interrupt_page) "version %d.%d 
target vp %d mon pages 0x%"PRIx64",0x%"PRIx64" int page 0x%"PRIx64
+vmbus_send_offer(uint32_t chan_id, void *dev) "channel #%d dev %p"
+vmbus_terminate_offers(void) ""
+vmbus_gpadl_header(uint32_t gpadl_id, uint16_t num_gfns) "gpadl #%d gfns %d"
+vmbus_gpadl_body(uint32_t gpadl_id) "gpadl #%d"
+vmbus_gpadl_created(uint32_t gpadl_id) "gpadl #%d"
+vmbus_gpadl_teardown(uint32_t gpadl_id) "gpadl #%d"
+vmbus_gpadl_torndown(uint32_t gpadl_id) "gpadl #%d"
+vmbus_open_channel(uint32_t chan_id, uint32_t gpadl_id, uint32_t target_vp) 
"channel #%d gpadl #%d target vp %d"
+vmbus_channel_open(uint32_t chan_id, uint32_t status) "channel #%d status %d"
+vmbus_close_channel(uint32_t chan_id) "channel #%d"
diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
new file mode 100644
index 00..1f5873ab60
--- /dev/null
+++ b/hw/hyperv/vmbus.c
@@ -0,0 +1,2672 @@
+/*
+ * QEMU Hyper-V VMBus
+ *
+ * Copyright (c) 2017-2018 Virtuozzo International GmbH.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "hw/hyperv/hyperv.h"
+#include "hw/hyperv/vmbus.h"
+#include "hw/hyperv/vmbus-bridge.h"
+#include "hw/sysbus.h"
+#include "cpu.h"
+#include "trace.h"
+
+#define TYPE_VMBUS "vmbus"
+#define VMBUS(obj) OBJECT_CHECK(VMBus, (obj), TYPE_VMBUS)
+
+enum 

[PATCH v3 6/7] i386: Hyper-V VMBus ACPI DSDT entry

[Jon Doron]

Guest OS uses ACPI to discover VMBus presence.  Add a corresponding
entry to DSDT in case VMBus has been enabled.

Experimentally Windows guests were found to require this entry to
include two IRQ resources. They seem to never be used but they still
have to be there.

Make IRQ numbers user-configurable via corresponding properties; use 7
and 13 by default.

Signed-off-by: Evgeny Yakovlev 
Signed-off-by: Roman Kagan 
Signed-off-by: Maciej S. Szmigiero 
Signed-off-by: Jon Doron 
---
 hw/hyperv/vmbus.c|  7 ++
 hw/i386/acpi-build.c | 43 
 include/hw/hyperv/vmbus-bridge.h |  3 +++
 3 files changed, 53 insertions(+)

diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index 1f5873ab60..0df7afe0ca 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -2641,6 +2641,12 @@ static const VMStateDescription vmstate_vmbus_bridge = {
 },
 };
 
+static Property vmbus_bridge_props[] = {
+DEFINE_PROP_UINT8("irq0", VMBusBridge, irq0, 7),
+DEFINE_PROP_UINT8("irq1", VMBusBridge, irq1, 13),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void vmbus_bridge_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
@@ -2651,6 +2657,7 @@ static void vmbus_bridge_class_init(ObjectClass *klass, 
void *data)
 sk->explicit_ofw_unit_address = vmbus_bridge_ofw_unit_address;
 set_bit(DEVICE_CATEGORY_BRIDGE, k->categories);
 k->vmsd = _vmbus_bridge;
+device_class_set_props(k, vmbus_bridge_props);
 /* override SysBusDevice's default */
 k->user_creatable = true;
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2a7e55bae7..d235074fb8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -50,6 +50,7 @@
 #include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
+#include "hw/hyperv/vmbus-bridge.h"
 
 /* Supported chipsets: */
 #include "hw/southbridge/piix.h"
@@ -1270,9 +1271,47 @@ static Aml *build_com_device_aml(uint8_t uid)
 return dev;
 }
 
+static Aml *build_vmbus_device_aml(VMBusBridge *vmbus_bridge)
+{
+Aml *dev;
+Aml *method;
+Aml *crs;
+
+dev = aml_device("VMBS");
+aml_append(dev, aml_name_decl("STA", aml_int(0xF)));
+aml_append(dev, aml_name_decl("_HID", aml_string("VMBus")));
+aml_append(dev, aml_name_decl("_UID", aml_int(0x0)));
+aml_append(dev, aml_name_decl("_DDN", aml_string("VMBUS")));
+
+method = aml_method("_DIS", 0, AML_NOTSERIALIZED);
+aml_append(method, aml_store(aml_and(aml_name("STA"), aml_int(0xD), NULL),
+ aml_name("STA")));
+aml_append(dev, method);
+
+method = aml_method("_PS0", 0, AML_NOTSERIALIZED);
+aml_append(method, aml_store(aml_or(aml_name("STA"), aml_int(0xF), NULL),
+ aml_name("STA")));
+aml_append(dev, method);
+
+method = aml_method("_STA", 0, AML_NOTSERIALIZED);
+aml_append(method, aml_return(aml_name("STA")));
+aml_append(dev, method);
+
+aml_append(dev, aml_name_decl("_PS3", aml_int(0x0)));
+
+crs = aml_resource_template();
+aml_append(crs, aml_irq_no_flags(vmbus_bridge->irq0));
+/* FIXME: newer HyperV gets by with only one IRQ */
+aml_append(crs, aml_irq_no_flags(vmbus_bridge->irq1));
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+return dev;
+}
+
 static void build_isa_devices_aml(Aml *table)
 {
 ISADevice *fdc = pc_find_fdc0();
+VMBusBridge *vmbus_bridge = vmbus_bridge_find();
 bool ambiguous;
 
 Aml *scope = aml_scope("_SB.PCI0.ISA");
@@ -1296,6 +1335,10 @@ static void build_isa_devices_aml(Aml *table)
 build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
 }
 
+if (vmbus_bridge) {
+aml_append(scope, build_vmbus_device_aml(vmbus_bridge));
+}
+
 aml_append(table, scope);
 }
 
diff --git a/include/hw/hyperv/vmbus-bridge.h b/include/hw/hyperv/vmbus-bridge.h
index 9cc8f780de..c0a06d832c 100644
--- a/include/hw/hyperv/vmbus-bridge.h
+++ b/include/hw/hyperv/vmbus-bridge.h
@@ -19,6 +19,9 @@ typedef struct VMBus VMBus;
 typedef struct VMBusBridge {
 SysBusDevice parent_obj;
 
+uint8_t irq0;
+uint8_t irq1;
+
 VMBus *bus;
 } VMBusBridge;
 
-- 
2.24.1

[PATCH v3 5/7] i386:pc: whitelist dynamic vmbus-bridge

[Jon Doron]

As vmbus-bridge is derived from sysbus device, it has to be whitelisted
to be allowed to be created with -device.

Signed-off-by: Roman Kagan 
Signed-off-by: Maciej S. Szmigiero 
Signed-off-by: Jon Doron 
---
 hw/i386/pc_piix.c | 2 ++
 hw/i386/pc_q35.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9cceae3e2c..6daa0770fa 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -60,6 +60,7 @@
 #include "migration/global_state.h"
 #include "migration/misc.h"
 #include "sysemu/numa.h"
+#include "hw/hyperv/vmbus-bridge.h"
 #include "hw/mem/nvdimm.h"
 
 #define MAX_IDE_BUS 2
@@ -417,6 +418,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
 m->default_machine_opts = "firmware=bios-256k.bin";
 m->default_display = "std";
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
+machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
 }
 
 static void pc_i440fx_5_0_machine_options(MachineClass *m)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d37c425e22..faaa39ced2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -53,6 +53,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
+#include "hw/hyperv/vmbus-bridge.h"
 #include "hw/mem/nvdimm.h"
 
 /* ICH9 AHCI has 6 ports */
@@ -346,6 +347,7 @@ static void pc_q35_machine_options(MachineClass *m)
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
 machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
+machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
 m->max_cpus = 288;
 }
 
-- 
2.24.1

[PATCH v3 2/7] hyperv: SControl is optional to enable SynIc

[Jon Doron]

SynIc can be enabled regardless of the SControl mechanisim which can
register a GSI for a given SintRoute.

This behaviour can achived by setting enabling SIMP and then the guest
will poll on the message slot.

Once there is another message pending the host will set the message slot
with the pending flag.
When the guest polls from the message slot, incase the pending flag is
set it will write to the HV_X64_MSR_EOM indicating it has cleared the
slow and we can try and push our message again.

Signed-off-by: Jon Doron 
---
 hw/hyperv/hyperv.c | 242 -
 include/hw/hyperv/hyperv.h |   2 +
 target/i386/hyperv.c   |   2 +
 3 files changed, 164 insertions(+), 82 deletions(-)

diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index ddf4f32c60..1dc577a0ab 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -20,18 +20,72 @@
 #include "qemu/rcu_queue.h"
 #include "hw/hyperv/hyperv.h"
 
+/*
+ * KVM has its own message producers (SynIC timers).  To guarantee
+ * serialization with both KVM vcpu and the guest cpu, the messages are first
+ * staged in an intermediate area and then posted to the SynIC message page in
+ * the vcpu thread.
+ */
+typedef struct HvSintStagedMessage {
+/* message content staged by hyperv_post_msg */
+struct hyperv_message msg;
+/* callback + data (r/o) to complete the processing in a BH */
+HvSintMsgCb cb;
+void *cb_data;
+/* message posting status filled by cpu_post_msg */
+int status;
+/* passing the buck: */
+enum {
+/* initial state */
+HV_STAGED_MSG_FREE,
+/*
+ * hyperv_post_msg (e.g. in main loop) grabs the staged area (FREE ->
+ * BUSY), copies msg, and schedules cpu_post_msg on the assigned cpu
+ */
+HV_STAGED_MSG_BUSY,
+/*
+ * cpu_post_msg (vcpu thread) tries to copy staged msg to msg slot,
+ * notify the guest, records the status, marks the posting done (BUSY
+ * -> POSTED), and schedules sint_msg_bh BH
+ */
+HV_STAGED_MSG_POSTED,
+/*
+ * sint_msg_bh (BH) verifies that the posting is done, runs the
+ * callback, and starts over (POSTED -> FREE)
+ */
+} state;
+} HvSintStagedMessage;
+
+struct SynICState;
+
+struct HvSintRoute {
+uint32_t sint;
+struct SynICState *synic;
+int gsi;
+EventNotifier sint_set_notifier;
+EventNotifier sint_ack_notifier;
+
+HvSintStagedMessage *staged_msg;
+
+unsigned refcount;
+QLIST_ENTRY(HvSintRoute) link;
+};
+
 typedef struct SynICState {
 DeviceState parent_obj;
 
 CPUState *cs;
 
-bool enabled;
+bool sctl_enabled;
 hwaddr msg_page_addr;
 hwaddr event_page_addr;
 MemoryRegion msg_page_mr;
 MemoryRegion event_page_mr;
 struct hyperv_message_page *msg_page;
 struct hyperv_event_flags_page *event_page;
+
+QemuMutex sint_routes_mutex;
+QLIST_HEAD(, HvSintRoute) sint_routes;
 } SynICState;
 
 #define TYPE_SYNIC "hyperv-synic"
@@ -49,11 +103,11 @@ static SynICState *get_synic(CPUState *cs)
 return SYNIC(object_resolve_path_component(OBJECT(cs), "synic"));
 }
 
-static void synic_update(SynICState *synic, bool enable,
+static void synic_update(SynICState *synic, bool sctl_enable,
  hwaddr msg_page_addr, hwaddr event_page_addr)
 {
 
-synic->enabled = enable;
+synic->sctl_enabled = sctl_enable;
 if (synic->msg_page_addr != msg_page_addr) {
 if (synic->msg_page_addr) {
 memory_region_del_subregion(get_system_memory(),
@@ -78,7 +132,7 @@ static void synic_update(SynICState *synic, bool enable,
 }
 }
 
-void hyperv_synic_update(CPUState *cs, bool enable,
+void hyperv_synic_update(CPUState *cs, bool sctl_enable,
  hwaddr msg_page_addr, hwaddr event_page_addr)
 {
 SynICState *synic = get_synic(cs);
@@ -87,7 +141,7 @@ void hyperv_synic_update(CPUState *cs, bool enable,
 return;
 }
 
-synic_update(synic, enable, msg_page_addr, event_page_addr);
+synic_update(synic, sctl_enable, msg_page_addr, event_page_addr);
 }
 
 static void synic_realize(DeviceState *dev, Error **errp)
@@ -108,16 +162,20 @@ static void synic_realize(DeviceState *dev, Error **errp)
sizeof(*synic->event_page), _abort);
 synic->msg_page = memory_region_get_ram_ptr(>msg_page_mr);
 synic->event_page = memory_region_get_ram_ptr(>event_page_mr);
+qemu_mutex_init(>sint_routes_mutex);
+QLIST_INIT(>sint_routes);
 
 g_free(msgp_name);
 g_free(eventp_name);
 }
+
 static void synic_reset(DeviceState *dev)
 {
 SynICState *synic = SYNIC(dev);
 memset(synic->msg_page, 0, sizeof(*synic->msg_page));
 memset(synic->event_page, 0, sizeof(*synic->event_page));
 synic_update(synic, false, 0, 0);
+assert(QLIST_EMPTY(>sint_routes));
 }
 
 static void synic_class_init(ObjectClass *klass, void *data)
@@ -166,54 +224,6 @@ static void 

[PATCH v3 7/7] vmbus: add infrastructure to save/load vmbus requests

[Jon Doron]

This can be allow to include controller-specific data while
saving/loading in-flight scsi requests of the vmbus scsi controller.

Signed-off-by: Roman Kagan 
Signed-off-by: Maciej S. Szmigiero 
Signed-off-by: Jon Doron 
---
 hw/hyperv/vmbus.c | 99 +++
 include/hw/hyperv/vmbus.h |  3 ++
 2 files changed, 102 insertions(+)

diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index 0df7afe0ca..ab72a59a4a 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1272,6 +1272,105 @@ void vmbus_free_req(void *req)
 g_free(req);
 }
 
+static const VMStateDescription vmstate_sgent = {
+.name = "vmbus/sgentry",
+.version_id = 0,
+.minimum_version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_UINT64(base, ScatterGatherEntry),
+VMSTATE_UINT64(len, ScatterGatherEntry),
+VMSTATE_END_OF_LIST()
+}
+};
+
+typedef struct VMBusChanReqSave {
+uint16_t chan_idx;
+uint16_t pkt_type;
+uint32_t msglen;
+void *msg;
+uint64_t transaction_id;
+bool need_comp;
+uint32_t num;
+ScatterGatherEntry *sgl;
+} VMBusChanReqSave;
+
+static const VMStateDescription vmstate_vmbus_chan_req = {
+.name = "vmbus/vmbus_chan_req",
+.version_id = 0,
+.minimum_version_id = 0,
+.fields = (VMStateField[]) {
+VMSTATE_UINT16(chan_idx, VMBusChanReqSave),
+VMSTATE_UINT16(pkt_type, VMBusChanReqSave),
+VMSTATE_UINT32(msglen, VMBusChanReqSave),
+VMSTATE_VBUFFER_ALLOC_UINT32(msg, VMBusChanReqSave, 0, NULL, msglen),
+VMSTATE_UINT64(transaction_id, VMBusChanReqSave),
+VMSTATE_BOOL(need_comp, VMBusChanReqSave),
+VMSTATE_UINT32(num, VMBusChanReqSave),
+VMSTATE_STRUCT_VARRAY_POINTER_UINT32(sgl, VMBusChanReqSave, num,
+ vmstate_sgent, 
ScatterGatherEntry),
+VMSTATE_END_OF_LIST()
+}
+};
+
+void vmbus_save_req(QEMUFile *f, VMBusChanReq *req)
+{
+VMBusChanReqSave req_save;
+
+req_save.chan_idx = req->chan->subchan_idx;
+req_save.pkt_type = req->pkt_type;
+req_save.msglen = req->msglen;
+req_save.msg = req->msg;
+req_save.transaction_id = req->transaction_id;
+req_save.need_comp = req->need_comp;
+req_save.num = req->sgl.nsg;
+req_save.sgl = g_memdup(req->sgl.sg,
+req_save.num * sizeof(ScatterGatherEntry));
+
+vmstate_save_state(f, _vmbus_chan_req, _save, NULL);
+
+g_free(req_save.sgl);
+}
+
+void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size)
+{
+VMBusChanReqSave req_save;
+VMBusChanReq *req = NULL;
+VMBusChannel *chan = NULL;
+uint32_t i;
+
+vmstate_load_state(f, _vmbus_chan_req, _save, 0);
+
+if (req_save.chan_idx >= dev->num_channels) {
+error_report("%s: %u(chan_idx) > %u(num_channels)", __func__,
+ req_save.chan_idx, dev->num_channels);
+goto out;
+}
+chan = >channels[req_save.chan_idx];
+
+if (vmbus_channel_reserve(chan, 0, req_save.msglen)) {
+goto out;
+}
+
+req = vmbus_alloc_req(chan, size, req_save.pkt_type, req_save.msglen,
+  req_save.transaction_id, req_save.need_comp);
+if (req_save.msglen) {
+memcpy(req->msg, req_save.msg, req_save.msglen);
+}
+
+for (i = 0; i < req_save.num; i++) {
+qemu_sglist_add(>sgl, req_save.sgl[i].base, req_save.sgl[i].len);
+}
+
+out:
+if (req_save.msglen) {
+g_free(req_save.msg);
+}
+if (req_save.num) {
+g_free(req_save.sgl);
+}
+return req;
+}
+
 static void channel_event_cb(EventNotifier *e)
 {
 VMBusChannel *chan = container_of(e, VMBusChannel, notifier);
diff --git a/include/hw/hyperv/vmbus.h b/include/hw/hyperv/vmbus.h
index 63a5b807b6..9219f34d6b 100644
--- a/include/hw/hyperv/vmbus.h
+++ b/include/hw/hyperv/vmbus.h
@@ -224,4 +224,7 @@ int vmbus_map_sgl(VMBusChanReq *req, DMADirection dir, 
struct iovec *iov,
 void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
  unsigned iov_cnt, size_t accessed);
 
+void vmbus_save_req(QEMUFile *f, VMBusChanReq *req);
+void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size);
+
 #endif
-- 
2.24.1

[PATCH v3 3/7] vmbus: add vmbus protocol definitions

[Jon Doron]

Add a header with data structures and constants used in Hyper-V VMBus
hypervisor <-> guest interactions.

Based on the respective stuff from Linux kernel.

Signed-off-by: Roman Kagan 
Signed-off-by: Maciej S. Szmigiero 
Signed-off-by: Jon Doron 
---
 include/hw/hyperv/vmbus-proto.h | 222 
 1 file changed, 222 insertions(+)
 create mode 100644 include/hw/hyperv/vmbus-proto.h

diff --git a/include/hw/hyperv/vmbus-proto.h b/include/hw/hyperv/vmbus-proto.h
new file mode 100644
index 00..4628d3b323
--- /dev/null
+++ b/include/hw/hyperv/vmbus-proto.h
@@ -0,0 +1,222 @@
+/*
+ * QEMU Hyper-V VMBus support
+ *
+ * Copyright (c) 2017-2018 Virtuozzo International GmbH.
+ *
+ * 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 HW_HYPERV_VMBUS_PROTO_H
+#define HW_HYPERV_VMBUS_PROTO_H
+
+#define VMBUS_VERSION_WS2008((0 << 16) | (13))
+#define VMBUS_VERSION_WIN7  ((1 << 16) | (1))
+#define VMBUS_VERSION_WIN8  ((2 << 16) | (4))
+#define VMBUS_VERSION_WIN8_1((3 << 16) | (0))
+#define VMBUS_VERSION_WIN10 ((4 << 16) | (0))
+#define VMBUS_VERSION_INVAL -1
+#define VMBUS_VERSION_CURRENT   VMBUS_VERSION_WIN10
+
+#define VMBUS_MESSAGE_CONNECTION_ID 1
+#define VMBUS_EVENT_CONNECTION_ID   2
+#define VMBUS_MONITOR_CONNECTION_ID 3
+#define VMBUS_SINT  2
+
+#define VMBUS_MSG_INVALID   0
+#define VMBUS_MSG_OFFERCHANNEL  1
+#define VMBUS_MSG_RESCIND_CHANNELOFFER  2
+#define VMBUS_MSG_REQUESTOFFERS 3
+#define VMBUS_MSG_ALLOFFERS_DELIVERED   4
+#define VMBUS_MSG_OPENCHANNEL   5
+#define VMBUS_MSG_OPENCHANNEL_RESULT6
+#define VMBUS_MSG_CLOSECHANNEL  7
+#define VMBUS_MSG_GPADL_HEADER  8
+#define VMBUS_MSG_GPADL_BODY9
+#define VMBUS_MSG_GPADL_CREATED 10
+#define VMBUS_MSG_GPADL_TEARDOWN11
+#define VMBUS_MSG_GPADL_TORNDOWN12
+#define VMBUS_MSG_RELID_RELEASED13
+#define VMBUS_MSG_INITIATE_CONTACT  14
+#define VMBUS_MSG_VERSION_RESPONSE  15
+#define VMBUS_MSG_UNLOAD16
+#define VMBUS_MSG_UNLOAD_RESPONSE   17
+#define VMBUS_MSG_COUNT 18
+
+#define VMBUS_MESSAGE_SIZE_ALIGNsizeof(uint64_t)
+
+#define VMBUS_PACKET_INVALID0x0
+#define VMBUS_PACKET_SYNCH  0x1
+#define VMBUS_PACKET_ADD_XFER_PAGESET   0x2
+#define VMBUS_PACKET_RM_XFER_PAGESET0x3
+#define VMBUS_PACKET_ESTABLISH_GPADL0x4
+#define VMBUS_PACKET_TEARDOWN_GPADL 0x5
+#define VMBUS_PACKET_DATA_INBAND0x6
+#define VMBUS_PACKET_DATA_USING_XFER_PAGES  0x7
+#define VMBUS_PACKET_DATA_USING_GPADL   0x8
+#define VMBUS_PACKET_DATA_USING_GPA_DIRECT  0x9
+#define VMBUS_PACKET_CANCEL_REQUEST 0xa
+#define VMBUS_PACKET_COMP   0xb
+#define VMBUS_PACKET_DATA_USING_ADDITIONAL_PKT  0xc
+#define VMBUS_PACKET_ADDITIONAL_DATA0xd
+
+#define VMBUS_CHANNEL_USER_DATA_SIZE120
+
+#define VMBUS_OFFER_MONITOR_ALLOCATED   0x1
+#define VMBUS_OFFER_INTERRUPT_DEDICATED 0x1
+
+#define VMBUS_RING_BUFFER_FEAT_PENDING_SZ   (1ul << 0)
+
+#define VMBUS_CHANNEL_ENUMERATE_DEVICE_INTERFACE  0x1
+#define VMBUS_CHANNEL_SERVER_SUPPORTS_TRANSFER_PAGES  0x2
+#define VMBUS_CHANNEL_SERVER_SUPPORTS_GPADLS  0x4
+#define VMBUS_CHANNEL_NAMED_PIPE_MODE 0x10
+#define VMBUS_CHANNEL_LOOPBACK_OFFER  0x100
+#define VMBUS_CHANNEL_PARENT_OFFER0x200
+#define VMBUS_CHANNEL_REQUEST_MONITORED_NOTIFICATION  0x400
+#define VMBUS_CHANNEL_TLNPI_PROVIDER_OFFER0x2000
+
+#define VMBUS_PACKET_FLAG_REQUEST_COMPLETION1
+
+typedef struct vmbus_message_header {
+uint32_t message_type;
+uint32_t _padding;
+} vmbus_message_header;
+
+typedef struct vmbus_message_initiate_contact {
+vmbus_message_header header;
+uint32_t version_requested;
+uint32_t target_vcpu;
+uint64_t interrupt_page;
+uint64_t monitor_page1;
+uint64_t monitor_page2;
+} vmbus_message_initiate_contact;
+
+typedef struct vmbus_message_version_response {
+vmbus_message_header header;
+uint8_t version_supported;
+uint8_t status;
+} vmbus_message_version_response;
+
+typedef struct vmbus_message_offer_channel {
+vmbus_message_header header;
+uint8_t  type_uuid[16];
+uint8_t  instance_uuid[16];
+uint64_t _reserved1;
+uint64_t _reserved2;
+uint16_t channel_flags;
+uint16_t mmio_size_mb;
+uint8_t  user_data[VMBUS_CHANNEL_USER_DATA_SIZE];
+uint16_t sub_channel_index;
+uint16_t _reserved3;
+uint32_t child_relid;
+uint8_t  monitor_id;
+uint8_t  monitor_flags;
+uint16_t 

[PATCH v3 1/7] hyperv: expose API to determine if synic is enabled

[Jon Doron]

Signed-off-by: Jon Doron 
---
 hw/hyperv/hyperv.c | 8 
 include/hw/hyperv/hyperv.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 8ca3706f5b..ddf4f32c60 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -37,6 +37,13 @@ typedef struct SynICState {
 #define TYPE_SYNIC "hyperv-synic"
 #define SYNIC(obj) OBJECT_CHECK(SynICState, (obj), TYPE_SYNIC)
 
+static bool synic_enabled;
+
+bool hyperv_is_synic_enabled(void)
+{
+return synic_enabled;
+}
+
 static SynICState *get_synic(CPUState *cs)
 {
 return SYNIC(object_resolve_path_component(OBJECT(cs), "synic"));
@@ -133,6 +140,7 @@ void hyperv_synic_add(CPUState *cs)
 object_property_add_child(OBJECT(cs), "synic", obj, _abort);
 object_unref(obj);
 object_property_set_bool(obj, true, "realized", _abort);
+synic_enabled = true;
 }
 
 void hyperv_synic_reset(CPUState *cs)
diff --git a/include/hw/hyperv/hyperv.h b/include/hw/hyperv/hyperv.h
index 597381cb01..a63ee0003c 100644
--- a/include/hw/hyperv/hyperv.h
+++ b/include/hw/hyperv/hyperv.h
@@ -79,5 +79,6 @@ void hyperv_synic_add(CPUState *cs);
 void hyperv_synic_reset(CPUState *cs);
 void hyperv_synic_update(CPUState *cs, bool enable,
  hwaddr msg_page_addr, hwaddr event_page_addr);
+bool hyperv_is_synic_enabled(void);
 
 #endif
-- 
2.24.1

[PATCH v3 0/7] hyperv: VMBus implementation

[Jon Doron]

This is a rebase of the old patchset from Roman for HyperV VMBus
implementation.

How to use:
-device vmbus-bridge

Later on new paravirtualized devices can be implemented on top of it
(Network/SCSI/etc.)

VMBus is a collection of technologies.  At its lowest layer, it's a message
passing and signaling mechanism, allowing efficient passing of messages to and
from guest VMs.  A layer higher, it's a mechanism for defining channels of
communication, where each channel is tagged with a type (which implies a
protocol) and a instance ID.  A layer higher than that, it's a bus driver,
serving as the basis of device enumeration within a VM, where a channel can
optionally be exposed as a paravirtual device.  When a server-side (paravirtual
back-end) component wishes to offer a channel to a guest VM, it does so by
specifying a channel type, a mode, and an instance ID.  VMBus then exposes this
in the guest.

More information about VMBus can be found in the file
vmbuskernelmodeclientlibapi.h in Microsoft's WDK.

v3:
Fixed an error asan

v2:
Rebased on top of latest patchset from Roman and Maciej

Jon Doron (7):
  hyperv: expose API to determine if synic is enabled
  hyperv: SControl is optional to enable SynIc
  vmbus: add vmbus protocol definitions
  vmbus: vmbus implementation
  i386:pc: whitelist dynamic vmbus-bridge
  i386: Hyper-V VMBus ACPI DSDT entry
  vmbus: add infrastructure to save/load vmbus requests

 Makefile.objs|1 +
 hw/hyperv/Kconfig|5 +
 hw/hyperv/Makefile.objs  |1 +
 hw/hyperv/hyperv.c   |  250 ++-
 hw/hyperv/trace-events   |   18 +
 hw/hyperv/vmbus.c| 2778 ++
 hw/i386/acpi-build.c |   43 +
 hw/i386/pc_piix.c|2 +
 hw/i386/pc_q35.c |2 +
 include/hw/hyperv/hyperv.h   |3 +
 include/hw/hyperv/vmbus-bridge.h |   35 +
 include/hw/hyperv/vmbus-proto.h  |  222 +++
 include/hw/hyperv/vmbus.h|  230 +++
 target/i386/hyperv.c |2 +
 14 files changed, 3510 insertions(+), 82 deletions(-)
 create mode 100644 hw/hyperv/trace-events
 create mode 100644 hw/hyperv/vmbus.c
 create mode 100644 include/hw/hyperv/vmbus-bridge.h
 create mode 100644 include/hw/hyperv/vmbus-proto.h
 create mode 100644 include/hw/hyperv/vmbus.h

-- 
2.24.1

Re: [PATCH for-5.0 v2 1/3] block-backend: Reorder flush/pdiscard function definitions

[Vladimir Sementsov-Ogievskiy]

06.04.2020 20:14, Kevin Wolf wrote:

Move all variants of the flush/pdiscard functions to a single place and
put the blk_co_*() version first because it is called by all other
variants (and will become static in the next patch).

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH for-5.0] xen-block: Fix uninitialized variable

[Markus Armbruster]

Anthony PERARD  writes:

> On Mon, Apr 06, 2020 at 06:50:41PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/6/20 6:42 PM, Anthony PERARD wrote:
>> > Since 7f5d9b206d1e ("object-add: don't create return value if
>> > failed"), qmp_object_add() don't write any value in 'ret_data', thus
>> > has random data. Then qobject_unref() fails and abort().
>> > 
>> > Fix by initialising 'ret_data' properly.
>> 
>> Or move qobject_unref() after the error check?
>> 
>> -- >8 --
>> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
>> index 07bb32e22b..f3f1cbef65 100644
>> --- a/hw/block/xen-block.c
>> +++ b/hw/block/xen-block.c
>> @@ -869,7 +869,6 @@ static XenBlockIOThread *xen_block_iothread_create(const
>> char *id,
>>  qdict_put_str(opts, "id", id);
>>  qmp_object_add(opts, _data, _err);
>>  qobject_unref(opts);
>> -qobject_unref(ret_data);
>> 
>>  if (local_err) {
>>  error_propagate(errp, local_err);
>> @@ -878,6 +877,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
>> char *id,
>>  g_free(iothread);
>>  return NULL;
>>  }
>> +qobject_unref(ret_data);
>
> That won't help, qmp_object_add() doesn't change the value of ret_data
> at all. The other users of qmp_object_add() passes an initialised
> 'ret_data', so we should do the same I think.

Since the QMP core does it, other callers should do it, too.

For QAPI commands that don't return anything, the marshaller should not
use @ret_data, let alone store through it.  qmp_object_add() complies.
Thus, assert(!ret_data) would do.  qobject_unref(ret_data) is also
correct.

Reviewed-by: Markus Armbruster 

Re: [PATCH v9 5/9] docs/clocks: add device's clock documentation

[Markus Armbruster]

Damien Hedde  writes:

> Add the documentation about the clock inputs and outputs in devices.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde 
> Reviewed-by: Alistair Francis 
> Reviewed-by: Edgar E. Iglesias 
> ---
> v9:
>  + fix a few typos (Alistair)
>
> v8:
>  + fix list indentation
>  + reduce title size
>
> v7:
>  + update ClockIn/Out types
>  + switch to rst format
> ---
>  docs/devel/clocks.rst | 360 ++
>  docs/devel/index.rst  |   1 +
>  2 files changed, 361 insertions(+)
>  create mode 100644 docs/devel/clocks.rst
>
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> new file mode 100644
> index 00..ead9f55561
> --- /dev/null
> +++ b/docs/devel/clocks.rst
> @@ -0,0 +1,360 @@
> +Modeling a clock tree in QEMU
> +=
> +
> +What are clocks
> +---
> +
> +Clocks are QOM objects developed for the purpose of modeling the
> +distribution of clocks in QEMU.
> +
> +They allow us to model the clock distribution of a platform and detect
> +configuration errors in the clock tree such as badly configured PLL, clock
> +source selection or disabled clock.
> +
> +The object is *Clock* and its QOM name is ``CLOCK``.

PATCH 1 has

#define TYPE_CLOCK "clock"

Ignorant question: how is this related to *Clock* and ``CLOCK``?

[...]

Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs

[Richard Henderson]

On 4/6/20 9:04 PM, Max Filippov wrote:
> On Mon, Apr 6, 2020 at 8:09 PM Richard Henderson
>  wrote:
>>
>> Rather than dynamically allocate, and risk failing to free
>> when we longjmp out of the translator, allocate the maximum
>> buffer size from any of the supported cpus, which is 8:
> 
> There's macro MAX_INSN_LENGTH that defines maximal supported
> instruction length in bytes. Maybe the following instead, along the lines
> of what libisa does dynamically?:

Thanks for the pointer.  Looks better than mine.

>  #define MAX_INSN_LENGTH 64
> +#define MAX_INSNBUF_LENGTH \
> +((MAX_INSN_LENGTH + sizeof(xtensa_insnbuf_word) - 1) / \
> + sizeof(xtensa_insnbuf_word))

There is a ROUND_UP macro, but it seems unnecessary.


r~

[PULL 09/10] pseries: Update SLOF firmware image

[David Gibson]

From: Alexey Kardashevskiy 

This is a single regression fix for for 5.0:

Greg Kurz (1):
  slof: Only close stdout for virtio-serial devices

Signed-off-by: Alexey Kardashevskiy 
Signed-off-by: David Gibson 
---
 pc-bios/README   |   2 +-
 pc-bios/slof.bin | Bin 965008 -> 965112 bytes
 roms/SLOF|   2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/README b/pc-bios/README
index f54c2743d0..a5a770f066 100644
--- a/pc-bios/README
+++ b/pc-bios/README
@@ -14,7 +14,7 @@
 - SLOF (Slimline Open Firmware) is a free IEEE 1275 Open Firmware
   implementation for certain IBM POWER hardware.  The sources are at
   https://github.com/aik/SLOF, and the image currently in qemu is
-  built from git tag qemu-slof-20200317.
+  built from git tag qemu-slof-20200327.
 
 - sgabios (the Serial Graphics Adapter option ROM) provides a means for
   legacy x86 software to communicate with an attached serial console as
diff --git a/pc-bios/slof.bin b/pc-bios/slof.bin
index 
40499a1451f4f63f79f64f2457ae62b08d7d66eb..80bbf91a186c7eec76af3ae6af50c2c841a77579
 100644
GIT binary patch
delta 4653
zcmbVOeRNah8Ncs+)8u}%X%Gaf^*;Y$qK^m~ML5GyX#_Aag&1QN&^)lEu|fd
z2IrutjIdgss^>st4mx+H;q!Ki5}jvuoT^=qJL`6K?uq=t7(Fxo%*y=S^-Ty?^YF
z^_<)HKEL1dd%oT`_ula5OU6H660UHJZwj=Pdi?IY%NrLpH8yTs^o0NM9qyhvWTshB
z=p9qm9u7`5T`H5_he|~#>2Z_0PHD%hZJCPljegIWuy=r8SYDx?aCc
zQI4cMuvU54zr8iEZL6|WG3H7tci%s9Q5qQK*q<~IfpwIu=Fi5VpOOmFqQ)t?Zu-9>
zs|kU0QC_>MhO#Nao9U1F;ii2?K7OAln6Y6MT(0mlK=X{RLTNCt>v?Grx5T
z|EDqaTLt{Z<)l_M`S`UZ=ur1GUw98wgf8;;n@F?z?r->A0tu?#A-E(wn$={U-jhdnrJy(ld$(!Ipy4sXPJoMO|IsI6$4GZS>R%rv!(nx2Q4zS+;CTP*c#T@ArKIS`1
zIHl_qFLxMSyhk~k-Y?5E0Jkb>F?`%eA1u%YtB-T0d*9Uh#`kdCt{-tq^^=^b=ax1E
z-}ci2t2W5WJ5bDVJ=YLx^V7|UU9V4!9>R{A(6J0SU
zT6r85KiB#zpX5xTr?i)`e4bU}@o~4Xg+cIbq8p|~SAF%>0Uwv#NmJR+IBtBi)(;+yTUT-2%jlrrhuxBq9JiJ>{+J`t(qvwVYTe;
zn%?PslC-h=U2i%Y8Rxm+7dfth~{lU(4_mPF?k#iyiNS>}
z^c5*JckFZsf-STl!{H{lTKtE_4~{Iw-CkV0pLWiDYU96)v^co8QW7H9U%5f?GU1h_DmTo3{6sDPi{+e~gv3iH%wTYvT0b@Sujec^W>9w?ep_uSmK9^IzSm?PXt@Ci{C*uoY1~UkGKQOTo!-);#)sTqs^{W^<-T6gBph
z@SfxfG1HpoxP(h4A5tTNIN{_Ui9|*gS;NWj32VBpoww>Z@gm4h+5X(pwjCF|4wD%=R_bz8@|Efr4#8aW?Azt
z72uLl9n!UoW)Pzb4K#%iqfI(pVxUbB?Zgr|h@%(b)bUQSVhRz!eGpAStvM)m5ton>
ze`+>}1vUN?af68N-oB*!x<2uS*=0+)Pox{s=e*tqzGGNQnsykX+0^hc5iG9!q}*;=
zr1M^8;~etAds4hX^1m8sR6sN*2Ec{d}lBhM1pW
zH9A(6!g>*d4I`ojUQLL_a0u`0^>3jM%auMZMKG4YDHrL|4-9l}~U9U$fONQd>
zKZVeU=;+-i<+F*sZos$AB)SfKymSc44)Ilb62UhK5TJ?ZDo?K_$D(gCg9Th7nzy9`!<~rIJ%z
zsM8wM;viukmGY9af}>PAkPJP=-uQs7^)hFih!?yjoEJMVu)~A{^r+n?={=I1AUtGF
zjWjpN4n>6JE*0UI2xc9N@4>FS+!>{GVaQ%$Y2NVT-<3kR`B8
zYQ7+CBBTyN;_YEIy`$^W{T4wKX;B|_(-nxI34awO8q1tvsk8CeKk>Gn`IZA}fO
zUAkTOrXQqwIe{3a9FU|Hr0$4}6|oZri)5)db;_G$eUkR(^zSZRN*#n{c8;N+>B{;=
z3*USnxKD}>^-EbIBz3l%Q)e3-IkyjY^OuHc)PXk#+hD)}A2<7@xRxY9vR>
zkgJ3(0wH`eyi(P#NE~-anSu=XK
zD>{DFfU|0`_~f@hrhghQGWdIR|C$tDlHON)txhyg76@S-@5Xnu?0lquGHnm)^q>I`
z>G)Lx_lmh448%rqj~A2jZBni*0dPfOx|q3<2oLijI$S7(w~LRh~rRn
zJZiwbIg<6Z;Ks$y~0CGkhU{_#0t
zIuT3~xA9>e?>69Z9gi6BF&!TOpDH^FrZauUc$_w!zY1YhcCKVrCe8Z5`ex)CaF33=
z4S0=?*T5KtOMVTq58u=y<(GT)YG!&=Wk9wPe$61be=T?6w@Z{QHQ;_--p8-M1p}|k
z6$C$|a1mQrOwKa11>A4Q)uaW2Z0UpO8*$arO|?h83P|16FsMO;=d
zM2}dXf%r?-9Dd~ku=fS43%&=|`KC2B%U8qOV9kdY%6W`Zx7VBls2?%
zYxS45wl!|s(WbN(D#fp^^J|CPwWWzTO0hP
p9c|LR;C;}FAD+)x=Yi5=wW!zntXFt8b#?ef>xG%7*BrmE{4a5VBOw3)

delta 4787
zcmbVPdvsIv9lyVOFUfthw@HDfDYOLiC@q5qAR_a=#dSLVn6MS1>>P1|PIZdkb3F~4y^?ZSHdLk#NIuf6dG(c`
z%R7mGhdFW(Oed7Q?o#4B8aY^H%Jf(;zhUlizca+npS#F5?n`EPI?P4#6FWe>gaKTqb!cmv@9QbvI*17xc}Z@jM(%W_h!c79J*=8|*j#|HMniR`B{
z)nx_j+2y2D<=yPs5;R!%6gy)9_3)i#m-3`vee2Kcc7X&`=X-1wSstih8NRP6^71`R
zJAZXg-J6yBsv}1#Ze#d;C6SjZW+b^ktnx9oc7YvOY-1SYUMb(e@Q!}Q;P5dCM_nYK
zzV$x)d=Uw%?yuQt6Hxm8$v!Qh%=$O`s4+W4{b{VniX$P_Pq=9Y5{96W`;dfHpTJ$F

[PULL 10/10] ppc/pnv: Create BMC devices only when defaults are enabled

[David Gibson]

From: Cédric Le Goater 

Commit e2392d4395dd ("ppc/pnv: Create BMC devices at machine init")
introduced default BMC devices which can be a problem when the same
devices are defined on the command line with :

  -device ipmi-bmc-sim,id=bmc0 -device isa-ipmi-bt,bmc=bmc0,irq=10

QEMU fails with :

  qemu-system-ppc64: error creating device tree: node: FDT_ERR_EXISTS

Use defaults_enabled() when creating the default BMC devices to let
the user provide its own BMC devices using '-nodefaults'. If no BMC
device are provided, output a warning but let QEMU run as this is a
supported configuration. However, when multiple BMC devices are
defined, stop QEMU with a clear error as the results are unexpected.

Fixes: e2392d4395dd ("ppc/pnv: Create BMC devices at machine init")
Reported-by: Nathan Chancellor 
Signed-off-by: Cédric Le Goater 
Message-Id: <20200404153655.166834-1-...@kaod.org>
Tested-by: Nathan Chancellor 
Signed-off-by: David Gibson 
---
 hw/ppc/pnv.c | 32 ++-
 hw/ppc/pnv_bmc.c | 45 
 include/hw/ppc/pnv.h |  2 ++
 3 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index b75ad06390..c9cb6fa357 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -571,10 +571,29 @@ static void pnv_powerdown_notify(Notifier *n, void 
*opaque)
 
 static void pnv_reset(MachineState *machine)
 {
+PnvMachineState *pnv = PNV_MACHINE(machine);
+IPMIBmc *bmc;
 void *fdt;
 
 qemu_devices_reset();
 
+/*
+ * The machine should provide by default an internal BMC simulator.
+ * If not, try to use the BMC device that was provided on the command
+ * line.
+ */
+bmc = pnv_bmc_find(_fatal);
+if (!pnv->bmc) {
+if (!bmc) {
+warn_report("machine has no BMC device. Use '-device "
+"ipmi-bmc-sim,id=bmc0 -device 
isa-ipmi-bt,bmc=bmc0,irq=10' "
+"to define one");
+} else {
+pnv_bmc_set_pnor(bmc, pnv->pnor);
+pnv->bmc = bmc;
+}
+}
+
 fdt = pnv_dt_create(machine);
 
 /* Pack resulting tree */
@@ -833,9 +852,6 @@ static void pnv_init(MachineState *machine)
 }
 g_free(chip_typename);
 
-/* Create the machine BMC simulator */
-pnv->bmc = pnv_bmc_create(pnv->pnor);
-
 /* Instantiate ISA bus on chip 0 */
 pnv->isa_bus = pnv_isa_create(pnv->chips[0], _fatal);
 
@@ -845,8 +861,14 @@ static void pnv_init(MachineState *machine)
 /* Create an RTC ISA device too */
 mc146818_rtc_init(pnv->isa_bus, 2000, NULL);
 
-/* Create the IPMI BT device for communication with the BMC */
-pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
+/*
+ * Create the machine BMC simulator and the IPMI BT device for
+ * communication with the BMC
+ */
+if (defaults_enabled()) {
+pnv->bmc = pnv_bmc_create(pnv->pnor);
+pnv_ipmi_bt_init(pnv->isa_bus, pnv->bmc, 10);
+}
 
 /*
  * OpenPOWER systems use a IPMI SEL Event message to notify the
diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
index 8863354c1c..4e018b8b70 100644
--- a/hw/ppc/pnv_bmc.c
+++ b/hw/ppc/pnv_bmc.c
@@ -213,6 +213,18 @@ static const IPMINetfn hiomap_netfn = {
 .cmd_handlers = hiomap_cmds
 };
 
+
+void pnv_bmc_set_pnor(IPMIBmc *bmc, PnvPnor *pnor)
+{
+object_ref(OBJECT(pnor));
+object_property_add_const_link(OBJECT(bmc), "pnor", OBJECT(pnor),
+   _abort);
+
+/* Install the HIOMAP protocol handlers to access the PNOR */
+ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(bmc), IPMI_NETFN_OEM,
+_netfn);
+}
+
 /*
  * Instantiate the machine BMC. PowerNV uses the QEMU internal
  * simulator but it could also be external.
@@ -232,3 +244,36 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
 
 return IPMI_BMC(obj);
 }
+
+typedef struct ForeachArgs {
+const char *name;
+Object *obj;
+} ForeachArgs;
+
+static int bmc_find(Object *child, void *opaque)
+{
+ForeachArgs *args = opaque;
+
+if (object_dynamic_cast(child, args->name)) {
+if (args->obj) {
+return 1;
+}
+args->obj = child;
+}
+return 0;
+}
+
+IPMIBmc *pnv_bmc_find(Error **errp)
+{
+ForeachArgs args = { TYPE_IPMI_BMC_SIMULATOR, NULL };
+int ret;
+
+ret = object_child_foreach_recursive(object_get_root(), bmc_find, );
+if (ret) {
+error_setg(errp, "machine should have only one BMC device. "
+   "Use '-nodefaults'");
+return NULL;
+}
+
+return args.obj ? IPMI_BMC(args.obj) : NULL;
+}
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index fb4d0c0234..d4b0b0e2ff 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -241,6 +241,8 @@ struct PnvMachineState {
 void pnv_dt_bmc_sensors(IPMIBmc *bmc, void *fdt);
 void pnv_bmc_powerdown(IPMIBmc *bmc);
 IPMIBmc *pnv_bmc_create(PnvPnor *pnor);
+IPMIBmc *pnv_bmc_find(Error 

[PULL 05/10] ppc/spapr: Add FWNMI machine check delivery warnings

[David Gibson]

From: Nicholas Piggin 

Add some messages which explain problems and guest misbehaviour that
may be difficult to diagnose in rare cases of machine checks.

Signed-off-by: Nicholas Piggin 
Message-Id: <20200325142906.221248-4-npig...@gmail.com>
Reviewed-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_events.c | 4 
 hw/ppc/spapr_rtas.c   | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index a908c5d0e9..c8964eb25d 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -833,6 +833,8 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool 
recovered)
 /* get rtas addr from fdt */
 rtas_addr = spapr_get_rtas_addr();
 if (!rtas_addr) {
+error_report(
+"FWNMI: Unable to deliver machine check to guest: rtas_addr not found.");
 qemu_system_guest_panicked(NULL);
 g_free(ext_elog);
 return;
@@ -874,6 +876,8 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
  * that CPU called "ibm,nmi-interlock")
  */
 if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) {
+error_report(
+"FWNMI: Unable to deliver machine check to guest: nested machine check.");
 qemu_system_guest_panicked(NULL);
 return;
 }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 29abe66d01..bcac0d00e7 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -462,6 +462,9 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
 }
 
 if (spapr->fwnmi_machine_check_addr == -1) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"FWNMI: ibm,nmi-interlock RTAS called with FWNMI not registered.\n");
+
 /* NMI register not called */
 rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 return;
-- 
2.25.2

[PULL 06/10] ppc/spapr: Don't kill the guest if a recovered FWNMI machine check delivery fails

[David Gibson]

From: Nicholas Piggin 

Try to be tolerant of FWNMI delivery errors if the machine check had been
recovered by the host.

Signed-off-by: Nicholas Piggin 
Message-Id: <20200325142906.221248-5-npig...@gmail.com>
Reviewed-by: Greg Kurz 
[dwg: Updated comment at Greg's suggestion]
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_events.c | 30 +-
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index c8964eb25d..1069d0197b 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -833,13 +833,28 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool 
recovered)
 /* get rtas addr from fdt */
 rtas_addr = spapr_get_rtas_addr();
 if (!rtas_addr) {
-error_report(
+if (!recovered) {
+error_report(
 "FWNMI: Unable to deliver machine check to guest: rtas_addr not found.");
-qemu_system_guest_panicked(NULL);
+qemu_system_guest_panicked(NULL);
+} else {
+warn_report(
+"FWNMI: Unable to deliver machine check to guest: rtas_addr not found. "
+"Machine check recovered.");
+}
 g_free(ext_elog);
 return;
 }
 
+/*
+ * By taking the interlock, we assume that the MCE will be
+ * delivered to the guest. CAUTION: don't add anything that could
+ * prevent the MCE to be delivered after this line, otherwise the
+ * guest won't be able to release the interlock and ultimately
+ * hang/crash?
+ */
+spapr->fwnmi_machine_check_interlock = cpu->vcpu_id;
+
 stq_be_phys(_space_memory, rtas_addr + RTAS_ERROR_LOG_OFFSET,
 env->gpr[3]);
 cpu_physical_memory_write(rtas_addr + RTAS_ERROR_LOG_OFFSET +
@@ -876,9 +891,15 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
  * that CPU called "ibm,nmi-interlock")
  */
 if (spapr->fwnmi_machine_check_interlock == cpu->vcpu_id) {
-error_report(
+if (!recovered) {
+error_report(
 "FWNMI: Unable to deliver machine check to guest: nested machine check.");
-qemu_system_guest_panicked(NULL);
+qemu_system_guest_panicked(NULL);
+} else {
+warn_report(
+"FWNMI: Unable to deliver machine check to guest: nested machine check. "
+"Machine check recovered.");
+}
 return;
 }
 qemu_cond_wait_iothread(>fwnmi_machine_check_interlock_cond);
@@ -906,7 +927,6 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
 warn_report("Received a fwnmi while migration was in progress");
 }
 
-spapr->fwnmi_machine_check_interlock = cpu->vcpu_id;
 spapr_mce_dispatch_elog(cpu, recovered);
 }
 
-- 
2.25.2

[PULL 08/10] hw/ppc/ppc440_uc.c: Remove incorrect iothread locking from dcr_write_pcie()

[David Gibson]

From: Peter Maydell 

In dcr_write_pcie() we take the iothread lock around a call to
pcie_host_mmcfg_udpate().  This is an incorrect attempt to deal with
the bug fixed in commit 235352ee6e73d7716, where we were not taking
the iothread lock before calling device dcr read/write functions.
(It's not sufficient locking, because although the other cases in the
switch statement won't assert, there is no locking which prevents
multiple guest CPUs from trying to access the PPC460EXPCIEState
struct at the same time and corrupting data.)

Unfortunately with commit 235352ee6e73d7716 we are now trying
to recursively take the iothread lock, which will assert:

  $ qemu-system-ppc -M sam460ex --display none
  **
  
ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/cpus.c:1830:qemu_mutex_lock_iothread_impl:
 assertion failed: (!qemu_mutex_iothread_locked())
  Aborted (core dumped)

Remove the locking within dcr_write_pcie().

Fixes: 235352ee6e73d7716
Signed-off-by: Peter Maydell 
Message-Id: <20200330125228.24994-1-peter.mayd...@linaro.org>
Tested-by: BALATON Zoltan 
Signed-off-by: David Gibson 
---
 hw/ppc/ppc440_uc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index d5ea962249..b30e093cbb 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -13,7 +13,6 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "cpu.h"
 #include "hw/irq.h"
@@ -1183,9 +1182,7 @@ static void dcr_write_pcie(void *opaque, int dcrn, 
uint32_t val)
 case PEGPL_CFGMSK:
 s->cfg_mask = val;
 size = ~(val & 0xfffe) + 1;
-qemu_mutex_lock_iothread();
 pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, 
size);
-qemu_mutex_unlock_iothread();
 break;
 case PEGPL_MSGBAH:
 s->msg_base = ((uint64_t)val << 32) | (s->msg_base & 0x);
-- 
2.25.2

[PULL 04/10] ppc/spapr: Improve FWNMI machine check delivery corner case comments

[David Gibson]

From: Nicholas Piggin 

Some of the conditions are not as clearly documented as they could be.
Also the non-FWNMI case does not need a large comment.

Reviewed-by: Greg Kurz 
Signed-off-by: Nicholas Piggin 
Message-Id: <20200325142906.221248-3-npig...@gmail.com>
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_events.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index a4a540f43d..a908c5d0e9 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -860,17 +860,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
 Error *local_err = NULL;
 
 if (spapr->fwnmi_machine_check_addr == -1) {
-/*
- * This implies that we have hit a machine check either when the
- * guest has not registered FWNMI (i.e., "ibm,nmi-register" not
- * called) or between system reset and "ibm,nmi-register".
- * Fall back to the old machine check behavior in such cases.
- */
+/* Non-FWNMI case, deliver it like an architected CPU interrupt. */
 cs->exception_index = POWERPC_EXCP_MCHECK;
 ppc_cpu_do_interrupt(cs);
 return;
 }
 
+/* Wait for FWNMI interlock. */
 while (spapr->fwnmi_machine_check_interlock != -1) {
 /*
  * Check whether the same CPU got machine check error
@@ -882,8 +878,13 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
 return;
 }
 qemu_cond_wait_iothread(>fwnmi_machine_check_interlock_cond);
-/* Meanwhile if the system is reset, then just return */
 if (spapr->fwnmi_machine_check_addr == -1) {
+/*
+ * If the machine was reset while waiting for the interlock,
+ * abort the delivery. The machine check applies to a context
+ * that no longer exists, so it wouldn't make sense to deliver
+ * it now.
+ */
 return;
 }
 }
@@ -894,7 +895,9 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
  * We don't want to abort so we let the migration to continue.
  * In a rare case, the machine check handler will run on the target.
  * Though this is not preferable, it is better than aborting
- * the migration or killing the VM.
+ * the migration or killing the VM. It is okay to call
+ * migrate_del_blocker on a blocker that was not added (which the
+ * nmi-interlock handler would do when it's called after this).
  */
 warn_report("Received a fwnmi while migration was in progress");
 }
-- 
2.25.2

[PULL 03/10] ppc/spapr: KVM FWNMI should not be enabled until guest requests it

[David Gibson]

From: Nicholas Piggin 

The KVM FWNMI capability should be enabled with the "ibm,nmi-register"
rtas call. Although MCEs from KVM will be delivered as architected
interrupts to the guest before "ibm,nmi-register" is called, KVM has
different behaviour depending on whether the guest has enabled FWNMI
(it attempts to do more recovery on behalf of a non-FWNMI guest).

Signed-off-by: Nicholas Piggin 
Message-Id: <20200325142906.221248-2-npig...@gmail.com>
Reviewed-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_caps.c  | 7 ---
 hw/ppc/spapr_rtas.c  | 7 +++
 target/ppc/kvm.c | 7 +++
 target/ppc/kvm_ppc.h | 6 ++
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 679ae7959f..eb54f94227 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -517,9 +517,10 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
uint8_t val,
 }
 
 if (kvm_enabled()) {
-if (kvmppc_set_fwnmi() < 0) {
-error_setg(errp, "Firmware Assisted Non-Maskable Interrupts(FWNMI) 
"
- "not supported by KVM");
+if (!kvmppc_get_fwnmi()) {
+error_setg(errp,
+"Firmware Assisted Non-Maskable Interrupts(FWNMI) not supported by KVM.");
+error_append_hint(errp, "Try appending -machine cap-fwnmi=off\n");
 }
 }
 }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 9fb8c8632a..29abe66d01 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -437,6 +437,13 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
 return;
 }
 
+if (kvm_enabled()) {
+if (kvmppc_set_fwnmi() < 0) {
+rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+return;
+}
+}
+
 spapr->fwnmi_system_reset_addr = sreset_addr;
 spapr->fwnmi_machine_check_addr = mce_addr;
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 597f72be1b..03d0667e8f 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -88,6 +88,7 @@ static int cap_ppc_safe_indirect_branch;
 static int cap_ppc_count_cache_flush_assist;
 static int cap_ppc_nested_kvm_hv;
 static int cap_large_decr;
+static int cap_fwnmi;
 
 static uint32_t debug_inst_opcode;
 
@@ -136,6 +137,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 kvmppc_get_cpu_characteristics(s);
 cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
 cap_large_decr = kvmppc_get_dec_bits();
+cap_fwnmi = kvm_vm_check_extension(s, KVM_CAP_PPC_FWNMI);
 /*
  * Note: setting it to false because there is not such capability
  * in KVM at this moment.
@@ -2064,6 +2066,11 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int 
mpic_proxy)
 }
 }
 
+bool kvmppc_get_fwnmi(void)
+{
+return cap_fwnmi;
+}
+
 int kvmppc_set_fwnmi(void)
 {
 PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 332fa0aa1c..fcaf745516 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -27,6 +27,7 @@ void kvmppc_enable_h_page_init(void);
 void kvmppc_set_papr(PowerPCCPU *cpu);
 int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
 void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
+bool kvmppc_get_fwnmi(void);
 int kvmppc_set_fwnmi(void);
 int kvmppc_smt_threads(void);
 void kvmppc_error_append_smt_possible_hint(Error *const *errp);
@@ -163,6 +164,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, 
int mpic_proxy)
 {
 }
 
+static inline bool kvmppc_get_fwnmi(void)
+{
+return false;
+}
+
 static inline int kvmppc_set_fwnmi(void)
 {
 return -1;
-- 
2.25.2

[PULL 02/10] vfio/spapr: Fix page size calculation

[David Gibson]

From: Alexey Kardashevskiy 

Coverity detected an issue (CID 1421903) with potential call of clz64(0)
which returns 64 which make it do "<<" with a negative number.

This checks the mask and avoids undefined behaviour.

In practice pgsizes and memory_region_iommu_get_min_page_size() always
have some common page sizes and even if they did not, the resulting page
size would be 0x8000... (gcc 9.2) and
ioctl(VFIO_IOMMU_SPAPR_TCE_CREATE) would fail anyway.

Signed-off-by: Alexey Kardashevskiy 
Message-Id: <20200324063912.25063-1-...@ozlabs.ru>
Reviewed-by: Greg Kurz 
Signed-off-by: David Gibson 
---
 hw/vfio/spapr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 33692fc86f..2900bd1941 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -147,7 +147,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
 {
 int ret = 0;
 IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
-uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr);
+uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr), 
pgmask;
 unsigned entries, bits_total, bits_per_level, max_levels;
 struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) };
 long rampagesize = qemu_minrampagesize();
@@ -159,8 +159,8 @@ int vfio_spapr_create_window(VFIOContainer *container,
 if (pagesize > rampagesize) {
 pagesize = rampagesize;
 }
-pagesize = 1ULL << (63 - clz64(container->pgsizes &
-   (pagesize | (pagesize - 1;
+pgmask = container->pgsizes & (pagesize | (pagesize - 1));
+pagesize = pgmask ? (1ULL << (63 - clz64(pgmask))) : 0;
 if (!pagesize) {
 error_report("Host doesn't support page size 0x%"PRIx64
  ", the supported mask is 0x%lx",
-- 
2.25.2

[PULL 00/10] ppc-for-5.0 queue 20200407

[David Gibson]

The following changes since commit 53ef8a92eb04ee19640f5aad3bff36cd4a36c250:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200406' 
into staging (2020-04-06 12:36:45 +0100)

are available in the Git repository at:

  git://github.com/dgibson/qemu.git tags/ppc-for-5.0-20200407

for you to fetch changes up to 25f3170b06544e4de620336da5b2ea3b392d66bc:

  ppc/pnv: Create BMC devices only when defaults are enabled (2020-04-07 
08:55:11 +1000)


ppc patch queue 2020-04-07

An assortment of fixes for qemu-5.0, including a number for the FWNMI
feature which is new this release.


Alexey Kardashevskiy (2):
  vfio/spapr: Fix page size calculation
  pseries: Update SLOF firmware image

Cédric Le Goater (1):
  ppc/pnv: Create BMC devices only when defaults are enabled

David Gibson (1):
  spapr: Fix failure path for attempting to hot unplug PCI bridges

Nicholas Piggin (4):
  ppc/spapr: KVM FWNMI should not be enabled until guest requests it
  ppc/spapr: Improve FWNMI machine check delivery corner case comments
  ppc/spapr: Add FWNMI machine check delivery warnings
  ppc/spapr: Don't kill the guest if a recovered FWNMI machine check 
delivery fails

Peter Maydell (2):
  hw/ppc/e500.c: Handle qemu_find_file() failure
  hw/ppc/ppc440_uc.c: Remove incorrect iothread locking from 
dcr_write_pcie()

 hw/ppc/e500.c |   4 
 hw/ppc/pnv.c  |  32 +++-
 hw/ppc/pnv_bmc.c  |  45 +
 hw/ppc/ppc440_uc.c|   3 ---
 hw/ppc/spapr_caps.c   |   7 ---
 hw/ppc/spapr_events.c |  49 ++---
 hw/ppc/spapr_pci.c|   1 +
 hw/ppc/spapr_rtas.c   |  10 ++
 hw/vfio/spapr.c   |   6 +++---
 include/hw/ppc/pnv.h  |   2 ++
 pc-bios/README|   2 +-
 pc-bios/slof.bin  | Bin 965008 -> 965112 bytes
 roms/SLOF |   2 +-
 target/ppc/kvm.c  |   7 +++
 target/ppc/kvm_ppc.h  |   6 ++
 15 files changed, 149 insertions(+), 27 deletions(-)

[PULL 01/10] hw/ppc/e500.c: Handle qemu_find_file() failure

[David Gibson]

From: Peter Maydell 

If qemu_find_file() doesn't find the BIOS it returns NULL; we were
passing that unchecked through to load_elf(), which assumes a non-NULL
pointer and may misbehave. In practice it fails with a weird message:

  $ qemu-system-ppc -M ppce500 -display none -kernel nonesuch
  Bad address
  qemu-system-ppc: could not load firmware '(null)'

Handle the failure case better:

  $ qemu-system-ppc -M ppce500 -display none -kernel nonesuch
  qemu-system-ppc: could not find firmware/kernel file 'nonesuch'

Spotted by Coverity (CID 1238954).

Signed-off-by: Peter Maydell 
Message-Id: <20200324121216.23899-1-peter.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: David Gibson 
---
 hw/ppc/e500.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 854cd3ac46..0d1f41197c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1047,6 +1047,10 @@ void ppce500_init(MachineState *machine)
 }
 
 filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, payload_name);
+if (!filename) {
+error_report("could not find firmware/kernel file '%s'", payload_name);
+exit(1);
+}
 
 payload_size = load_elf(filename, NULL, NULL, NULL,
 _entry, , NULL, NULL,
-- 
2.25.2

[PULL 07/10] spapr: Fix failure path for attempting to hot unplug PCI bridges

[David Gibson]

For various technical reasons we can't currently allow unplug a PCI to PCI
bridge on the pseries machine.  spapr_pci_unplug_request() correctly
generates an error message if that's attempted.

But.. if the given errp is not error_abort or error_fatal, it doesn't
actually stop trying to unplug the bridge anyway.

Fixes: 14e714900f6b "spapr: Allow hot plug/unplug of PCI bridges and devices 
under PCI bridges"
Signed-off-by: David Gibson 
Reviewed-by: Greg Kurz 
---
 hw/ppc/spapr_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 709a52780d..55ca9dee1e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1663,6 +1663,7 @@ static void spapr_pci_unplug_request(HotplugHandler 
*plug_handler,
 
 if (pc->is_bridge) {
 error_setg(errp, "PCI: Hot unplug of PCI bridges not supported");
+return;
 }
 
 /* ensure any other present functions are pending unplug */
-- 
2.25.2

Re: [PATCH v16 QEMU 04/16] vfio: Add save and load functions for VFIO PCI devices

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

On 2020/3/25 5:09, Kirti Wankhede wrote:
> These functions save and restore PCI device specific data - config
> space of PCI device.
> Tested save and restore with MSI and MSIX type.
> 
> Signed-off-by: Kirti Wankhede 
> Reviewed-by: Neo Jia 
> ---
>  hw/vfio/pci.c | 163 
> ++
>  include/hw/vfio/vfio-common.h |   2 +
>  2 files changed, 165 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 6c77c12e44b9..8deb11e87ef7 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -41,6 +41,7 @@
>  #include "trace.h"
>  #include "qapi/error.h"
>  #include "migration/blocker.h"
> +#include "migration/qemu-file.h"
>  
>  #define TYPE_VFIO_PCI "vfio-pci"
>  #define PCI_VFIO(obj)OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI)
> @@ -1632,6 +1633,50 @@ static void vfio_bars_prepare(VFIOPCIDevice *vdev)
>  }
>  }
>  
> +static int vfio_bar_validate(VFIOPCIDevice *vdev, int nr)
> +{
> +PCIDevice *pdev = >pdev;
> +VFIOBAR *bar = >bars[nr];
> +uint64_t addr;
> +uint32_t addr_lo, addr_hi = 0;
> +
> +/* Skip unimplemented BARs and the upper half of 64bit BARS. */
> +if (!bar->size) {
> +return 0;
> +}
> +
> +addr_lo = pci_default_read_config(pdev, PCI_BASE_ADDRESS_0 + nr * 4, 4);
> +
> +addr_lo = addr_lo & (bar->ioport ? PCI_BASE_ADDRESS_IO_MASK :
> +   PCI_BASE_ADDRESS_MEM_MASK);
> +if (bar->type == PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +addr_hi = pci_default_read_config(pdev,
> + PCI_BASE_ADDRESS_0 + (nr + 1) * 4, 
> 4);
> +}
> +
> +addr = ((uint64_t)addr_hi << 32) | addr_lo;
> +
> +if (!QEMU_IS_ALIGNED(addr, bar->size)) {
> +return -EINVAL;
> +}
> +
> +return 0;
> +}
> +
> +static int vfio_bars_validate(VFIOPCIDevice *vdev)
> +{
> +int i, ret;
> +
> +for (i = 0; i < PCI_ROM_SLOT; i++) {
> +ret = vfio_bar_validate(vdev, i);
> +if (ret) {
> +error_report("vfio: BAR address %d validation failed", i);
> +return ret;
> +}
> +}
> +return 0;
> +}
> +
>  static void vfio_bar_register(VFIOPCIDevice *vdev, int nr)
>  {
>  VFIOBAR *bar = >bars[nr];
> @@ -2414,11 +2459,129 @@ static Object *vfio_pci_get_object(VFIODevice 
> *vbasedev)
>  return OBJECT(vdev);
>  }
>  
> +static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> +{
> +VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +PCIDevice *pdev = >pdev;
> +uint16_t pci_cmd;
> +int i;
> +
> +for (i = 0; i < PCI_ROM_SLOT; i++) {
> +uint32_t bar;
> +
> +bar = pci_default_read_config(pdev, PCI_BASE_ADDRESS_0 + i * 4, 4);
> +qemu_put_be32(f, bar);
> +}
> +
> +qemu_put_be32(f, vdev->interrupt);
> +if (vdev->interrupt == VFIO_INT_MSI) {
> +uint32_t msi_flags, msi_addr_lo, msi_addr_hi = 0, msi_data;
> +bool msi_64bit;
> +
> +msi_flags = pci_default_read_config(pdev, pdev->msi_cap + 
> PCI_MSI_FLAGS,
> +2);
> +msi_64bit = (msi_flags & PCI_MSI_FLAGS_64BIT);
> +
> +msi_addr_lo = pci_default_read_config(pdev,
> + pdev->msi_cap + PCI_MSI_ADDRESS_LO, 
> 4);
> +qemu_put_be32(f, msi_addr_lo);
> +
> +if (msi_64bit) {
> +msi_addr_hi = pci_default_read_config(pdev,
> + pdev->msi_cap + 
> PCI_MSI_ADDRESS_HI,
> + 4);
> +}
> +qemu_put_be32(f, msi_addr_hi);
> +
> +msi_data = pci_default_read_config(pdev,
> +pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 : 
> PCI_MSI_DATA_32),
> +2);
> +qemu_put_be32(f, msi_data);
> +} else if (vdev->interrupt == VFIO_INT_MSIX) {
> +uint16_t offset;
> +
> +/* save enable bit and maskall bit */
> +offset = pci_default_read_config(pdev,
> +   pdev->msix_cap + PCI_MSIX_FLAGS + 1, 
> 2);
> +qemu_put_be16(f, offset);
> +msix_save(pdev, f);
> +}
> +pci_cmd = pci_default_read_config(pdev, PCI_COMMAND, 2);
> +qemu_put_be16(f, pci_cmd);
> +}
> +
> +static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> +{
> +VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +PCIDevice *pdev = >pdev;
> +uint32_t interrupt_type;
> +uint32_t msi_flags, msi_addr_lo, msi_addr_hi = 0, msi_data;
> +uint16_t pci_cmd;
> +bool msi_64bit;
> +int i, ret;
> +
> +/* retore pci bar configuration */
> +pci_cmd = pci_default_read_config(pdev, PCI_COMMAND, 2);
> +vfio_pci_write_config(pdev, PCI_COMMAND,
> +pci_cmd & (!(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)), 
> 2);
> +for (i = 0; i < PCI_ROM_SLOT; i++) {
> +uint32_t bar 

Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs

[Max Filippov]

On Mon, Apr 6, 2020 at 9:04 PM Max Filippov  wrote:
> diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
> index 376a61f3397c..278415ae0e06 100644
> --- a/target/xtensa/helper.c
> +++ b/target/xtensa/helper.c
> @@ -96,6 +96,7 @@ static void init_libisa(XtensaConfig *config)
>
>  config->isa = xtensa_isa_init(config->isa_internal, NULL, NULL);
>  assert(xtensa_isa_maxlength(config->isa) <= MAX_INSN_LENGTH);
> +assert(xtensa_insnbuf_size(dc->config->isa) <= MAX_INSNBUF_LENGTH);

No 'dc->' here...

-- 
Thanks.
-- Max

Re: [PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs

[Max Filippov]

On Mon, Apr 6, 2020 at 8:09 PM Richard Henderson
 wrote:
>
> Rather than dynamically allocate, and risk failing to free
> when we longjmp out of the translator, allocate the maximum
> buffer size from any of the supported cpus, which is 8:

There's macro MAX_INSN_LENGTH that defines maximal supported
instruction length in bytes. Maybe the following instead, along the lines
of what libisa does dynamically?:

--8<--
>From 08cc91b0d51e244766d73aae23aebd194b598378 Mon Sep 17 00:00:00 2001
From: Max Filippov 
Date: Mon, 6 Apr 2020 20:59:54 -0700
Subject: [PATCH] target/xtensa: statically allocate xtensa_insnbufs in
DisasContext

Signed-off-by: Max Filippov 
---
 target/xtensa/cpu.h   |  3 +++
 target/xtensa/helper.c|  1 +
 target/xtensa/translate.c | 12 ++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h
index c0d69fad96c5..7a46dccbe11b 100644
--- a/target/xtensa/cpu.h
+++ b/target/xtensa/cpu.h
@@ -213,6 +213,9 @@ enum {
 #define MEMCTL_IL0EN 0x1

 #define MAX_INSN_LENGTH 64
+#define MAX_INSNBUF_LENGTH \
+((MAX_INSN_LENGTH + sizeof(xtensa_insnbuf_word) - 1) / \
+ sizeof(xtensa_insnbuf_word))
 #define MAX_INSN_SLOTS 32
 #define MAX_OPCODE_ARGS 16
 #define MAX_NAREG 64
diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index 376a61f3397c..278415ae0e06 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -96,6 +96,7 @@ static void init_libisa(XtensaConfig *config)

 config->isa = xtensa_isa_init(config->isa_internal, NULL, NULL);
 assert(xtensa_isa_maxlength(config->isa) <= MAX_INSN_LENGTH);
+assert(xtensa_insnbuf_size(dc->config->isa) <= MAX_INSNBUF_LENGTH);
 opcodes = xtensa_isa_num_opcodes(config->isa);
 formats = xtensa_isa_num_formats(config->isa);
 regfiles = xtensa_isa_num_regfiles(config->isa);
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 8aa972cafdf3..91c7776c2544 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -72,8 +72,8 @@ struct DisasContext {
 unsigned cpenable;

 uint32_t op_flags;
-xtensa_insnbuf insnbuf;
-xtensa_insnbuf slotbuf;
+xtensa_insnbuf_word insnbuf[MAX_INSNBUF_LENGTH];
+xtensa_insnbuf_word slotbuf[MAX_INSNBUF_LENGTH];
 };

 static TCGv_i32 cpu_pc;
@@ -1174,10 +1174,6 @@ static void
xtensa_tr_init_disas_context(DisasContextBase *dcbase,
 dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
XTENSA_TBFLAG_CALLINC_SHIFT);

-if (dc->config->isa) {
-dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
-dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
-}
 init_sar_tracker(dc);
 }

@@ -1267,10 +1263,6 @@ static void xtensa_tr_tb_stop(DisasContextBase
*dcbase, CPUState *cpu)
 DisasContext *dc = container_of(dcbase, DisasContext, base);

 reset_sar_tracker(dc);
-if (dc->config->isa) {
-xtensa_insnbuf_free(dc->config->isa, dc->insnbuf);
-xtensa_insnbuf_free(dc->config->isa, dc->slotbuf);
-}
 if (dc->icount) {
 tcg_temp_free(dc->next_icount);
 }
--8<--
-- 
Thanks.
-- Max

Re: [PATCH v2 0/3] drop writes to read-only ram device & vfio regions

[Yan Zhao]

On Fri, Apr 03, 2020 at 08:53:12PM +0800, Peter Maydell wrote:
> On Fri, 3 Apr 2020 at 09:13, Yan Zhao  wrote:
> >
> > patch 1 modifies handler of ram device memory regions to drop guest writes
> > to read-only ram device memory regions
> >
> > patch 2 modifies handler of non-mmap'd read-only vfio regions to drop guest
> > writes to those regions
> >
> > patch 3 let mmap'd read-only vfio regions be able to generate vmexit for
> > guest write. so, without patch 1, host qemu would crash on guest write to
> > this read-only region. with patch 1, host qemu would drop the writes.
> 
> Just FYI, there seems to be a minor clock or timezone issue with
> however you're sending these emails: the Date header you
> sent reads "Date: Fri, 3 Apr 2020 16:56:57 +" but that
> time in the UTC + timezone is (as I write this) still several
> hours in the future. This isn't a big deal but you probably want
> to look into fixing it.
> 
> (Noticed because the wrong-date makes your patches stick to
> the top of the https://patchew.org/QEMU/ list even after other
> patches arrive after them.)
>
thanks for informing of it! will pay attention to it.
Thanks
Yan

[PATCH for-5.0?] linux-user/ppc: Fix padding in mcontext_t for ppc64

[Richard Henderson]

The padding that was added in 95cda4c44ee was added to a union,
and so it had no effect.  This fixes misalignment errors detected
by clang sanitizers for ppc64 and ppc64le.

In addition, only ppc64 allocates space for VSX registers, so do
not save them for ppc32.  The kernel only has references to
CONFIG_SPE in signal_32.c, so do not attempt to save them for ppc64.

Fixes: 95cda4c44ee
Signed-off-by: Richard Henderson 
---

Note that ppc64abi32 is *not* fixed by this patch.  It looks to
me that all of the defined(TARGET_PPC64) tests in this file are
incorrect, and that we should instead be testing TARGET_ABI_BITS
vs 32/64.  In addition, virtually all of the target_ulong structure
members would need to be abi_ulong.

Should we in fact disable ppc64abi32?
I can't see how it could work enough to be useful as-is.


r~

---
 linux-user/ppc/signal.c | 69 +
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index ecd99736b7..20a02c197c 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -35,12 +35,26 @@ struct target_mcontext {
 target_ulong mc_gregs[48];
 /* Includes fpscr.  */
 uint64_t mc_fregs[33];
+
 #if defined(TARGET_PPC64)
 /* Pointer to the vector regs */
 target_ulong v_regs;
+/*
+ * On ppc64, this mcontext structure is naturally *unaligned*,
+ * or rather it is aligned on a 8 bytes boundary but not on
+ * a 16 byte boundary.  This pad fixes it up.  This is why we
+ * cannot use ppc_avr_t, which would force alignment.  This is
+ * also why the vector regs are referenced in the ABI by the
+ * v_regs pointer above so any amount of padding can be added here.
+ */
+target_ulong pad;
+/* VSCR and VRSAVE are saved separately.  Also reserve space for VSX. */
+struct {
+uint64_t altivec[34 + 16][2];
+} mc_vregs;
 #else
 target_ulong mc_pad[2];
-#endif
+
 /* We need to handle Altivec and SPE at the same time, which no
kernel needs to do.  Fortunately, the kernel defines this bit to
be Altivec-register-large all the time, rather than trying to
@@ -48,32 +62,14 @@ struct target_mcontext {
 union {
 /* SPE vector registers.  One extra for SPEFSCR.  */
 uint32_t spe[33];
-/* Altivec vector registers.  The packing of VSCR and VRSAVE
-   varies depending on whether we're PPC64 or not: PPC64 splits
-   them apart; PPC32 stuffs them together.
-   We also need to account for the VSX registers on PPC64
-*/
-#if defined(TARGET_PPC64)
-#define QEMU_NVRREG (34 + 16)
-/* On ppc64, this mcontext structure is naturally *unaligned*,
- * or rather it is aligned on a 8 bytes boundary but not on
- * a 16 bytes one. This pad fixes it up. This is also why the
- * vector regs are referenced by the v_regs pointer above so
- * any amount of padding can be added here
+/*
+ * Altivec vector registers.  One extra for VRSAVE.
+ * On ppc32, we are already aligned to 16 bytes.  We could
+ * use ppc_avr_t, but choose to share the same type as ppc64.
  */
-target_ulong pad;
-#else
-/* On ppc32, we are already aligned to 16 bytes */
-#define QEMU_NVRREG 33
-#endif
-/* We cannot use ppc_avr_t here as we do *not* want the implied
- * 16-bytes alignment that would result from it. This would have
- * the effect of making the whole struct target_mcontext aligned
- * which breaks the layout of struct target_ucontext on ppc64.
- */
-uint64_t altivec[QEMU_NVRREG][2];
-#undef QEMU_NVRREG
+uint64_t altivec[33][2];
 } mc_vregs;
+#endif
 };
 
 /* See arch/powerpc/include/asm/sigcontext.h.  */
@@ -278,6 +274,7 @@ static void save_user_regs(CPUPPCState *env, struct 
target_mcontext *frame)
 __put_user((uint32_t)env->spr[SPR_VRSAVE], vrsave);
 }
 
+#if defined(TARGET_PPC64)
 /* Save VSX second halves */
 if (env->insns_flags2 & PPC2_VSX) {
 uint64_t *vsregs = (uint64_t *)>mc_vregs.altivec[34];
@@ -286,6 +283,7 @@ static void save_user_regs(CPUPPCState *env, struct 
target_mcontext *frame)
 __put_user(*vsrl, [i]);
 }
 }
+#endif
 
 /* Save floating point registers.  */
 if (env->insns_flags & PPC_FLOAT) {
@@ -296,22 +294,18 @@ static void save_user_regs(CPUPPCState *env, struct 
target_mcontext *frame)
 __put_user((uint64_t) env->fpscr, >mc_fregs[32]);
 }
 
+#if !defined(TARGET_PPC64)
 /* Save SPE registers.  The kernel only saves the high half.  */
 if (env->insns_flags & PPC_SPE) {
-#if defined(TARGET_PPC64)
-for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
-__put_user(env->gpr[i] >> 32, >mc_vregs.spe[i]);
-}
-#else
 for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
 __put_user(env->gprh[i], >mc_vregs.spe[i]);
   

[PATCH for-5.0?] target/xtensa: Statically allocate xtensa_insnbufs

[Richard Henderson]

Rather than dynamically allocate, and risk failing to free
when we longjmp out of the translator, allocate the maximum
buffer size from any of the supported cpus, which is 8:

core-dc232b/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-dc233c/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-de212/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-fsf/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-sample_controller/xtensa-modules.inc.c:  3 /* insn_size */, 0,
core-test_kc705_be/xtensa-modules.inc.c:  8 /* insn_size */, 0,
core-test_mmuhifi_c3/xtensa-modules.inc.c:  8 /* insn_size */, 0,

Cc: Max Filippov 
Signed-off-by: Richard Henderson 
---
 target/xtensa/translate.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 37f65b1f03..86369aa623 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -72,8 +72,10 @@ struct DisasContext {
 unsigned cpenable;
 
 uint32_t op_flags;
-xtensa_insnbuf insnbuf;
-xtensa_insnbuf slotbuf;
+
+/* The maximum of all supported cpus is 8. */
+xtensa_insnbuf_word insnbuf[8];
+xtensa_insnbuf_word slotbuf[8];
 };
 
 static TCGv_i32 cpu_pc;
@@ -1174,14 +1176,11 @@ static void 
xtensa_tr_init_disas_context(DisasContextBase *dcbase,
 dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
XTENSA_TBFLAG_CALLINC_SHIFT);
 
-/*
- * FIXME: This will leak when a failed instruction load or similar
- * event causes us to longjump out of the translation loop and
- * hence not clean-up in xtensa_tr_tb_stop
- */
 if (dc->config->isa) {
-dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
-dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
+size_t size = (xtensa_insnbuf_size(dc->config->isa) *
+   sizeof(xtensa_insnbuf_word));
+assert(sizeof(dc->insnbuf) >= size);
+assert(sizeof(dc->slotbuf) >= size);
 }
 init_sar_tracker(dc);
 }
@@ -1272,10 +1271,6 @@ static void xtensa_tr_tb_stop(DisasContextBase *dcbase, 
CPUState *cpu)
 DisasContext *dc = container_of(dcbase, DisasContext, base);
 
 reset_sar_tracker(dc);
-if (dc->config->isa) {
-xtensa_insnbuf_free(dc->config->isa, dc->insnbuf);
-xtensa_insnbuf_free(dc->config->isa, dc->slotbuf);
-}
 if (dc->icount) {
 tcg_temp_free(dc->next_icount);
 }
-- 
2.20.1

[Bug 1863441] Re: cmd_mode_sense always reports 0x70, no CDROM present

[Benjamin David Lunt]

Since I posted this bug report, I have done a little more research and
this specific part of this command is actually quite obsolete.  It use
to be documented (MMC v1.2 Draft), but later versions have actually
removed this part of the command, even stating "obsolete" in some of the
documentation.

If you want to still allow it, it actually does state 0x70 is an empty
drive.  However, being it is so obsolete, you may wish to change this
status to closed.

Thanks for asking.  I don't mind if you close this bug report.

Ben

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1863441

Title:
  cmd_mode_sense always reports 0x70, no CDROM present

Status in QEMU:
  New

Bug description:
  cmd_mode_sense

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/ide/atapi.c;hb=refs/heads/master#l852
  always reports 0x70 in byte 2 returned, indicating no CD-ROM present.

  If CD-ROM is present, should report 0x01 (or 0x11).
  If CD-ROM absent, should report 0x70.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1863441/+subscriptions

[Bug 1871267] Re: Multiple (Repeating) Keystrokes in macOS

[Russell Morris]

BTW, it does make the guest unusable ... can't even enter a password (if
I could get that far, having issues even running setup).

Thanks!

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871267

Title:
  Multiple (Repeating) Keystrokes in macOS

Status in QEMU:
  New

Bug description:
  Hi,

  I am finding this issue with v4.2.0, or the latest master - on a
  Windows host, with macOS guest. It happens using gtk (SPICE?) or VNC.
  When I get to a place to enter a keystroke, I quite reliably get
  multiple of the same key (i.e. press A, get ).

  Thinking there may be a basic setting to address this? I did try it in
  Linux (kvm), no issue there.

  Thanks!

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871267/+subscriptions

[Bug 1871267] [NEW] Multiple (Repeating) Keystrokes in macOS

[Russell Morris]

Public bug reported:

Hi,

I am finding this issue with v4.2.0, or the latest master - on a Windows
host, with macOS guest. It happens using gtk (SPICE?) or VNC. When I get
to a place to enter a keystroke, I quite reliably get multiple of the
same key (i.e. press A, get ).

Thinking there may be a basic setting to address this? I did try it in
Linux (kvm), no issue there.

Thanks!

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871267

Title:
  Multiple (Repeating) Keystrokes in macOS

Status in QEMU:
  New

Bug description:
  Hi,

  I am finding this issue with v4.2.0, or the latest master - on a
  Windows host, with macOS guest. It happens using gtk (SPICE?) or VNC.
  When I get to a place to enter a keystroke, I quite reliably get
  multiple of the same key (i.e. press A, get ).

  Thinking there may be a basic setting to address this? I did try it in
  Linux (kvm), no issue there.

  Thanks!

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871267/+subscriptions

[Bug 1871270] [NEW] [Feature Request] add usbredir device reset blacklist options support to allow macOS guest to iOS device usbredir

[Michael Lee]

Public bug reported:

Description of problem:
Currently, when a iOS device is redirected to a macOS VM, it falls into a 
reset-not-found loop.
Version-Release number of selected component (if applicable):
latest
How reproducible:
100%
Steps to Reproduce:


Connect an iOS device to Ubuntu 18.04.2 LTS (I believe it is the same for any 
distro.)


Connect virt-manager/virt-viewer to a macOS VM through SPICE (I am using OSX 
10.15 Catalina)


Attempt to redirect the iOS device (iPad in my case) to the VM through usb 
redirection.


Actual results:
For any odd number of attempt, the guest macOS will send a reset to the iOS 
device which causes the host to reset the USB connection in the host side. In 
the UI, it will be displayed as a successful connection for a few seconds 
before it disconnects. After this, the iOS device will reconnect itself, but 
via a different device name /dev/bus/usb/x/y+1.
For any even number of attempt, when I select the iOS device in the 
virt-manager/virt-viewer UI, the connection will not success and instead a 
LIBUSB_ERROR_NOT_FOUND error will be provided. Then the UI will reload and get 
the new device name of the iOS device, falling into the behavior of the 
aforementioned odd number of attempt.
Expected results:
The macOS detects the iOS device and connects to it happily.
Additional info:
It seems that this bug has been first identified as in 
https://bugs.freedesktop.org/show_bug.cgi?id=100149, for a Samsung Android 
device, which the developers of SPICE applied a hotfix in 
https://gitlab.freedesktop.org/spice/usbredir/-/blob/master/usbredirhost/usbredirhost.c#L147.
 However, there were no settings available for users to fix it.
A similar bug that also consists of a macOS guest/iOS device pair, but instead 
of being usbredir, is usb-host, has been identified and patched in 
https://github.com/qemu/qemu/commit/ba4c735b4fc74e309ce4b2551d258e442ef513a5, 
which is further modified into 
https://github.com/qemu/qemu/blame/146aa0f104bb3bf88e43c4082a0bfc4bbda4fbd8/hw/usb/host-libusb.c#L1486.
 Following such patch, I have attempted to apply such patch at host-side in 
https://github.com/michaellee8/qemu/blob/master/hw/usb/redirect.c (not 
correctly formatted currently, pls ignore it atm), however I discovered that 
this is not enough since it is also a SPICE issue, which resolves to 
virt-manager/virt-viewer.
This is probably a cross-project issue between qemu, spice (usbredir) and 
virt-manager/virt-viewer, which would some effort to coordinate a solution. 
However a working solution for this problem would probably benefits a lot of 
users whom relies on connecting a mobile device into a VM, for purposes like 
easier mobile development. Considering the report for the Samsung Android 
Device on a PC use case, such issue is probably cross-OS/cross-device.

cross-references:
- https://bugzilla.redhat.com/show_bug.cgi?id=1821518
- https://bugzilla.redhat.com/show_bug.cgi?id=1821517
- https://gitlab.freedesktop.org/spice/usbredir/-/issues/10

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871270

Title:
  [Feature Request] add usbredir device reset blacklist options support
  to allow macOS guest to iOS device usbredir

Status in QEMU:
  New

Bug description:
  Description of problem:
  Currently, when a iOS device is redirected to a macOS VM, it falls into a 
reset-not-found loop.
  Version-Release number of selected component (if applicable):
  latest
  How reproducible:
  100%
  Steps to Reproduce:

  
  Connect an iOS device to Ubuntu 18.04.2 LTS (I believe it is the same for any 
distro.)

  
  Connect virt-manager/virt-viewer to a macOS VM through SPICE (I am using OSX 
10.15 Catalina)

  
  Attempt to redirect the iOS device (iPad in my case) to the VM through usb 
redirection.

  
  Actual results:
  For any odd number of attempt, the guest macOS will send a reset to the iOS 
device which causes the host to reset the USB connection in the host side. In 
the UI, it will be displayed as a successful connection for a few seconds 
before it disconnects. After this, the iOS device will reconnect itself, but 
via a different device name /dev/bus/usb/x/y+1.
  For any even number of attempt, when I select the iOS device in the 
virt-manager/virt-viewer UI, the connection will not success and instead a 
LIBUSB_ERROR_NOT_FOUND error will be provided. Then the UI will reload and get 
the new device name of the iOS device, falling into the behavior of the 
aforementioned odd number of attempt.
  Expected results:
  The macOS detects the iOS device and connects to it happily.
  Additional info:
  It seems that this bug has been first identified as in 
https://bugs.freedesktop.org/show_bug.cgi?id=100149, for a Samsung Android 
device, which the developers of SPICE applied a hotfix in 

Re: [RFC PATCH-for-5.0?] target/mips/translate: Report exception in delay slot as UNPREDICTABLE

[Philippe Mathieu-Daudé]

On 4/7/20 1:54 AM, Philippe Mathieu-Daudé wrote:

Using the BC1ANY4F instruction with a 24Kf core (MIPS32R2
& ASE_MIPS16) we get:

   $ echo -ne '\x03\x20\xf8\x09' > cop1x.bin
   $ qemu-system-mipsel -bios cop1x.bin
   unknown branch 0x13000
   Aborted (core dumped)

   (gdb) bt
   #0  0x7fe2d38b1e35 in raise () at /lib64/libc.so.6
   #1  0x7fe2d389c895 in abort () at /lib64/libc.so.6
   #2  0x55aa9fe066e5 in gen_branch (ctx=0x7fe27bdfa590, insn_bytes=4) at 
target/mips/translate.c:13167
   #3  0x55aa9fe2baf4 in mips_tr_translate_insn (dcbase=0x7fe27bdfa590, 
cs=0x55aaa0e2d530) at target/mips/translate.c:30928
   #4  0x55aa9fd40138 in translator_loop (ops=0x55aaa05e94e0 , 
db=0x7fe27bdfa590, cpu=0x55aaa0e2d530, tb=0x7fe28440 , 
max_insns=512) at accel/tcg/translator.c:102
   #5  0x55aa9fe2bd21 in gen_intermediate_code (cs=0x55aaa0e2d530, 
tb=0x7fe28440 , max_insns=512) at 
target/mips/translate.c:30999
   #6  0x55aa9fd3e3d4 in tb_gen_code (cpu=0x55aaa0e2d530, pc=3217031168, 
cs_base=0, flags=268435600, cflags=-16252928) at accel/tcg/translate-all.c:1718
   #7  0x55aa9fd3ac06 in tb_find (cpu=0x55aaa0e2d530, last_tb=0x0, 
tb_exit=0, cf_mask=524288) at accel/tcg/cpu-exec.c:407
   #8  0x55aa9fd3b4d5 in cpu_exec (cpu=0x55aaa0e2d530) at 
accel/tcg/cpu-exec.c:731
   #9  0x55aa9fcfe33a in tcg_cpu_exec (cpu=0x55aaa0e2d530) at cpus.c:1405
   #10 0x55aa9fcfeb90 in qemu_tcg_cpu_thread_fn (arg=0x55aaa0e2d530) at 
cpus.c:1713
   #11 0x55aaa02ea7d7 in qemu_thread_start (args=0x55aaa0e465f0) at 
util/qemu-thread-posix.c:519
   #12 0x7fe2d3a484c0 in start_thread () at /lib64/libpthread.so.0
   #13 0x7fe2d3976163 in clone () at /lib64/libc.so.6

This is because this COP1X instruction generates a Reserved
Instruction when used with this core, however we are in a delay
slot, and exceptions in delay slot are architecturally unpredictable.

Core dumps confunse users. Instead, report a friendlier error message:

   $ qemu-system-mipsel -bios cop1x.bin
   qemu-system-mipsel: Exception in delay slot is UNPREDICTABLE

Buglink: https://bugs.launchpad.net/qemu/+bug/1663287
Signed-off-by: Philippe Mathieu-Daudé 
---
  target/mips/translate.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 25b595a17d..99e675b87a 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -30925,6 +30925,10 @@ static void mips_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cs)
  }
  }
  if (is_slot) {
+if (ctx->base.is_jmp == DISAS_NORETURN) {
+error_report("Exception in delay slot is UNPREDICTABLE");


"Exception/branch" actually.


+exit(1);
+}
  gen_branch(ctx, insn_bytes);
  }
  ctx->base.pc_next += insn_bytes;

[RFC PATCH-for-5.0?] target/mips/translate: Report exception in delay slot as UNPREDICTABLE

[Philippe Mathieu-Daudé]

Using the BC1ANY4F instruction with a 24Kf core (MIPS32R2
& ASE_MIPS16) we get:

  $ echo -ne '\x03\x20\xf8\x09' > cop1x.bin
  $ qemu-system-mipsel -bios cop1x.bin
  unknown branch 0x13000
  Aborted (core dumped)

  (gdb) bt
  #0  0x7fe2d38b1e35 in raise () at /lib64/libc.so.6
  #1  0x7fe2d389c895 in abort () at /lib64/libc.so.6
  #2  0x55aa9fe066e5 in gen_branch (ctx=0x7fe27bdfa590, insn_bytes=4) at 
target/mips/translate.c:13167
  #3  0x55aa9fe2baf4 in mips_tr_translate_insn (dcbase=0x7fe27bdfa590, 
cs=0x55aaa0e2d530) at target/mips/translate.c:30928
  #4  0x55aa9fd40138 in translator_loop (ops=0x55aaa05e94e0 , 
db=0x7fe27bdfa590, cpu=0x55aaa0e2d530, tb=0x7fe28440 , 
max_insns=512) at accel/tcg/translator.c:102
  #5  0x55aa9fe2bd21 in gen_intermediate_code (cs=0x55aaa0e2d530, 
tb=0x7fe28440 , max_insns=512) at 
target/mips/translate.c:30999
  #6  0x55aa9fd3e3d4 in tb_gen_code (cpu=0x55aaa0e2d530, pc=3217031168, 
cs_base=0, flags=268435600, cflags=-16252928) at accel/tcg/translate-all.c:1718
  #7  0x55aa9fd3ac06 in tb_find (cpu=0x55aaa0e2d530, last_tb=0x0, 
tb_exit=0, cf_mask=524288) at accel/tcg/cpu-exec.c:407
  #8  0x55aa9fd3b4d5 in cpu_exec (cpu=0x55aaa0e2d530) at 
accel/tcg/cpu-exec.c:731
  #9  0x55aa9fcfe33a in tcg_cpu_exec (cpu=0x55aaa0e2d530) at cpus.c:1405
  #10 0x55aa9fcfeb90 in qemu_tcg_cpu_thread_fn (arg=0x55aaa0e2d530) at 
cpus.c:1713
  #11 0x55aaa02ea7d7 in qemu_thread_start (args=0x55aaa0e465f0) at 
util/qemu-thread-posix.c:519
  #12 0x7fe2d3a484c0 in start_thread () at /lib64/libpthread.so.0
  #13 0x7fe2d3976163 in clone () at /lib64/libc.so.6

This is because this COP1X instruction generates a Reserved
Instruction when used with this core, however we are in a delay
slot, and exceptions in delay slot are architecturally unpredictable.

Core dumps confunse users. Instead, report a friendlier error message:

  $ qemu-system-mipsel -bios cop1x.bin
  qemu-system-mipsel: Exception in delay slot is UNPREDICTABLE

Buglink: https://bugs.launchpad.net/qemu/+bug/1663287
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/translate.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 25b595a17d..99e675b87a 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -30925,6 +30925,10 @@ static void mips_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cs)
 }
 }
 if (is_slot) {
+if (ctx->base.is_jmp == DISAS_NORETURN) {
+error_report("Exception in delay slot is UNPREDICTABLE");
+exit(1);
+}
 gen_branch(ctx, insn_bytes);
 }
 ctx->base.pc_next += insn_bytes;
-- 
2.21.1

[Bug 1871250] [NEW] Failed to create HAX VM

[Russell Morris]

Public bug reported:

Hi,

I'm running the latest (master) of QEMU, though the version doesn't seem
to matter - I also checked back to v4.2.0, exactly the same issue. And
this isn't about the VM (guest), if I even just try to run,

> "c:\Program Files\qemu\qemu-system-x86_64.exe" -accel hax

Basically, just get a window to open, with acceleration enabled ... I get,
Open the vm device error:/dev/hax_vm/vm00, ec:3
Failed to open vm 0
Failed to create HAX VM
No accelerator found.

But I checked - I have installed Intel HAXM, and verified it's running,
> sc query intelhaxm
SERVICE_NAME: intelhaxm
TYPE   : 1  KERNEL_DRIVER
STATE  : 4  RUNNING
(STOPPABLE, NOT_PAUSABLE, IGNORES_SHUTDOWN)
WIN32_EXIT_CODE: 0  (0x0)
SERVICE_EXIT_CODE  : 0  (0x0)
CHECKPOINT : 0x0
WAIT_HINT  : 0x0

Just remove the accelerator (-accel hax), and I get a window - so this
is related to QEMU being able to contact / use the accelerator.

Help!?!?

Thanks!

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1871250

Title:
  Failed to create HAX VM

Status in QEMU:
  New

Bug description:
  Hi,

  I'm running the latest (master) of QEMU, though the version doesn't
  seem to matter - I also checked back to v4.2.0, exactly the same
  issue. And this isn't about the VM (guest), if I even just try to run,

  > "c:\Program Files\qemu\qemu-system-x86_64.exe" -accel hax

  Basically, just get a window to open, with acceleration enabled ... I get,
  Open the vm device error:/dev/hax_vm/vm00, ec:3
  Failed to open vm 0
  Failed to create HAX VM
  No accelerator found.

  But I checked - I have installed Intel HAXM, and verified it's running,
  > sc query intelhaxm
  SERVICE_NAME: intelhaxm
  TYPE   : 1  KERNEL_DRIVER
  STATE  : 4  RUNNING
  (STOPPABLE, NOT_PAUSABLE, IGNORES_SHUTDOWN)
  WIN32_EXIT_CODE: 0  (0x0)
  SERVICE_EXIT_CODE  : 0  (0x0)
  CHECKPOINT : 0x0
  WAIT_HINT  : 0x0

  Just remove the accelerator (-accel hax), and I get a window - so this
  is related to QEMU being able to contact / use the accelerator.

  Help!?!?

  Thanks!

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1871250/+subscriptions

[Bug 1663287] Re: Illegal delay slot code causes abort on mips64

[Philippe Mathieu-Daudé]

Hi Brian,

You try to execute a CP1 instruction in a delay slot,
which triggers a Reserved Instruction exception.
Per the ISA the processor operation is UNPREDICTABLE in such case.

What is the behavior on real hardware?
An assertion() seems appropriate.

Your compiler might be buggy, or you are not compiling for the correct CPU
(or you are not using the correct QEMU cpu).

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1663287

Title:
  Illegal delay slot code causes abort on mips64

Status in QEMU:
  New

Bug description:
  During some randomised testing of an experimental MIPS implementation
  I found an instruction sequence that also causes aborts on mainline
  qemu's MIPS support.  The problem is triggered by an MSA branch
  instruction appearing in a delay slot when emulating a processor
  without MSA support.

  For example, with the current repository HEAD
  (f073cd3a2bf1054135271b837c58a7da650dd84b) configured for
  mips64-softmmu, if I run the attached binary using

  mips64-softmmu/qemu-system-mips64 -bios ../abort2.bin -machine
  mipssim -nographic

  it will report

  unknown branch 0x13000
  Aborted (core dumped)

  The binary contains the following two instructions:

  0028 jr at
  47081e61 bz.b   w8,0xbfc0798c

  The jr sets up a jump, and hflags is set accordingly in
  gen_compute_branch (in target/mips/translate.c).  When processing the
  bz.b, check_insn generates an exception because the instruction isn't
  support, but gen_msa_branch skips the usual delay slot check for the
  same reason, and sets more bits in hflags, leading to an abort in
  gen_branch because the hflags are now invalid.

  I suspect the best fix is to remove the instruction set condition from
  the delay slot check in gen_msa_branch.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1663287/+subscriptions

[PATCH v1] nrf51: Fix last GPIO CNF address

[Cameron Esfahani via]

NRF51_GPIO_REG_CNF_END doesn't actually refer to the start of the last
valid CNF register: it's referring to the last byte of the last valid
CNF register.

This hasn't been a problem up to now, as current implementation in
memory.c turns an unaligned 4-byte read from 0x77f to a single byte read
and the qtest only looks at the least-significant byte of the register.

But, when running with Cedric Le Goater's  pending fix for
unaligned accesses in memory.c, the qtest breaks.

Considering NRF51 doesn't support unaligned accesses, the simplest fix
is to actually set NRF51_GPIO_REG_CNF_END to the start of the last valid
CNF register: 0x77c.

Now, qtests work with or without Cedric's patch.

Signed-off-by: Cameron Esfahani 
---
 include/hw/gpio/nrf51_gpio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
index 337ee534bb..1d62bbc928 100644
--- a/include/hw/gpio/nrf51_gpio.h
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -42,7 +42,7 @@
 #define NRF51_GPIO_REG_DIRSET   0x518
 #define NRF51_GPIO_REG_DIRCLR   0x51C
 #define NRF51_GPIO_REG_CNF_START0x700
-#define NRF51_GPIO_REG_CNF_END  0x77F
+#define NRF51_GPIO_REG_CNF_END  0x77C
 
 #define NRF51_GPIO_PULLDOWN 1
 #define NRF51_GPIO_PULLUP 3
-- 
2.24.0

Re: [PATCH v1] usb: Add read support for HCIVERSION register to XHCI

[Cameron Esfahani via]

I had a look at this failing test case after running applying Cedric's patch 
(https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65).

It looks like this is just a bug in the test case.  The test case is attempting 
to verify that the CNF registers are programmed correctly by sampling the first 
and last CNF register.  The bug is that NRF51_GPIO_REG_CNF_END doesn't actually 
refer to the start of the last valid CNF register.  It refers to the last byte 
of the last valid CNF register.  So either NRF51_GPIO_REG_CNF_END needs to be 
adjusted to 0x77C or we need to subtract 3 to get to the start of the register. 
 Considering the NRF51 doesn't appear to support unaligned access, it seems 
like changing NRF51_GPIO_REG_CNF_END to 0x77C is reasonable.

Here's a patch which seems to fix it for me:

diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
index 337ee53..1d62bbc 100644
--- a/include/hw/gpio/nrf51_gpio.h
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -42,7 +42,7 @@
 #define NRF51_GPIO_REG_DIRSET   0x518
 #define NRF51_GPIO_REG_DIRCLR   0x51C
 #define NRF51_GPIO_REG_CNF_START0x700
-#define NRF51_GPIO_REG_CNF_END  0x77F
+#define NRF51_GPIO_REG_CNF_END  0x77C
 
 #define NRF51_GPIO_PULLDOWN 1
 #define NRF51_GPIO_PULLUP 3

Considering this change works for pre-Cedric patch and post, I'll post at 
official version shortly.

And hopefully this unblocks review of Cedric's patch...

Cameron Esfahani
di...@apple.com

"The cake is a lie."

Common wisdom

> On Apr 1, 2020, at 4:23 AM, Cédric Le Goater  wrote:
> 
> Hello,
> 
> On 3/31/20 1:02 PM, Philippe Mathieu-Daudé wrote:
>> On 3/31/20 11:57 AM, Cameron Esfahani wrote:
>>> Philippe -
>>>  From what I've seen, access size has nothing to do with alignment.
>> 
>> Yes, I was wondering if you were using unaligned accesses.
>> 
>> I *think* the correct fix is in the "memory: Support unaligned accesses on 
>> aligned-only models" patch from Andrew Jeffery:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg461247.html
> 
> Here is an updated version I have been keeping since : 
> 
>   
> https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65
> 
> which breaks qtest :
> 
>   microbit-test.c:307:test_nrf51_gpio: assertion failed (actual == 0x01): 
> (0 == 1)
> 
> I haven't had time to look at this closely but it would be nice to get this 
> patch merged. It's a requirement for the Aspeed ADC model.
> 
> Thanks,
> 
> c. 
> 
>>> 
>>> What the code in access_with_adjusted_size() will do is make sure that 
>>> "size" is >= min_access_size and <= max_access_size.
>>> 
>>> So reading 2-bytes from address 2 turns into reading 4-bytes from address 
>>> 2: xhci_cap_read() is called with reg 2, size 4.
>>> 
>>> But, due to the fact our change to support reg 2 only returns back 2-bytes, 
>>> and how the loops work in access_with_adjusted_size(), we only call 
>>> xhci_cap_read() once.
>>> 
>>> It seems like we should also change impl.min_access_size for xhci_cap_ops 
>>> to be 2.
>>> 
>>> But, after that, to support people doing strange things like reading 
>>> traditionally 4-byte values as 2 2-byte values, we probably need to change 
>>> xhci_cap_read() to handle every memory range in steps of 2-bytes.
>>> 
>>> But I'll defer to Gerd on this...
>>> 
>>> Cameron Esfahani
>>> di...@apple.com
>>> 
>>> "Americans are very skilled at creating a custom meaning from something 
>>> that's mass-produced."
>>> 
>>> Ann Powers
>>> 
>>> 
 On Mar 31, 2020, at 12:52 AM, Philippe Mathieu-Daudé  
 wrote:
 
 On 3/30/20 11:44 PM, Cameron Esfahani via wrote:
> macOS will read HCIVERSION separate from CAPLENGTH.  Add a distinct
> handler for that register.
 
 Apparently a fix for https://bugs.launchpad.net/qemu/+bug/1693050.
 
> Signed-off-by: Cameron Esfahani 
> ---
>   hw/usb/hcd-xhci.c | 3 +++
>   1 file changed, 3 insertions(+)
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index b330e36fe6..061f8438de 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2739,6 +2739,9 @@ static uint64_t xhci_cap_read(void *ptr, hwaddr 
> reg, unsigned size)
>   case 0x00: /* HCIVERSION, CAPLENGTH */
>   ret = 0x0100 | LEN_CAP;
>   break;
> +case 0x02: /* HCIVERSION */
> +ret = 0x0100;
> +break;
 
 But we have:
 
 static const MemoryRegionOps xhci_cap_ops = {
 .read = xhci_cap_read,
 .write = xhci_cap_write,
 .valid.min_access_size = 1,
 .valid.max_access_size = 4,
 .impl.min_access_size = 4,
 .impl.max_access_size = 4,
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 IIUC ".impl.min_access_size = 4" means the case 'reg == 2' can not happen. 
 It seems we have a bug in memory.c elsewhere.
 
 How can we reproduce?
 
 If not easy, can you share the 

Re: qcow2: Zero-initialization of external data files

[Eric Blake]

On 2/17/20 10:56 AM, Max Reitz wrote:

Hi,

AFAIU, external data files with data_file_raw=on are supposed to return
the same data as the qcow2 file when read.  But we still use the qcow2
metadata structures (which are by default initialized to “everything
unallocated”), even though we never ensure that the external data file
is zero, too, so this can happen:

$ dd if=/dev/urandom of=foo.raw 64M
[...]

$ sudo losetup -f --show foo.raw
/dev/loop0

$ sudo ./qemu-img create -f qcow2 -o \
 data_file=/dev/loop0,data_file_raw=on foo.qcow2 64M
[...]

$ sudo ./qemu-io -c 'read -P 0 0 64M' foo.qcow2
read 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 00.00 sec (25.036 GiB/sec and 400.5751 ops/sec)


This looks like a bug (and we should fix it for 5.0 if possible) - read 
of a data_file_raw=on should not treat unallocated clusters as reading 
0, but rather as reading whatever the raw data contains.




$ sudo ./qemu-io -c 'read -P 0 0 64M' -f raw foo.raw
Pattern verification failed at offset 0, 67108864 bytes
read 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 00.01 sec (5.547 GiB/sec and 88.7484 ops/sec)

I suppose this behavior is fine for blockdev-create because I guess it’s
the user’s responsibility to ensure that the external data file is zero.
  But maybe it isn’t, so that’s my first question: Is it really the
user’s responsibility or should we always ensure it’s zero?


I'd argue that requiring the user to pre-zero the raw data file is 
undesirable; and that we should instead fix our code to not report the 
image as reading all zeroes when creating with data_file_raw=on.




My second question is: If we decide that this is fine for
blockdev-create, should at least qcow2_co_create_opts() ensure the data
file it just created is zero?


Having an option to make qemu force-zero the raw image during 
qcow2_co_create_opts seems reasonable, but for performance reasons, I 
don't think the flag should be on by default.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH-for-5.0?] .github: Enable repo-lockdown bot to refuse GitHub pull requests

[John Snow]

On 4/6/20 5:41 PM, Philippe Mathieu-Daudé wrote:
> Some GitHub users try to open pull requests against the GitHub
> mirror. Unfortunate these get ignored until eventually someone
> notices and closes the request.
> 
> Enable the 'Repo Lockdown' [*] 3rd party bot which can autorespond
> to pull requests with a friendly comment, close the request, and
> then lock it to prevent further comments.
> 
> [*] https://github.com/dessant/repo-lockdown
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Repo Lockdown is enabled on https://github.com/qemu/qemu/
> ---
>  .github/lockdown.yml | 35 +++
>  MAINTAINERS  |  1 +
>  2 files changed, 36 insertions(+)
>  create mode 100644 .github/lockdown.yml
> 
> diff --git a/.github/lockdown.yml b/.github/lockdown.yml
> new file mode 100644
> index 00..94472d1256
> --- /dev/null
> +++ b/.github/lockdown.yml
> @@ -0,0 +1,35 @@
> +# Configuration for Repo Lockdown - https://github.com/dessant/repo-lockdown
> +
> +# Close issues and pull requests
> +close: true
> +
> +# Lock issues and pull requests
> +lock: true
> +
> +issues:
> +  comment: |
> +Thank you for your interest in the QEMU project.
> +
> +This repository is a read-only mirror of the project's master
> +repostories hosted on https://git.qemu.org/git/qemu.git.
> +The project does not process issues filed on GitHub.
> +
> +The project issues are tracked on Launchpad:
> +https://bugs.launchpad.net/qemu
> +
> +QEMU welcomes bug report contributions. You can fill new ones on:
> +https://bugs.launchpad.net/qemu/+filebug
> +
> +pulls:
> +  comment: |
> +Thank you for your interest in the QEMU project.
> +
> +This repository is a read-only mirror of the project's master
> +repostories hosted on https://git.qemu.org/git/qemu.git.
> +The project does not process merge requests filed on GitHub.
> +
> +QEMU welcomes contributions of code (either fixing bugs or adding new
> +functionality). However, we get a lot of patches, and so we have some
> +guidelines about submitting patches described in our Wiki:
> +https://wiki.qemu.org/Contribute/SubmitAPatch and
> +https://wiki.qemu.org/Contribute/FAQ
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d156d73b3..0559e84790 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2833,6 +2833,7 @@ M: Alex Bennée 
>  M: Fam Zheng 
>  R: Philippe Mathieu-Daudé 
>  S: Maintained
> +F: .github/lockdown.yml
>  F: .travis.yml
>  F: scripts/travis/
>  F: .shippable.yml
> 

Looks good, and I'd be happy with this as-is, assuming this is enough to
enable it.

Reviewed-by: John Snow 

[PATCH-for-5.0?] .github: Enable repo-lockdown bot to refuse GitHub pull requests

[Philippe Mathieu-Daudé]

Some GitHub users try to open pull requests against the GitHub
mirror. Unfortunate these get ignored until eventually someone
notices and closes the request.

Enable the 'Repo Lockdown' [*] 3rd party bot which can autorespond
to pull requests with a friendly comment, close the request, and
then lock it to prevent further comments.

[*] https://github.com/dessant/repo-lockdown

Suggested-by: Daniel P. Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
Repo Lockdown is enabled on https://github.com/qemu/qemu/
---
 .github/lockdown.yml | 35 +++
 MAINTAINERS  |  1 +
 2 files changed, 36 insertions(+)
 create mode 100644 .github/lockdown.yml

diff --git a/.github/lockdown.yml b/.github/lockdown.yml
new file mode 100644
index 00..94472d1256
--- /dev/null
+++ b/.github/lockdown.yml
@@ -0,0 +1,35 @@
+# Configuration for Repo Lockdown - https://github.com/dessant/repo-lockdown
+
+# Close issues and pull requests
+close: true
+
+# Lock issues and pull requests
+lock: true
+
+issues:
+  comment: |
+Thank you for your interest in the QEMU project.
+
+This repository is a read-only mirror of the project's master
+repostories hosted on https://git.qemu.org/git/qemu.git.
+The project does not process issues filed on GitHub.
+
+The project issues are tracked on Launchpad:
+https://bugs.launchpad.net/qemu
+
+QEMU welcomes bug report contributions. You can fill new ones on:
+https://bugs.launchpad.net/qemu/+filebug
+
+pulls:
+  comment: |
+Thank you for your interest in the QEMU project.
+
+This repository is a read-only mirror of the project's master
+repostories hosted on https://git.qemu.org/git/qemu.git.
+The project does not process merge requests filed on GitHub.
+
+QEMU welcomes contributions of code (either fixing bugs or adding new
+functionality). However, we get a lot of patches, and so we have some
+guidelines about submitting patches described in our Wiki:
+https://wiki.qemu.org/Contribute/SubmitAPatch and
+https://wiki.qemu.org/Contribute/FAQ
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d156d73b3..0559e84790 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2833,6 +2833,7 @@ M: Alex Bennée 
 M: Fam Zheng 
 R: Philippe Mathieu-Daudé 
 S: Maintained
+F: .github/lockdown.yml
 F: .travis.yml
 F: scripts/travis/
 F: .shippable.yml
-- 
2.21.1

Re: [RFC PATCH v2 7/7] vfio-ccw: Add support for the CRW irq

[Eric Farman]

On 4/6/20 12:22 PM, Cornelia Huck wrote:
> On Thu,  6 Feb 2020 22:45:09 +0100
> Eric Farman  wrote:
> 
>> From: Farhan Ali 
>>
>> The CRW irq will be used by vfio-ccw to notify the userspace
>> about any CRWs the userspace needs to handle. Let's add support
>> for it.
>>
>> Signed-off-by: Farhan Ali 
>> Signed-off-by: Eric Farman 
>> ---
>>
>> Notes:
>> v1->v2:
>>  - Add a loop to continually read region while data is
>>present, queueing CRWs as found [CH]
>> v0->v1: [EF]
>>  - Check vcdev->crw_region before registering the irq,
>>in case host kernel does not have matching support
>>  - Split the refactoring changes to an earlier (new) patch
>>(and don't remove the "num_irqs" check in the register
>>routine, but adjust it to the check the input variable)
>>  - Don't revert the cool vfio_set_irq_signaling() stuff
>>  - Unregister CRW IRQ before IO IRQ in unrealize
>>  - s/crw1/crw0/
>>
>>  hw/vfio/ccw.c | 51 +++
>>  1 file changed, 51 insertions(+)
>>
> 
>> @@ -265,6 +266,40 @@ static void vfio_ccw_reset(DeviceState *dev)
>>  ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET);
>>  }
>>  
>> +static void vfio_ccw_crw_notifier_handler(void *opaque)
>> +{
>> +VFIOCCWDevice *vcdev = opaque;
>> +struct ccw_crw_region *region = vcdev->crw_region;
>> +CRW crw;
>> +int size;
>> +uint8_t rsc, erc;
>> +
>> +if (!event_notifier_test_and_clear(>crw_notifier)) {
>> +return;
>> +}
>> +
>> +do {
>> +memset(region, 0, sizeof(*region));
>> +size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size,
>> + vcdev->crw_region_offset);
>> +
>> +if (size == -1) {
>> +error_report("vfio-ccw: Read crw region failed with errno=%d", 
>> errno);
>> +break;
>> +}
>> +
>> +if (size == 0 || region->crw0 == 0) {
> 
> Does it make any sense to expect both of them as an indication that
> there are no more crws at the moment? Grabbing a zeroed crw makes the
> most sense as a stop condition, I think.

I think it was overkill on my part.  Though it appears I am missing the
"zeroing" of the region once it got read.  Whoops.  Okay, those are easy
fixups.

> 
> Also, I'm not sure anymore whether having space for two crws makes too
> much sense. If we have a case in the future where we get two chained
> crws, the code will retry anyway and just fetch the chained crw and
> queue it, wouldn't it?

I suppose.

I thought the reason for including them now was to avoid "if region size
== 4 vs 8 vs XX" logic at some mysterious time in the future.  But
certainly ripping it out so we only pass a single CRW at a time would
simplify this quite a bit.

> 
>> +/* No more CRWs to queue */
>> +break;
>> +}
>> +
>> +memcpy(, >crw0, sizeof(CRW));
>> +rsc = (crw.flags & 0x0f00) >> 8;
>> +erc = crw.flags & 0x003f;
> 
> I think we already have something for that... ah yes,
> CRW_FLAGS_MASK_RSC and CRW_FLAGS_MASK_ERC.

Huh, look at that.  :)

> 
>> +css_queue_crw(rsc, erc, 0, 0, crw.rsid);
> 
> ...or maybe an alternative interface that allows us to queue a
> ready-made crw?

Sure, that would be nice.  I'll add that as an additional patch to this
series, prior to this one.

> 
>> +} while (1);
>> +}
>> +
>>  static void vfio_ccw_io_notifier_handler(void *opaque)
>>  {
>>  VFIOCCWDevice *vcdev = opaque;
> 

Re: [PATCH v3 12/12] configure: Add -Werror to PIE probe

[Philippe Mathieu-Daudé]

On 4/3/20 9:11 PM, Alex Bennée wrote:

From: Richard Henderson 

Without -Werror, the probe may succeed, but then compilation fails
later when -Werror is added for other reasons.  Shows up on windows,
where the compiler complains about -fPIC.

Signed-off-by: Richard Henderson 
Signed-off-by: Alex Bennée 
Message-Id: <20200401214756.6559-1-richard.hender...@linaro.org>


Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 


---
  configure | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 22870f38672..233c671aaa9 100755
--- a/configure
+++ b/configure
@@ -2119,7 +2119,7 @@ if compile_prog "-Werror -fno-pie" "-no-pie"; then
  fi
  
  if test "$static" = "yes"; then

-  if test "$pie" != "no" && compile_prog "-fPIE -DPIE" "-static-pie"; then
+  if test "$pie" != "no" && compile_prog "-Werror -fPIE -DPIE" "-static-pie"; 
then
  QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
  QEMU_LDFLAGS="-static-pie $QEMU_LDFLAGS"
  pie="yes"
@@ -2132,7 +2132,7 @@ if test "$static" = "yes"; then
  elif test "$pie" = "no"; then
QEMU_CFLAGS="$CFLAGS_NOPIE $QEMU_CFLAGS"
QEMU_LDFLAGS="$LDFLAGS_NOPIE $QEMU_LDFLAGS"
-elif compile_prog "-fPIE -DPIE" "-pie"; then
+elif compile_prog "-Werror -fPIE -DPIE" "-pie"; then
QEMU_CFLAGS="-fPIE -DPIE $QEMU_CFLAGS"
QEMU_LDFLAGS="-pie $QEMU_LDFLAGS"
pie="yes"

[Bug 1663287] Re: Illegal delay slot code causes abort on mips64

[martin short]

I found the exact same bug. Tested on several hosts and qemu releases.
The newest one I tested was on FreeBSD 12.1 host and qemu-4.1.1_1 built
from ports.

Instructions:
  4000d0:   0320f809jalrt9
  4000d4:   454545450x45454545 # bc1any4t $fcc1,0x800101f8

I was running qemu-mips as:

qemu-system-mipsel -s -m 1024 -M malta \
-kernel vmlinux-3.16.0-6-4kc-malta -initrd 
initrd.img-3.16.0-6-4kc-malta \
-device virtio-blk-pci,drive=hd0 -drive 
if=none,id=hd0,format=qcow2,file=debian_wheezy_mipsel_standard.qcow2\
-append "root=/dev/vda1" \
-device virtio-net-pci,netdev=net0 \
-netdev user,id=net0,hostfwd=tcp::1666-:22,ipv6=off  \
-curses 

abort() was in target/mips/translate.c:12945, in gen_branch().

Doesn't really matter if the instruction is supported on given CPU, user
can crash the qemu within guest.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1663287

Title:
  Illegal delay slot code causes abort on mips64

Status in QEMU:
  New

Bug description:
  During some randomised testing of an experimental MIPS implementation
  I found an instruction sequence that also causes aborts on mainline
  qemu's MIPS support.  The problem is triggered by an MSA branch
  instruction appearing in a delay slot when emulating a processor
  without MSA support.

  For example, with the current repository HEAD
  (f073cd3a2bf1054135271b837c58a7da650dd84b) configured for
  mips64-softmmu, if I run the attached binary using

  mips64-softmmu/qemu-system-mips64 -bios ../abort2.bin -machine
  mipssim -nographic

  it will report

  unknown branch 0x13000
  Aborted (core dumped)

  The binary contains the following two instructions:

  0028 jr at
  47081e61 bz.b   w8,0xbfc0798c

  The jr sets up a jump, and hflags is set accordingly in
  gen_compute_branch (in target/mips/translate.c).  When processing the
  bz.b, check_insn generates an exception because the instruction isn't
  support, but gen_msa_branch skips the usual delay slot check for the
  same reason, and sets more bits in hflags, leading to an abort in
  gen_branch because the hflags are now invalid.

  I suspect the best fix is to remove the instruction set condition from
  the delay slot check in gen_msa_branch.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1663287/+subscriptions

[PATCH] ati-vga: Fix checks in ati_2d_blt() to avoid crash

[BALATON Zoltan]

In some corner cases (that never happen during normal operation but a
malicious guest could program wrong values) pixman functions were
called with parameters that result in a crash. Fix this and add more
checks to disallow such cases.

Reported-by: Ziming Zhang 
Signed-off-by: BALATON Zoltan 
---
 hw/display/ati_2d.c | 37 ++---
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 42e82311eb..23a8ae0cd8 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -53,12 +53,20 @@ void ati_2d_blt(ATIVGAState *s)
 s->vga.vbe_start_addr, surface_data(ds), surface_stride(ds),
 surface_bits_per_pixel(ds),
 (s->regs.dp_mix & GMC_ROP3_MASK) >> 16);
-int dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
- s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
-int dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
- s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
+unsigned dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+  s->regs.dst_x : s->regs.dst_x + 1 - s->regs.dst_width);
+unsigned dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+  s->regs.dst_y : s->regs.dst_y + 1 - s->regs.dst_height);
 int bpp = ati_bpp_from_datatype(s);
+if (!bpp) {
+qemu_log_mask(LOG_GUEST_ERROR, "Invalid bpp\n");
+return;
+}
 int dst_stride = DEFAULT_CNTL ? s->regs.dst_pitch : s->regs.default_pitch;
+if (!dst_stride) {
+qemu_log_mask(LOG_GUEST_ERROR, "Zero dest pitch\n");
+return;
+}
 uint8_t *dst_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
 s->regs.dst_offset : s->regs.default_offset);
 
@@ -82,12 +90,16 @@ void ati_2d_blt(ATIVGAState *s)
 switch (s->regs.dp_mix & GMC_ROP3_MASK) {
 case ROP3_SRCCOPY:
 {
-int src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
- s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
-int src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
- s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
+unsigned src_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+   s->regs.src_x : s->regs.src_x + 1 - s->regs.dst_width);
+unsigned src_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+   s->regs.src_y : s->regs.src_y + 1 - s->regs.dst_height);
 int src_stride = DEFAULT_CNTL ?
  s->regs.src_pitch : s->regs.default_pitch;
+if (!src_stride) {
+qemu_log_mask(LOG_GUEST_ERROR, "Zero source pitch\n");
+return;
+}
 uint8_t *src_bits = s->vga.vram_ptr + (DEFAULT_CNTL ?
 s->regs.src_offset : s->regs.default_offset);
 
@@ -137,8 +149,10 @@ void ati_2d_blt(ATIVGAState *s)
 dst_y * surface_stride(ds),
 s->regs.dst_height * surface_stride(ds));
 }
-s->regs.dst_x += s->regs.dst_width;
-s->regs.dst_y += s->regs.dst_height;
+s->regs.dst_x = (s->regs.dp_cntl & DST_X_LEFT_TO_RIGHT ?
+ dst_x + s->regs.dst_width : dst_x);
+s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+ dst_y + s->regs.dst_height : dst_y);
 break;
 }
 case ROP3_PATCOPY:
@@ -179,7 +193,8 @@ void ati_2d_blt(ATIVGAState *s)
 dst_y * surface_stride(ds),
 s->regs.dst_height * surface_stride(ds));
 }
-s->regs.dst_y += s->regs.dst_height;
+s->regs.dst_y = (s->regs.dp_cntl & DST_Y_TOP_TO_BOTTOM ?
+ dst_y + s->regs.dst_height : dst_y);
 break;
 }
 default:
-- 
2.21.1

Re: [PATCH-for-5.1 v2 00/54] various: Fix error-propagation with Coccinelle scripts

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200406174743.16956-1-f4...@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  BUILD   fp-test
  CC  tests/qtest/endianness-test.o
/tmp/qemu-test/src/tests/test-qdev-global-props.c: In function 
'dynamic_instance_init':
/tmp/qemu-test/src/tests/test-qdev-global-props.c:154:38: error: 'error_abort' 
undeclared (first use in this function)
 NULL, NULL, _abort);
  ^
/tmp/qemu-test/src/tests/test-qdev-global-props.c:154:38: note: each undeclared 
identifier is reported only once for each function it appears in
make: *** [tests/test-qdev-global-props.o] Error 1
make: *** Waiting for unfinished jobs
  CC  fp-test.o
  CC  slowfloat.o
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=c834ecdcff4e47c596ebdcf857245223', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-12shpxs0/src/docker-src.2020-04-06-16.36.57.9698:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=c834ecdcff4e47c596ebdcf857245223
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-12shpxs0/src'
make: *** [docker-run-test-quick@centos7] Error 2

real2m47.727s
user0m8.997s


The full log is available at
http://patchew.org/logs/20200406174743.16956-1-f4...@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH-for-5.1 v2 00/54] various: Fix error-propagation with Coccinelle scripts

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200406174743.16956-1-f4...@amsat.org/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

clang -iquote /tmp/qemu-test/build/. -iquote . -iquote 
/tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem 
/tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote 
/tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote 
/tmp/qemu-test/src/disas/libvixl -I/tmp/qemu-test/src/tests/fp 
-I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/include 
-I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/8086-SSE 
-I/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source 
-I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address 
-pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 
-mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 
-Wno-string-plus-int -Wno-typedef-redefinition -Wno-initializer-overrides 
-Wexpansion-to-defined -Wendif-labels -Wno-shift-negative-value 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition 
-Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1 
-DLEGACY_RDMA_REG_MR -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 
-I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
-I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid 
-I/usr/include/uuid -I/usr/include/pixman-1 -DHW_POISON_H -DTARGET_ARM  
-DSOFTFLOAT_ROUND_ODD -DINLINE_LEVEL=5 -DSOFTFLOAT_FAST_DIV32TO16 
-DSOFTFLOAT_FAST_DIV64TO32 -DSOFTFLOAT_FAST_INT64  -DFLOAT16 -DFLOAT64 
-DEXTFLOAT80 -DFLOAT128 -DFLOAT_ROUND_ODD -DLONG_DOUBLE_IS_EXTFLOAT80  
-Wno-strict-prototypes -Wno-unknown-pragmas -Wno-uninitialized 
-Wno-missing-prototypes -Wno-return-type -Wno-unused-function -Wno-error -MMD 
-MP -MT verCases_common.o -MF ./verCases_common.d -g   -c -o verCases_common.o 
/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source/verCases_common.c
clang -iquote /tmp/qemu-test/build/. -iquote . -iquote 
/tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem 
/tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote 
/tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote 
/tmp/qemu-test/src/disas/libvixl -I/tmp/qemu-test/src/tests/fp 
-I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/include 
-I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/8086-SSE 
-I/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source 
-I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address 
-pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 
-mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 
-Wno-string-plus-int -Wno-typedef-redefinition -Wno-initializer-overrides 
-Wexpansion-to-defined -Wendif-labels -Wno-shift-negative-value 
-Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security 
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition 
-Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1 
-DLEGACY_RDMA_REG_MR -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 
-I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
-I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid 
-I/usr/include/uuid -I/usr/include/pixman-1 -DHW_POISON_H -DTARGET_ARM  
-DSOFTFLOAT_ROUND_ODD -DINLINE_LEVEL=5 -DSOFTFLOAT_FAST_DIV32TO16 
-DSOFTFLOAT_FAST_DIV64TO32 -DSOFTFLOAT_FAST_INT64  -DFLOAT16 -DFLOAT64 
-DEXTFLOAT80 -DFLOAT128 -DFLOAT_ROUND_ODD -DLONG_DOUBLE_IS_EXTFLOAT80  
-Wno-strict-prototypes -Wno-unknown-pragmas -Wno-uninitialized 
-Wno-missing-prototypes -Wno-return-type -Wno-unused-function -Wno-error -MMD 
-MP -MT verCases_writeFunctionName.o -MF ./verCases_writeFunctionName.d -g   -c 
-o verCases_writeFunctionName.o 
/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source/verCases_writeFunctionName.c
clang -iquote /tmp/qemu-test/build/tests/qtest/libqos -iquote 
tests/qtest/libqos -iquote /tmp/qemu-test/src/tcg/i386 -isystem 
/tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers 
-iquote . -iquote 

Re: [PATCH v4 0/3] Fix some AIO context locking in jobs

[John Snow]

On 4/2/20 8:48 AM, Kevin Wolf wrote:
> Am 01.04.2020 um 10:15 hat Stefan Reiter geschrieben:
>> Contains three seperate but related patches cleaning up and fixing some
>> issues regarding aio_context_acquire/aio_context_release for jobs. Mostly
>> affects blockjobs running for devices that have IO threads enabled AFAICT.
>>
>> This is based on the discussions here:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07929.html
> 
> I'm getting segfaults in some qemu-iotests cases:
> 
> Failures: 155 219 245 255 257 258
> 
> This is the backtrace of one of the coredumps I got, looks like use
> after free:
> 

fwiw, this appears to be the case after the very first patch, all six tests.

--js

Re: [PATCH-for-5.1 v2 37/54] scripts/coccinelle: Add script to catch missing error_propagate() calls

[Eric Blake]

On 4/6/20 12:47 PM, Philippe Mathieu-Daudé wrote:

In some places in we put an error into a local Error*, but forget
to check for failure and pass it back to the caller.
Add a Coccinelle patch to catch automatically add the missing code.


s/catch/catch and/



Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  .../add-missing-error_propagate.cocci | 30 +++
  MAINTAINERS   |  1 +
  2 files changed, 31 insertions(+)
  create mode 100644 scripts/coccinelle/add-missing-error_propagate.cocci

diff --git a/scripts/coccinelle/add-missing-error_propagate.cocci 
b/scripts/coccinelle/add-missing-error_propagate.cocci
new file mode 100644
index 00..7991c9e2c2
--- /dev/null
+++ b/scripts/coccinelle/add-missing-error_propagate.cocci
@@ -0,0 +1,30 @@
+// Add missing error-propagation code where caller provide a Error* argument


provides


+//
+// Copyright: (C) 2020 Philippe Mathieu-Daudé
+// This work is licensed under the terms of the GNU GPLv2 or later.
+//
+// spatch \
+//  --macro-file scripts/cocci-macro-file.h --include-headers \
+//  --sp-file scripts/coccinelle/add-missing-error_propagate.cocci \
+//  --keep-comments --in-place
+//
+// Inspired by 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691638.html
+
+
+@ add_missing_error_propagate @
+typedef Error;
+Error *local_err;
+identifier func, errp, errfunc1, errfunc2;
+@@
+func(..., Error **errp)
+{
+<...
+errfunc1(..., _err);
++   if (local_err) {
++   error_propagate(errp, local_err);
++   return;


Do we have to ensure that 'func' returns void?  But then again, I guess 
it's easy enough to validate whether things still compile after this 
script makes cleanups.



++   }
+... when != local_err
+errfunc2(..., _err);
+...>
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b58f02efb..14de2a31dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2053,6 +2053,7 @@ F: include/qemu/error-report.h
  F: qapi/error.json
  F: util/error.c
  F: util/qemu-error.c
+F: scripts/coccinelle/add-missing-error_propagate.cocci
  F: scripts/coccinelle/err-bad-newline.cocci
  F: scripts/coccinelle/error-use-after-free.cocci
  F: scripts/coccinelle/error_propagate_null.cocci



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/4] atomics: update documentation for C11

[Eric Blake]

On 4/6/20 2:13 PM, Paolo Bonzini wrote:

Deprecate atomic_mb_read and atomic_mb_set; it is not really possible to
use them correctly because they do not interoperate with sequentially-consistent
RMW operations.

Signed-off-by: Paolo Bonzini 
---
  docs/devel/atomics.rst | 290 -
  1 file changed, 114 insertions(+), 176 deletions(-)




@@ -24,6 +29,14 @@ Macros defined by ``qemu/atomic.h`` fall in three camps:
  
  - sequentially consistent atomic access: everything else.
  
+In general, use of ``qemu/atomic.h`` should be wrapped with more easily

+used data structures (e.g. the lock-free singly-liked list operations


linked


+``QSLIST_INSERT_HEAD_ATOMIC`` and ``QSLIST_MOVE_ATOMIC``) or synchronization
+primitives (such as RCU, ``QemuEvent`` or ``QemuLockCnt``).  Bare use of
+atomic operations and memory barriers should be limited to inter-thread
+checking of flags and documented thoroughly.
+
+
  
  Compiler memory barrier

  ===
@@ -85,36 +98,14 @@ Similar operations return the new value of ``*ptr``::
  typeof(*ptr) atomic_or_fetch(ptr, val)
  typeof(*ptr) atomic_xor_fetch(ptr, val)
  
-Sequentially consistent loads and stores can be done using::

-
-atomic_fetch_add(ptr, 0) for loads
-atomic_xchg(ptr, val) for stores
+These operations operate on any type that is as wide as an int or smaller.
  
-However, they are quite expensive on some platforms, notably POWER and

-Arm.  Therefore, qemu/atomic.h provides two primitives with slightly
-weaker constraints::
+``qemu/atomic.h`` also provides sequentially consistent loads and stores can::
  


s/ can//


  typeof(*ptr) atomic_mb_read(ptr)
  void atomic_mb_set(ptr, val)
  
-The semantics of these primitives map to Java volatile variables,

-and are strongly related to memory barriers as used in the Linux
-kernel (see below).
-
-As long as you use atomic_mb_read and atomic_mb_set, accesses cannot
-be reordered with each other, and it is also not possible to reorder
-"normal" accesses around them.
-
-However, and this is the important difference between
-atomic_mb_read/atomic_mb_set and sequential consistency, it is important
-for both threads to access the same volatile variable.  It is not the
-case that everything visible to thread A when it writes volatile field f
-becomes visible to thread B after it reads volatile field g. The store
-and load have to "match" (i.e., be performed on the same volatile
-field) to achieve the right semantics.
-
-
-These operations operate on any type that is as wide as an int or smaller.
+which however are deprecated.
  

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/4] atomics: convert to reStructuredText

[Eric Blake]

On 4/6/20 2:13 PM, Paolo Bonzini wrote:

Signed-off-by: Paolo Bonzini 
---
  docs/devel/atomics.rst | 447 +
  docs/devel/atomics.txt | 403 -
  docs/devel/index.rst   |   1 +
  3 files changed, 448 insertions(+), 403 deletions(-)
  create mode 100644 docs/devel/atomics.rst
  delete mode 100644 docs/devel/atomics.txt



Pre-existing grammar nits, that you may want to touch up while reformatting:


+Compiler memory barrier
+===
+
+``barrier()`` prevents the compiler from moving the memory accesses either
+side of it to the other side.  The compiler barrier has no direct effect


s/either/on either/



+``qemu/atomic.h`` provides the following set of atomic read-modify-write
+operations::
+
+void atomic_inc(ptr)
+void atomic_dec(ptr)
+void atomic_add(ptr, val)
+void atomic_sub(ptr, val)
+void atomic_and(ptr, val)
+void atomic_or(ptr, val)
+
+typeof(*ptr) atomic_fetch_inc(ptr)
+typeof(*ptr) atomic_fetch_dec(ptr)
+typeof(*ptr) atomic_fetch_add(ptr, val)
+typeof(*ptr) atomic_fetch_sub(ptr, val)
+typeof(*ptr) atomic_fetch_and(ptr, val)
+typeof(*ptr) atomic_fetch_or(ptr, val)
+typeof(*ptr) atomic_fetch_xor(ptr, val)
+typeof(*ptr) atomic_fetch_inc_nonzero(ptr)
+typeof(*ptr) atomic_xchg(ptr, val)
+typeof(*ptr) atomic_cmpxchg(ptr, old, new)
+
+all of which return the old value of ``*ptr``.  These operations are
+polymorphic; they operate on any type that is as wide as a pointer.


Is th is 'as wide as a pointer' or 'no wider than a pointer'? In other 
words, can 'val' be a narrower type?




+However, and this is the important difference between
+atomic_mb_read/atomic_mb_set and sequential consistency, it is important
+for both threads to access the same volatile variable.  It is not the
+case that everything visible to thread A when it writes volatile field f
+becomes visible to thread B after it reads volatile field g. The store
+and load have to "match" (i.e., be performed on the same volatile
+field) to achieve the right semantics.
+
+
+These operations operate on any type that is as wide as an int or smaller.


Is that all operations in this same section, or only the last set of two 
operations (atomic_mb_read/set)?  What is the appropriate code to use if 
a pointer is wider than int?




+
+You can see that the two possible definitions of ``atomic_mb_read()``
+and ``atomic_mb_set()`` are the following:
+
+  1) | atomic_mb_read(p)   = atomic_read(p); smp_mb_acquire()
+ | atomic_mb_set(p, v) = smp_mb_release(); atomic_set(p, v); smp_mb()
+
+  2) | atomic_mb_read(p)   = smp_mb() atomic_read(p); smp_mb_acquire()


Missing semicolon after smp_mb()


+ | atomic_mb_set(p, v) = smp_mb_release(); atomic_set(p, v);
+
+Usually the former is used, because ``smp_mb()`` is expensive and a program
+normally has more reads than writes.  Therefore it makes more sense to
+make ``atomic_mb_set()`` the more expensive operation.
+



+Memory barrier pairing
+--
+
+A useful rule of thumb is that memory barriers should always, or almost
+always, be paired with another barrier.  In the case of QEMU, however,
+note that the other barrier may actually be in a driver that runs in
+the guest!
+
+For the purposes of pairing, ``smp_read_barrier_depends()`` and ``smp_rmb()``
+both count as read barriers.  A read barrier shall pair with a write
+barrier or a full barrier; a write barrier shall pair with a read


'shall' is awkward (if this is not a formal RFC-style requirement), 
better for colloquial English is 'must' or 'should' (twice)



+barrier or a full barrier.  A full barrier can pair with anything.



+Comparison with Linux kernel memory barriers
+
+
+Here is a list of differences between Linux kernel atomic operations
+and memory barriers, and the equivalents in QEMU:
+
+- atomic operations in Linux are always on a 32-bit int type and
+  use a boxed atomic_t type; atomic operations in QEMU are polymorphic
+  and use normal C types.
+
+- Originally, atomic_read and atomic_set in Linux gave no guarantee
+  at all. Linux 4.1 updated them to implement volatile
+  semantics via ACCESS_ONCE (or the more recent READ/WRITE_ONCE).
+
+  QEMU's atomic_read/set implement, if the compiler supports it, C11
+  atomic relaxed semantics, and volatile semantics otherwise.


Reads better as:

QEMU's atomic_read/set implement C11 atomic relaxed semantics if the 
compiler supports it, and volatile semantics otherwise.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: FYI GitHub pull request / issue tracker lockdown bot

[John Snow]

On 4/3/20 10:22 AM, Daniel P. Berrangé wrote:
> QEMU, like libvirt, has a github.com project which contains automated
> read-only mirrors of QEMU repositories.
> 
>   https://github.com/qemu/
> 
> An unfortunate side effect of this is that some users will try to open
> pull requests against these mirrors. These get ignored until eventually
> someone notices and closes the request. QEMU has had about 90 prs opened
> over the years.
> 
>   https://github.com/qemu/qemu/pulls
> 
> The same applies to the issue tracker, but fortunately github lets
> projects disable this feature, which QEMU has done.
> 
> I have recently discovered that there is a nice 3rd party bot for github
> which can autorespond to pull requests with a friendly comment, close the
> request, and then lock it to prevent further comments.
> 
>   https://github.com/apps/repo-lockdown
> 
> I'm setting this up for libvirt and it was suggested QEMU can probably
> benefit from it too as an example see:
> 
>   https://github.com/libvirt/test/issues/2
>   https://github.com/libvirt/test/pull/3
> 
> 
> Configuration just requires creation of a ".github/lockdown.yml" file
> which provides the friendly message to add to the merge requests. This
> can be either done per-repository, or a special repo can be created
> called ".github" and this will apply to all repos within the project.
> 
> Ideally each repo would have a CONTRIBUTING.md file created too, since
> both GitHub and GitLab will direct users to this file for guidelines
> on how to contribute.
> 
> I don't have time right now to do this for QEMU, so consider this email
> a friendly suggestion for some other interested person to do for QEMU...
> 
> Regards,
> Daniel
> 

This looks cool. Who has access to our github to request it start
scanning our repo to look for said .github/lockdown.yml file?

--js

Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Peter Xu]

On Sat, Apr 04, 2020 at 12:00:12PM +, Liu, Yi L wrote:
> Hi Peter,
> 
> > From: Peter Xu 
> > Sent: Saturday, April 4, 2020 12:11 AM
> > To: Liu, Yi L 
> > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context 
> > cache
> > invalidation
> > 
> > On Fri, Apr 03, 2020 at 03:21:10PM +, Liu, Yi L wrote:
> > > > From: Peter Xu 
> > > > Sent: Friday, April 3, 2020 10:46 PM
> > > > To: Liu, Yi L 
> > > > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after 
> > > > context
> > cache
> > > > invalidation
> > > >
> > > > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote:
> > > > > This patch replays guest pasid bindings after context cache
> > > > > invalidation. This is a behavior to ensure safety. Actually,
> > > > > programmer should issue pasid cache invalidation with proper
> > > > > granularity after issuing a context cache invalidation.
> > > > >
> > > > > Cc: Kevin Tian 
> > > > > Cc: Jacob Pan 
> > > > > Cc: Peter Xu 
> > > > > Cc: Yi Sun 
> > > > > Cc: Paolo Bonzini 
> > > > > Cc: Richard Henderson 
> > > > > Cc: Eduardo Habkost 
> > > > > Signed-off-by: Liu Yi L 
> > > > > ---
> > > > >  hw/i386/intel_iommu.c  | 51
> > > > ++
> > > > >  hw/i386/intel_iommu_internal.h |  6 -
> > > > >  hw/i386/trace-events   |  1 +
> > > > >  3 files changed, 57 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > > index d87f608..883aeac 100644
> > > > > --- a/hw/i386/intel_iommu.c
> > > > > +++ b/hw/i386/intel_iommu.c
> > > > > @@ -68,6 +68,10 @@ static void
> > > > vtd_address_space_refresh_all(IntelIOMMUState *s);
> > > > >  static void vtd_address_space_unmap(VTDAddressSpace *as, 
> > > > > IOMMUNotifier
> > *n);
> > > > >
> > > > >  static void vtd_pasid_cache_reset(IntelIOMMUState *s);
> > > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > > > > + VTDPASIDCacheInfo *pc_info);
> > > > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s,
> > > > > +  VTDBus *vtd_bus, uint16_t devfn);
> > > > >
> > > > >  static void vtd_panic_require_caching_mode(void)
> > > > >  {
> > > > > @@ -1853,7 +1857,10 @@ static void
> > vtd_iommu_replay_all(IntelIOMMUState
> > > > *s)
> > > > >
> > > > >  static void vtd_context_global_invalidate(IntelIOMMUState *s)
> > > > >  {
> > > > > +VTDPASIDCacheInfo pc_info;
> > > > > +
> > > > >  trace_vtd_inv_desc_cc_global();
> > > > > +
> > > > >  /* Protects context cache */
> > > > >  vtd_iommu_lock(s);
> > > > >  s->context_cache_gen++;
> > > > > @@ -1870,6 +1877,9 @@ static void
> > > > vtd_context_global_invalidate(IntelIOMMUState *s)
> > > > >   * VT-d emulation codes.
> > > > >   */
> > > > >  vtd_iommu_replay_all(s);
> > > > > +
> > > > > +pc_info.flags = VTD_PASID_CACHE_GLOBAL;
> > > > > +vtd_pasid_cache_sync(s, _info);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > @@ -2005,6 +2015,22 @@ static void
> > > > vtd_context_device_invalidate(IntelIOMMUState *s,
> > > > >   * happened.
> > > > >   */
> > > > >  vtd_sync_shadow_page_table(vtd_as);
> > > > > +/*
> > > > > + * Per spec, context flush should also
> > > > > followed with PASID
> > > > > + * cache and iotlb flush. Regards to
> > > > > a device selective
> > > > > + * context cache invalidation:
> > > >
> > > > If context entry flush should also follow another pasid cache flush,
> > > > then this is still needed?  Shouldn't the pasid flush do the same
> > > > thing again?
> > >
> > > yes, but how about guest software failed to follow it? It will do
> > > the same thing when pasid cache flush comes. But this only happens
> > > for the rid2pasid case (the IOVA page table).
> > 
> > Do you mean it will not happen when nested page table is used (so it's
> > required for nested tables)?
> 
> no, by the IOVA page table case, I just want to confirm the duplicate
> replay is true. But it is not "only" case. :-) my bad. any scalable mode
> context entry modification will result in duplicate replay as this patch
> enforces a pasid replay after context cache invalidation. But for normal
> guest SVM usage, it won't have such duplicate work as it only modifies
> pasid entry.
> 
> > Yeah we can keep them for safe no matter what; at least I'm fine with
> > it (I believe most of the code we're discussing is not fast path).
> > Just want to be sure of it since if it's definitely duplicated then we
> > can instead drop it.
> 
> yes, it is not fast path. BTW. I guess the iova shadow sync applies
> the same notion. right?

Yes I rem we have similar things, but the same to that - if we can
confirm that it'll be duplicated then I think we should remove that
too.  But feel free to ignore this question for now and keep it.  The
comment explaining that 

[Bug 1863441] Re: cmd_mode_sense always reports 0x70, no CDROM present

[John Snow]

Hi, if you can quote the relevant spec, would you like to include that
in a commit message and send a patch?

Also, does this result in a bug anywhere visible that we can test
against?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1863441

Title:
  cmd_mode_sense always reports 0x70, no CDROM present

Status in QEMU:
  New

Bug description:
  cmd_mode_sense

https://git.qemu.org/?p=qemu.git;a=blob;f=hw/ide/atapi.c;hb=refs/heads/master#l852
  always reports 0x70 in byte 2 returned, indicating no CD-ROM present.

  If CD-ROM is present, should report 0x01 (or 0x11).
  If CD-ROM absent, should report 0x70.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1863441/+subscriptions

Re: [PATCH-for-5.1 v2 53/54] hw/mips/mips_malta: Add missing error-propagation code

[Philippe Mathieu-Daudé]

On 4/6/20 7:47 PM, Philippe Mathieu-Daudé wrote:

Running the coccinelle script produced:

   $ spatch \
 --macro-file scripts/cocci-macro-file.h --include-headers \
 --sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
 --keep-comments --smpl-spacing --dir .
   HANDLING: ./hw/mips/mips_malta.c
   [[manual check required: error_propagate() might be missing in 
object_property_set_int() ./hw/mips/mips_malta.c:1193:4]]
   [[manual check required: error_propagate() might be missing in 
object_property_set_str() ./hw/mips/mips_malta.c:1192:4]]

Add the missing error_propagate() after review by adding
a Error* parameter to create_cps().

Reviewed-by: Peter Maydell 
Reviewed-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/mips/mips_malta.c | 19 ++-
  1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..8d43cfd41b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1183,18 +1183,27 @@ static void create_cpu_without_cps(MachineState *ms,
  }
  
  static void create_cps(MachineState *ms, MaltaState *s,

-   qemu_irq *cbus_irq, qemu_irq *i8259_irq)
+   qemu_irq *cbus_irq, qemu_irq *i8259_irq,
+   Error **errp)
  {
  Error *err = NULL;
  
  sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(>cps), sizeof(s->cps),

TYPE_MIPS_CPS);
  object_property_set_str(OBJECT(>cps), ms->cpu_type, "cpu-type", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
  object_property_set_int(OBJECT(>cps), ms->smp.cpus, "num-vp", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
  object_property_set_bool(OBJECT(>cps), true, "realized", );
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
+if (err) {
+error_propagate(errp, err);
+return;
  }
  
  sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>cps), 0, 0, 1);

@@ -1207,7 +1216,7 @@ static void mips_create_cpu(MachineState *ms, MaltaState 
*s,
  qemu_irq *cbus_irq, qemu_irq *i8259_irq)
  {
  if ((ms->smp.cpus > 1) && cpu_supports_cps_smp(ms->cpu_type)) {
-create_cps(ms, s, cbus_irq, i8259_irq);
+create_cps(ms, s, cbus_irq, i8259_irq, _fatal);
  } else {
  create_cpu_without_cps(ms, cbus_irq, i8259_irq);
  }



This patch also requires:

-- >8 --
@@ -1241,7 +1241,7 @@ void mips_malta_init(MachineState *machine)
 int64_t kernel_entry, bootloader_run_addr;
 PCIBus *pci_bus;
 ISABus *isa_bus;
-qemu_irq cbus_irq, i8259_irq;
+qemu_irq cbus_irq, i8259_irq = NULL;
 I2CBus *smbus;
 DriveInfo *dinfo;
 int fl_idx = 0;
---

else it fails building with gcc -O3:

hw/mips/mips_malta.c: In function ‘mips_malta_init’:
hw/mips/mips_malta.c:1419:5: warning: ‘i8259_irq’ may be used 
uninitialized in this function [-Wmaybe-uninitialized]

 1419 | qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
  | ^~

[PATCH 4/4] async: use explicit memory barriers and relaxed accesses

[Paolo Bonzini]

When using C11 atomics, non-seqcst reads and writes do not participate
in the total order of seqcst operations.  In util/async.c and util/aio-posix.c,
in particular, the pattern that we use

  write ctx->notify_me write bh->scheduled
  read bh->scheduled   read ctx->notify_me
  if !bh->scheduled, sleep if ctx->notify_me, notify

needs to use seqcst operations for both the write and the read.  In
general this is something that we do not want, because there can be
many sources that are polled in addition to bottom halves.  The
alternative is to place a seqcst memory barrier between the write
and the read.  This also comes with a disadvantage, in that the
memory barrier is implicit on strongly-ordered architectures and
it wastes a few dozen clock cycles.

Fortunately, ctx->notify_me is never written concurrently by two
threads, so we can instead relax the writes to ctx->notify_me.
[This part of the commit message is still to be written more
in detail and is what I am not sure about.]

Analyzed-by: Ying Fang 
Signed-off-by: Paolo Bonzini 
---
 util/aio-posix.c |  9 -
 util/aio-win32.c |  8 +++-
 util/async.c | 12 ++--
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index cd6cf0a4a9..37afec726f 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -569,7 +569,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
  * so disable the optimization now.
  */
 if (blocking) {
-atomic_add(>notify_me, 2);
+atomic_set(>notify_me, atomic_read(>notify_me) + 2);
+/*
+ * Write ctx->notify_me before computing the timeout
+ * (reading bottom half flags, etc.).  Pairs with
+ * atomic_xchg in aio_notify().
+ */
+smp_mb();
 }
 
 qemu_lockcnt_inc(>list_lock);
@@ -590,6 +596,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
 }
 
 if (blocking) {
+/* Finish the poll before clearing the flag.  */
 atomic_sub(>notify_me, 2);
 aio_notify_accept(ctx);
 }
diff --git a/util/aio-win32.c b/util/aio-win32.c
index a23b9c364d..2cccdb35c1 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -331,7 +331,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
  * so disable the optimization now.
  */
 if (blocking) {
-atomic_add(>notify_me, 2);
+atomic_set(>notify_me, atomic_read(>notify_me) + 2);
+/*
+ * Write ctx->notify_me before computing the timeout
+ * (reading bottom half flags, etc.).  Pairs with
+ * atomic_xchg in aio_notify().
+ */
+smp_mb();
 }
 
 qemu_lockcnt_inc(>list_lock);
diff --git a/util/async.c b/util/async.c
index b94518b948..ee1bc87d2b 100644
--- a/util/async.c
+++ b/util/async.c
@@ -249,7 +249,14 @@ aio_ctx_prepare(GSource *source, gint*timeout)
 {
 AioContext *ctx = (AioContext *) source;
 
-atomic_or(>notify_me, 1);
+atomic_set(>notify_me, atomic_read(>notify_me) | 1);
+
+/*
+ * Write ctx->notify_me before computing the timeout
+ * (reading bottom half flags, etc.).  Pairs with
+ * atomic_xchg in aio_notify().
+ */
+smp_mb();
 
 /* We assume there is no timeout already supplied */
 *timeout = qemu_timeout_ns_to_ms(aio_compute_timeout(ctx));
@@ -414,7 +422,7 @@ void aio_notify(AioContext *ctx)
  * with atomic_or in aio_ctx_prepare or atomic_add in aio_poll.
  */
 smp_mb();
-if (ctx->notify_me) {
+if (atomic_read(>notify_me)) {
 event_notifier_set(>notifier);
 atomic_mb_set(>notified, true);
 }
-- 
2.18.2

[PATCH 3/4] rcu: do not mention atomic_mb_read/set in documentation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 docs/devel/rcu.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/devel/rcu.txt b/docs/devel/rcu.txt
index d83fed2f79..0ce15ba198 100644
--- a/docs/devel/rcu.txt
+++ b/docs/devel/rcu.txt
@@ -132,7 +132,7 @@ The core RCU API is small:
 
  typeof(*p) atomic_rcu_read(p);
 
-atomic_rcu_read() is similar to atomic_mb_read(), but it makes
+atomic_rcu_read() is similar to atomic_load_acquire(), but it makes
 some assumptions on the code that calls it.  This allows a more
 optimized implementation.
 
@@ -154,7 +154,7 @@ The core RCU API is small:
 
  void atomic_rcu_set(p, typeof(*p) v);
 
-atomic_rcu_set() is also similar to atomic_mb_set(), and it also
+atomic_rcu_set() is similar to atomic_store_release(), though it also
 makes assumptions on the code that calls it in order to allow a more
 optimized implementation.
 
-- 
2.18.2

[PATCH 2/4] atomics: update documentation for C11

[Paolo Bonzini]

Deprecate atomic_mb_read and atomic_mb_set; it is not really possible to
use them correctly because they do not interoperate with sequentially-consistent
RMW operations.

Signed-off-by: Paolo Bonzini 
---
 docs/devel/atomics.rst | 290 -
 1 file changed, 114 insertions(+), 176 deletions(-)

diff --git a/docs/devel/atomics.rst b/docs/devel/atomics.rst
index 00f3f7d3ed..e92752a64d 100644
--- a/docs/devel/atomics.rst
+++ b/docs/devel/atomics.rst
@@ -11,10 +11,15 @@ that is consistent with the expectations of the programmer.
 The most basic tool is locking.  Mutexes, condition variables and
 semaphores are used in QEMU, and should be the default approach to
 synchronization.  Anything else is considerably harder, but it's
-also justified more often than one would like.  The two tools that
-are provided by ``qemu/atomic.h`` are memory barriers and atomic operations.
+also justified more often than one would like;
+the most performance-critical parts of QEMU in particular require
+a very low level approach to concurrency, involving memory barriers
+and atomic operations.  The semantics of concurrent memory accesses are 
governed
+by the C11 memory model.
 
-Macros defined by ``qemu/atomic.h`` fall in three camps:
+QEMU provides a header, ``qemu/atomic.h``, which wraps C11 atomics to
+provide better portability and a less verbose syntax.  ``qemu/atomic.h``
+provides macros that fall in three camps:
 
 - compiler barriers: ``barrier()``;
 
@@ -24,6 +29,14 @@ Macros defined by ``qemu/atomic.h`` fall in three camps:
 
 - sequentially consistent atomic access: everything else.
 
+In general, use of ``qemu/atomic.h`` should be wrapped with more easily
+used data structures (e.g. the lock-free singly-liked list operations
+``QSLIST_INSERT_HEAD_ATOMIC`` and ``QSLIST_MOVE_ATOMIC``) or synchronization
+primitives (such as RCU, ``QemuEvent`` or ``QemuLockCnt``).  Bare use of
+atomic operations and memory barriers should be limited to inter-thread
+checking of flags and documented thoroughly.
+
+
 
 Compiler memory barrier
 ===
@@ -85,36 +98,14 @@ Similar operations return the new value of ``*ptr``::
 typeof(*ptr) atomic_or_fetch(ptr, val)
 typeof(*ptr) atomic_xor_fetch(ptr, val)
 
-Sequentially consistent loads and stores can be done using::
-
-atomic_fetch_add(ptr, 0) for loads
-atomic_xchg(ptr, val) for stores
+These operations operate on any type that is as wide as an int or smaller.
 
-However, they are quite expensive on some platforms, notably POWER and
-Arm.  Therefore, qemu/atomic.h provides two primitives with slightly
-weaker constraints::
+``qemu/atomic.h`` also provides sequentially consistent loads and stores can::
 
 typeof(*ptr) atomic_mb_read(ptr)
 void atomic_mb_set(ptr, val)
 
-The semantics of these primitives map to Java volatile variables,
-and are strongly related to memory barriers as used in the Linux
-kernel (see below).
-
-As long as you use atomic_mb_read and atomic_mb_set, accesses cannot
-be reordered with each other, and it is also not possible to reorder
-"normal" accesses around them.
-
-However, and this is the important difference between
-atomic_mb_read/atomic_mb_set and sequential consistency, it is important
-for both threads to access the same volatile variable.  It is not the
-case that everything visible to thread A when it writes volatile field f
-becomes visible to thread B after it reads volatile field g. The store
-and load have to "match" (i.e., be performed on the same volatile
-field) to achieve the right semantics.
-
-
-These operations operate on any type that is as wide as an int or smaller.
+which however are deprecated.
 
 
 Weak atomic access and manual memory barriers
@@ -208,135 +199,62 @@ They come in six kinds:
   dependency and a full read barrier or better is required.
 
 
-This is the set of barriers that is required *between* two ``atomic_read()``
-and ``atomic_set()`` operations to achieve sequential consistency:
-
-   ++---+
-   || 2nd operation |
-   |+--+-+--+
-   | 1st operation  | (after last) | atomic_read | atomic_set   |
-   ++--+-+--+
-   | (before first) | ..   | none| smp_mb_release() |
-   ++--+-+--+
-   | atomic_read| smp_mb_acquire() | smp_rmb() [1]_  | [2]_ |
-   ++--+-+--+
-   | atomic_set | none | smp_mb() [3]_   | smp_wmb()|
-   ++--+-+--+
-
-   .. [1] Or smp_read_barrier_depends().
-
-   .. [2] This requires a load-store barrier.  This is 

[RFC PATCH 0/4] async: fix hangs on weakly-ordered architectures

[Paolo Bonzini]

Patch 4 fixes qemu-img and qemu-io hangs on weakly-ordered architectures.
Patch 1-3 are related docs fixes and improvements.

This is RFC because it relies on the iothread being locked during aio_poll
on the main AioContext.  If I add assertions for this however I see a
failure for test 267, so I am posting it as a preview before I debug that.
The doc patches can also go in independently of course.

Paolo

Paolo Bonzini (4):
  atomics: convert to reStructuredText
  atomics: update documentation for C11
  rcu: do not mention atomic_mb_read/set in documentation
  async: use explicit memory barriers

 docs/devel/atomics.rst | 385 +++
 docs/devel/atomics.txt | 403 -
 docs/devel/index.rst   |   1 +
 docs/devel/rcu.txt |   4 +-
 util/aio-posix.c   |   9 +-
 util/aio-win32.c   |   8 +-
 util/async.c   |  12 +-
 7 files changed, 413 insertions(+), 409 deletions(-)
 create mode 100644 docs/devel/atomics.rst
 delete mode 100644 docs/devel/atomics.txt

-- 
2.18.2

[PATCH 1/4] atomics: convert to reStructuredText

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 docs/devel/atomics.rst | 447 +
 docs/devel/atomics.txt | 403 -
 docs/devel/index.rst   |   1 +
 3 files changed, 448 insertions(+), 403 deletions(-)
 create mode 100644 docs/devel/atomics.rst
 delete mode 100644 docs/devel/atomics.txt

diff --git a/docs/devel/atomics.rst b/docs/devel/atomics.rst
new file mode 100644
index 00..00f3f7d3ed
--- /dev/null
+++ b/docs/devel/atomics.rst
@@ -0,0 +1,447 @@
+=
+Atomic operations in QEMU
+=
+
+CPUs perform independent memory operations effectively in random order.
+but this can be a problem for CPU-CPU interaction (including interactions
+between QEMU and the guest).  Multi-threaded programs use various tools
+to instruct the compiler and the CPU to restrict the order to something
+that is consistent with the expectations of the programmer.
+
+The most basic tool is locking.  Mutexes, condition variables and
+semaphores are used in QEMU, and should be the default approach to
+synchronization.  Anything else is considerably harder, but it's
+also justified more often than one would like.  The two tools that
+are provided by ``qemu/atomic.h`` are memory barriers and atomic operations.
+
+Macros defined by ``qemu/atomic.h`` fall in three camps:
+
+- compiler barriers: ``barrier()``;
+
+- weak atomic access and manual memory barriers: ``atomic_read()``,
+  ``atomic_set()``, ``smp_rmb()``, ``smp_wmb()``, ``smp_mb()``, 
``smp_mb_acquire()``,
+  ``smp_mb_release()``, ``smp_read_barrier_depends()``;
+
+- sequentially consistent atomic access: everything else.
+
+
+Compiler memory barrier
+===
+
+``barrier()`` prevents the compiler from moving the memory accesses either
+side of it to the other side.  The compiler barrier has no direct effect
+on the CPU, which may then reorder things however it wishes.
+
+``barrier()`` is mostly used within ``qemu/atomic.h`` itself.  On some
+architectures, CPU guarantees are strong enough that blocking compiler
+optimizations already ensures the correct order of execution.  In this
+case, ``qemu/atomic.h`` will reduce stronger memory barriers to simple
+compiler barriers.
+
+Still, ``barrier()`` can be useful when writing code that can be interrupted
+by signal handlers.
+
+
+Sequentially consistent atomic access
+=
+
+Most of the operations in the ``qemu/atomic.h`` header ensure *sequential
+consistency*, where "the result of any execution is the same as if the
+operations of all the processors were executed in some sequential order,
+and the operations of each individual processor appear in this sequence
+in the order specified by its program".
+
+``qemu/atomic.h`` provides the following set of atomic read-modify-write
+operations::
+
+void atomic_inc(ptr)
+void atomic_dec(ptr)
+void atomic_add(ptr, val)
+void atomic_sub(ptr, val)
+void atomic_and(ptr, val)
+void atomic_or(ptr, val)
+
+typeof(*ptr) atomic_fetch_inc(ptr)
+typeof(*ptr) atomic_fetch_dec(ptr)
+typeof(*ptr) atomic_fetch_add(ptr, val)
+typeof(*ptr) atomic_fetch_sub(ptr, val)
+typeof(*ptr) atomic_fetch_and(ptr, val)
+typeof(*ptr) atomic_fetch_or(ptr, val)
+typeof(*ptr) atomic_fetch_xor(ptr, val)
+typeof(*ptr) atomic_fetch_inc_nonzero(ptr)
+typeof(*ptr) atomic_xchg(ptr, val)
+typeof(*ptr) atomic_cmpxchg(ptr, old, new)
+
+all of which return the old value of ``*ptr``.  These operations are
+polymorphic; they operate on any type that is as wide as a pointer.
+
+Similar operations return the new value of ``*ptr``::
+
+typeof(*ptr) atomic_inc_fetch(ptr)
+typeof(*ptr) atomic_dec_fetch(ptr)
+typeof(*ptr) atomic_add_fetch(ptr, val)
+typeof(*ptr) atomic_sub_fetch(ptr, val)
+typeof(*ptr) atomic_and_fetch(ptr, val)
+typeof(*ptr) atomic_or_fetch(ptr, val)
+typeof(*ptr) atomic_xor_fetch(ptr, val)
+
+Sequentially consistent loads and stores can be done using::
+
+atomic_fetch_add(ptr, 0) for loads
+atomic_xchg(ptr, val) for stores
+
+However, they are quite expensive on some platforms, notably POWER and
+Arm.  Therefore, qemu/atomic.h provides two primitives with slightly
+weaker constraints::
+
+typeof(*ptr) atomic_mb_read(ptr)
+void atomic_mb_set(ptr, val)
+
+The semantics of these primitives map to Java volatile variables,
+and are strongly related to memory barriers as used in the Linux
+kernel (see below).
+
+As long as you use atomic_mb_read and atomic_mb_set, accesses cannot
+be reordered with each other, and it is also not possible to reorder
+"normal" accesses around them.
+
+However, and this is the important difference between
+atomic_mb_read/atomic_mb_set and sequential consistency, it is important
+for both threads to access the same volatile variable.  It is not the
+case that everything visible to thread A when it writes volatile field f

Re: [PATCH-for-5.1 7/8] .travis.yml: Run fetch-acceptance-assets before check-acceptance

[Willian Rampazzo]

On Mon, Apr 6, 2020 at 3:21 PM Alex Bennée  wrote:
>
>
> Willian Rampazzo  writes:
>
> > On Mon, Apr 6, 2020 at 12:39 PM Philippe Mathieu-Daudé
> >  wrote:
> >>
> >> On 4/6/20 5:31 PM, Alex Bennée wrote:
> >> >
> >> > Philippe Mathieu-Daudé  writes:
> >> >
> >> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> >> ---
> >> >>   .travis.yml | 2 +-
> >> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/.travis.yml b/.travis.yml
> >> >> index 2fd63eceaa..c6b32da447 100644
> >> >> --- a/.travis.yml
> >> >> +++ b/.travis.yml
> >> >> @@ -317,7 +317,7 @@ jobs:
> >> >> dist: bionic
> >> >> env:
> >> >>   - CONFIG="--enable-tools 
> >> >> --target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
> >> >> -- TEST_CMD="make check-acceptance"
> >> >> +- TEST_CMD="travis_retry make -j1 fetch-acceptance-assets
> >> >> check-acceptance DEBUG=1"
> >> >
> >> > You could use TEST_BUILD_CMD for the fetching of acceptance tests - can
> >> > that be done in parallel?
> >
> > If by `parallel` you mean running it with other targets, the
> > fetch-acceptance-assets target can run at any time before the
> > check-acceptance runs, concurrently with different targets. Now, if by
> > `parallel` you mean fetch more than one asset at a time, right now it
> > is not supported by Avocado, assets are fetched one by one, but it is
> > an excellent idea. I have added it to my list.
>
> See the comment I made about the make file. We could expand the
> fetch-acceptance-assets target to have one dependency per python file so
> you could invoke avocado in parallel to fetch the assets for each test
> group in parallel. Of course this works best if the assets mentioned in
> each file are unique otherwise avocado might race with itself.

I see what you mean. About the unique asset definitions, this should
not be a big problem as Avocado downloads the asset on a temporary
file first and then copy it to the target file. In the worst case,
multiple instances of Avocado will download the same asset at the same
time, but just one will make it the target asset. The other instances
will fail, but as one already succeeded, the downside is the time and
bandwidth spent by the other instances.

>
> >> I'd say calling fetch-acceptance-assets parallelized is what seems to
> >> break this Travis job (which is why I enforced -j1), but I'll refer to
> >> Willian here.
> >>
> >
> > As I mentioned above, no problem with running it in parallel to other
> > targets. The errors we saw when you were testing are not related to
> > the parallel run. It should be fixed in the next release of Avocado.
> >
> >> >
> >> > Also no point in -j1 here - it's implied.
> >>
> >> You are right, I wanted to be sure this still work even if the globlal
> >> script running the tests is updated. But the correct fix is probably to
> >> use .NOTPARALLEL in the fetch-acceptance-assets rule in the previous patch.
> >>
> >> >
> >> >>   - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
> >> >> after_script:
> >> >>   - python3 -c 'import json; r = 
> >> >> json.load(open("tests/results/latest/results.json")); 
> >> >> [print(t["logfile"]) for t in r["tests"] if t["status"] not in ("PASS", 
> >> >> "SKIP")]' | xargs cat
> >> >
> >> >
> >>
>
>
> --
> Alex Bennée
>

Re: [PATCH for-5.0] tcg/i386: Fix %r12 guest_base initialization

[Alex Bennée]

Richard Henderson  writes:

> When %gs cannot be used, we use register offset addressing.
> This path is almost never used, so it was clearly not tested.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 
Tested-by: Alex Bennée 

> ---
>  tcg/i386/tcg-target.inc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index 7f61eeedd0..ec083bddcf 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -3737,7 +3737,7 @@ static void tcg_target_qemu_prologue(TCGContext *s)
>  } else {
>  /* Choose R12 because, as a base, it requires a SIB byte. */
>  x86_guest_base_index = TCG_REG_R12;
> -tcg_out_mov(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base);
> +tcg_out_movi(s, TCG_TYPE_PTR, x86_guest_base_index, guest_base);
>  tcg_regset_set_reg(s->reserved_regs, x86_guest_base_index);
>  }
>  }


-- 
Alex Bennée

Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Eric Farman]

On 4/1/20 4:52 AM, Cornelia Huck wrote:
> On Wed, 25 Mar 2020 03:24:28 +0100
> Halil Pasic  wrote:
> 
>> On Tue, 24 Mar 2020 18:04:30 +0100
>> Cornelia Huck  wrote:
>>
>>> On Thu,  6 Feb 2020 22:45:03 +0100
>>> Eric Farman  wrote:
>>>   
 From: Farhan Ali 

 EIO is returned by vfio-ccw mediated device when the backing
 host subchannel is not operational anymore. So return cc=3
 back to the guest, rather than returning a unit check.
 This way the guest can take appropriate action such as
 issue an 'stsch'.  
>>
>> I believe this is not the only situation when vfio-ccw returns
>> EIO, or?
>>

 Signed-off-by: Farhan Ali 
 Signed-off-by: Eric Farman 
 ---

 Notes:
 v1->v2: [EF]
  - Add s-o-b
  - [Seems the discussion on v1 centered on the return code
set in the kernel, rather than anything that needs to
change here, unless I've missed something.]  
>>
>> Does this need to change here? If the kernel is supposed to return ENODEV
>> then this does not need to change.
>>
>>>
>>> I've stared at this and at the kernel code for some time again; and I'm
>>> not sure if "return -EIO == not operational" is even true. That said,
>>> I'm not sure a unit check is the right response, either. The only thing
>>> I'm sure about is that the kernel code needs some review of return
>>> codes and some documentation...  
>>
>> I could not agree more, this is semantically uapi and needs to be
>> properly documented.
>>
>> With regards to "linux error codes: vs "ionist cc's" an where
>> the mapping is different example:
>>
>> """
>> /**  
>>
>>  * cio_cancel_halt_clear - Cancel running I/O by performing cancel, halt 
>>
>>  * and clear ordinally if subchannel is valid.   
>>
>>  * @sch: subchannel on which to perform the cancel_halt_clear operation  
>>
>>  * @iretry: the number of the times remained to retry the next operation 
>>
>>  *   
>>
>>  * This should be called repeatedly since halt/clear are asynchronous
>>
>>  * operations. We do one try with cio_cancel, three tries with cio_halt, 
>>
>>  * 255 tries with cio_clear. The caller should initialize @iretry with   
>>
>>  * the value 255 for its first call to this, and keep using the same 
>>
>>  * @iretry in the subsequent calls until it gets a non -EBUSY return.
>>
>>  *   
>>
>>  * Returns 0 if device now idle, -ENODEV for device not operational, 
>>
>>  * -EBUSY if an interrupt is expected (either from halt/clear or from a  
>>
>>  * status pending), and -EIO if out of retries.  
>>
>>  */  
>>
>> int cio_cancel_halt_clear(struct subchannel *sch, int *iretry)   
>>
>> """
>> Here -ENODEV is not operational.
> 
> Ok, I went through the kernel code and checked which error may be
> returned in which case (hope I caught all of them). Here's what I
> currently have:

Thanks for doing all this mapping!

> 
> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> index fca9c4f5bd9c..43f375a03cce 100644
> --- a/Documentation/s390/vfio-ccw.rst
> +++ b/Documentation/s390/vfio-ccw.rst
> @@ -210,7 +210,36 @@ Subchannel.
>  
>  irb_area stores the I/O result.
>  
> -ret_code stores a return code for each access of the region.
> +ret_code stores a return code for each access of the region. The following
> +values may occur:
> +
> +0
> +  The operation was successful.
> +
> +-EOPNOTSUPP
> +  The orb specified transport mode or an unidentified IDAW format, did not
> +  specify prefetch mode, or the scsw specified a function other than the
> +  start function.
> +
> +-EIO
> +  A request was issued while the device was not in a state ready to accept
> +  requests, or an internal error occurred.
> +
> +-EBUSY
> +  The subchannel was status pending or busy, or a request is already active.
> +
> +-EAGAIN
> +  A request was being processed, and the caller should retry.
> +
> +-EACCES
> +  The channel path(s) used for the I/O were found to be not operational.
> +
> +-ENODEV
> +  The device was found to be not operational.
> +
> +-EINVAL
> +  The orb specified a chain longer than 255 ccws, or an internal error
> +  occurred.
>  
>  This region is always available.
>  
> @@ -231,6 +260,29 @@ This region is exposed via region type 
> VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
>  
>  Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
>  
> +command specifies the command to be issued; ret_code stores a return code
> +for each access of the region. The following values may occur:
> +
> +0
> +  The operation was 

Re: [PATCH-for-5.1 7/8] .travis.yml: Run fetch-acceptance-assets before check-acceptance

[Willian Rampazzo]

On Mon, Apr 6, 2020 at 12:39 PM Philippe Mathieu-Daudé
 wrote:
>
> On 4/6/20 5:31 PM, Alex Bennée wrote:
> >
> > Philippe Mathieu-Daudé  writes:
> >
> >> Signed-off-by: Philippe Mathieu-Daudé 
> >> ---
> >>   .travis.yml | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/.travis.yml b/.travis.yml
> >> index 2fd63eceaa..c6b32da447 100644
> >> --- a/.travis.yml
> >> +++ b/.travis.yml
> >> @@ -317,7 +317,7 @@ jobs:
> >> dist: bionic
> >> env:
> >>   - CONFIG="--enable-tools 
> >> --target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
> >> -- TEST_CMD="make check-acceptance"
> >> +- TEST_CMD="travis_retry make -j1 fetch-acceptance-assets
> >> check-acceptance DEBUG=1"
> >
> > You could use TEST_BUILD_CMD for the fetching of acceptance tests - can
> > that be done in parallel?

If by `parallel` you mean running it with other targets, the
fetch-acceptance-assets target can run at any time before the
check-acceptance runs, concurrently with different targets. Now, if by
`parallel` you mean fetch more than one asset at a time, right now it
is not supported by Avocado, assets are fetched one by one, but it is
an excellent idea. I have added it to my list.

>
> I'd say calling fetch-acceptance-assets parallelized is what seems to
> break this Travis job (which is why I enforced -j1), but I'll refer to
> Willian here.
>

As I mentioned above, no problem with running it in parallel to other
targets. The errors we saw when you were testing are not related to
the parallel run. It should be fixed in the next release of Avocado.

> >
> > Also no point in -j1 here - it's implied.
>
> You are right, I wanted to be sure this still work even if the globlal
> script running the tests is updated. But the correct fix is probably to
> use .NOTPARALLEL in the fetch-acceptance-assets rule in the previous patch.
>
> >
> >>   - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
> >> after_script:
> >>   - python3 -c 'import json; r = 
> >> json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) 
> >> for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
> >
> >
>

Re: [PATCH-for-5.1 7/8] .travis.yml: Run fetch-acceptance-assets before check-acceptance

[Alex Bennée]

Willian Rampazzo  writes:

> On Mon, Apr 6, 2020 at 12:39 PM Philippe Mathieu-Daudé
>  wrote:
>>
>> On 4/6/20 5:31 PM, Alex Bennée wrote:
>> >
>> > Philippe Mathieu-Daudé  writes:
>> >
>> >> Signed-off-by: Philippe Mathieu-Daudé 
>> >> ---
>> >>   .travis.yml | 2 +-
>> >>   1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/.travis.yml b/.travis.yml
>> >> index 2fd63eceaa..c6b32da447 100644
>> >> --- a/.travis.yml
>> >> +++ b/.travis.yml
>> >> @@ -317,7 +317,7 @@ jobs:
>> >> dist: bionic
>> >> env:
>> >>   - CONFIG="--enable-tools 
>> >> --target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
>> >> -- TEST_CMD="make check-acceptance"
>> >> +- TEST_CMD="travis_retry make -j1 fetch-acceptance-assets
>> >> check-acceptance DEBUG=1"
>> >
>> > You could use TEST_BUILD_CMD for the fetching of acceptance tests - can
>> > that be done in parallel?
>
> If by `parallel` you mean running it with other targets, the
> fetch-acceptance-assets target can run at any time before the
> check-acceptance runs, concurrently with different targets. Now, if by
> `parallel` you mean fetch more than one asset at a time, right now it
> is not supported by Avocado, assets are fetched one by one, but it is
> an excellent idea. I have added it to my list.

See the comment I made about the make file. We could expand the
fetch-acceptance-assets target to have one dependency per python file so
you could invoke avocado in parallel to fetch the assets for each test
group in parallel. Of course this works best if the assets mentioned in
each file are unique otherwise avocado might race with itself.

>> I'd say calling fetch-acceptance-assets parallelized is what seems to
>> break this Travis job (which is why I enforced -j1), but I'll refer to
>> Willian here.
>>
>
> As I mentioned above, no problem with running it in parallel to other
> targets. The errors we saw when you were testing are not related to
> the parallel run. It should be fixed in the next release of Avocado.
>
>> >
>> > Also no point in -j1 here - it's implied.
>>
>> You are right, I wanted to be sure this still work even if the globlal
>> script running the tests is updated. But the correct fix is probably to
>> use .NOTPARALLEL in the fetch-acceptance-assets rule in the previous patch.
>>
>> >
>> >>   - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
>> >> after_script:
>> >>   - python3 -c 'import json; r = 
>> >> json.load(open("tests/results/latest/results.json")); 
>> >> [print(t["logfile"]) for t in r["tests"] if t["status"] not in ("PASS", 
>> >> "SKIP")]' | xargs cat
>> >
>> >
>>


-- 
Alex Bennée

[PATCH-for-5.1 v2 50/54] scripts/coccinelle: Find eventually missing error_propagate() calls

[Philippe Mathieu-Daudé]

In some places in we put an error into a local Error*, but
forget to use it. Add a Coccinelle patch to find such cases
and report them.

Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .../find-missing-error_propagate.cocci| 53 +++
 MAINTAINERS   |  1 +
 2 files changed, 54 insertions(+)
 create mode 100644 scripts/coccinelle/find-missing-error_propagate.cocci

diff --git a/scripts/coccinelle/find-missing-error_propagate.cocci 
b/scripts/coccinelle/find-missing-error_propagate.cocci
new file mode 100644
index 00..8b75b37b64
--- /dev/null
+++ b/scripts/coccinelle/find-missing-error_propagate.cocci
@@ -0,0 +1,53 @@
+// Find places likely missing error-propagation code, but code is too
+// complex for automatic transformation, so manual analysis is required.
+//
+// Copyright: (C) 2020 Philippe Mathieu-Daudé
+// This work is licensed under the terms of the GNU GPLv2 or later.
+//
+// spatch \
+//  --macro-file scripts/cocci-macro-file.h --include-headers \
+//  --sp-file scripts/coccinelle/find-missing-error_propagate.cocci
+//
+// Inspired by 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691638.html
+
+
+// First match two subsequent calls using local Error*
+// in function provided a Error** argument
+//
+@discard_func_with_errp_argument@
+typedef Error;
+Error *local_err;
+identifier func, errp, errfunc1, errfunc2;
+@@
+void func(..., Error **errp)
+{
+ <+...
+ errfunc1(..., _err);
+ ... when != local_err  // local_err is not used between the calls
+ errfunc2(..., _err);
+ ...+>
+}
+
+
+// Again, match two subsequent calls using local Error*
+// but ignoring within functions provided a Error** argument
+//
+@manual depends on never discard_func_with_errp_argument@
+Error *local_err;
+identifier errfunc1, errfunc2;
+position p;
+@@
+ errfunc1@p(..., _err);
+ ... when != local_err
+ errfunc2(..., _err);
+
+
+// As it is likely too complex to transform, report the hit
+//
+@script:python@
+f << manual.errfunc1;
+p << manual.p;
+@@
+print("[[manual check required: "
+  "error_propagate() might be missing in {}() {}:{}:{}]]".format(
+f, p[0].file, p[0].line, p[0].column))
diff --git a/MAINTAINERS b/MAINTAINERS
index ae71a0a4b0..29d29461f3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2057,6 +2057,7 @@ F: scripts/coccinelle/add-missing-error_propagate.cocci
 F: scripts/coccinelle/err-bad-newline.cocci
 F: scripts/coccinelle/error-use-after-free.cocci
 F: scripts/coccinelle/error_propagate_null.cocci
+F: scripts/coccinelle/find-missing-error_propagate.cocci
 F: scripts/coccinelle/remove_local_err.cocci
 F: scripts/coccinelle/simplify-init-realize-error_propagate.cocci
 F: scripts/coccinelle/use-error_abort-in-instance_init.cocci
-- 
2.21.1

[PATCH-for-5.1 v2 52/54] hw/mips/boston: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Running the coccinelle script produced:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
--keep-comments --smpl-spacing --dir .
  HANDLING: ./hw/mips/boston.c
  [[manual check required: error_propagate() might be missing in 
object_property_set_int() ./hw/mips/boston.c:462:4]]
  [[manual check required: error_propagate() might be missing in 
object_property_set_str() ./hw/mips/boston.c:460:4]]

Since the uses are inside a MachineClass::init() function,
directly use _fatal instead of error_propagate().

Reviewed-by: Peter Maydell 
Reviewed-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/boston.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 98ecd25e8e..2e821ca7d6 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -426,7 +426,6 @@ static void boston_mach_init(MachineState *machine)
 {
 DeviceState *dev;
 BostonState *s;
-Error *err = NULL;
 MemoryRegion *flash, *ddr_low_alias, *lcd, *platreg;
 MemoryRegion *sys_mem = get_system_memory();
 XilinxPCIEHost *pcie2;
@@ -458,19 +457,15 @@ static void boston_mach_init(MachineState *machine)
 sysbus_init_child_obj(OBJECT(machine), "cps", OBJECT(>cps),
   sizeof(s->cps), TYPE_MIPS_CPS);
 object_property_set_str(OBJECT(>cps), machine->cpu_type, "cpu-type",
-);
-object_property_set_int(OBJECT(>cps), machine->smp.cpus, "num-vp", 
);
-object_property_set_bool(OBJECT(>cps), true, "realized", );
-
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
-}
-
+_fatal);
+object_property_set_int(OBJECT(>cps), machine->smp.cpus, "num-vp",
+_fatal);
+object_property_set_bool(OBJECT(>cps), true, "realized", _fatal);
 sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>cps), 0, 0, 1);
 
 flash =  g_new(MemoryRegion, 1);
-memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB, );
+memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB,
+   _fatal);
 memory_region_add_subregion_overlap(sys_mem, 0x1800, flash, 0);
 
 memory_region_add_subregion_overlap(sys_mem, 0x8000, machine->ram, 0);
-- 
2.21.1

[PATCH-for-5.1 v2 43/54] hw/mips/cps: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Unfortunately the cocci script doesn't properly patch trailing
error_propagate() block and emitted duplicate if() block statements.
These 3 blocks were manually removed.

Reviewed-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/cps.c | 48 
 1 file changed, 48 insertions(+)

diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 92b9b1a5f6..18943b64e0 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -102,9 +102,21 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_child_obj(OBJECT(dev), "itu", >itu, sizeof(s->itu),
   TYPE_MIPS_ITU);
 object_property_set_int(OBJECT(>itu), 16, "num-fifo", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>itu), 16, "num-semaphores", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>itu), saar_present, "saar-present",
  );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 if (saar_present) {
 s->itu.saar = >CP0_SAAR;
 }
@@ -122,7 +134,15 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_child_obj(OBJECT(dev), "cpc", >cpc, sizeof(s->cpc),
   TYPE_MIPS_CPC);
 object_property_set_int(OBJECT(>cpc), s->num_vp, "num-vp", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>cpc), 1, "vp-start-running", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>cpc), true, "realized", );
 if (err != NULL) {
 error_propagate(errp, err);
@@ -136,7 +156,15 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_child_obj(OBJECT(dev), "gic", >gic, sizeof(s->gic),
   TYPE_MIPS_GIC);
 object_property_set_int(OBJECT(>gic), s->num_vp, "num-vp", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>gic), 128, "num-irq", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>gic), true, "realized", );
 if (err != NULL) {
 error_propagate(errp, err);
@@ -152,10 +180,30 @@ static void mips_cps_realize(DeviceState *dev, Error 
**errp)
 sysbus_init_child_obj(OBJECT(dev), "gcr", >gcr, sizeof(s->gcr),
   TYPE_MIPS_GCR);
 object_property_set_int(OBJECT(>gcr), s->num_vp, "num-vp", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>gcr), 0x800, "gcr-rev", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>gcr), gcr_base, "gcr-base", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_link(OBJECT(>gcr), OBJECT(>gic.mr), "gic", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_link(OBJECT(>gcr), OBJECT(>cpc.mr), "cpc", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>gcr), true, "realized", );
 if (err != NULL) {
 error_propagate(errp, err);
-- 
2.21.1

Re: [PATCH-for-5.1 v2 03/54] hw/arm/allwinner-a10: Move some code from realize() to init()

[Eric Blake]

On 4/6/20 12:46 PM, Philippe Mathieu-Daudé wrote:

Coccinelle reported:

   $ spatch ... --timeout 60 --sp-file \
 scripts/coccinelle/simplify-init-realize-error_propagate.cocci
   HANDLING: ./hw/arm/allwinner-a10.c
   >>> possible moves from aw_a10_init() to aw_a10_realize() in 
./hw/arm/allwinner-a10.c:77

Move the calls using _fatal which don't depend of input


s/of/on/ (here, and in many subsequent messages in the series)


updated before realize() to init().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/allwinner-a10.c | 26 +-
  1 file changed, 13 insertions(+), 13 deletions(-)



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH-for-5.1 v2 38/54] hw/arm/bcm2835_peripherals: Add missing error-propagation code

[Philippe Mathieu-Daudé]

On 4/6/20 7:47 PM, Philippe Mathieu-Daudé wrote:

Patch created mechanically by running:

   $ spatch \
 --macro-file scripts/cocci-macro-file.h --include-headers \
 --sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci 
\


sigh I forgot to update this, the scrip has been renamed 
add-missing-error_propagate.cocci :(



 --keep-comments --smpl-spacing --in-place --dir hw

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/bcm2835_peripherals.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index edcaa4916d..a111e91069 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -289,8 +289,16 @@ static void bcm2835_peripherals_realize(DeviceState *dev, 
Error **errp)
   *   SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf
   */
  object_property_set_uint(OBJECT(>sdhci), 3, "sd-spec-version", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
  object_property_set_uint(OBJECT(>sdhci), BCM2835_SDHC_CAPAREG, 
"capareg",
   );
+if (err) {
+error_propagate(errp, err);
+return;
+}
  object_property_set_bool(OBJECT(>sdhci), true, "pending-insert-quirk",
   );
  if (err) {

[PATCH-for-5.1 v2 48/54] scripts/coccinelle: Use _abort in TypeInfo::instance_init()

[Philippe Mathieu-Daudé]

The instance_init() calls are not suppose to fail. Add a
Coccinelle script to use _abort instead of ignoring
errors by using a NULL Error*.

Signed-off-by: Philippe Mathieu-Daudé 
---
 .../use-error_abort-in-instance_init.cocci| 52 +++
 MAINTAINERS   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 scripts/coccinelle/use-error_abort-in-instance_init.cocci

diff --git a/scripts/coccinelle/use-error_abort-in-instance_init.cocci 
b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
new file mode 100644
index 00..8302d74a0c
--- /dev/null
+++ b/scripts/coccinelle/use-error_abort-in-instance_init.cocci
@@ -0,0 +1,52 @@
+// Use _abort in TypeInfo::instance_init()
+//
+// Copyright: (C) 2020 Philippe Mathieu-Daudé
+// This work is licensed under the terms of the GNU GPLv2 or later.
+//
+// spatch \
+//  --macro-file scripts/cocci-macro-file.h --include-headers \
+//  --sp-file scripts/coccinelle/use-error_abort-in-instance_init.cocci \
+//  --keep-comments --in-place
+//
+// Inspired by 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg692500.html
+// and https://www.mail-archive.com/qemu-devel@nongnu.org/msg693637.html
+
+
+@ has_qapi_error @
+@@
+#include "qapi/error.h"
+
+
+@ match_instance_init @
+TypeInfo info;
+identifier instance_initfn;
+@@
+info.instance_init = instance_initfn;
+
+
+@ use_error_abort @
+identifier match_instance_init.instance_initfn;
+identifier func_with_error;
+expression parentobj, propname, childobj, size, type, errp;
+position pos;
+@@
+void instance_initfn(...)
+{
+   <+...
+(
+   object_initialize_child(parentobj, propname,
+   childobj, size, type,
+   errp, NULL);
+|
+   func_with_error@pos(...,
+-   NULL);
++   _abort);
+)
+   ...+>
+}
+
+
+@script:python depends on use_error_abort && !has_qapi_error@
+p << use_error_abort.pos;
+@@
+print('[[manual edit required, %s misses #include "qapi/error.h"]]' % 
p[0].file)
diff --git a/MAINTAINERS b/MAINTAINERS
index 14de2a31dc..ae71a0a4b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2059,6 +2059,7 @@ F: scripts/coccinelle/error-use-after-free.cocci
 F: scripts/coccinelle/error_propagate_null.cocci
 F: scripts/coccinelle/remove_local_err.cocci
 F: scripts/coccinelle/simplify-init-realize-error_propagate.cocci
+F: scripts/coccinelle/use-error_abort-in-instance_init.cocci
 F: scripts/coccinelle/use-error_fatal.cocci
 F: scripts/coccinelle/use-error_propagate-in-realize.cocci
 
-- 
2.21.1

[PATCH-for-5.1 v2 42/54] hw/i386/x86: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Running the coccinelle script:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

inserted a block after object_property_set_uint("apic-id") which
calls error_propagate() and return.
Thanksfully code review noticed returning here would skip the
'object_unref(cpu)' call.
Manually fix the error propagation code by adding a label to the
existing call.

Reported-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i386/x86.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b82770024c..ec807ce94f 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -124,8 +124,12 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, 
Error **errp)
 cpu = object_new(MACHINE(x86ms)->cpu_type);
 
 object_property_set_uint(cpu, apic_id, "apic-id", _err);
+if (local_err) {
+goto out;
+}
 object_property_set_bool(cpu, true, "realized", _err);
 
+out:
 object_unref(cpu);
 error_propagate(errp, local_err);
 }
-- 
2.21.1

Re: [PATCH-for-5.1 6/8] tests/Makefile: Add fetch-acceptance-assets rule

[Alex Bennée]

Willian Rampazzo  writes:

> On Mon, Apr 6, 2020 at 12:41 PM Alex Bennée  wrote:
>>
>>
>> Philippe Mathieu-Daudé  writes:
>>
>> > Signed-off-by: Philippe Mathieu-Daudé 
>> > ---
>> >  tests/Makefile.include | 7 +++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/tests/Makefile.include b/tests/Makefile.include
>> > index 51de676298..90f457593c 100644
>> > --- a/tests/Makefile.include
>> > +++ b/tests/Makefile.include
>> > @@ -906,6 +906,13 @@ get-vm-image-fedora-31-%: check-venv
>> >  # download all vm images, according to defined targets
>> >  get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, 
>> > $(FEDORA_31_DOWNLOAD))
>> >
>> > +fetch-acceptance-assets: check-venv
>> > + $(call quiet-command, \
>> > +$(TESTS_VENV_DIR)/bin/python -m avocado \
>> > +--show=$(if $(DEBUG),avocado.test,$(AVOCADO_SHOW)) assets 
>> > fetch \
>> > +tests/acceptance/*py, \
>> > +"AVOCADO", "tests/acceptance")
>> > +
>>
>> I'm wondering if we could expand this to a rule per-test group and
>> therefor allow parallel fetching of groups of assets?
>
> This is a valid idea! Additionally, my suggestion is to organize the
> tests by subdirectories under `tests/acceptance/`. Doing so makes it
> easy to handle the target rule, adding the subdirectory to the assets
> fetch command instead of listing specific tests for each group. Making
> a list of tests for each group requires extra editing to the makefile
> target when a new test is added. This can lead to new tests being
> missed in their groups by mistake.

I'd use wildcards to build up the groups so the Makefile doesn't need to
be messed with again.

>
>>
>> >  check-acceptance: check-venv $(TESTS_RESULTS_DIR) get-vm-images
>> >   $(call quiet-command, \
>> >  $(TESTS_VENV_DIR)/bin/python -m avocado \
>>
>>
>> --
>> Alex Bennée
>>


-- 
Alex Bennée

[PATCH-for-5.1 v2 41/54] hw/dma/xilinx_axidma: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

Then review showed this file has a 'xilinx_axidma_realize_fail'
label that calls error_propagate().  Updated the patch to use
the label.

Reviewed-by: Alistair Francis 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: New cocci patch generated both transformations
(Peter noticed v1 only catched one)
---
 hw/dma/xilinx_axidma.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 018f36991b..2d36346319 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -531,6 +531,9 @@ static void xilinx_axidma_realize(DeviceState *dev, Error 
**errp)
  object_property_allow_set_link,
  OBJ_PROP_LINK_STRONG,
  _err);
+if (local_err) {
+goto xilinx_axidma_realize_fail;
+}
 object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
  (Object **)>dma,
  object_property_allow_set_link,
@@ -540,6 +543,9 @@ static void xilinx_axidma_realize(DeviceState *dev, Error 
**errp)
 goto xilinx_axidma_realize_fail;
 }
 object_property_set_link(OBJECT(ds), OBJECT(s), "dma", _err);
+if (local_err) {
+goto xilinx_axidma_realize_fail;
+}
 object_property_set_link(OBJECT(cs), OBJECT(s), "dma", _err);
 if (local_err) {
 goto xilinx_axidma_realize_fail;
-- 
2.21.1

[PATCH-for-5.1 v2 49/54] various: Use _abort in instance_init()

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h \
--include-headers --keep-comments --in-place \
--sp-file \
  scripts/coccinelle/use-error_abort-in-instance_init.cocci

Signed-off-by: Philippe Mathieu-Daudé 
---
 backends/cryptodev-vhost-user.c |  2 +-
 backends/rng-egd.c  |  2 +-
 backends/rng-random.c   |  2 +-
 backends/vhost-user.c   |  3 ++-
 hw/arm/bcm2835_peripherals.c|  6 --
 hw/arm/vexpress.c   |  8 
 hw/arm/xlnx-zcu102.c|  8 
 hw/block/fdc.c  |  4 ++--
 hw/block/vhost-user-blk.c   |  2 +-
 hw/block/virtio-blk.c   |  2 +-
 hw/core/machine.c   |  6 +++---
 hw/cpu/core.c   |  4 ++--
 hw/display/vga-pci.c|  7 +--
 hw/display/xlnx_dp.c|  4 ++--
 hw/dma/sparc32_dma.c|  2 +-
 hw/gpio/aspeed_gpio.c   |  2 +-
 hw/ide/macio.c  |  4 +++-
 hw/ide/qdev.c   |  4 ++--
 hw/intc/apic_common.c   |  2 +-
 hw/mem/nvdimm.c |  4 ++--
 hw/misc/aspeed_sdmc.c   |  2 +-
 hw/misc/edu.c   |  3 ++-
 hw/misc/macio/macio.c   |  4 ++--
 hw/misc/macio/pmu.c |  3 ++-
 hw/misc/pca9552.c   |  2 +-
 hw/misc/tmp105.c|  2 +-
 hw/misc/tmp421.c|  8 
 hw/net/e1000.c  |  3 ++-
 hw/net/lance.c  |  3 ++-
 hw/net/lasi_i82596.c|  3 ++-
 hw/net/ne2000-isa.c |  4 ++--
 hw/net/spapr_llan.c |  2 +-
 hw/net/virtio-net.c |  2 +-
 hw/pci-host/grackle.c   |  2 +-
 hw/pci-host/i440fx.c|  8 
 hw/pci-host/prep.c  |  2 +-
 hw/pci-host/q35.c   | 23 ++-
 hw/pci-host/sabre.c |  3 ++-
 hw/pci-host/uninorth.c  |  9 +
 hw/pcmcia/pxa2xx.c  |  3 ++-
 hw/ppc/spapr_drc.c  |  6 +++---
 hw/ppc/spapr_rng.c  |  2 +-
 hw/riscv/sifive_u.c |  4 ++--
 hw/s390x/event-facility.c   |  6 +++---
 hw/s390x/s390-ccw.c |  2 +-
 hw/s390x/s390-skeys.c   |  4 ++--
 hw/s390x/s390-stattrib.c|  4 ++--
 hw/s390x/sclp.c |  2 +-
 hw/scsi/scsi-bus.c  |  2 +-
 hw/ssi/xilinx_spips.c   |  2 +-
 hw/usb/bus.c|  4 ++--
 hw/usb/dev-network.c|  2 +-
 hw/usb/dev-storage.c|  4 ++--
 hw/usb/host-libusb.c|  2 +-
 hw/usb/redirect.c   |  2 +-
 hw/virtio/virtio-balloon.c  |  4 ++--
 net/dump.c  |  4 ++--
 net/filter-buffer.c |  2 +-
 net/filter-mirror.c | 10 +-
 net/filter-rewriter.c   |  3 ++-
 target/arm/cpu64.c  |  4 ++--
 target/i386/cpu.c   | 23 ++-
 target/s390x/cpu.c  |  3 ++-
 tests/check-qom-proplist.c  |  7 ---
 tests/test-qdev-global-props.c  |  4 ++--
 65 files changed, 155 insertions(+), 126 deletions(-)

diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index 6edada8e9e..2fb28c13ac 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -341,7 +341,7 @@ static void cryptodev_vhost_user_instance_int(Object *obj)
 object_property_add_str(obj, "chardev",
 cryptodev_vhost_user_get_chardev,
 cryptodev_vhost_user_set_chardev,
-NULL);
+_abort);
 }
 
 static void cryptodev_vhost_user_finalize(Object *obj)
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index e380519408..58fb73f03a 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -139,7 +139,7 @@ static void rng_egd_init(Object *obj)
 {
 object_property_add_str(obj, "chardev",
 rng_egd_get_chardev, rng_egd_set_chardev,
-NULL);
+_abort);
 }
 
 static void rng_egd_finalize(Object *obj)
diff --git a/backends/rng-random.c b/backends/rng-random.c
index a810581393..6429276a95 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -111,7 +111,7 @@ static void rng_random_init(Object *obj)
 object_property_add_str(obj, "filename",
 rng_random_get_filename,
 rng_random_set_filename,
-NULL);
+_abort);
 
 s->filename = g_strdup("/dev/urandom");
 s->fd = -1;
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 2bf3406525..491da81653 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -177,7 +177,8 @@ static char *get_chardev(Object *obj, Error **errp)
 
 static void vhost_user_backend_init(Object *obj)
 {
-object_property_add_str(obj, "chardev", 

Re: [PATCH-for-5.1 v2 49/54] various: Use _abort in instance_init()

[Philippe Mathieu-Daudé]

On 4/6/20 7:47 PM, Philippe Mathieu-Daudé wrote:

Patch created mechanically by running:

   $ spatch \
 --macro-file scripts/cocci-macro-file.h \
 --include-headers --keep-comments --in-place \
 --sp-file \
   scripts/coccinelle/use-error_abort-in-instance_init.cocci

Signed-off-by: Philippe Mathieu-Daudé 
---
  backends/cryptodev-vhost-user.c |  2 +-
  backends/rng-egd.c  |  2 +-
  backends/rng-random.c   |  2 +-
  backends/vhost-user.c   |  3 ++-
  hw/arm/bcm2835_peripherals.c|  6 --
  hw/arm/vexpress.c   |  8 
  hw/arm/xlnx-zcu102.c|  8 
  hw/block/fdc.c  |  4 ++--
  hw/block/vhost-user-blk.c   |  2 +-
  hw/block/virtio-blk.c   |  2 +-
  hw/core/machine.c   |  6 +++---
  hw/cpu/core.c   |  4 ++--
  hw/display/vga-pci.c|  7 +--
  hw/display/xlnx_dp.c|  4 ++--
  hw/dma/sparc32_dma.c|  2 +-
  hw/gpio/aspeed_gpio.c   |  2 +-
  hw/ide/macio.c  |  4 +++-
  hw/ide/qdev.c   |  4 ++--
  hw/intc/apic_common.c   |  2 +-
  hw/mem/nvdimm.c |  4 ++--
  hw/misc/aspeed_sdmc.c   |  2 +-
  hw/misc/edu.c   |  3 ++-
  hw/misc/macio/macio.c   |  4 ++--
  hw/misc/macio/pmu.c |  3 ++-
  hw/misc/pca9552.c   |  2 +-
  hw/misc/tmp105.c|  2 +-
  hw/misc/tmp421.c|  8 
  hw/net/e1000.c  |  3 ++-
  hw/net/lance.c  |  3 ++-
  hw/net/lasi_i82596.c|  3 ++-
  hw/net/ne2000-isa.c |  4 ++--
  hw/net/spapr_llan.c |  2 +-
  hw/net/virtio-net.c |  2 +-
  hw/pci-host/grackle.c   |  2 +-
  hw/pci-host/i440fx.c|  8 
  hw/pci-host/prep.c  |  2 +-
  hw/pci-host/q35.c   | 23 ++-
  hw/pci-host/sabre.c |  3 ++-
  hw/pci-host/uninorth.c  |  9 +
  hw/pcmcia/pxa2xx.c  |  3 ++-
  hw/ppc/spapr_drc.c  |  6 +++---
  hw/ppc/spapr_rng.c  |  2 +-
  hw/riscv/sifive_u.c |  4 ++--
  hw/s390x/event-facility.c   |  6 +++---
  hw/s390x/s390-ccw.c |  2 +-
  hw/s390x/s390-skeys.c   |  4 ++--
  hw/s390x/s390-stattrib.c|  4 ++--
  hw/s390x/sclp.c |  2 +-
  hw/scsi/scsi-bus.c  |  2 +-
  hw/ssi/xilinx_spips.c   |  2 +-
  hw/usb/bus.c|  4 ++--
  hw/usb/dev-network.c|  2 +-
  hw/usb/dev-storage.c|  4 ++--
  hw/usb/host-libusb.c|  2 +-
  hw/usb/redirect.c   |  2 +-
  hw/virtio/virtio-balloon.c  |  4 ++--
  net/dump.c  |  4 ++--
  net/filter-buffer.c |  2 +-
  net/filter-mirror.c | 10 +-
  net/filter-rewriter.c   |  3 ++-
  target/arm/cpu64.c  |  4 ++--
  target/i386/cpu.c   | 23 ++-
  target/s390x/cpu.c  |  3 ++-
  tests/check-qom-proplist.c  |  7 ---
  tests/test-qdev-global-props.c  |  4 ++--
  65 files changed, 155 insertions(+), 126 deletions(-)


[...]

diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 270c690479..09315f412c 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c


Oops I forgot to commit this hunk:

@@ -27,6 +27,7 @@
 #include "hw/qdev-properties.h"
 #include "qom/object.h"
 #include "qapi/visitor.h"
+#include "qapi/error.h"


 #define TYPE_STATIC_PROPS "static_prop_type"


@@ -151,9 +151,9 @@ static void prop2_accessor(Object *obj, Visitor *v, const 
char *name,
  static void dynamic_instance_init(Object *obj)
  {
  object_property_add(obj, "prop1", "uint32", prop1_accessor, 
prop1_accessor,
-NULL, NULL, NULL);
+NULL, NULL, _abort);
  object_property_add(obj, "prop2", "uint32", prop2_accessor, 
prop2_accessor,
-NULL, NULL, NULL);
+NULL, NULL, _abort);
  }
  
  static void dynamic_class_init(ObjectClass *oc, void *data)

[PATCH-for-5.1 v2 36/54] hw/block/onenand: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/onenand.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 898ac563a3..1f68dba28b 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -810,7 +810,11 @@ static void onenand_realize(DeviceState *dev, Error **errp)
 s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT),
 0xff, (64 + 2) << PAGE_SHIFT);
 memory_region_init_ram_nomigrate(>ram, OBJECT(s), "onenand.ram",
-   0xc000 << s->shift, _fatal);
+   0xc000 << s->shift, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
 vmstate_register_ram_global(>ram);
 ram = memory_region_get_ram_ptr(>ram);
 s->boot[0] = ram + (0x << s->shift);
-- 
2.21.1

[PATCH-for-5.1 v2 54/54] qga/commands-win32: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Running the coccinelle script produced:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
--keep-comments --smpl-spacing --dir .
  HANDLING: ./qga/commands-win32.c
  [[manual check required: error_propagate() might be missing in 
acquire_privilege() ./qga/commands-win32.c:1344:4]]
  [[manual check required: error_propagate() might be missing in 
acquire_privilege() ./qga/commands-win32.c:1360:4]]
  [[manual check required: error_propagate() might be missing in 
check_suspend_mode() ./qga/commands-win32.c:1343:4]]
  [[manual check required: error_propagate() might be missing in 
check_suspend_mode() ./qga/commands-win32.c:1359:4]]

Add the missing error_propagate() after review.

Signed-off-by: Philippe Mathieu-Daudé 
---
 qga/commands-win32.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b49920e201..8b66098056 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)
 
 *mode = GUEST_SUSPEND_MODE_DISK;
 check_suspend_mode(*mode, _err);
+if (local_err) {
+goto out;
+}
 acquire_privilege(SE_SHUTDOWN_NAME, _err);
+if (local_err) {
+goto out;
+}
 execute_async(do_suspend, mode, _err);
 
-if (local_err) {
-error_propagate(errp, local_err);
-g_free(mode);
-}
+out:
+error_propagate(errp, local_err);
+g_free(mode);
 }
 
 void qmp_guest_suspend_ram(Error **errp)
@@ -1357,13 +1362,18 @@ void qmp_guest_suspend_ram(Error **errp)
 
 *mode = GUEST_SUSPEND_MODE_RAM;
 check_suspend_mode(*mode, _err);
+if (local_err) {
+goto out;
+}
 acquire_privilege(SE_SHUTDOWN_NAME, _err);
+if (local_err) {
+goto out;
+}
 execute_async(do_suspend, mode, _err);
 
-if (local_err) {
-error_propagate(errp, local_err);
-g_free(mode);
-}
+out:
+error_propagate(errp, local_err);
+g_free(mode);
 }
 
 void qmp_guest_suspend_hybrid(Error **errp)
-- 
2.21.1

[PATCH-for-5.1 v2 34/54] hw/microblaze/xlnx-zynqmp-pmu: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/microblaze/xlnx-zynqmp-pmu.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c
index 2aa602cf85..4ecbea7fdc 100644
--- a/hw/microblaze/xlnx-zynqmp-pmu.c
+++ b/hw/microblaze/xlnx-zynqmp-pmu.c
@@ -123,7 +123,11 @@ static void xlnx_zynqmp_pmu_soc_realize(DeviceState *dev, 
Error **errp)
 /* Connect the IPI device */
 for (int i = 0; i < XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
 object_property_set_bool(OBJECT(>ipi[i]), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>ipi[i]), 0, ipi_addr[i]);
 sysbus_connect_irq(SYS_BUS_DEVICE(>ipi[i]), 0,
qdev_get_gpio_in(DEVICE(>intc), ipi_irq[i]));
-- 
2.21.1

[PATCH-for-5.1 v2 51/54] migration/colo: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Running the coccinelle script produced:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
--keep-comments --smpl-spacing --dir .
  HANDLING: ./migration/colo.c
  [[manual check required: error_propagate() might be missing in 
migrate_set_block_enabled() ./migration/colo.c:439:4]]

Add the missing error_propagate() after review.

Signed-off-by: Philippe Mathieu-Daudé 
---
 migration/colo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/colo.c b/migration/colo.c
index a54ac84f41..57b2adb0cc 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -437,6 +437,9 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
 
 /* Disable block migration */
 migrate_set_block_enabled(false, _err);
+if (local_err) {
+goto out;
+}
 qemu_mutex_lock_iothread();
 
 #ifdef CONFIG_REPLICATION
-- 
2.21.1

Re: [PATCH-for-5.1 6/8] tests/Makefile: Add fetch-acceptance-assets rule

[Willian Rampazzo]

On Mon, Apr 6, 2020 at 12:41 PM Alex Bennée  wrote:
>
>
> Philippe Mathieu-Daudé  writes:
>
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  tests/Makefile.include | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 51de676298..90f457593c 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -906,6 +906,13 @@ get-vm-image-fedora-31-%: check-venv
> >  # download all vm images, according to defined targets
> >  get-vm-images: check-venv $(patsubst %,get-vm-image-fedora-31-%, 
> > $(FEDORA_31_DOWNLOAD))
> >
> > +fetch-acceptance-assets: check-venv
> > + $(call quiet-command, \
> > +$(TESTS_VENV_DIR)/bin/python -m avocado \
> > +--show=$(if $(DEBUG),avocado.test,$(AVOCADO_SHOW)) assets 
> > fetch \
> > +tests/acceptance/*py, \
> > +"AVOCADO", "tests/acceptance")
> > +
>
> I'm wondering if we could expand this to a rule per-test group and
> therefor allow parallel fetching of groups of assets?

This is a valid idea! Additionally, my suggestion is to organize the
tests by subdirectories under `tests/acceptance/`. Doing so makes it
easy to handle the target rule, adding the subdirectory to the assets
fetch command instead of listing specific tests for each group. Making
a list of tests for each group requires extra editing to the makefile
target when a new test is added. This can lead to new tests being
missed in their groups by mistake.

>
> >  check-acceptance: check-venv $(TESTS_RESULTS_DIR) get-vm-images
> >   $(call quiet-command, \
> >  $(TESTS_VENV_DIR)/bin/python -m avocado \
>
>
> --
> Alex Bennée
>

[PATCH-for-5.1 v2 32/54] hw/arm/armv7m: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/armv7m.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 7531b97ccd..249a7605f6 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -168,7 +168,11 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 }
 
 object_property_set_link(OBJECT(s->cpu), OBJECT(>container), "memory",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 if (object_property_find(OBJECT(s->cpu), "idau", NULL)) {
 object_property_set_link(OBJECT(s->cpu), s->idau, "idau", );
 if (err != NULL) {
@@ -256,7 +260,11 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
 return;
 }
 object_property_set_link(obj, OBJECT(s->board_memory),
- "source-memory", _abort);
+ "source-memory", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(obj, true, "realized", );
 if (err != NULL) {
 error_propagate(errp, err);
-- 
2.21.1

[PATCH-for-5.1 v2 44/54] hw/misc/macio/macio: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

Reviewed-by: David Gibson 
Acked-by: David Gibson 
Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/macio/macio.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 79222192e8..fffb64a7d5 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -348,6 +348,10 @@ static void macio_newworld_realize(PCIDevice *d, Error 
**errp)
 memory_region_add_subregion(>bar, 0x50,
 sysbus_mmio_get_region(sysbus_dev, 0));
 object_property_set_bool(OBJECT(>gpio), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 
 /* PMU */
 object_initialize_child(OBJECT(s), "pmu", >pmu, sizeof(s->pmu),
-- 
2.21.1

Re: [PATCH-for-5.1 v2 01/54] various: Remove suspicious '\' character outside of #define in C code

[Marc-André Lureau]

Hi

On Mon, Apr 6, 2020 at 7:48 PM Philippe Mathieu-Daudé  wrote:
>
> Fixes the following coccinelle warnings:
>
>   $ spatch --sp-file --verbose-parsing  ... \
>   scripts/coccinelle/remove_local_err.cocci
>   ...
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/ppc/translate_init.inc.c:5213
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/ppc/translate_init.inc.c:5261
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:166
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:167
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:169
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:170
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:171
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:172
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/microblaze/cpu.c:173
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5787
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5789
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5800
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5801
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5802
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5804
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5805
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:5806
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./target/i386/cpu.c:6329
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./hw/sd/sdhci.c:1133
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./hw/scsi/scsi-disk.c:3081
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./hw/net/virtio-net.c:1529
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./hw/riscv/sifive_u.c:468
>   SUSPICIOUS: a \ character appears outside of a #define at ./dump/dump.c:1895
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/vhdx.c:2209
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/vhdx.c:2215
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/vhdx.c:2221
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/vhdx.c:
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/replication.c:172
>   SUSPICIOUS: a \ character appears outside of a #define at 
> ./block/replication.c:173
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/replication.c |  4 ++--
>  block/vhdx.c|  8 
>  dump/dump.c |  2 +-
>  hw/net/virtio-net.c |  2 +-
>  hw/riscv/sifive_u.c |  2 +-
>  hw/scsi/scsi-disk.c |  2 +-
>  hw/sd/sdhci.c   |  2 +-
>  target/i386/cpu.c   | 18 +-
>  target/microblaze/cpu.c | 14 +++---
>  target/ppc/translate_init.inc.c |  4 ++--
>  10 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/block/replication.c b/block/replication.c
> index 413d95407d..5e09951c6b 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -169,8 +169,8 @@ static void replication_child_perm(BlockDriverState *bs, 
> BdrvChild *c,
>  if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
>  *nperm |= BLK_PERM_WRITE;
>  }
> -*nshared = BLK_PERM_CONSISTENT_READ \
> -   | BLK_PERM_WRITE \
> +*nshared = BLK_PERM_CONSISTENT_READ
> +   | BLK_PERM_WRITE
> | BLK_PERM_WRITE_UNCHANGED;
>  return;
>  }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 33e57cd656..e16fdc2f2d 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2206,20 +2206,20 @@ static QemuOptsList vhdx_create_opts = {
> .name = VHDX_BLOCK_OPT_BLOCK_SIZE,
> .type = QEMU_OPT_SIZE,
> .def_value_str = stringify(0),
> -   .help = "Block Size; min 1MB, max 256MB. " \
> +   .help = "Block Size; min 1MB, max 256MB. "
> "0 means auto-calculate based on image size."
> },
> {
> .name = BLOCK_OPT_SUBFMT,
> .type = QEMU_OPT_STRING,
> -   .help = "VHDX format type, can be either 'dynamic' or 'fixed'. "\
> +   .help = "VHDX format type, can be either 'dynamic' or 'fixed'. "
> "Default is 'dynamic'."
> },
> {
> .name = VHDX_BLOCK_OPT_ZERO,
> .type = QEMU_OPT_BOOL,
> -   .help = "Force use of 

Re: [PATCH for-5.0? v2] qcow2: Explicit mention of padding bytes

[Vladimir Sementsov-Ogievskiy]

06.04.2020 16:32, Eric Blake wrote:

On 4/6/20 3:50 AM, Vladimir Sementsov-Ogievskiy wrote:

03.04.2020 21:19, Eric Blake wrote:

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
relied on the rest of the text for implicitly covering 7 padding
bytes.  For consistency with other parts of the header (such as the
header extension format listing padding from n - m, or the snapshot
table entry mentioning variable padding), we might as well call out
the remaining 7 bytes as padding until such time (as any) as they gain
another meaning.

Signed-off-by: Eric Blake 
---

v2: Call out explicit byte range rather than '105 - m' [Max]

Safe for 5.0 as it is just a doc fix, but only if we actually want it.

  docs/interop/qcow2.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 640e0eca4000..80728bc2008d 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -210,3 +210,4 @@ version 2.
  Available compression type values:
  0: zlib 

+    105 - 111:  Padding, leave as zero.



Looking on this in separate, I'd make a software which will zero this padding 
unconditionally. However, if it's an existing image which we just open, we 
should keep the content we read.. On the other hand, of course, if read the 
whole spec, everything is clear.


Maybe:
   105 - 111: Padding, contents defined below


Works for me, no objections)



rather than an explicit mention of setting to 0?




--
Best regards,
Vladimir

[PATCH-for-5.1 v2 31/54] hw/riscv/sifive: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/riscv/sifive_e.c |  6 +-
 hw/riscv/sifive_u.c | 24 
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 0be8b52147..6d4e141ff7 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -156,7 +156,11 @@ static void riscv_sifive_e_soc_realize(DeviceState *dev, 
Error **errp)
 MemoryRegion *sys_mem = get_system_memory();
 
 object_property_set_bool(OBJECT(>cpus), true, "realized",
-_abort);
+);
+if (err) {
+error_propagate(errp, err);
+return;
+}
 
 /* MMIO */
 s->plic = sifive_plic_create(memmap[SIFIVE_E_PLIC].base,
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e13ab34de4..b07526aba1 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -508,9 +508,17 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
 NICInfo *nd = _table[0];
 
 object_property_set_bool(OBJECT(>e_cpus), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>u_cpus), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 /*
  * The cluster must be realized after the RISC-V hart array container,
  * as the container's CPU object is only created on realize, and the
@@ -518,9 +526,17 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
  * cluster is realized.
  */
 object_property_set_bool(OBJECT(>e_cluster), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>u_cluster), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 
 /* create PLIC hart topology configuration string */
 plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) *
-- 
2.21.1

[PATCH-for-5.1 v2 47/54] hw/sd/milkymist-memcard: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/sd/milkymist-memcard.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 926e1af475..87294c1b71 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -280,6 +280,10 @@ static void milkymist_memcard_realize(DeviceState *dev, 
Error **errp)
 blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
 carddev = qdev_create(BUS(>sdbus), TYPE_SD_CARD);
 qdev_prop_set_drive(carddev, "drive", blk, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(carddev), true, "realized", );
 if (err) {
 error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
-- 
2.21.1

[PATCH-for-5.1 v2 30/54] hw/arm/msf2-soc: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/msf2-soc.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index e448b0ab74..7619e71cfa 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -93,7 +93,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error 
**errp)
 MemoryRegion *system_memory = get_system_memory();
 
 memory_region_init_rom(>nvm, OBJECT(dev_soc), "MSF2.eNVM", s->envm_size,
-   _fatal);
+   );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 /*
  * On power-on, the eNVM region 0x6000 is automatically
  * remapped to the Cortex-M3 processor executable region
@@ -107,7 +111,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error 
**errp)
 memory_region_add_subregion(system_memory, 0, >nvm_alias);
 
 memory_region_init_ram(>sram, NULL, "MSF2.eSRAM", s->esram_size,
-   _fatal);
+   );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, >sram);
 
 armv7m = DEVICE(>armv7m);
@@ -115,7 +123,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error 
**errp)
 qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
 qdev_prop_set_bit(armv7m, "enable-bitband", true);
 object_property_set_link(OBJECT(>armv7m), OBJECT(get_system_memory()),
- "memory", _abort);
+ "memory", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>armv7m), true, "realized", );
 if (err != NULL) {
 error_propagate(errp, err);
@@ -184,8 +196,12 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error 
**errp)
 bus_name = g_strdup_printf("spi%d", i);
 object_property_add_alias(OBJECT(s), bus_name,
   OBJECT(>spi[i]), "spi",
-  _abort);
+  );
 g_free(bus_name);
+if (err) {
+error_propagate(errp, err);
+return;
+}
 }
 
 /* Below devices are not modelled yet. */
-- 
2.21.1

[PATCH-for-5.1 v2 26/54] hw/arm/fsl-imx: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/fsl-imx25.c |  6 +-
 hw/arm/fsl-imx6.c  | 18 +++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 6f1a82ce3d..3d87fe867e 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -295,7 +295,11 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 };
 
 object_property_set_bool(OBJECT(>usb[i]), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>usb[i]), 0, usb_table[i].addr);
 sysbus_connect_irq(SYS_BUS_DEVICE(>usb[i]), 0,
qdev_get_gpio_in(DEVICE(>avic),
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 6bf8aa0404..b3cef5bb57 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -359,7 +359,11 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 /* USB */
 for (i = 0; i < FSL_IMX6_NUM_USB_PHYS; i++) {
 object_property_set_bool(OBJECT(>usbphy[i]), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>usbphy[i]), 0,
 FSL_IMX6_USBPHY1_ADDR + i * 0x1000);
 }
@@ -372,7 +376,11 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 };
 
 object_property_set_bool(OBJECT(>usb[i]), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>usb[i]), 0,
 FSL_IMX6_USBOH3_USB_ADDR + i * 0x200);
 sysbus_connect_irq(SYS_BUS_DEVICE(>usb[i]), 0,
@@ -430,7 +438,11 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 };
 
 object_property_set_bool(OBJECT(>wdt[i]), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 
 sysbus_mmio_map(SYS_BUS_DEVICE(>wdt[i]), 0, FSL_IMX6_WDOGn_ADDR[i]);
 }
-- 
2.21.1

[PATCH-for-5.1 v2 37/54] scripts/coccinelle: Add script to catch missing error_propagate() calls

[Philippe Mathieu-Daudé]

In some places in we put an error into a local Error*, but forget
to check for failure and pass it back to the caller.
Add a Coccinelle patch to catch automatically add the missing code.

Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .../add-missing-error_propagate.cocci | 30 +++
 MAINTAINERS   |  1 +
 2 files changed, 31 insertions(+)
 create mode 100644 scripts/coccinelle/add-missing-error_propagate.cocci

diff --git a/scripts/coccinelle/add-missing-error_propagate.cocci 
b/scripts/coccinelle/add-missing-error_propagate.cocci
new file mode 100644
index 00..7991c9e2c2
--- /dev/null
+++ b/scripts/coccinelle/add-missing-error_propagate.cocci
@@ -0,0 +1,30 @@
+// Add missing error-propagation code where caller provide a Error* argument
+//
+// Copyright: (C) 2020 Philippe Mathieu-Daudé
+// This work is licensed under the terms of the GNU GPLv2 or later.
+//
+// spatch \
+//  --macro-file scripts/cocci-macro-file.h --include-headers \
+//  --sp-file scripts/coccinelle/add-missing-error_propagate.cocci \
+//  --keep-comments --in-place
+//
+// Inspired by 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691638.html
+
+
+@ add_missing_error_propagate @
+typedef Error;
+Error *local_err;
+identifier func, errp, errfunc1, errfunc2;
+@@
+func(..., Error **errp)
+{
+<...
+errfunc1(..., _err);
++   if (local_err) {
++   error_propagate(errp, local_err);
++   return;
++   }
+... when != local_err
+errfunc2(..., _err);
+...>
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b58f02efb..14de2a31dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2053,6 +2053,7 @@ F: include/qemu/error-report.h
 F: qapi/error.json
 F: util/error.c
 F: util/qemu-error.c
+F: scripts/coccinelle/add-missing-error_propagate.cocci
 F: scripts/coccinelle/err-bad-newline.cocci
 F: scripts/coccinelle/error-use-after-free.cocci
 F: scripts/coccinelle/error_propagate_null.cocci
-- 
2.21.1

[PATCH-for-5.1 v2 53/54] hw/mips/mips_malta: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Running the coccinelle script produced:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/find-missing-error_propagate.cocci \
--keep-comments --smpl-spacing --dir .
  HANDLING: ./hw/mips/mips_malta.c
  [[manual check required: error_propagate() might be missing in 
object_property_set_int() ./hw/mips/mips_malta.c:1193:4]]
  [[manual check required: error_propagate() might be missing in 
object_property_set_str() ./hw/mips/mips_malta.c:1192:4]]

Add the missing error_propagate() after review by adding
a Error* parameter to create_cps().

Reviewed-by: Peter Maydell 
Reviewed-by: Aleksandar Markovic 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/mips_malta.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index e4c4de1b4e..8d43cfd41b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1183,18 +1183,27 @@ static void create_cpu_without_cps(MachineState *ms,
 }
 
 static void create_cps(MachineState *ms, MaltaState *s,
-   qemu_irq *cbus_irq, qemu_irq *i8259_irq)
+   qemu_irq *cbus_irq, qemu_irq *i8259_irq,
+   Error **errp)
 {
 Error *err = NULL;
 
 sysbus_init_child_obj(OBJECT(s), "cps", OBJECT(>cps), sizeof(s->cps),
   TYPE_MIPS_CPS);
 object_property_set_str(OBJECT(>cps), ms->cpu_type, "cpu-type", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_int(OBJECT(>cps), ms->smp.cpus, "num-vp", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>cps), true, "realized", );
-if (err != NULL) {
-error_report("%s", error_get_pretty(err));
-exit(1);
+if (err) {
+error_propagate(errp, err);
+return;
 }
 
 sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>cps), 0, 0, 1);
@@ -1207,7 +1216,7 @@ static void mips_create_cpu(MachineState *ms, MaltaState 
*s,
 qemu_irq *cbus_irq, qemu_irq *i8259_irq)
 {
 if ((ms->smp.cpus > 1) && cpu_supports_cps_smp(ms->cpu_type)) {
-create_cps(ms, s, cbus_irq, i8259_irq);
+create_cps(ms, s, cbus_irq, i8259_irq, _fatal);
 } else {
 create_cpu_without_cps(ms, cbus_irq, i8259_irq);
 }
-- 
2.21.1

[PATCH-for-5.1 v2 38/54] hw/arm/bcm2835_peripherals: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/bcm2835_peripherals.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index edcaa4916d..a111e91069 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -289,8 +289,16 @@ static void bcm2835_peripherals_realize(DeviceState *dev, 
Error **errp)
  *   SD3.0_Host_AHB_eMMC4.4_Usersguide_ver5.9_jan11_10.pdf
  */
 object_property_set_uint(OBJECT(>sdhci), 3, "sd-spec-version", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_uint(OBJECT(>sdhci), BCM2835_SDHC_CAPAREG, 
"capareg",
  );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>sdhci), true, "pending-insert-quirk",
  );
 if (err) {
-- 
2.21.1

[PATCH-for-5.1 v2 25/54] scripts/coccinelle: Catch missing error_propagate() calls in realize()

[Philippe Mathieu-Daudé]

In some DeviceClass::realize() while we can propagate errors
to the caller, we forgot to do so. Add a Coccinelle patch to
automatically add the missing code.

Inspired-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 .../use-error_propagate-in-realize.cocci  | 54 +++
 MAINTAINERS   |  1 +
 2 files changed, 55 insertions(+)
 create mode 100644 scripts/coccinelle/use-error_propagate-in-realize.cocci

diff --git a/scripts/coccinelle/use-error_propagate-in-realize.cocci 
b/scripts/coccinelle/use-error_propagate-in-realize.cocci
new file mode 100644
index 00..7b59277a50
--- /dev/null
+++ b/scripts/coccinelle/use-error_propagate-in-realize.cocci
@@ -0,0 +1,54 @@
+// Add missing error-propagation code in DeviceClass::realize()
+//
+// Copyright: (C) 2020 Philippe Mathieu-Daudé
+// This work is licensed under the terms of the GNU GPLv2 or later.
+//
+// spatch \
+//  --macro-file scripts/cocci-macro-file.h --include-headers \
+//  --sp-file scripts/coccinelle/use-error_abort-in-instance_init.cocci \
+//  --keep-comments --timeout 60 --in-place
+//
+// Inspired by 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691638.html
+
+
+@ match_class_init @
+TypeInfo info;
+identifier class_initfn;
+@@
+info.class_init = class_initfn;
+
+
+@ match_realize @
+identifier match_class_init.class_initfn;
+DeviceClass *dc;
+identifier realizefn;
+@@
+void class_initfn(...)
+{
+...
+dc->realize = realizefn;
+...
+}
+
+
+@ propagate_in_realize @
+identifier match_realize.realizefn;
+identifier err;
+identifier errp;
+identifier func_with_errp;
+symbol error_abort, error_fatal;
+@@
+void realizefn(..., Error **errp)
+{
+...
+Error *err = NULL;
+<+...
+func_with_errp(...,
+-  \(_abort\|_fatal\));
++  );
++   if (err) {
++   error_propagate(errp, err);
++   return;
++   }
+...+>
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index d06ffeddd5..7b58f02efb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2059,6 +2059,7 @@ F: scripts/coccinelle/error_propagate_null.cocci
 F: scripts/coccinelle/remove_local_err.cocci
 F: scripts/coccinelle/simplify-init-realize-error_propagate.cocci
 F: scripts/coccinelle/use-error_fatal.cocci
+F: scripts/coccinelle/use-error_propagate-in-realize.cocci
 
 GDB stub
 M: Alex Bennée 
-- 
2.21.1

[PATCH-for-5.1 v2 33/54] hw/intc/arm_gicv3_its_kvm: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/intc/arm_gicv3_its_kvm.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index ad0ebabc87..3d2c4e22f5 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -101,7 +101,11 @@ static void kvm_arm_its_realize(DeviceState *dev, Error 
**errp)
 
 /* explicit init of the ITS */
 kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
-  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, _abort);
+  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
 
 /* register the base address */
 kvm_arm_register_device(>iomem_its_cntrl, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
-- 
2.21.1

[PATCH-for-5.1 v2 45/54] hw/net/xilinx_axienet: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

Then review showed this file has a 'xilinx_enet_realize_fail'
label that calls error_propagate(). Updated the patch to use
the label.

Reviewed-by: Alistair Francis 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: New cocci patch generated both transformations
(Peter noticed v1 only catched one)
---
 hw/net/xilinx_axienet.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 704788811a..db2c675b16 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -958,6 +958,9 @@ static void xilinx_enet_realize(DeviceState *dev, Error 
**errp)
  object_property_allow_set_link,
  OBJ_PROP_LINK_STRONG,
  _err);
+if (local_err) {
+goto xilinx_enet_realize_fail;
+}
 object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
  (Object **) >enet,
  object_property_allow_set_link,
@@ -967,6 +970,9 @@ static void xilinx_enet_realize(DeviceState *dev, Error 
**errp)
 goto xilinx_enet_realize_fail;
 }
 object_property_set_link(OBJECT(ds), OBJECT(s), "enet", _err);
+if (local_err) {
+goto xilinx_enet_realize_fail;
+}
 object_property_set_link(OBJECT(cs), OBJECT(s), "enet", _err);
 if (local_err) {
 goto xilinx_enet_realize_fail;
-- 
2.21.1

[PATCH-for-5.1 v2 39/54] hw/arm/fsl-imx: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/fsl-imx25.c | 8 
 hw/arm/fsl-imx6.c  | 8 
 2 files changed, 16 insertions(+)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 3d87fe867e..69d8645dcd 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -271,8 +271,16 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
**errp)
 
 object_property_set_uint(OBJECT(>esdhc[i]), 2, "sd-spec-version",
  );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_uint(OBJECT(>esdhc[i]), 
IMX25_ESDHC_CAPABILITIES,
  "capareg", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>esdhc[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index b3cef5bb57..c254294a70 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -343,8 +343,16 @@ static void fsl_imx6_realize(DeviceState *dev, Error 
**errp)
 /* UHS-I SDIO3.0 SDR104 1.8V ADMA */
 object_property_set_uint(OBJECT(>esdhc[i]), 3, "sd-spec-version",
  );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_uint(OBJECT(>esdhc[i]), IMX6_ESDHC_CAPABILITIES,
  "capareg", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>esdhc[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
-- 
2.21.1

[PATCH-for-5.1 v2 21/54] hw/riscv/sifive_u: Move some code from realize() to init()

[Philippe Mathieu-Daudé]

Coccinelle reported:

  $ spatch ... --timeout 60 --sp-file \
scripts/coccinelle/simplify-init-realize-error_propagate.cocci
  HANDLING: ./hw/riscv/sifive_u.c
  >>> possible moves from riscv_sifive_u_soc_init() to 
riscv_sifive_u_soc_realize() in ./hw/riscv/sifive_u.c:473

Move the calls using _abort which don't depend of input
updated before realize() to init().

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/riscv/sifive_u.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9c90c94c33..754af19eef 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -441,6 +441,8 @@ static void riscv_sifive_u_soc_init(Object *obj)
 qdev_prop_set_uint32(DEVICE(>otp), "serial", OTP_SERIAL);
 sysbus_init_child_obj(obj, "gem", >gem, sizeof(s->gem),
   TYPE_CADENCE_GEM);
+object_property_set_int(OBJECT(>gem), GEM_REVISION, "revision",
+_abort);
 }
 
 static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
@@ -569,8 +571,6 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
 qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
 qdev_set_nic_properties(DEVICE(>gem), nd);
 }
-object_property_set_int(OBJECT(>gem), GEM_REVISION, "revision",
-_abort);
 object_property_set_bool(OBJECT(>gem), true, "realized", );
 if (err) {
 error_propagate(errp, err);
-- 
2.21.1

[PATCH-for-5.1 v2 46/54] hw/riscv/sifive_u: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/object_property_missing_error_propagate.cocci \
--keep-comments --smpl-spacing --dir hw

Reviewed-by: Alistair Francis 
Reviewed-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/riscv/sifive_u.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index b07526aba1..b6c27bc970 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -574,9 +574,17 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
 SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE, false);
 
 object_property_set_bool(OBJECT(>prci), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>prci), 0, memmap[SIFIVE_U_PRCI].base);
 
 object_property_set_bool(OBJECT(>otp), true, "realized", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>otp), 0, memmap[SIFIVE_U_OTP].base);
 
 for (i = 0; i < SIFIVE_U_PLIC_NUM_SOURCES; i++) {
-- 
2.21.1

[PATCH-for-5.1 v2 28/54] hw/arm/aspeed: Add missing error-propagation code

[Philippe Mathieu-Daudé]

Patch created mechanically by running:

  $ spatch \
--macro-file scripts/cocci-macro-file.h --include-headers \
--sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
--keep-comments --smpl-spacing --in-place --dir hw

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/aspeed_ast2600.c | 36 ++--
 hw/arm/aspeed_soc.c | 12 ++--
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index c8e0171824..d810df928c 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -277,7 +277,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 if (s->num_cpus > 1) {
 object_property_set_int(OBJECT(>cpu[i]),
 ASPEED_A7MPCORE_ADDR,
-"reset-cbar", _abort);
+"reset-cbar", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 }
 /*
  * TODO: the secondary CPUs are started and a boot helper
@@ -293,10 +297,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 
 /* A7MPCORE */
 object_property_set_int(OBJECT(>a7mpcore), s->num_cpus, "num-cpu",
-_abort);
+);
+if (err) {
+error_propagate(errp, err);
+return;
+}
 
 object_property_set_bool(OBJECT(>a7mpcore), true, "realized",
- _abort);
+ );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 sysbus_mmio_map(SYS_BUS_DEVICE(>a7mpcore), 0, ASPEED_A7MPCORE_ADDR);
 
 for (i = 0; i < s->num_cpus; i++) {
@@ -343,7 +355,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 
 /* Timer */
 object_property_set_link(OBJECT(>timerctrl),
- OBJECT(>scu), "scu", _abort);
+ OBJECT(>scu), "scu", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>timerctrl), true, "realized", );
 if (err) {
 error_propagate(errp, err);
@@ -459,7 +475,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
 AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(>wdt[i]);
 
 object_property_set_link(OBJECT(>wdt[i]),
- OBJECT(>scu), "scu", _abort);
+ OBJECT(>scu), "scu", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>wdt[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
@@ -490,7 +510,11 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, 
Error **errp)
aspeed_soc_get_irq(s, ASPEED_ETH1 + i));
 
 object_property_set_link(OBJECT(>mii[i]), OBJECT(>ftgmac100[i]),
- "nic", _abort);
+ "nic", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>mii[i]), true, "realized",
  );
 if (err) {
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index aa6d739ad0..5f90215187 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -301,7 +301,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 
 /* Timer */
 object_property_set_link(OBJECT(>timerctrl),
- OBJECT(>scu), "scu", _abort);
+ OBJECT(>scu), "scu", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>timerctrl), true, "realized", );
 if (err) {
 error_propagate(errp, err);
@@ -398,7 +402,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
 AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(>wdt[i]);
 
 object_property_set_link(OBJECT(>wdt[i]),
- OBJECT(>scu), "scu", _abort);
+ OBJECT(>scu), "scu", );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 object_property_set_bool(OBJECT(>wdt[i]), true, "realized", );
 if (err) {
 error_propagate(errp, err);
-- 
2.21.1