* [PULL 01/35] vhost-vdpa: Do not send empty IOTLB update batches
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
@ 2021-09-04 21:35 ` Michael S. Tsirkin
2021-09-04 21:35 ` [PULL 02/35] hw/virtio: Fix leak of host-notifier memory-region Michael S. Tsirkin
` (34 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Eugenio Pérez
From: Eugenio Pérez <eperezma@redhat.com>
With the introduction of the batch hinting, meaningless batches can be
created with no IOTLB updates if the memory region was skipped by
vhost_vdpa_listener_skipped_section. This is the case of host notifiers
memory regions, device un/realize, and others. This causes the vdpa
device to receive dma mapping settings with no changes, a possibly
expensive operation for nothing.
To avoid that, VHOST_IOTLB_BATCH_BEGIN hint is delayed until we have a
meaningful (not skipped section) mapping or unmapping operation, and
VHOST_IOTLB_BATCH_END is not written unless at least one of _UPDATE /
_INVALIDATE has been issued.
v3:
* Use a bool instead of a counter avoiding potential number wrapping
* Fix bad check on _commit
* Move VHOST_BACKEND_F_IOTLB_BATCH check to
vhost_vdpa_iotlb_batch_begin_once
v2 (from RFC):
* Rename misleading name
* Abstract start batching function for listener_add/del
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Message-Id: <20210812140933.226288-1-eperezma@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/vhost-vdpa.h | 1 +
hw/virtio/vhost-vdpa.c | 35 ++++++++++++++++++++++------------
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 9188226d8b..a8963da2d9 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -22,6 +22,7 @@ typedef struct VhostVDPAHostNotifier {
typedef struct vhost_vdpa {
int device_fd;
uint32_t msg_type;
+ bool iotlb_batch_begin_sent;
MemoryListener listener;
struct vhost_dev *dev;
VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 4fa414feea..ca1227e5dc 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -89,19 +89,13 @@ static int vhost_vdpa_dma_unmap(struct vhost_vdpa *v, hwaddr iova,
return ret;
}
-static void vhost_vdpa_listener_begin(MemoryListener *listener)
+static void vhost_vdpa_listener_begin_batch(struct vhost_vdpa *v)
{
- struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
- struct vhost_dev *dev = v->dev;
- struct vhost_msg_v2 msg = {};
int fd = v->device_fd;
-
- if (!(dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH))) {
- return;
- }
-
- msg.type = v->msg_type;
- msg.iotlb.type = VHOST_IOTLB_BATCH_BEGIN;
+ struct vhost_msg_v2 msg = {
+ .type = v->msg_type,
+ .iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
+ };
if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
error_report("failed to write, fd=%d, errno=%d (%s)",
@@ -109,6 +103,16 @@ static void vhost_vdpa_listener_begin(MemoryListener *listener)
}
}
+static void vhost_vdpa_iotlb_batch_begin_once(struct vhost_vdpa *v)
+{
+ if (v->dev->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH) &&
+ !v->iotlb_batch_begin_sent) {
+ vhost_vdpa_listener_begin_batch(v);
+ }
+
+ v->iotlb_batch_begin_sent = true;
+}
+
static void vhost_vdpa_listener_commit(MemoryListener *listener)
{
struct vhost_vdpa *v = container_of(listener, struct vhost_vdpa, listener);
@@ -120,6 +124,10 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener)
return;
}
+ if (!v->iotlb_batch_begin_sent) {
+ return;
+ }
+
msg.type = v->msg_type;
msg.iotlb.type = VHOST_IOTLB_BATCH_END;
@@ -127,6 +135,8 @@ static void vhost_vdpa_listener_commit(MemoryListener *listener)
error_report("failed to write, fd=%d, errno=%d (%s)",
fd, errno, strerror(errno));
}
+
+ v->iotlb_batch_begin_sent = false;
}
static void vhost_vdpa_listener_region_add(MemoryListener *listener,
@@ -170,6 +180,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener *listener,
llsize = int128_sub(llend, int128_make64(iova));
+ vhost_vdpa_iotlb_batch_begin_once(v);
ret = vhost_vdpa_dma_map(v, iova, int128_get64(llsize),
vaddr, section->readonly);
if (ret) {
@@ -221,6 +232,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
llsize = int128_sub(llend, int128_make64(iova));
+ vhost_vdpa_iotlb_batch_begin_once(v);
ret = vhost_vdpa_dma_unmap(v, iova, int128_get64(llsize));
if (ret) {
error_report("vhost_vdpa dma unmap error!");
@@ -234,7 +246,6 @@ static void vhost_vdpa_listener_region_del(MemoryListener *listener,
* depends on the addnop().
*/
static const MemoryListener vhost_vdpa_memory_listener = {
- .begin = vhost_vdpa_listener_begin,
.commit = vhost_vdpa_listener_commit,
.region_add = vhost_vdpa_listener_region_add,
.region_del = vhost_vdpa_listener_region_del,
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 02/35] hw/virtio: Fix leak of host-notifier memory-region
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
2021-09-04 21:35 ` [PULL 01/35] vhost-vdpa: Do not send empty IOTLB update batches Michael S. Tsirkin
@ 2021-09-04 21:35 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 03/35] vhost: make SET_VRING_ADDR, SET_FEATURES send replies Michael S. Tsirkin
` (33 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Yajun Wu
From: Yajun Wu <yajunw@nvidia.com>
If call virtio_queue_set_host_notifier_mr fails, should free
host-notifier memory-region.
Fixes: 44866521bd ("vhost-user: support registering external host notifiers")
Signed-off-by: Yajun Wu <yajunw@nvidia.com>
Message-Id: <1629077555-19907-1-git-send-email-yajunw@nvidia.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost-user.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2407836fac..33002300c2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1474,6 +1474,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
g_free(name);
if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
+ object_unparent(OBJECT(&n->mr));
munmap(addr, page_size);
return -1;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 03/35] vhost: make SET_VRING_ADDR, SET_FEATURES send replies
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
2021-09-04 21:35 ` [PULL 01/35] vhost-vdpa: Do not send empty IOTLB update batches Michael S. Tsirkin
2021-09-04 21:35 ` [PULL 02/35] hw/virtio: Fix leak of host-notifier memory-region Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 04/35] hw/acpi: define PIIX4 acpi pci hotplug property strings at a single place Michael S. Tsirkin
` (32 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Denis Plotnikov, Peter Maydell
From: Denis Plotnikov <den-plotnikov@yandex-team.ru>
On vhost-user-blk migration, qemu normally sends a number of commands
to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
data logging.
The issue is that qemu doesn't wait for reply from the vhost daemon
for these commands which may result in races between qemu expectation
of logging starting and actual login starting in vhost daemon.
The race can appear as follows: on migration setup, qemu enables dirty page
logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
vhost-user-blk daemon immediately and the daemon needs some time to turn the
logging on internally. If qemu doesn't wait for reply, after sending the
command, qemu may start migrateing memory pages to a destination. At this time,
the logging may not be actually turned on in the daemon but some guest pages,
which the daemon is about to write to, may have already been transferred
without logging to the destination. Since the logging wasn't turned on,
those pages won't be transferred again as dirty. So we may end up with
corrupted data on the destination.
The same scenario is applicable for "used ring" data logging, which is
turned on with VHOST_USER_SET_VRING_ADDR command.
To resolve this issue, this patch makes qemu wait for the command result
explicitly if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging enabled.
Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
Message-Id: <20210809104824.78830-1-den-plotnikov@yandex-team.ru>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost-user.c | 145 ++++++++++++++++++++++++++++-------------
1 file changed, 101 insertions(+), 44 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 33002300c2..a4eb6cde7e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
return 0;
}
-static int vhost_user_set_vring_addr(struct vhost_dev *dev,
- struct vhost_vring_addr *addr)
-{
- VhostUserMsg msg = {
- .hdr.request = VHOST_USER_SET_VRING_ADDR,
- .hdr.flags = VHOST_USER_VERSION,
- .payload.addr = *addr,
- .hdr.size = sizeof(msg.payload.addr),
- };
-
- if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
- return -1;
- }
-
- return 0;
-}
-
static int vhost_user_set_vring_endian(struct vhost_dev *dev,
struct vhost_vring_state *ring)
{
@@ -1288,33 +1271,6 @@ static int vhost_user_set_vring_call(struct vhost_dev *dev,
return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
}
-static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
-{
- VhostUserMsg msg = {
- .hdr.request = request,
- .hdr.flags = VHOST_USER_VERSION,
- .payload.u64 = u64,
- .hdr.size = sizeof(msg.payload.u64),
- };
-
- if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
- return -1;
- }
-
- return 0;
-}
-
-static int vhost_user_set_features(struct vhost_dev *dev,
- uint64_t features)
-{
- return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
-}
-
-static int vhost_user_set_protocol_features(struct vhost_dev *dev,
- uint64_t features)
-{
- return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
-}
static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
{
@@ -1360,6 +1316,107 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
return 0;
}
+static int enforce_reply(struct vhost_dev *dev,
+ const VhostUserMsg *msg)
+{
+ uint64_t dummy;
+
+ if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
+ return process_message_reply(dev, msg);
+ }
+
+ /*
+ * We need to wait for a reply but the backend does not
+ * support replies for the command we just sent.
+ * Send VHOST_USER_GET_FEATURES which makes all backends
+ * send a reply.
+ */
+ return vhost_user_get_features(dev, &dummy);
+}
+
+static int vhost_user_set_vring_addr(struct vhost_dev *dev,
+ struct vhost_vring_addr *addr)
+{
+ VhostUserMsg msg = {
+ .hdr.request = VHOST_USER_SET_VRING_ADDR,
+ .hdr.flags = VHOST_USER_VERSION,
+ .payload.addr = *addr,
+ .hdr.size = sizeof(msg.payload.addr),
+ };
+
+ bool reply_supported = virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+ /*
+ * wait for a reply if logging is enabled to make sure
+ * backend is actually logging changes
+ */
+ bool wait_for_reply = addr->flags & (1 << VHOST_VRING_F_LOG);
+
+ if (reply_supported && wait_for_reply) {
+ msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+ }
+
+ if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+ return -1;
+ }
+
+ if (wait_for_reply) {
+ return enforce_reply(dev, &msg);
+ }
+
+ return 0;
+}
+
+static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64,
+ bool wait_for_reply)
+{
+ VhostUserMsg msg = {
+ .hdr.request = request,
+ .hdr.flags = VHOST_USER_VERSION,
+ .payload.u64 = u64,
+ .hdr.size = sizeof(msg.payload.u64),
+ };
+
+ if (wait_for_reply) {
+ bool reply_supported = virtio_has_feature(dev->protocol_features,
+ VHOST_USER_PROTOCOL_F_REPLY_ACK);
+ if (reply_supported) {
+ msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+ }
+ }
+
+ if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+ return -1;
+ }
+
+ if (wait_for_reply) {
+ return enforce_reply(dev, &msg);
+ }
+
+ return 0;
+}
+
+static int vhost_user_set_features(struct vhost_dev *dev,
+ uint64_t features)
+{
+ /*
+ * wait for a reply if logging is enabled to make sure
+ * backend is actually logging changes
+ */
+ bool log_enabled = features & (0x1ULL << VHOST_F_LOG_ALL);
+
+ return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
+ log_enabled);
+}
+
+static int vhost_user_set_protocol_features(struct vhost_dev *dev,
+ uint64_t features)
+{
+ return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features,
+ false);
+}
+
static int vhost_user_set_owner(struct vhost_dev *dev)
{
VhostUserMsg msg = {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 04/35] hw/acpi: define PIIX4 acpi pci hotplug property strings at a single place
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (2 preceding siblings ...)
2021-09-04 21:36 ` [PULL 03/35] vhost: make SET_VRING_ADDR, SET_FEATURES send replies Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 05/35] q35: catch invalid cpu hotplug configuration Michael S. Tsirkin
` (31 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Eduardo Habkost, Richard Henderson,
Philippe Mathieu-Daudé,
Paolo Bonzini, Ani Sinha, Igor Mammedov,
Philippe Mathieu-Daudé,
Aurelien Jarno
From: Ani Sinha <ani@anisinha.ca>
Now that we have "acpi-pci-hotplug-with-bridge-support" PIIX4 PM property being
used for both q35 and i440fx machine types, it is better that we defined this
property string at a single place within a header file like other PIIX4
properties. We can then use this single definition at all the places that needs
it instead of duplicating the string everywhere. While at it, this change also
adds a definition for "acpi-root-pci-hotplug" PIIX4 PM property and uses
this definition at all places that were formally using the string value.
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Message-Id: <20210816083214.105740-1-ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/acpi.h | 2 ++
hw/acpi/ich9.c | 2 +-
hw/acpi/piix4.c | 4 ++--
hw/i386/acpi-build.c | 4 ++--
hw/i386/pc.c | 4 ++--
hw/i386/pc_q35.c | 2 +-
6 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 9e8a76f2e2..cc0d370745 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -47,6 +47,8 @@
#define ACPI_PM_PROP_PM_IO_BASE "pm_io_base"
#define ACPI_PM_PROP_GPE0_BLK "gpe0_blk"
#define ACPI_PM_PROP_GPE0_BLK_LEN "gpe0_blk_len"
+#define ACPI_PM_PROP_ACPI_PCIHP_BRIDGE "acpi-pci-hotplug-with-bridge-support"
+#define ACPI_PM_PROP_ACPI_PCI_ROOTHP "acpi-root-pci-hotplug"
/* PM Timer ticks per second (HZ) */
#define PM_TIMER_FREQUENCY 3579545
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 778e27b659..1ee2ba2c50 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -451,7 +451,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
ich9_pm_get_enable_tco,
ich9_pm_set_enable_tco);
- object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+ object_property_add_bool(obj, ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
ich9_pm_get_acpi_pci_hotplug,
ich9_pm_set_acpi_pci_hotplug);
}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 48f7a1edbc..f0b5fac44a 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -647,9 +647,9 @@ static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
- DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
+ DEFINE_PROP_BOOL(ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, PIIX4PMState,
use_acpi_hotplug_bridge, true),
- DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
+ DEFINE_PROP_BOOL(ACPI_PM_PROP_ACPI_PCI_ROOTHP, PIIX4PMState,
use_acpi_root_pci_hotplug, true),
DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
acpi_memory_hotplug.is_enabled, true),
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a33ac8b91e..6c27e12e2a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -267,10 +267,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
qobject_unref(o);
pm->pcihp_bridge_en =
- object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+ object_property_get_bool(obj, ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
NULL);
pm->pcihp_root_en =
- object_property_get_bool(obj, "acpi-root-pci-hotplug",
+ object_property_get_bool(obj, ACPI_PM_PROP_ACPI_PCI_ROOTHP,
NULL);
}
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1276bfeee4..22aa598d50 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -101,7 +101,7 @@ GlobalProperty pc_compat_6_0[] = {
{ "qemu64" "-" TYPE_X86_CPU, "model", "6" },
{ "qemu64" "-" TYPE_X86_CPU, "stepping", "3" },
{ TYPE_X86_CPU, "x-vendor-cpuid-only", "off" },
- { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
+ { "ICH9-LPC", ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, "off" },
};
const size_t pc_compat_6_0_len = G_N_ELEMENTS(pc_compat_6_0);
@@ -313,7 +313,7 @@ const size_t pc_compat_2_0_len = G_N_ELEMENTS(pc_compat_2_0);
GlobalProperty pc_compat_1_7[] = {
PC_CPU_MODEL_IDS("1.7.0")
{ TYPE_USB_DEVICE, "msos-desc", "no" },
- { "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" },
+ { "PIIX4_PM", ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, "off" },
{ "hpet", HPET_INTCAP, "4" },
};
const size_t pc_compat_1_7_len = G_N_ELEMENTS(pc_compat_1_7);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 565fadce54..46cd542d17 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -238,7 +238,7 @@ static void pc_q35_init(MachineState *machine)
OBJECT(lpc), &error_abort);
acpi_pcihp = object_property_get_bool(OBJECT(lpc),
- "acpi-pci-hotplug-with-bridge-support",
+ ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
NULL);
if (acpi_pcihp) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 05/35] q35: catch invalid cpu hotplug configuration
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (3 preceding siblings ...)
2021-09-04 21:36 ` [PULL 04/35] hw/acpi: define PIIX4 acpi pci hotplug property strings at a single place Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need Michael S. Tsirkin
` (30 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Gerd Hoffmann, Igor Mammedov
From: Gerd Hoffmann <kraxel@redhat.com>
Related: https://bugzilla.redhat.com//show_bug.cgi?id=1985924
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-Id: <20210812102341.3316254-1-kraxel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/isa/lpc_ich9.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 5f9de0239c..5f143dca17 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -31,6 +31,7 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include "cpu.h"
+#include "qapi/error.h"
#include "qapi/visitor.h"
#include "qemu/range.h"
#include "hw/isa/isa.h"
@@ -676,6 +677,18 @@ static void ich9_lpc_realize(PCIDevice *d, Error **errp)
DeviceState *dev = DEVICE(d);
ISABus *isa_bus;
+ if ((lpc->smi_host_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT)) &&
+ !(lpc->smi_host_features & BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT))) {
+ /*
+ * smi_features_ok_callback() throws an error on this.
+ *
+ * So bail out here instead of advertizing the invalid
+ * configuration and get obscure firmware failures from that.
+ */
+ error_setg(errp, "cpu hot-unplug requires cpu hot-plug");
+ return;
+ }
+
isa_bus = isa_bus_new(DEVICE(d), get_system_memory(), get_system_io(),
errp);
if (!isa_bus) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (4 preceding siblings ...)
2021-09-04 21:36 ` [PULL 05/35] q35: catch invalid cpu hotplug configuration Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-06 9:58 ` Philippe Mathieu-Daudé
2022-07-19 16:12 ` Peter Maydell
2021-09-04 21:36 ` [PULL 07/35] hw/virtio: move vhost_set_backend_type() to vhost.c Michael S. Tsirkin
` (29 subsequent siblings)
35 siblings, 2 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Aleksandar Rikalo, Philippe Mathieu-Daudé,
Ani Sinha, Igor Mammedov, Aurelien Jarno
From: Ani Sinha <ani@anisinha.ca>
Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
This brings in support for whole lot of subsystems that some targets like
mips does not need. They are added just to satisfy symbol dependencies. This
is ugly and should be avoided. Targets should be able to pull in just what they
need and no more. For example, mips only needs support for PIIX4 and does not
need acpi pci hotplug support or cpu hotplug support or memory hotplug support
etc. This change is an effort to clean this up.
In this change, new config variables are added for various acpi hotplug
subsystems. Targets like mips can only enable PIIX4 support and not the rest
of all the other modules which were being previously pulled in as a part of
CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
are not required by mips (for example, symbols specific to pci hotplug etc)
are available to satisfy the dependencies.
Currently, this change only addresses issues with mips malta targets. In future
we might be able to clean up other targets which are similarly pulling in lot
of unnecessary hotplug modules by enabling ACPI_X86.
This change should also address issues such as the following:
https://gitlab.com/qemu-project/qemu/-/issues/221
https://gitlab.com/qemu-project/qemu/-/issues/193
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
configs/devices/mips-softmmu/common.mak | 5 +--
hw/acpi/acpi-cpu-hotplug-stub.c | 50 +++++++++++++++++++++++++
hw/acpi/acpi-mem-hotplug-stub.c | 35 +++++++++++++++++
hw/acpi/acpi-nvdimm-stub.c | 8 ++++
hw/acpi/acpi-pci-hotplug-stub.c | 47 +++++++++++++++++++++++
hw/acpi/Kconfig | 10 +++++
hw/acpi/meson.build | 14 +++++--
7 files changed, 161 insertions(+), 8 deletions(-)
create mode 100644 hw/acpi/acpi-cpu-hotplug-stub.c
create mode 100644 hw/acpi/acpi-mem-hotplug-stub.c
create mode 100644 hw/acpi/acpi-nvdimm-stub.c
create mode 100644 hw/acpi/acpi-pci-hotplug-stub.c
diff --git a/configs/devices/mips-softmmu/common.mak b/configs/devices/mips-softmmu/common.mak
index ea78fe7275..752b62b1e6 100644
--- a/configs/devices/mips-softmmu/common.mak
+++ b/configs/devices/mips-softmmu/common.mak
@@ -18,10 +18,7 @@ CONFIG_PCSPK=y
CONFIG_PCKBD=y
CONFIG_FDC=y
CONFIG_ACPI=y
-CONFIG_ACPI_X86=y
-CONFIG_ACPI_MEMORY_HOTPLUG=y
-CONFIG_ACPI_NVDIMM=y
-CONFIG_ACPI_CPU_HOTPLUG=y
+CONFIG_ACPI_PIIX4=y
CONFIG_APM=y
CONFIG_I8257=y
CONFIG_PIIX4=y
diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
new file mode 100644
index 0000000000..3fc4b14c26
--- /dev/null
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -0,0 +1,50 @@
+#include "qemu/osdep.h"
+#include "hw/acpi/cpu_hotplug.h"
+#include "migration/vmstate.h"
+
+
+/* Following stubs are all related to ACPI cpu hotplug */
+const VMStateDescription vmstate_cpu_hotplug;
+
+void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
+ CPUHotplugState *cpuhp_state,
+ uint16_t io_port)
+{
+ return;
+}
+
+void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
+ AcpiCpuHotplug *gpe_cpu, uint16_t base)
+{
+ return;
+}
+
+void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
+{
+ return;
+}
+
+void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
+ CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
+ AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
+ CPUHotplugState *cpu_st,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
new file mode 100644
index 0000000000..73a076a265
--- /dev/null
+++ b/hw/acpi/acpi-mem-hotplug-stub.c
@@ -0,0 +1,35 @@
+#include "qemu/osdep.h"
+#include "hw/acpi/memory_hotplug.h"
+#include "migration/vmstate.h"
+
+const VMStateDescription vmstate_memory_hotplug;
+
+void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
+ MemHotplugState *state, hwaddr io_base)
+{
+ return;
+}
+
+void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
+{
+ return;
+}
+
+void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_memory_unplug_cb(MemHotplugState *mem_st,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
+ MemHotplugState *mem_st,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
diff --git a/hw/acpi/acpi-nvdimm-stub.c b/hw/acpi/acpi-nvdimm-stub.c
new file mode 100644
index 0000000000..8baff9be6f
--- /dev/null
+++ b/hw/acpi/acpi-nvdimm-stub.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "hw/mem/nvdimm.h"
+#include "hw/hotplug.h"
+
+void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+ return;
+}
diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
new file mode 100644
index 0000000000..734e4c5986
--- /dev/null
+++ b/hw/acpi/acpi-pci-hotplug-stub.c
@@ -0,0 +1,47 @@
+#include "qemu/osdep.h"
+#include "hw/acpi/pcihp.h"
+#include "migration/vmstate.h"
+
+const VMStateDescription vmstate_acpi_pcihp_pci_status;
+
+void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
+ MemoryRegion *address_space_io, bool bridges_enabled,
+ uint16_t io_base)
+{
+ return;
+}
+
+void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
+ DeviceState *dev, Error **errp)
+{
+ return;
+}
+
+void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+ AcpiPciHpState *s, DeviceState *dev,
+ Error **errp)
+{
+ return;
+}
+
+void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
+{
+ return;
+}
+
+bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
+{
+ return false;
+}
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index cfc4ede8d9..3b5e118c54 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -8,6 +8,8 @@ config ACPI_X86
select ACPI_CPU_HOTPLUG
select ACPI_MEMORY_HOTPLUG
select ACPI_HMAT
+ select ACPI_PIIX4
+ select ACPI_PCIHP
config ACPI_X86_ICH
bool
@@ -24,6 +26,14 @@ config ACPI_NVDIMM
bool
depends on ACPI
+config ACPI_PIIX4
+ bool
+ depends on ACPI
+
+config ACPI_PCIHP
+ bool
+ depends on ACPI
+
config ACPI_HMAT
bool
depends on ACPI
diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
index 29f804d13e..7d8c0eb43e 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -6,16 +6,20 @@ acpi_ss.add(files(
'core.c',
'utils.c',
))
-acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c'))
-acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu_hotplug.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c', 'cpu_hotplug.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false: files('acpi-cpu-hotplug-stub.c'))
acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_true: files('memory_hotplug.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_false: files('acpi-mem-hotplug-stub.c'))
acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_true: files('nvdimm.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_false: files('acpi-nvdimm-stub.c'))
acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
-acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c'))
acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
@@ -23,4 +27,6 @@ acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
- 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
+ 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c',
+ 'acpi-mem-hotplug-stub.c', 'acpi-cpu-hotplug-stub.c',
+ 'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c'))
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-04 21:36 ` [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need Michael S. Tsirkin
@ 2021-09-06 9:58 ` Philippe Mathieu-Daudé
2021-09-06 10:03 ` Ani Sinha
2022-07-19 16:12 ` Peter Maydell
1 sibling, 1 reply; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-06 9:58 UTC (permalink / raw)
To: Michael S. Tsirkin, qemu-devel
Cc: Ani Sinha, Peter Maydell, Aleksandar Rikalo, Aurelien Jarno,
Igor Mammedov
Hi Ani,
On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
> From: Ani Sinha <ani@anisinha.ca>
>
> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> This brings in support for whole lot of subsystems that some targets like
> mips does not need. They are added just to satisfy symbol dependencies. This
> is ugly and should be avoided. Targets should be able to pull in just what they
> need and no more. For example, mips only needs support for PIIX4 and does not
> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> etc. This change is an effort to clean this up.
> In this change, new config variables are added for various acpi hotplug
> subsystems. Targets like mips can only enable PIIX4 support and not the rest
> of all the other modules which were being previously pulled in as a part of
> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> are not required by mips (for example, symbols specific to pci hotplug etc)
> are available to satisfy the dependencies.
>
> Currently, this change only addresses issues with mips malta targets. In future
> we might be able to clean up other targets which are similarly pulling in lot
> of unnecessary hotplug modules by enabling ACPI_X86.
>
> This change should also address issues such as the following:
> https://gitlab.com/qemu-project/qemu/-/issues/221
> https://gitlab.com/qemu-project/qemu/-/issues/193
FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
this should have been:
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
Can we close these issues manually?
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> configs/devices/mips-softmmu/common.mak | 5 +--
> hw/acpi/acpi-cpu-hotplug-stub.c | 50 +++++++++++++++++++++++++
> hw/acpi/acpi-mem-hotplug-stub.c | 35 +++++++++++++++++
> hw/acpi/acpi-nvdimm-stub.c | 8 ++++
> hw/acpi/acpi-pci-hotplug-stub.c | 47 +++++++++++++++++++++++
> hw/acpi/Kconfig | 10 +++++
> hw/acpi/meson.build | 14 +++++--
> 7 files changed, 161 insertions(+), 8 deletions(-)
> create mode 100644 hw/acpi/acpi-cpu-hotplug-stub.c
> create mode 100644 hw/acpi/acpi-mem-hotplug-stub.c
> create mode 100644 hw/acpi/acpi-nvdimm-stub.c
> create mode 100644 hw/acpi/acpi-pci-hotplug-stub.c
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-06 9:58 ` Philippe Mathieu-Daudé
@ 2021-09-06 10:03 ` Ani Sinha
2021-09-06 10:24 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 53+ messages in thread
From: Ani Sinha @ 2021-09-06 10:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Ani Sinha, Igor Mammedov, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]
On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
> Hi Ani,
>
> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> > hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> > This brings in support for whole lot of subsystems that some targets like
> > mips does not need. They are added just to satisfy symbol dependencies. This
> > is ugly and should be avoided. Targets should be able to pull in just what they
> > need and no more. For example, mips only needs support for PIIX4 and does not
> > need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> > etc. This change is an effort to clean this up.
> > In this change, new config variables are added for various acpi hotplug
> > subsystems. Targets like mips can only enable PIIX4 support and not the rest
> > of all the other modules which were being previously pulled in as a part of
> > CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> > are not required by mips (for example, symbols specific to pci hotplug etc)
> > are available to satisfy the dependencies.
> >
> > Currently, this change only addresses issues with mips malta targets. In future
> > we might be able to clean up other targets which are similarly pulling in lot
> > of unnecessary hotplug modules by enabling ACPI_X86.
> >
> > This change should also address issues such as the following:
> > https://gitlab.com/qemu-project/qemu/-/issues/221
> > https://gitlab.com/qemu-project/qemu/-/issues/193
>
> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
> this should have been:
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
>
Ah my apologies. Will do this next time.
> Can we close these issues manually?
Since both you and I have verified that those issues gets fixed with my
change, yes we can close them. I do not have a gitlab account. Should I
have one? Is there special permissions needed to handle these tickets?
>
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > configs/devices/mips-softmmu/common.mak | 5 +--
> > hw/acpi/acpi-cpu-hotplug-stub.c | 50 +++++++++++++++++++++++++
> > hw/acpi/acpi-mem-hotplug-stub.c | 35 +++++++++++++++++
> > hw/acpi/acpi-nvdimm-stub.c | 8 ++++
> > hw/acpi/acpi-pci-hotplug-stub.c | 47 +++++++++++++++++++++++
> > hw/acpi/Kconfig | 10 +++++
> > hw/acpi/meson.build | 14 +++++--
> > 7 files changed, 161 insertions(+), 8 deletions(-)
> > create mode 100644 hw/acpi/acpi-cpu-hotplug-stub.c
> > create mode 100644 hw/acpi/acpi-mem-hotplug-stub.c
> > create mode 100644 hw/acpi/acpi-nvdimm-stub.c
> > create mode 100644 hw/acpi/acpi-pci-hotplug-stub.c
>
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-06 10:03 ` Ani Sinha
@ 2021-09-06 10:24 ` Philippe Mathieu-Daudé
2021-09-06 10:49 ` Ani Sinha
0 siblings, 1 reply; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-06 10:24 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Igor Mammedov, Aurelien Jarno
On 9/6/21 12:03 PM, Ani Sinha wrote:
> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
>> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
>>> From: Ani Sinha <ani@anisinha.ca>
>>>
>>> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
>>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
>>> This brings in support for whole lot of subsystems that some targets like
>>> mips does not need. They are added just to satisfy symbol dependencies. This
>>> is ugly and should be avoided. Targets should be able to pull in just what they
>>> need and no more. For example, mips only needs support for PIIX4 and does not
>>> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
>>> etc. This change is an effort to clean this up.
>>> In this change, new config variables are added for various acpi hotplug
>>> subsystems. Targets like mips can only enable PIIX4 support and not the rest
>>> of all the other modules which were being previously pulled in as a part of
>>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
>>> are not required by mips (for example, symbols specific to pci hotplug etc)
>>> are available to satisfy the dependencies.
>>>
>>> Currently, this change only addresses issues with mips malta targets. In future
>>> we might be able to clean up other targets which are similarly pulling in lot
>>> of unnecessary hotplug modules by enabling ACPI_X86.
>>>
>>> This change should also address issues such as the following:
>>> https://gitlab.com/qemu-project/qemu/-/issues/221
>>> https://gitlab.com/qemu-project/qemu/-/issues/193
>>
>> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
>> this should have been:
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
>>
>
> Ah my apologies. Will do this next time.
>
>> Can we close these issues manually?
>
> Since both you and I have verified that those issues gets fixed with my
> change, yes we can close them. I do not have a gitlab account. Should I
> have one? Is there special permissions needed to handle these tickets?
Since you are listed in the MAINTAINERS file, long-term you'll
eventually use it anyway (i.e. to run the CI pipelines before sending
patches, to subscribe to the 'ACPI' label to get notifications or
comment ACPI-related issues).
The process is quite straight-forward, once having an account you
simply request to be member of the project via the WebUI then you
can help triaging the issues (and closing these two).
Regards,
Phil.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-06 10:24 ` Philippe Mathieu-Daudé
@ 2021-09-06 10:49 ` Ani Sinha
2021-09-07 5:55 ` Ani Sinha
0 siblings, 1 reply; 53+ messages in thread
From: Ani Sinha @ 2021-09-06 10:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin,
QEMU Developers, Igor Mammedov, Aurelien Jarno
On Mon, Sep 6, 2021 at 3:54 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 9/6/21 12:03 PM, Ani Sinha wrote:
> > On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
> >> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
> >>> From: Ani Sinha <ani@anisinha.ca>
> >>>
> >>> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> >>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> >>> This brings in support for whole lot of subsystems that some targets like
> >>> mips does not need. They are added just to satisfy symbol dependencies. This
> >>> is ugly and should be avoided. Targets should be able to pull in just what they
> >>> need and no more. For example, mips only needs support for PIIX4 and does not
> >>> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> >>> etc. This change is an effort to clean this up.
> >>> In this change, new config variables are added for various acpi hotplug
> >>> subsystems. Targets like mips can only enable PIIX4 support and not the rest
> >>> of all the other modules which were being previously pulled in as a part of
> >>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> >>> are not required by mips (for example, symbols specific to pci hotplug etc)
> >>> are available to satisfy the dependencies.
> >>>
> >>> Currently, this change only addresses issues with mips malta targets. In future
> >>> we might be able to clean up other targets which are similarly pulling in lot
> >>> of unnecessary hotplug modules by enabling ACPI_X86.
> >>>
> >>> This change should also address issues such as the following:
> >>> https://gitlab.com/qemu-project/qemu/-/issues/221
> >>> https://gitlab.com/qemu-project/qemu/-/issues/193
> >>
> >> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
> >> this should have been:
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
> >>
> >
> > Ah my apologies. Will do this next time.
> >
> >> Can we close these issues manually?
> >
> > Since both you and I have verified that those issues gets fixed with my
> > change, yes we can close them. I do not have a gitlab account. Should I
> > have one? Is there special permissions needed to handle these tickets?
>
> Since you are listed in the MAINTAINERS file, long-term you'll
> eventually use it anyway (i.e. to run the CI pipelines before sending
> patches, to subscribe to the 'ACPI' label to get notifications or
> comment ACPI-related issues).
>
> The process is quite straight-forward, once having an account you
> simply request to be member of the project via the WebUI then you
> can help triaging the issues (and closing these two).
Hmm. I created an account and added a comment to the tickets. However
I am unable to close them. I requested access to the project.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-06 10:49 ` Ani Sinha
@ 2021-09-07 5:55 ` Ani Sinha
2021-09-07 6:13 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 53+ messages in thread
From: Ani Sinha @ 2021-09-07 5:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin,
QEMU Developers, Igor Mammedov, Aurelien Jarno
On Mon, Sep 6, 2021 at 4:19 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Mon, Sep 6, 2021 at 3:54 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> > On 9/6/21 12:03 PM, Ani Sinha wrote:
> > > On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
> > >> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
> > >>> From: Ani Sinha <ani@anisinha.ca>
> > >>>
> > >>> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> > >>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> > >>> This brings in support for whole lot of subsystems that some targets like
> > >>> mips does not need. They are added just to satisfy symbol dependencies. This
> > >>> is ugly and should be avoided. Targets should be able to pull in just what they
> > >>> need and no more. For example, mips only needs support for PIIX4 and does not
> > >>> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> > >>> etc. This change is an effort to clean this up.
> > >>> In this change, new config variables are added for various acpi hotplug
> > >>> subsystems. Targets like mips can only enable PIIX4 support and not the rest
> > >>> of all the other modules which were being previously pulled in as a part of
> > >>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> > >>> are not required by mips (for example, symbols specific to pci hotplug etc)
> > >>> are available to satisfy the dependencies.
> > >>>
> > >>> Currently, this change only addresses issues with mips malta targets. In future
> > >>> we might be able to clean up other targets which are similarly pulling in lot
> > >>> of unnecessary hotplug modules by enabling ACPI_X86.
> > >>>
> > >>> This change should also address issues such as the following:
> > >>> https://gitlab.com/qemu-project/qemu/-/issues/221
> > >>> https://gitlab.com/qemu-project/qemu/-/issues/193
> > >>
> > >> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
> > >> this should have been:
> > >>
> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
> > >>
> > >
> > > Ah my apologies. Will do this next time.
> > >
> > >> Can we close these issues manually?
> > >
> > > Since both you and I have verified that those issues gets fixed with my
> > > change, yes we can close them. I do not have a gitlab account. Should I
> > > have one? Is there special permissions needed to handle these tickets?
> >
> > Since you are listed in the MAINTAINERS file, long-term you'll
> > eventually use it anyway (i.e. to run the CI pipelines before sending
> > patches, to subscribe to the 'ACPI' label to get notifications or
> > comment ACPI-related issues).
> >
> > The process is quite straight-forward, once having an account you
> > simply request to be member of the project via the WebUI then you
> > can help triaging the issues (and closing these two).
>
> Hmm. I created an account and added a comment to the tickets. However
> I am unable to close them. I requested access to the project.
I could be wrong, but I think only reporters can open and close bugs
like yourself on gitlab.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-07 5:55 ` Ani Sinha
@ 2021-09-07 6:13 ` Philippe Mathieu-Daudé
2021-09-07 6:34 ` Ani Sinha
0 siblings, 1 reply; 53+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07 6:13 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin,
QEMU Developers, Igor Mammedov, Aurelien Jarno
On 9/7/21 7:55 AM, Ani Sinha wrote:
> On Mon, Sep 6, 2021 at 4:19 PM Ani Sinha <ani@anisinha.ca> wrote:
>>
>> On Mon, Sep 6, 2021 at 3:54 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> On 9/6/21 12:03 PM, Ani Sinha wrote:
>>>> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
>>>>> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
>>>>>> From: Ani Sinha <ani@anisinha.ca>
>>>>>>
>>>>>> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
>>>>>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
>>>>>> This brings in support for whole lot of subsystems that some targets like
>>>>>> mips does not need. They are added just to satisfy symbol dependencies. This
>>>>>> is ugly and should be avoided. Targets should be able to pull in just what they
>>>>>> need and no more. For example, mips only needs support for PIIX4 and does not
>>>>>> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
>>>>>> etc. This change is an effort to clean this up.
>>>>>> In this change, new config variables are added for various acpi hotplug
>>>>>> subsystems. Targets like mips can only enable PIIX4 support and not the rest
>>>>>> of all the other modules which were being previously pulled in as a part of
>>>>>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
>>>>>> are not required by mips (for example, symbols specific to pci hotplug etc)
>>>>>> are available to satisfy the dependencies.
>>>>>>
>>>>>> Currently, this change only addresses issues with mips malta targets. In future
>>>>>> we might be able to clean up other targets which are similarly pulling in lot
>>>>>> of unnecessary hotplug modules by enabling ACPI_X86.
>>>>>>
>>>>>> This change should also address issues such as the following:
>>>>>> https://gitlab.com/qemu-project/qemu/-/issues/221
>>>>>> https://gitlab.com/qemu-project/qemu/-/issues/193
>>>>>
>>>>> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
>>>>> this should have been:
>>>>>
>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
>>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
>>>>>
>>>>
>>>> Ah my apologies. Will do this next time.
>>>>
>>>>> Can we close these issues manually?
>>>>
>>>> Since both you and I have verified that those issues gets fixed with my
>>>> change, yes we can close them. I do not have a gitlab account. Should I
>>>> have one? Is there special permissions needed to handle these tickets?
>>>
>>> Since you are listed in the MAINTAINERS file, long-term you'll
>>> eventually use it anyway (i.e. to run the CI pipelines before sending
>>> patches, to subscribe to the 'ACPI' label to get notifications or
>>> comment ACPI-related issues).
>>>
>>> The process is quite straight-forward, once having an account you
>>> simply request to be member of the project via the WebUI then you
>>> can help triaging the issues (and closing these two).
>>
>> Hmm. I created an account and added a comment to the tickets. However
>> I am unable to close them. I requested access to the project.
>
> I could be wrong, but I think only reporters can open and close bugs
> like yourself on gitlab.
Hmm it is unclear who can close an issue, per:
https://docs.gitlab.com/ee/user/permissions.html#project-members-permissions
Let's wait until you get added to the project as a member: I assume
you are currently 'guest' and would become 'reporter'.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-07 6:13 ` Philippe Mathieu-Daudé
@ 2021-09-07 6:34 ` Ani Sinha
2021-09-07 9:49 ` Ani Sinha
0 siblings, 1 reply; 53+ messages in thread
From: Ani Sinha @ 2021-09-07 6:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin,
QEMU Developers, Igor Mammedov, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 3952 bytes --]
On Tue, Sep 7, 2021 at 11:44 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:
> On 9/7/21 7:55 AM, Ani Sinha wrote:
> > On Mon, Sep 6, 2021 at 4:19 PM Ani Sinha <ani@anisinha.ca> wrote:
> >>
> >> On Mon, Sep 6, 2021 at 3:54 PM Philippe Mathieu-Daudé <
> philmd@redhat.com> wrote:
> >>>
> >>> On 9/6/21 12:03 PM, Ani Sinha wrote:
> >>>> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
> >>>>> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
> >>>>>> From: Ani Sinha <ani@anisinha.ca>
> >>>>>>
> >>>>>> Currently various acpi hotplug modules like cpu hotplug, memory
> hotplug, pci
> >>>>>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is
> turned on.
> >>>>>> This brings in support for whole lot of subsystems that some
> targets like
> >>>>>> mips does not need. They are added just to satisfy symbol
> dependencies. This
> >>>>>> is ugly and should be avoided. Targets should be able to pull in
> just what they
> >>>>>> need and no more. For example, mips only needs support for PIIX4
> and does not
> >>>>>> need acpi pci hotplug support or cpu hotplug support or memory
> hotplug support
> >>>>>> etc. This change is an effort to clean this up.
> >>>>>> In this change, new config variables are added for various acpi
> hotplug
> >>>>>> subsystems. Targets like mips can only enable PIIX4 support and not
> the rest
> >>>>>> of all the other modules which were being previously pulled in as a
> part of
> >>>>>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4
> needs but
> >>>>>> are not required by mips (for example, symbols specific to pci
> hotplug etc)
> >>>>>> are available to satisfy the dependencies.
> >>>>>>
> >>>>>> Currently, this change only addresses issues with mips malta
> targets. In future
> >>>>>> we might be able to clean up other targets which are similarly
> pulling in lot
> >>>>>> of unnecessary hotplug modules by enabling ACPI_X86.
> >>>>>>
> >>>>>> This change should also address issues such as the following:
> >>>>>> https://gitlab.com/qemu-project/qemu/-/issues/221
> >>>>>> https://gitlab.com/qemu-project/qemu/-/issues/193
> >>>>>
> >>>>> FYI per
> https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
> >>>>> this should have been:
> >>>>>
> >>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
> >>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
> >>>>>
> >>>>
> >>>> Ah my apologies. Will do this next time.
> >>>>
> >>>>> Can we close these issues manually?
> >>>>
> >>>> Since both you and I have verified that those issues gets fixed with
> my
> >>>> change, yes we can close them. I do not have a gitlab account. Should
> I
> >>>> have one? Is there special permissions needed to handle these tickets?
> >>>
> >>> Since you are listed in the MAINTAINERS file, long-term you'll
> >>> eventually use it anyway (i.e. to run the CI pipelines before sending
> >>> patches, to subscribe to the 'ACPI' label to get notifications or
> >>> comment ACPI-related issues).
> >>>
> >>> The process is quite straight-forward, once having an account you
> >>> simply request to be member of the project via the WebUI then you
> >>> can help triaging the issues (and closing these two).
> >>
> >> Hmm. I created an account and added a comment to the tickets. However
> >> I am unable to close them. I requested access to the project.
> >
> > I could be wrong, but I think only reporters can open and close bugs
> > like yourself on gitlab.
>
> Hmm it is unclear who can close an issue, per:
>
> https://docs.gitlab.com/ee/user/permissions.html#project-members-permissions
>
> Let's wait until you get added to the project as a member: I assume
> you are currently 'guest' and would become 'reporter'.
Ok will ping people on IRC today. Btw the gitlab issue list is a goldmine
for people like me to work on my spare time :-)
>
>
[-- Attachment #2: Type: text/html, Size: 6249 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-07 6:34 ` Ani Sinha
@ 2021-09-07 9:49 ` Ani Sinha
0 siblings, 0 replies; 53+ messages in thread
From: Ani Sinha @ 2021-09-07 9:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Aleksandar Rikalo, Michael S. Tsirkin,
QEMU Developers, Igor Mammedov, Aurelien Jarno
On Tue, Sep 7, 2021 at 12:04 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Tue, Sep 7, 2021 at 11:44 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 9/7/21 7:55 AM, Ani Sinha wrote:
>> > On Mon, Sep 6, 2021 at 4:19 PM Ani Sinha <ani@anisinha.ca> wrote:
>> >>
>> >> On Mon, Sep 6, 2021 at 3:54 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> >>>
>> >>> On 9/6/21 12:03 PM, Ani Sinha wrote:
>> >>>> On Mon, 6 Sep 2021, Philippe Mathieu-Daudé wrote:
>> >>>>> On 9/4/21 11:36 PM, Michael S. Tsirkin wrote:
>> >>>>>> From: Ani Sinha <ani@anisinha.ca>
>> >>>>>>
>> >>>>>> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
>> >>>>>> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
>> >>>>>> This brings in support for whole lot of subsystems that some targets like
>> >>>>>> mips does not need. They are added just to satisfy symbol dependencies. This
>> >>>>>> is ugly and should be avoided. Targets should be able to pull in just what they
>> >>>>>> need and no more. For example, mips only needs support for PIIX4 and does not
>> >>>>>> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
>> >>>>>> etc. This change is an effort to clean this up.
>> >>>>>> In this change, new config variables are added for various acpi hotplug
>> >>>>>> subsystems. Targets like mips can only enable PIIX4 support and not the rest
>> >>>>>> of all the other modules which were being previously pulled in as a part of
>> >>>>>> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
>> >>>>>> are not required by mips (for example, symbols specific to pci hotplug etc)
>> >>>>>> are available to satisfy the dependencies.
>> >>>>>>
>> >>>>>> Currently, this change only addresses issues with mips malta targets. In future
>> >>>>>> we might be able to clean up other targets which are similarly pulling in lot
>> >>>>>> of unnecessary hotplug modules by enabling ACPI_X86.
>> >>>>>>
>> >>>>>> This change should also address issues such as the following:
>> >>>>>> https://gitlab.com/qemu-project/qemu/-/issues/221
>> >>>>>> https://gitlab.com/qemu-project/qemu/-/issues/193
>> >>>>>
>> >>>>> FYI per https://docs.gitlab.com/ee/administration/issue_closing_pattern.html
>> >>>>> this should have been:
>> >>>>>
>> >>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/193
>> >>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/221
>> >>>>>
>> >>>>
>> >>>> Ah my apologies. Will do this next time.
>> >>>>
>> >>>>> Can we close these issues manually?
>> >>>>
>> >>>> Since both you and I have verified that those issues gets fixed with my
>> >>>> change, yes we can close them. I do not have a gitlab account. Should I
>> >>>> have one? Is there special permissions needed to handle these tickets?
>> >>>
>> >>> Since you are listed in the MAINTAINERS file, long-term you'll
>> >>> eventually use it anyway (i.e. to run the CI pipelines before sending
>> >>> patches, to subscribe to the 'ACPI' label to get notifications or
>> >>> comment ACPI-related issues).
>> >>>
>> >>> The process is quite straight-forward, once having an account you
>> >>> simply request to be member of the project via the WebUI then you
>> >>> can help triaging the issues (and closing these two).
>> >>
>> >> Hmm. I created an account and added a comment to the tickets. However
>> >> I am unable to close them. I requested access to the project.
>> >
>> > I could be wrong, but I think only reporters can open and close bugs
>> > like yourself on gitlab.
>>
>> Hmm it is unclear who can close an issue, per:
>> https://docs.gitlab.com/ee/user/permissions.html#project-members-permissions
>>
>> Let's wait until you get added to the project as a member: I assume
>> you are currently 'guest' and would become 'reporter'.
>
>
> Ok will ping people on IRC today.
Bonzini helped. I have closed both tickets.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-09-04 21:36 ` [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need Michael S. Tsirkin
2021-09-06 9:58 ` Philippe Mathieu-Daudé
@ 2022-07-19 16:12 ` Peter Maydell
2022-07-19 16:21 ` Peter Maydell
2022-07-20 18:37 ` Ani Sinha
1 sibling, 2 replies; 53+ messages in thread
From: Peter Maydell @ 2022-07-19 16:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Ani Sinha <ani@anisinha.ca>
>
> Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> This brings in support for whole lot of subsystems that some targets like
> mips does not need. They are added just to satisfy symbol dependencies. This
> is ugly and should be avoided. Targets should be able to pull in just what they
> need and no more. For example, mips only needs support for PIIX4 and does not
> need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> etc. This change is an effort to clean this up.
> In this change, new config variables are added for various acpi hotplug
> subsystems. Targets like mips can only enable PIIX4 support and not the rest
> of all the other modules which were being previously pulled in as a part of
> CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> are not required by mips (for example, symbols specific to pci hotplug etc)
> are available to satisfy the dependencies.
>
> Currently, this change only addresses issues with mips malta targets. In future
> we might be able to clean up other targets which are similarly pulling in lot
> of unnecessary hotplug modules by enabling ACPI_X86.
>
> This change should also address issues such as the following:
> https://gitlab.com/qemu-project/qemu/-/issues/221
> https://gitlab.com/qemu-project/qemu/-/issues/193
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Hi; I'm trying to track down the fix for a bug that I think
was introduced by this change. Specifically, if you
configure with a target list of
'--target-list=mips-linux-user,mips64-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips-softmmu,mipsel-softmmu,mips64-softmmu,mips64el-softmmu'
(ie "just mips"), then run 'make check', the iotest 267 fails
because QEMU segfaults trying to do a VM save/restore on
qemu-system-mips. (You can run just that iotest by cd'ing
into the build dir's tests/qemu-iotests/ subdir and running
./check -qcow2 -gdb 267
if you like).
This is because hw/acpi/piix4.c (used by the MIPS malta board)
has a vmstate that includes use of the VMSTATE_PCI_HOTPLUG()
macro. This macro uses the vmstate_acpi_pcihp_pci_status
vmstate struct. If the MIPS target is built along with some
other targets which require CONFIG_ACPI_PCIHP then we correctly
get the real definition of that vmstate struct from pcihp.c.
However, if we are only building the MIPS targets then
CONFIG_ACPI_PCIHP is false, and we get the dummy definition
of the struct from acpi-pci-hotplug-stub.c. The dummy definition
obviously doesn't actually work for migrating anything, and
in fact the migration code ends up segfaulting because
the 'name' field in the struct is NULL. (MIPS builds and
uses hw/acpi/piix4.c because
configs/devices/mips-softmmu/common.mak sets CONFIG_ACPI_PIIX4=y,
and it needs this because piix4_init() unconditionally
creates a TYPE_PIIX4_PM device. It's possible this is a bug
revealed/introduced by the recent piix4 refactoring, but I
had a look at the code at the time this change was committed
and afaict back then it also created the PIIX4_PM device,
just in a different place. Indeed it is this commit which adds
the CONFIG_ACPI_PIIX4=y to the config!)
How is this intended to work? The obvious fix from my point
of view would just be to say "piix4.c requires pcihp.c"
and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
but that seems like it would be rather undoing the point
of this change. But if Malta requires ACPI_PIIX4 and it
creates the PIIX4_PM device, it needs the real pcihp.c and
not the stubs, doesn't it ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-19 16:12 ` Peter Maydell
@ 2022-07-19 16:21 ` Peter Maydell
2022-07-20 18:37 ` Ani Sinha
1 sibling, 0 replies; 53+ messages in thread
From: Peter Maydell @ 2022-07-19 16:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Tue, 19 Jul 2022 at 17:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> Hi; I'm trying to track down the fix for a bug that I think
> was introduced by this change. Specifically, if you
> configure with a target list of
> '--target-list=mips-linux-user,mips64-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips-softmmu,mipsel-softmmu,mips64-softmmu,mips64el-softmmu'
>
> (ie "just mips"), then run 'make check', the iotest 267 fails
> because QEMU segfaults trying to do a VM save/restore on
> qemu-system-mips. (You can run just that iotest by cd'ing
> into the build dir's tests/qemu-iotests/ subdir and running
> ./check -qcow2 -gdb 267
I mean just "./check -qcow2 267" here (the -gdb starts a
gdbserver, which is what I was doing while trying to debug.)
-- PMM
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-19 16:12 ` Peter Maydell
2022-07-19 16:21 ` Peter Maydell
@ 2022-07-20 18:37 ` Ani Sinha
2022-07-20 21:34 ` Peter Maydell
1 sibling, 1 reply; 53+ messages in thread
From: Ani Sinha @ 2022-07-20 18:37 UTC (permalink / raw)
To: Peter Maydell
Cc: Michael S. Tsirkin, qemu-devel, Ani Sinha,
Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Tue, 19 Jul 2022, Peter Maydell wrote:
> On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> > hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> > This brings in support for whole lot of subsystems that some targets like
> > mips does not need. They are added just to satisfy symbol dependencies. This
> > is ugly and should be avoided. Targets should be able to pull in just what they
> > need and no more. For example, mips only needs support for PIIX4 and does not
> > need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> > etc. This change is an effort to clean this up.
> > In this change, new config variables are added for various acpi hotplug
> > subsystems. Targets like mips can only enable PIIX4 support and not the rest
> > of all the other modules which were being previously pulled in as a part of
> > CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> > are not required by mips (for example, symbols specific to pci hotplug etc)
> > are available to satisfy the dependencies.
> >
> > Currently, this change only addresses issues with mips malta targets. In future
> > we might be able to clean up other targets which are similarly pulling in lot
> > of unnecessary hotplug modules by enabling ACPI_X86.
> >
> > This change should also address issues such as the following:
> > https://gitlab.com/qemu-project/qemu/-/issues/221
> > https://gitlab.com/qemu-project/qemu/-/issues/193
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Hi; I'm trying to track down the fix for a bug that I think
> was introduced by this change. Specifically, if you
> configure with a target list of
> '--target-list=mips-linux-user,mips64-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips-softmmu,mipsel-softmmu,mips64-softmmu,mips64el-softmmu'
>
> (ie "just mips"), then run 'make check', the iotest 267 fails
> because QEMU segfaults trying to do a VM save/restore on
> qemu-system-mips. (You can run just that iotest by cd'ing
> into the build dir's tests/qemu-iotests/ subdir and running
> ./check -qcow2 -gdb 267
> if you like).
>
> This is because hw/acpi/piix4.c (used by the MIPS malta board)
> has a vmstate that includes use of the VMSTATE_PCI_HOTPLUG()
> macro. This macro uses the vmstate_acpi_pcihp_pci_status
> vmstate struct. If the MIPS target is built along with some
> other targets which require CONFIG_ACPI_PCIHP then we correctly
> get the real definition of that vmstate struct from pcihp.c.
> However, if we are only building the MIPS targets then
> CONFIG_ACPI_PCIHP is false, and we get the dummy definition
> of the struct from acpi-pci-hotplug-stub.c. The dummy definition
> obviously doesn't actually work for migrating anything, and
> in fact the migration code ends up segfaulting because
> the 'name' field in the struct is NULL. (MIPS builds and
> uses hw/acpi/piix4.c because
> configs/devices/mips-softmmu/common.mak sets CONFIG_ACPI_PIIX4=y,
> and it needs this because piix4_init() unconditionally
> creates a TYPE_PIIX4_PM device. It's possible this is a bug
> revealed/introduced by the recent piix4 refactoring, but I
> had a look at the code at the time this change was committed
> and afaict back then it also created the PIIX4_PM device,
> just in a different place. Indeed it is this commit which adds
> the CONFIG_ACPI_PIIX4=y to the config!)
>
> How is this intended to work? The obvious fix from my point
> of view would just be to say "piix4.c requires pcihp.c"
> and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> but that seems like it would be rather undoing the point
> of this change.
Yes. From the commit log and the vague recollection I have about this
change :
> For example, mips only needs support for PIIX4 and does not
> need acpi pci hotplug support or cpu hotplug support or memory hotplug
support
> etc
So does malta really need acpi hotplug? If not, then the stubbing out of
the vmstate struct is correct.
But if Malta requires ACPI_PIIX4 and it
> creates the PIIX4_PM device, it needs the real pcihp.c and
> not the stubs, doesn't it ?
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-20 18:37 ` Ani Sinha
@ 2022-07-20 21:34 ` Peter Maydell
2022-07-20 22:13 ` Ani Sinha
0 siblings, 1 reply; 53+ messages in thread
From: Peter Maydell @ 2022-07-20 21:34 UTC (permalink / raw)
To: Ani Sinha
Cc: Michael S. Tsirkin, qemu-devel, Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Wed, 20 Jul 2022 at 19:37, Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Tue, 19 Jul 2022, Peter Maydell wrote:
>
> > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> > How is this intended to work? The obvious fix from my point
> > of view would just be to say "piix4.c requires pcihp.c"
> > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> > but that seems like it would be rather undoing the point
> > of this change.
>
> Yes. From the commit log and the vague recollection I have about this
> change :
>
> > For example, mips only needs support for PIIX4 and does not
> > need acpi pci hotplug support or cpu hotplug support or memory hotplug
> support
> > etc
>
> So does malta really need acpi hotplug? If not, then the stubbing out of
> the vmstate struct is correct.
It's not, because the vmstate struct is actually used when you
savevm/loadvm a malta machine. If the malta shouldn't have
acpi hotplug then we need to arrange for the hotplug code
to be avoided at an earlier point, not just stub in the
vmstate struct field.
-- PMM
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-20 21:34 ` Peter Maydell
@ 2022-07-20 22:13 ` Ani Sinha
2022-07-21 10:51 ` BB
2022-07-21 12:35 ` Dr. David Alan Gilbert
0 siblings, 2 replies; 53+ messages in thread
From: Ani Sinha @ 2022-07-20 22:13 UTC (permalink / raw)
To: Peter Maydell
Cc: Ani Sinha, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Wed, 20 Jul 2022, Peter Maydell wrote:
> On Wed, 20 Jul 2022 at 19:37, Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> >
> > On Tue, 19 Jul 2022, Peter Maydell wrote:
> >
> > > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > How is this intended to work? The obvious fix from my point
> > > of view would just be to say "piix4.c requires pcihp.c"
> > > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> > > but that seems like it would be rather undoing the point
> > > of this change.
> >
> > Yes. From the commit log and the vague recollection I have about this
> > change :
> >
> > > For example, mips only needs support for PIIX4 and does not
> > > need acpi pci hotplug support or cpu hotplug support or memory hotplug
> > support
> > > etc
> >
> > So does malta really need acpi hotplug? If not, then the stubbing out of
> > the vmstate struct is correct.
>
> It's not, because the vmstate struct is actually used when you
> savevm/loadvm a malta machine. If the malta shouldn't have
> acpi hotplug then we need to arrange for the hotplug code
> to be avoided at an earlier point, not just stub in the
> vmstate struct field.
yes I think that would be more appropriate fix, I agree. Since mst added
that vmstate, maybe he can comment on this.
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-20 22:13 ` Ani Sinha
@ 2022-07-21 10:51 ` BB
2022-07-21 12:35 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 53+ messages in thread
From: BB @ 2022-07-21 10:51 UTC (permalink / raw)
To: qemu-devel, Ani Sinha, Peter Maydell
Cc: Michael S. Tsirkin, Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On July 21, 2022 12:13:06 AM GMT+02:00, Ani Sinha <ani@anisinha.ca> wrote:
>
>
>On Wed, 20 Jul 2022, Peter Maydell wrote:
>
>> On Wed, 20 Jul 2022 at 19:37, Ani Sinha <ani@anisinha.ca> wrote:
>> >
>> >
>> >
>> > On Tue, 19 Jul 2022, Peter Maydell wrote:
>> >
>> > > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > How is this intended to work? The obvious fix from my point
>> > > of view would just be to say "piix4.c requires pcihp.c"
>> > > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
>> > > but that seems like it would be rather undoing the point
>> > > of this change.
>> >
>> > Yes. From the commit log and the vague recollection I have about this
>> > change :
>> >
>> > > For example, mips only needs support for PIIX4 and does not
>> > > need acpi pci hotplug support or cpu hotplug support or memory hotplug
>> > support
>> > > etc
>> >
>> > So does malta really need acpi hotplug? If not, then the stubbing out of
>> > the vmstate struct is correct.
>>
>> It's not, because the vmstate struct is actually used when you
>> savevm/loadvm a malta machine. If the malta shouldn't have
>> acpi hotplug then we need to arrange for the hotplug code
>> to be avoided at an earlier point, not just stub in the
>> vmstate struct field.
>
>yes I think that would be more appropriate fix, I agree. Since mst added
>that vmstate, maybe he can comment on this.
>
Despite its name, ACPI_PIIX4 is also used by PIIX3, which is required by the "pc" machines. These machines support migration etc. which may explain why ACPI_PIIX4 has the vmstate struct field. To me it just looks like an oversight that the Malta board doesn't support all ACPI_PIIX4 features.
Best regards,
Bernhard
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-20 22:13 ` Ani Sinha
2022-07-21 10:51 ` BB
@ 2022-07-21 12:35 ` Dr. David Alan Gilbert
2022-07-25 17:57 ` Ani Sinha
1 sibling, 1 reply; 53+ messages in thread
From: Dr. David Alan Gilbert @ 2022-07-21 12:35 UTC (permalink / raw)
To: Ani Sinha
Cc: Peter Maydell, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
* Ani Sinha (ani@anisinha.ca) wrote:
>
>
> On Wed, 20 Jul 2022, Peter Maydell wrote:
>
> > On Wed, 20 Jul 2022 at 19:37, Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >
> > >
> > > On Tue, 19 Jul 2022, Peter Maydell wrote:
> > >
> > > > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > How is this intended to work? The obvious fix from my point
> > > > of view would just be to say "piix4.c requires pcihp.c"
> > > > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> > > > but that seems like it would be rather undoing the point
> > > > of this change.
> > >
> > > Yes. From the commit log and the vague recollection I have about this
> > > change :
> > >
> > > > For example, mips only needs support for PIIX4 and does not
> > > > need acpi pci hotplug support or cpu hotplug support or memory hotplug
> > > support
> > > > etc
> > >
> > > So does malta really need acpi hotplug? If not, then the stubbing out of
> > > the vmstate struct is correct.
> >
> > It's not, because the vmstate struct is actually used when you
> > savevm/loadvm a malta machine. If the malta shouldn't have
> > acpi hotplug then we need to arrange for the hotplug code
> > to be avoided at an earlier point, not just stub in the
> > vmstate struct field.
>
> yes I think that would be more appropriate fix, I agree. Since mst added
> that vmstate, maybe he can comment on this.
Can't we just change the stub to be a valid vmstate structure?
Dave
(I coincidentally found this today because I'd been cc'd on
https://gitlab.com/qemu-project/qemu/-/issues/995 a few months back
and only just noticed)
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2022-07-21 12:35 ` Dr. David Alan Gilbert
@ 2022-07-25 17:57 ` Ani Sinha
0 siblings, 0 replies; 53+ messages in thread
From: Ani Sinha @ 2022-07-25 17:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Ani Sinha, Peter Maydell, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Aurelien Jarno, Jiaxun Yang, Aleksandar Rikalo, Igor Mammedov
On Thu, 21 Jul 2022, Dr. David Alan Gilbert wrote:
> * Ani Sinha (ani@anisinha.ca) wrote:
> >
> >
> > On Wed, 20 Jul 2022, Peter Maydell wrote:
> >
> > > On Wed, 20 Jul 2022 at 19:37, Ani Sinha <ani@anisinha.ca> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, 19 Jul 2022, Peter Maydell wrote:
> > > >
> > > > > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > How is this intended to work? The obvious fix from my point
> > > > > of view would just be to say "piix4.c requires pcihp.c"
> > > > > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> > > > > but that seems like it would be rather undoing the point
> > > > > of this change.
> > > >
> > > > Yes. From the commit log and the vague recollection I have about this
> > > > change :
> > > >
> > > > > For example, mips only needs support for PIIX4 and does not
> > > > > need acpi pci hotplug support or cpu hotplug support or memory hotplug
> > > > support
> > > > > etc
> > > >
> > > > So does malta really need acpi hotplug? If not, then the stubbing out of
> > > > the vmstate struct is correct.
> > >
> > > It's not, because the vmstate struct is actually used when you
> > > savevm/loadvm a malta machine. If the malta shouldn't have
> > > acpi hotplug then we need to arrange for the hotplug code
> > > to be avoided at an earlier point, not just stub in the
> > > vmstate struct field.
> >
> > yes I think that would be more appropriate fix, I agree. Since mst added
> > that vmstate, maybe he can comment on this.
>
> Can't we just change the stub to be a valid vmstate structure?
This would be only a short term solution until we can re-org the code so
that Malta does not use acpi hotplug anymore.
>
> Dave
> (I coincidentally found this today because I'd been cc'd on
> https://gitlab.com/qemu-project/qemu/-/issues/995 a few months back
> and only just noticed)
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PULL 07/35] hw/virtio: move vhost_set_backend_type() to vhost.c
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (5 preceding siblings ...)
2021-09-04 21:36 ` [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 08/35] vhost-user: add missing space in error message Michael S. Tsirkin
` (28 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Tiberiu Georgescu, Raphael Norwitz
From: Tiberiu Georgescu <tiberiu.georgescu@nutanix.com>
Just a small refactor patch.
vhost_set_backend_type() gets called only in vhost.c, so we can move the
function there and make it static. We can then extern the visibility of
kernel_ops, to match the other VhostOps in vhost-backend.h.
The VhostOps constants now make more sense in vhost.h
Suggested-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Tiberiu Georgescu <tiberiu.georgescu@nutanix.com>
Message-Id: <20210809134015.67941-1-tiberiu.georgescu@nutanix.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/vhost-backend.h | 6 ------
include/hw/virtio/vhost.h | 4 ++++
hw/virtio/vhost-backend.c | 30 +-----------------------------
hw/virtio/vhost.c | 29 +++++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 35 deletions(-)
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 8475c5a29d..81bf3109f8 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -173,12 +173,6 @@ typedef struct VhostOps {
vhost_force_iommu_op vhost_force_iommu;
} VhostOps;
-extern const VhostOps user_ops;
-extern const VhostOps vdpa_ops;
-
-int vhost_set_backend_type(struct vhost_dev *dev,
- VhostBackendType backend_type);
-
int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
uint64_t iova, uint64_t uaddr,
uint64_t len,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 045d0fd9f2..5ee306568b 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -95,6 +95,10 @@ struct vhost_dev {
const VhostDevConfigOps *config_ops;
};
+extern const VhostOps kernel_ops;
+extern const VhostOps user_ops;
+extern const VhostOps vdpa_ops;
+
struct vhost_net {
struct vhost_dev dev;
struct vhost_virtqueue vqs[2];
diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 594d770b75..b65f8f7e97 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -293,7 +293,7 @@ static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
}
-static const VhostOps kernel_ops = {
+const VhostOps kernel_ops = {
.backend_type = VHOST_BACKEND_TYPE_KERNEL,
.vhost_backend_init = vhost_kernel_init,
.vhost_backend_cleanup = vhost_kernel_cleanup,
@@ -328,34 +328,6 @@ static const VhostOps kernel_ops = {
};
#endif
-int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
-{
- int r = 0;
-
- switch (backend_type) {
-#ifdef CONFIG_VHOST_KERNEL
- case VHOST_BACKEND_TYPE_KERNEL:
- dev->vhost_ops = &kernel_ops;
- break;
-#endif
-#ifdef CONFIG_VHOST_USER
- case VHOST_BACKEND_TYPE_USER:
- dev->vhost_ops = &user_ops;
- break;
-#endif
-#ifdef CONFIG_VHOST_VDPA
- case VHOST_BACKEND_TYPE_VDPA:
- dev->vhost_ops = &vdpa_ops;
- break;
-#endif
- default:
- error_report("Unknown vhost backend type");
- r = -1;
- }
-
- return r;
-}
-
int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
uint64_t iova, uint64_t uaddr,
uint64_t len,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 3c0b537f89..e21e144510 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -174,6 +174,35 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
return log_size;
}
+static int vhost_set_backend_type(struct vhost_dev *dev,
+ VhostBackendType backend_type)
+{
+ int r = 0;
+
+ switch (backend_type) {
+#ifdef CONFIG_VHOST_KERNEL
+ case VHOST_BACKEND_TYPE_KERNEL:
+ dev->vhost_ops = &kernel_ops;
+ break;
+#endif
+#ifdef CONFIG_VHOST_USER
+ case VHOST_BACKEND_TYPE_USER:
+ dev->vhost_ops = &user_ops;
+ break;
+#endif
+#ifdef CONFIG_VHOST_VDPA
+ case VHOST_BACKEND_TYPE_VDPA:
+ dev->vhost_ops = &vdpa_ops;
+ break;
+#endif
+ default:
+ error_report("Unknown vhost backend type");
+ r = -1;
+ }
+
+ return r;
+}
+
static struct vhost_log *vhost_log_alloc(uint64_t size, bool share)
{
Error *err = NULL;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 08/35] vhost-user: add missing space in error message
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (6 preceding siblings ...)
2021-09-04 21:36 ` [PULL 07/35] hw/virtio: move vhost_set_backend_type() to vhost.c Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 09/35] acpi: Delete broken ACPI_GED_X86 macro Michael S. Tsirkin
` (27 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Alyssa Ross
From: Alyssa Ross <hi@alyssa.is>
This would previously give error messages like
> Received unexpected msg type.Expected 0 received 1
Signed-off-by: Alyssa Ross <hi@alyssa.is>
Message-Id: <20210806143926.315725-1-hi@alyssa.is>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost-user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index a4eb6cde7e..360d9bc040 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -429,7 +429,7 @@ static int process_message_reply(struct vhost_dev *dev,
}
if (msg_reply.hdr.request != msg->hdr.request) {
- error_report("Received unexpected msg type."
+ error_report("Received unexpected msg type. "
"Expected %d received %d",
msg->hdr.request, msg_reply.hdr.request);
return -1;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 09/35] acpi: Delete broken ACPI_GED_X86 macro
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (7 preceding siblings ...)
2021-09-04 21:36 ` [PULL 08/35] vhost-user: add missing space in error message Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 10/35] Use PCI_HOST_BRIDGE macro Michael S. Tsirkin
` (26 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Peter Maydell, Gerd Hoffmann, Eduardo Habkost, Igor Mammedov
From: Eduardo Habkost <ehabkost@redhat.com>
The macro never worked and never will, because the
AcpiGedX86State type never existed.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20210805193431.307761-2-ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/generic_event_device.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 6bed92e8fc..d49217c445 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -70,8 +70,6 @@
OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
#define TYPE_ACPI_GED_X86 "acpi-ged-x86"
-#define ACPI_GED_X86(obj) \
- OBJECT_CHECK(AcpiGedX86State, (obj), TYPE_ACPI_GED_X86)
#define ACPI_GED_EVT_SEL_OFFSET 0x0
#define ACPI_GED_EVT_SEL_LEN 0x4
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 10/35] Use PCI_HOST_BRIDGE macro
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (8 preceding siblings ...)
2021-09-04 21:36 ` [PULL 09/35] acpi: Delete broken ACPI_GED_X86 macro Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 11/35] virtio-balloon: don't start free page hinting if postcopy is possible Michael S. Tsirkin
` (25 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Eduardo Habkost, Richard Henderson, Igor Mammedov,
Ani Sinha, Paolo Bonzini
From: Eduardo Habkost <ehabkost@redhat.com>
OBJECT_CHECK(PciHostState, ..., TYPE_PCI_HOST_BRIDGE) is exactly
what the PCI_HOST_BRIDGE macro does. We can just use the macro
instead of using OBJECT_CHECK manually.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20210805193431.307761-7-ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/i386/acpi-build.c | 8 ++------
hw/pci-host/i440fx.c | 4 +---
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6c27e12e2a..9a9572cadb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -303,13 +303,9 @@ Object *acpi_get_i386_pci_host(void)
{
PCIHostState *host;
- host = OBJECT_CHECK(PCIHostState,
- object_resolve_path("/machine/i440fx", NULL),
- TYPE_PCI_HOST_BRIDGE);
+ host = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
if (!host) {
- host = OBJECT_CHECK(PCIHostState,
- object_resolve_path("/machine/q35", NULL),
- TYPE_PCI_HOST_BRIDGE);
+ host = PCI_HOST_BRIDGE(object_resolve_path("/machine/q35", NULL));
}
return OBJECT(host);
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 28c9bae899..cd87e21a9b 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -316,9 +316,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
PCIBus *find_i440fx(void)
{
- PCIHostState *s = OBJECT_CHECK(PCIHostState,
- object_resolve_path("/machine/i440fx", NULL),
- TYPE_PCI_HOST_BRIDGE);
+ PCIHostState *s = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
return s ? s->bus : NULL;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 11/35] virtio-balloon: don't start free page hinting if postcopy is possible
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (9 preceding siblings ...)
2021-09-04 21:36 ` [PULL 10/35] Use PCI_HOST_BRIDGE macro Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 12/35] virtio-balloon: free page hinting cleanups Michael S. Tsirkin
` (24 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, David Hildenbrand, Juan Quintela, qemu-stable,
Alexander Duyck, Dr. David Alan Gilbert, Wei Wang, Peter Xu,
Philippe Mathieu-Daudé
From: David Hildenbrand <david@redhat.com>
Postcopy never worked properly with 'free-page-hint=on', as there are
at least two issues:
1) With postcopy, the guest will never receive a VIRTIO_BALLOON_CMD_ID_DONE
and consequently won't release free pages back to the OS once
migration finishes.
The issue is that for postcopy, we won't do a final bitmap sync while
the guest is stopped on the source and
virtio_balloon_free_page_hint_notify() will only call
virtio_balloon_free_page_done() on the source during
PRECOPY_NOTIFY_CLEANUP, after the VM state was already migrated to
the destination.
2) Once the VM touches a page on the destination that has been excluded
from migration on the source via qemu_guest_free_page_hint() while
postcopy is active, that thread will stall until postcopy finishes
and all threads are woken up. (with older Linux kernels that won't
retry faults when woken up via userfaultfd, we might actually get a
SEGFAULT)
The issue is that the source will refuse to migrate any pages that
are not marked as dirty in the dirty bmap -- for example, because the
page might just have been sent. Consequently, the faulting thread will
stall, waiting for the page to be migrated -- which could take quite
a while and result in guest OS issues.
While we could fix 1) comparatively easily, 2) is harder to get right and
might require more involved RAM migration changes on source and destination
[1].
As it never worked properly, let's not start free page hinting in the
precopy notifier if the postcopy migration capability was enabled to fix
it easily. Capabilities cannot be enabled once migration is already
running.
Note 1: in the future we might either adjust migration code on the source
to track pages that have actually been sent or adjust
migration code on source and destination to eventually send
pages multiple times from the source and and deal with pages
that are sent multiple times on the destination.
Note 2: virtio-mem has similar issues, however, access to "unplugged"
memory by the guest is very rare and we would have to be very
lucky for it to happen during migration. The spec states
"The driver SHOULD NOT read from unplugged memory blocks ..."
and "The driver MUST NOT write to unplugged memory blocks".
virtio-mem will move away from virtio_balloon_free_page_done()
soon and handle this case explicitly on the destination.
[1] https://lkml.kernel.org/r/e79fd18c-aa62-c1d8-c7f3-ba3fc2c25fc8@redhat.com
Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Cc: qemu-stable@nongnu.org
Cc: Wei Wang <wei.w.wang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
Message-Id: <20210708095339.20274-2-david@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
hw/virtio/virtio-balloon.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 4b5d9e5e50..ae7867a8db 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
#include "trace.h"
#include "qemu/error-report.h"
#include "migration/misc.h"
+#include "migration/migration.h"
#include "hw/virtio/virtio-bus.h"
#include "hw/virtio/virtio-access.h"
@@ -662,6 +663,18 @@ virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
return 0;
}
+ /*
+ * Pages hinted via qemu_guest_free_page_hint() are cleared from the dirty
+ * bitmap and will not get migrated, especially also not when the postcopy
+ * destination starts using them and requests migration from the source; the
+ * faulting thread will stall until postcopy migration finishes and
+ * all threads are woken up. Let's not start free page hinting if postcopy
+ * is possible.
+ */
+ if (migrate_postcopy_ram()) {
+ return 0;
+ }
+
switch (pnd->reason) {
case PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC:
virtio_balloon_free_page_stop(dev);
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 12/35] virtio-balloon: free page hinting cleanups
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (10 preceding siblings ...)
2021-09-04 21:36 ` [PULL 11/35] virtio-balloon: don't start free page hinting if postcopy is possible Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 13/35] virtio-bus: introduce iommu_enabled() Michael S. Tsirkin
` (23 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Juan Quintela, David Hildenbrand,
Dr. David Alan Gilbert, Alexander Duyck, Wei Wang, Peter Xu,
Philippe Mathieu-Daudé
From: David Hildenbrand <david@redhat.com>
Let's compress the code a bit to improve readability. We can drop the
vm_running check in virtio_balloon_free_page_start() as it's already
properly checked in the single caller.
Cc: Wei Wang <wei.w.wang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
Message-Id: <20210708095339.20274-3-david@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio-balloon.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index ae7867a8db..5a69dce35d 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -534,22 +534,18 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED &&
id == dev->free_page_hint_cmd_id) {
dev->free_page_hint_status = FREE_PAGE_HINT_S_START;
- } else {
+ } else if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
/*
* Stop the optimization only when it has started. This
* avoids a stale stop sign for the previous command.
*/
- if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
- dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
- }
+ dev->free_page_hint_status = FREE_PAGE_HINT_S_STOP;
}
}
- if (elem->in_num) {
- if (dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
- qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
- elem->in_sg[0].iov_len);
- }
+ if (elem->in_num && dev->free_page_hint_status == FREE_PAGE_HINT_S_START) {
+ qemu_guest_free_page_hint(elem->in_sg[0].iov_base,
+ elem->in_sg[0].iov_len);
}
out:
@@ -592,16 +588,10 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
{
VirtIODevice *vdev = VIRTIO_DEVICE(s);
- /* For the stop and copy phase, we don't need to start the optimization */
- if (!vdev->vm_running) {
- return;
- }
-
qemu_mutex_lock(&s->free_page_lock);
if (s->free_page_hint_cmd_id == UINT_MAX) {
- s->free_page_hint_cmd_id =
- VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
+ s->free_page_hint_cmd_id = VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
} else {
s->free_page_hint_cmd_id++;
}
@@ -649,8 +639,7 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
static int
virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data)
{
- VirtIOBalloon *dev = container_of(n, VirtIOBalloon,
- free_page_hint_notify);
+ VirtIOBalloon *dev = container_of(n, VirtIOBalloon, free_page_hint_notify);
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
PrecopyNotifyData *pnd = data;
@@ -919,8 +908,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
- if (virtio_has_feature(s->host_features,
- VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+ if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
virtio_balloon_handle_free_page_vq);
precopy_add_notifier(&s->free_page_hint_notify);
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 13/35] virtio-bus: introduce iommu_enabled()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (11 preceding siblings ...)
2021-09-04 21:36 ` [PULL 12/35] virtio-balloon: free page hinting cleanups Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 14/35] virtio-pci: implement iommu_enabled() Michael S. Tsirkin
` (22 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
This patch introduce a new method for the virtio-bus for the transport
to report whether or not the IOMMU is enabled for the device.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210804034803.1644-2-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/virtio-bus.h | 4 +++-
hw/virtio/virtio-bus.c | 14 ++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index ef8abe49c5..7ab8c9dab0 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -93,6 +93,7 @@ struct VirtioBusClass {
*/
bool has_variable_vring_alignment;
AddressSpace *(*get_dma_as)(DeviceState *d);
+ bool (*iommu_enabled)(DeviceState *d);
};
struct VirtioBusState {
@@ -154,5 +155,6 @@ void virtio_bus_release_ioeventfd(VirtioBusState *bus);
int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign);
/* Tell the bus that the ioeventfd handler is no longer required. */
void virtio_bus_cleanup_host_notifier(VirtioBusState *bus, int n);
-
+/* Whether the IOMMU is enabled for this device */
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev);
#endif /* VIRTIO_BUS_H */
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 859978d248..d23db98c56 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -325,6 +325,20 @@ static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
return NULL;
}
+bool virtio_bus_device_iommu_enabled(VirtIODevice *vdev)
+{
+ DeviceState *qdev = DEVICE(vdev);
+ BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+ VirtioBusState *bus = VIRTIO_BUS(qbus);
+ VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+
+ if (!klass->iommu_enabled) {
+ return false;
+ }
+
+ return klass->iommu_enabled(qbus->parent);
+}
+
static void virtio_bus_class_init(ObjectClass *klass, void *data)
{
BusClass *bus_class = BUS_CLASS(klass);
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 14/35] virtio-pci: implement iommu_enabled()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (12 preceding siblings ...)
2021-09-04 21:36 ` [PULL 13/35] virtio-bus: introduce iommu_enabled() Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 15/35] vhost: correctly detect the enabling IOMMU Michael S. Tsirkin
` (21 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
This patch implements the PCI transport version of iommu_enabled. This
is done by comparing the address space returned by
pci_device_iommu_address_space() against address_space_memory.
Note that an ideal approach is to use pci_device_iommu_address_space()
in get_dma_as(), but it might not work well since the IOMMU could be
initialized after the virtio-pci device is initialized.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210804034803.1644-3-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/virtio-pci.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 433060ac02..6e16e2705c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1121,6 +1121,19 @@ static AddressSpace *virtio_pci_get_dma_as(DeviceState *d)
return pci_get_address_space(dev);
}
+static bool virtio_pci_iommu_enabled(DeviceState *d)
+{
+ VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
+ PCIDevice *dev = &proxy->pci_dev;
+ AddressSpace *dma_as = pci_device_iommu_address_space(dev);
+
+ if (dma_as == &address_space_memory) {
+ return false;
+ }
+
+ return true;
+}
+
static bool virtio_pci_queue_enabled(DeviceState *d, int n)
{
VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
@@ -2202,6 +2215,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
k->ioeventfd_enabled = virtio_pci_ioeventfd_enabled;
k->ioeventfd_assign = virtio_pci_ioeventfd_assign;
k->get_dma_as = virtio_pci_get_dma_as;
+ k->iommu_enabled = virtio_pci_iommu_enabled;
k->queue_enabled = virtio_pci_queue_enabled;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 15/35] vhost: correctly detect the enabling IOMMU
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (13 preceding siblings ...)
2021-09-04 21:36 ` [PULL 14/35] virtio-pci: implement iommu_enabled() Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 16/35] hw/i386/acpi-build: Get NUMA information from struct NumaState Michael S. Tsirkin
` (20 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
Vhost used to compare the dma_as against the address_space_memory to
detect whether the IOMMU is enabled or not. This might not work well
since the virito-bus may call get_dma_as if VIRTIO_F_IOMMU_PLATFORM is
set without an actual IOMMU enabled when device is plugged. In the
case of PCI where pci_get_address_space() is used, the bus master as
is returned. So vhost actually tries to enable device IOTLB even if
the IOMMU is not enabled. This will lead a lots of unnecessary
transactions between vhost and Qemu and will introduce a huge drop of
the performance.
For PCI, an ideal approach is to use pci_device_iommu_address_space()
just for get_dma_as. But Qemu may choose to initialize the IOMMU after
the virtio-pci which lead a wrong address space is returned during
device plugged. So this patch switch to use transport specific way via
iommu_enabled() to detect the IOMMU during vhost start. In this case,
we are fine since we know the IOMMU is initialized correctly.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210804034803.1644-4-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e21e144510..b4b29413e6 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -315,7 +315,7 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
* does not have IOMMU, there's no need to enable this feature
* which may cause unnecessary IOTLB miss/update trnasactions.
*/
- return vdev->dma_as != &address_space_memory &&
+ return virtio_bus_device_iommu_enabled(vdev) &&
virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 16/35] hw/i386/acpi-build: Get NUMA information from struct NumaState
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (14 preceding siblings ...)
2021-09-04 21:36 ` [PULL 15/35] vhost: correctly detect the enabling IOMMU Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 17/35] hw/pci: remove all references to find_i440fx function Michael S. Tsirkin
` (19 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Eduardo Habkost, Jingqi Liu, Richard Henderson,
Paolo Bonzini, Ani Sinha, Igor Mammedov,
Philippe Mathieu-Daudé
From: Jingqi Liu <jingqi.liu@intel.com>
Since commits aa57020774b ("numa: move numa global variable
nb_numa_nodes into MachineState") and 7e721e7b10e ("numa: move
numa global variable numa_info into MachineState"), we can get
NUMA information completely from MachineState::numa_state.
Remove PCMachineState::numa_nodes and PCMachineState::node_mem,
since they are just copied from MachineState::numa_state.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
Message-Id: <20210823011254.28506-1-jingqi.liu@intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/i386/pc.h | 4 ----
hw/i386/acpi-build.c | 12 +++++++-----
hw/i386/pc.c | 9 ---------
3 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 97b4ab79b5..4d2e35a152 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -47,10 +47,6 @@ typedef struct PCMachineState {
bool default_bus_bypass_iommu;
uint64_t max_fw_size;
- /* NUMA information: */
- uint64_t numa_nodes;
- uint64_t *node_mem;
-
/* ACPI Memory hotplug IO base address */
hwaddr memhp_io_base;
} PCMachineState;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9a9572cadb..d1f5fa3b5a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1914,6 +1914,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
X86MachineState *x86ms = X86_MACHINE(machine);
const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
PCMachineState *pcms = PC_MACHINE(machine);
+ int nb_numa_nodes = machine->numa_state->num_nodes;
+ NodeInfo *numa_info = machine->numa_state->nodes;
ram_addr_t hotplugabble_address_space_size =
object_property_get_int(OBJECT(pcms), PC_MACHINE_DEVMEM_REGION_SIZE,
NULL);
@@ -1957,9 +1959,9 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
next_base = 0;
numa_start = table_data->len;
- for (i = 1; i < pcms->numa_nodes + 1; ++i) {
+ for (i = 1; i < nb_numa_nodes + 1; ++i) {
mem_base = next_base;
- mem_len = pcms->node_mem[i - 1];
+ mem_len = numa_info[i - 1].node_mem;
next_base = mem_base + mem_len;
/* Cut out the 640K hole */
@@ -2007,7 +2009,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
}
slots = (table_data->len - numa_start) / sizeof *numamem;
- for (; slots < pcms->numa_nodes + 2; slots++) {
+ for (; slots < nb_numa_nodes + 2; slots++) {
numamem = acpi_data_push(table_data, sizeof *numamem);
build_srat_memory(numamem, 0, 0, 0, MEM_AFFINITY_NOFLAGS);
}
@@ -2023,7 +2025,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
if (hotplugabble_address_space_size) {
numamem = acpi_data_push(table_data, sizeof *numamem);
build_srat_memory(numamem, machine->device_memory->base,
- hotplugabble_address_space_size, pcms->numa_nodes - 1,
+ hotplugabble_address_space_size, nb_numa_nodes - 1,
MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
}
@@ -2525,7 +2527,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
}
}
#endif
- if (pcms->numa_nodes) {
+ if (machine->numa_state->num_nodes) {
acpi_add_table(table_offsets, tables_blob);
build_srat(tables_blob, tables->linker, machine);
if (machine->numa_state->have_numa_distance) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 22aa598d50..7e523b913c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -802,18 +802,9 @@ void pc_machine_done(Notifier *notifier, void *data)
void pc_guest_info_init(PCMachineState *pcms)
{
- int i;
- MachineState *ms = MACHINE(pcms);
X86MachineState *x86ms = X86_MACHINE(pcms);
x86ms->apic_xrupt_override = true;
- pcms->numa_nodes = ms->numa_state->num_nodes;
- pcms->node_mem = g_malloc0(pcms->numa_nodes *
- sizeof *pcms->node_mem);
- for (i = 0; i < ms->numa_state->num_nodes; i++) {
- pcms->node_mem[i] = ms->numa_state->nodes[i].node_mem;
- }
-
pcms->machine_done.notify = pc_machine_done;
qemu_add_machine_init_done_notifier(&pcms->machine_done);
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 17/35] hw/pci: remove all references to find_i440fx function
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (15 preceding siblings ...)
2021-09-04 21:36 ` [PULL 16/35] hw/i386/acpi-build: Get NUMA information from struct NumaState Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 18/35] hw/acpi: use existing references to pci device struct within functions Michael S. Tsirkin
` (18 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Peter Maydell, Philippe Mathieu-Daudé, Paolo Bonzini
From: Ani Sinha <ani@anisinha.ca>
commit c0e427d6eb5fefc538 ("hw/acpi/ich9: Enable ACPI PCI hot-plug") removed all
uses of find_i440fx() function. This has been replaced by the more generic call
acpi_get_i386_pci_host() which maybe able to find the root bus both for i440fx
machine type as well as for the q35 machine type. There seems to be no more any
need to maintain a i440fx specific version of the api call. Remove it.
Tested by building from a clean tree successfully.
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210825031949.919376-2-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/pci-host/i440fx.h | 1 -
hw/pci-host/i440fx.c | 6 ------
stubs/pci-host-piix.c | 7 -------
stubs/meson.build | 1 -
4 files changed, 15 deletions(-)
delete mode 100644 stubs/pci-host-piix.c
diff --git a/include/hw/pci-host/i440fx.h b/include/hw/pci-host/i440fx.h
index 7fcfd9485c..f068aaba8f 100644
--- a/include/hw/pci-host/i440fx.h
+++ b/include/hw/pci-host/i440fx.h
@@ -45,6 +45,5 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
MemoryRegion *pci_memory,
MemoryRegion *ram_memory);
-PCIBus *find_i440fx(void);
#endif
diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index cd87e21a9b..e08716142b 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -314,12 +314,6 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
return b;
}
-PCIBus *find_i440fx(void)
-{
- PCIHostState *s = PCI_HOST_BRIDGE(object_resolve_path("/machine/i440fx", NULL));
- return s ? s->bus : NULL;
-}
-
static void i440fx_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/stubs/pci-host-piix.c b/stubs/pci-host-piix.c
deleted file mode 100644
index 93975adbfe..0000000000
--- a/stubs/pci-host-piix.c
+++ /dev/null
@@ -1,7 +0,0 @@
-#include "qemu/osdep.h"
-#include "hw/pci-host/i440fx.h"
-
-PCIBus *find_i440fx(void)
-{
- return NULL;
-}
diff --git a/stubs/meson.build b/stubs/meson.build
index 275ac89c16..beee31ec73 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -26,7 +26,6 @@ stub_ss.add(files('module-opts.c'))
stub_ss.add(files('monitor.c'))
stub_ss.add(files('monitor-core.c'))
stub_ss.add(files('pci-bus.c'))
-stub_ss.add(files('pci-host-piix.c'))
stub_ss.add(files('qemu-timer-notify-cb.c'))
stub_ss.add(files('qmp_memory_device.c'))
stub_ss.add(files('qmp-command-available.c'))
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 18/35] hw/acpi: use existing references to pci device struct within functions
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (16 preceding siblings ...)
2021-09-04 21:36 ` [PULL 17/35] hw/pci: remove all references to find_i440fx function Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 19/35] MAINTAINERS: Added myself as a reviewer for acpi/smbios subsystem Michael S. Tsirkin
` (17 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Peter Maydell, Philippe Mathieu-Daudé, Igor Mammedov
From: Ani Sinha <ani@anisinha.ca>
There is no need to use fresh typecasts to get references to pci device structs
when there is an existing reference to pci device struct. Use existing reference.
Minor cleanup.
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210825031949.919376-3-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/acpi/pcihp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index f4d706e47d..f610a25d2e 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -283,7 +283,7 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
/* Only hotplugged devices need the hotplug capability. */
if (dev->hotplugged &&
- acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))) < 0) {
+ acpi_pcihp_get_bsel(pci_get_bus(pdev)) < 0) {
error_setg(errp, "Unsupported bus. Bus doesn't have property '"
ACPI_PCIHP_PROP_BSEL "' set");
return;
@@ -363,8 +363,8 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
{
PCIDevice *pdev = PCI_DEVICE(dev);
- trace_acpi_pci_unplug(PCI_SLOT(PCI_DEVICE(dev)->devfn),
- acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))));
+ trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn),
+ acpi_pcihp_get_bsel(pci_get_bus(pdev)));
/*
* clean up acpi-index so it could reused by another device
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 19/35] MAINTAINERS: Added myself as a reviewer for acpi/smbios subsystem
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (17 preceding siblings ...)
2021-09-04 21:36 ` [PULL 18/35] hw/acpi: use existing references to pci device struct within functions Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:36 ` [PULL 20/35] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU Michael S. Tsirkin
` (16 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Thomas Huth, Richard Henderson, Paolo Bonzini,
Ani Sinha, Igor Mammedov, Philippe Mathieu-Daudé
From: Ani Sinha <ani@anisinha.ca>
I have developed an interest in this space and hopefully can lend some
helping hand to Igor and Michael in reviewing simpler patches.
Signed-off-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20210825031949.919376-4-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
MAINTAINERS | 1 +
1 file changed, 1 insertion(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5d923a6544..6c20634d63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1751,6 +1751,7 @@ F: docs/specs/*pci*
ACPI/SMBIOS
M: Michael S. Tsirkin <mst@redhat.com>
M: Igor Mammedov <imammedo@redhat.com>
+R: Ani Sinha <ani@anisinha.ca>
S: Supported
F: include/hw/acpi/*
F: include/hw/firmware/smbios.h
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 20/35] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (18 preceding siblings ...)
2021-09-04 21:36 ` [PULL 19/35] MAINTAINERS: Added myself as a reviewer for acpi/smbios subsystem Michael S. Tsirkin
@ 2021-09-04 21:36 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 21/35] hw/virtio: Remove NULL check in virtio_free_region_cache() Michael S. Tsirkin
` (15 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:36 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Stefan Hajnoczi, Stefano Garzarella
From: Philippe Mathieu-Daudé <philmd@redhat.com>
While virtio_queue_packed_empty_rcu() uses the '_rcu' suffix,
it is not obvious it is called within rcu_read_lock(). All other
functions from this file called with the RCU locked have a comment
describing it. Document this one similarly for consistency.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210826172658.2116840-2-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/virtio/virtio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 874377f37a..a5214bca61 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -634,6 +634,7 @@ static int virtio_queue_split_empty(VirtQueue *vq)
return empty;
}
+/* Called within rcu_read_lock(). */
static int virtio_queue_packed_empty_rcu(VirtQueue *vq)
{
struct VRingPackedDesc desc;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 21/35] hw/virtio: Remove NULL check in virtio_free_region_cache()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (19 preceding siblings ...)
2021-09-04 21:36 ` [PULL 20/35] hw/virtio: Document virtio_queue_packed_empty_rcu is called within RCU Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 22/35] hw/virtio: Add flatview update in vhost_user_cleanup() Michael S. Tsirkin
` (14 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Stefan Hajnoczi, Stefano Garzarella
From: Philippe Mathieu-Daudé <philmd@redhat.com>
virtio_free_region_cache() is called within call_rcu(),
always with a non-NULL argument. Ensure new code keep it
that way by replacing the NULL check by an assertion.
Add a comment this function is called within call_rcu().
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210826172658.2116840-3-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
hw/virtio/virtio.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a5214bca61..3a1f6c520c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -133,12 +133,10 @@ struct VirtQueue
QLIST_ENTRY(VirtQueue) node;
};
+/* Called within call_rcu(). */
static void virtio_free_region_cache(VRingMemoryRegionCaches *caches)
{
- if (!caches) {
- return;
- }
-
+ assert(caches != NULL);
address_space_cache_destroy(&caches->desc);
address_space_cache_destroy(&caches->avail);
address_space_cache_destroy(&caches->used);
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 22/35] hw/virtio: Add flatview update in vhost_user_cleanup()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (20 preceding siblings ...)
2021-09-04 21:37 ` [PULL 21/35] hw/virtio: Remove NULL check in virtio_free_region_cache() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 23/35] tests/vhost-user-bridge.c: Sanity check socket path length Michael S. Tsirkin
` (13 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Yuwei Zhang
From: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
Qemu will crash on vhost backend unexpected exit and re-connect │
in some case due to access released memory.
Signed-off-by: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
Message-Id: <20210830123433.45727-1-zhangyuwei.9149@bytedance.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost-user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 360d9bc040..2c8556237f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2480,7 +2480,7 @@ void vhost_user_cleanup(VhostUserState *user)
if (!user->chr) {
return;
}
-
+ memory_region_transaction_begin();
for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
if (user->notifier[i].addr) {
object_unparent(OBJECT(&user->notifier[i].mr));
@@ -2488,6 +2488,7 @@ void vhost_user_cleanup(VhostUserState *user)
user->notifier[i].addr = NULL;
}
}
+ memory_region_transaction_commit();
user->chr = NULL;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 23/35] tests/vhost-user-bridge.c: Sanity check socket path length
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (21 preceding siblings ...)
2021-09-04 21:37 ` [PULL 22/35] hw/virtio: Add flatview update in vhost_user_cleanup() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 24/35] tests/vhost-user-bridge.c: Fix typo in help message Michael S. Tsirkin
` (12 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Coiby Xu, Stefan Hajnoczi, Marc-André Lureau
From: Peter Maydell <peter.maydell@linaro.org>
The vhost-user-bridge binary accepts a UNIX socket path on
the command line. Sanity check that this is short enough to
fit into a sockaddr_un before copying it in.
Fixes: Coverity CID 1432866
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210901152632.25511-1-peter.maydell@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/vhost-user-bridge.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 24815920b2..cb009545fa 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -540,6 +540,11 @@ vubr_new(const char *path, bool client)
CallbackFunc cb;
size_t len;
+ if (strlen(path) >= sizeof(un.sun_path)) {
+ fprintf(stderr, "unix domain socket path '%s' is too long\n", path);
+ exit(1);
+ }
+
/* Get a UNIX socket. */
dev->sock = socket(AF_UNIX, SOCK_STREAM, 0);
if (dev->sock == -1) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 24/35] tests/vhost-user-bridge.c: Fix typo in help message
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (22 preceding siblings ...)
2021-09-04 21:37 ` [PULL 23/35] tests/vhost-user-bridge.c: Sanity check socket path length Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 25/35] vhost-vdpa: remove unused variable "acked_features" Michael S. Tsirkin
` (11 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Philippe Mathieu-Daudé,
Stefan Hajnoczi, Marc-André Lureau
From: Peter Maydell <peter.maydell@linaro.org>
Fix a typo in the help message printed by vhost-user-bridge.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210901152713.25701-1-peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
tests/vhost-user-bridge.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index cb009545fa..35088dd67f 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -831,7 +831,7 @@ main(int argc, char *argv[])
out:
fprintf(stderr, "Usage: %s ", argv[0]);
fprintf(stderr, "[-c] [-H] [-u ud_socket_path] [-l lhost:lport] [-r rhost:rport]\n");
- fprintf(stderr, "\t-u path to unix doman socket. default: %s\n",
+ fprintf(stderr, "\t-u path to unix domain socket. default: %s\n",
DEFAULT_UD_SOCKET);
fprintf(stderr, "\t-l local host and port. default: %s:%s\n",
DEFAULT_LHOST, DEFAULT_LPORT);
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 25/35] vhost-vdpa: remove unused variable "acked_features"
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (23 preceding siblings ...)
2021-09-04 21:37 ` [PULL 24/35] tests/vhost-user-bridge.c: Fix typo in help message Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 26/35] vhost-vdpa: correctly return err in vhost_vdpa_set_backend_cap() Michael S. Tsirkin
` (10 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
"acked_features" is unused, let's remove that.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-2-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 19187dce8c..72829884d7 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -29,7 +29,6 @@ typedef struct VhostVDPAState {
NetClientState nc;
struct vhost_vdpa vhost_vdpa;
VHostNetState *vhost_net;
- uint64_t acked_features;
bool started;
} VhostVDPAState;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 26/35] vhost-vdpa: correctly return err in vhost_vdpa_set_backend_cap()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (24 preceding siblings ...)
2021-09-04 21:37 ` [PULL 25/35] vhost-vdpa: remove unused variable "acked_features" Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 27/35] vhost_net: remove the meaningless assignment in vhost_net_start_one() Michael S. Tsirkin
` (9 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
We should return error code instead of zero, otherwise there's no way
for the caller to detect the failure.
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-3-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio/vhost-vdpa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ca1227e5dc..7633ea66d1 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -443,13 +443,13 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
int r;
if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
- return 0;
+ return -EFAULT;
}
features &= f;
r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
if (r) {
- return 0;
+ return -EFAULT;
}
dev->backend_cap = features;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 27/35] vhost_net: remove the meaningless assignment in vhost_net_start_one()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (25 preceding siblings ...)
2021-09-04 21:37 ` [PULL 26/35] vhost-vdpa: correctly return err in vhost_vdpa_set_backend_cap() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 28/35] vhost: use unsigned int for nvqs Michael S. Tsirkin
` (8 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Eli Cohen
From: Jason Wang <jasowang@redhat.com>
The nvqs and vqs have been initialized during vhost_net_init() and are
not expected to change during the life cycle of vhost_net
structure. So this patch removes the meaningless assignment.
Reviewed-by: Eli Cohen <elic@nvidia.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-4-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/net/vhost_net.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 10a7780a13..6ed0c39836 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -242,9 +242,6 @@ static int vhost_net_start_one(struct vhost_net *net,
struct vhost_vring_file file = { };
int r;
- net->dev.nvqs = 2;
- net->dev.vqs = net->vqs;
-
r = vhost_dev_enable_notifiers(&net->dev, dev);
if (r < 0) {
goto fail_notifiers;
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 28/35] vhost: use unsigned int for nvqs
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (26 preceding siblings ...)
2021-09-04 21:37 ` [PULL 27/35] vhost_net: remove the meaningless assignment in vhost_net_start_one() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 29/35] vhost_net: do not assume nvqs is always 2 Michael S. Tsirkin
` (7 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Eli Cohen
From: Jason Wang <jasowang@redhat.com>
Switch to use unsigned int for nvqs since it's not expected to be
negative.
Reviewed-by: Eli Cohen <elic@nvidia.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-5-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/virtio/vhost.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 5ee306568b..1a9fc65089 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -71,7 +71,7 @@ struct vhost_dev {
int n_tmp_sections;
MemoryRegionSection *tmp_sections;
struct vhost_virtqueue *vqs;
- int nvqs;
+ unsigned int nvqs;
/* the first virtqueue which would be used by this vhost dev */
int vq_index;
/* if non-zero, minimum required value for max_queues */
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 29/35] vhost_net: do not assume nvqs is always 2
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (27 preceding siblings ...)
2021-09-04 21:37 ` [PULL 28/35] vhost: use unsigned int for nvqs Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 30/35] vhost-vdpa: remove the unnecessary check in vhost_vdpa_add() Michael S. Tsirkin
` (6 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Eli Cohen, Stefano Garzarella
From: Jason Wang <jasowang@redhat.com>
This patch switches to initialize dev.nvqs from the VhostNetOptions
instead of assuming it was 2. This is useful for implementing control
virtqueue support which will be a single vhost_net structure with a
single cvq.
Note that nvqs is still set to 2 for all users and this patch does not
change functionality.
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-6-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/net/vhost_net.h | 1 +
hw/net/vhost_net.c | 2 +-
net/tap.c | 1 +
net/vhost-user.c | 1 +
net/vhost-vdpa.c | 1 +
5 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 172b0051d8..fba40cf695 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -14,6 +14,7 @@ typedef struct VhostNetOptions {
VhostBackendType backend_type;
NetClientState *net_backend;
uint32_t busyloop_timeout;
+ unsigned int nvqs;
void *opaque;
} VhostNetOptions;
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 6ed0c39836..386ec2eaa2 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -165,9 +165,9 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
goto fail;
}
net->nc = options->net_backend;
+ net->dev.nvqs = options->nvqs;
net->dev.max_queues = 1;
- net->dev.nvqs = 2;
net->dev.vqs = net->vqs;
if (backend_kernel) {
diff --git a/net/tap.c b/net/tap.c
index f5686bbf77..f716be3e3f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -749,6 +749,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
qemu_set_nonblock(vhostfd);
}
options.opaque = (void *)(uintptr_t)vhostfd;
+ options.nvqs = 2;
s->vhost_net = vhost_net_init(&options);
if (!s->vhost_net) {
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 6adfcd623a..4a939124d2 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -85,6 +85,7 @@ static int vhost_user_start(int queues, NetClientState *ncs[],
options.net_backend = ncs[i];
options.opaque = be;
options.busyloop_timeout = 0;
+ options.nvqs = 2;
net = vhost_net_init(&options);
if (!net) {
error_report("failed to init vhost_net for queue %d", i);
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 72829884d7..395117debd 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -104,6 +104,7 @@ static int vhost_vdpa_add(NetClientState *ncs, void *be)
options.net_backend = ncs;
options.opaque = be;
options.busyloop_timeout = 0;
+ options.nvqs = 2;
net = vhost_net_init(&options);
if (!net) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 30/35] vhost-vdpa: remove the unnecessary check in vhost_vdpa_add()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (28 preceding siblings ...)
2021-09-04 21:37 ` [PULL 29/35] vhost_net: do not assume nvqs is always 2 Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 31/35] vhost-vdpa: don't cleanup twice " Michael S. Tsirkin
` (5 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
The VhostVDPAState is just allocated by qemu_new_net_client() via
g_malloc0() in net_vhost_vdpa_init(). So s->vhost_net is NULL for
sure, let's remove this unnecessary check in vhost_vdpa_add().
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-7-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 395117debd..5c09cacd5a 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -111,10 +111,6 @@ static int vhost_vdpa_add(NetClientState *ncs, void *be)
error_report("failed to init vhost_net for queue");
goto err;
}
- if (s->vhost_net) {
- vhost_net_cleanup(s->vhost_net);
- g_free(s->vhost_net);
- }
s->vhost_net = net;
ret = vhost_vdpa_net_check_device_id(net);
if (ret) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 31/35] vhost-vdpa: don't cleanup twice in vhost_vdpa_add()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (29 preceding siblings ...)
2021-09-04 21:37 ` [PULL 30/35] vhost-vdpa: remove the unnecessary check in vhost_vdpa_add() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 32/35] vhost-vdpa: fix leaking of vhost_net " Michael S. Tsirkin
` (4 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Stefano Garzarella
From: Jason Wang <jasowang@redhat.com>
The previous vhost_net_cleanup is sufficient for freeing, calling
vhost_vdpa_del() in this case will lead an extra round of free. Note
that this kind of "double free" is safe since vhost_dev_cleanup() zero
the whole structure.
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-8-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 5c09cacd5a..3213e69d63 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -81,16 +81,6 @@ static int vhost_vdpa_net_check_device_id(struct vhost_net *net)
return ret;
}
-static void vhost_vdpa_del(NetClientState *ncs)
-{
- VhostVDPAState *s;
- assert(ncs->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
- s = DO_UPCAST(VhostVDPAState, nc, ncs);
- if (s->vhost_net) {
- vhost_net_cleanup(s->vhost_net);
- }
-}
-
static int vhost_vdpa_add(NetClientState *ncs, void *be)
{
VhostNetOptions options;
@@ -121,7 +111,6 @@ err:
if (net) {
vhost_net_cleanup(net);
}
- vhost_vdpa_del(ncs);
return -1;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 32/35] vhost-vdpa: fix leaking of vhost_net in vhost_vdpa_add()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (30 preceding siblings ...)
2021-09-04 21:37 ` [PULL 31/35] vhost-vdpa: don't cleanup twice " Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 33/35] vhost-vdpa: tweak the error label " Michael S. Tsirkin
` (3 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Stefano Garzarella
From: Jason Wang <jasowang@redhat.com>
Fixes: 1e0a84ea49b68 ("vhost-vdpa: introduce vhost-vdpa net client")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-9-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3213e69d63..b43df00a85 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -110,6 +110,7 @@ static int vhost_vdpa_add(NetClientState *ncs, void *be)
err:
if (net) {
vhost_net_cleanup(net);
+ g_free(net);
}
return -1;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 33/35] vhost-vdpa: tweak the error label in vhost_vdpa_add()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (31 preceding siblings ...)
2021-09-04 21:37 ` [PULL 32/35] vhost-vdpa: fix leaking of vhost_net " Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 34/35] vhost-vdpa: fix the wrong assertion in vhost_vdpa_init() Michael S. Tsirkin
` (2 subsequent siblings)
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang
From: Jason Wang <jasowang@redhat.com>
Introduce new error label to avoid the unnecessary checking of net
pointer.
Fixes: 1e0a84ea49b68 ("vhost-vdpa: introduce vhost-vdpa net client")
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-10-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index b43df00a85..99327d17b4 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -99,19 +99,18 @@ static int vhost_vdpa_add(NetClientState *ncs, void *be)
net = vhost_net_init(&options);
if (!net) {
error_report("failed to init vhost_net for queue");
- goto err;
+ goto err_init;
}
s->vhost_net = net;
ret = vhost_vdpa_net_check_device_id(net);
if (ret) {
- goto err;
+ goto err_check;
}
return 0;
-err:
- if (net) {
- vhost_net_cleanup(net);
- g_free(net);
- }
+err_check:
+ vhost_net_cleanup(net);
+ g_free(net);
+err_init:
return -1;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 34/35] vhost-vdpa: fix the wrong assertion in vhost_vdpa_init()
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (32 preceding siblings ...)
2021-09-04 21:37 ` [PULL 33/35] vhost-vdpa: tweak the error label " Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-04 21:37 ` [PULL 35/35] vhost-vdpa: remove the unncessary queue_index assignment Michael S. Tsirkin
2021-09-06 9:41 ` [PULL 00/35] pc,pci,virtio: fixes, cleanups Peter Maydell
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Stefano Garzarella
From: Jason Wang <jasowang@redhat.com>
Vhost_vdpa_add() can fail for various reasons, so the assertion of the
succeed is wrong. Instead, we should free the NetClientState and
propagate the error to the caller
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-11-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 99327d17b4..d02cad9855 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -173,7 +173,10 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
}
s->vhost_vdpa.device_fd = vdpa_device_fd;
ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
- assert(s->vhost_net);
+ if (ret) {
+ qemu_close(vdpa_device_fd);
+ qemu_del_net_client(nc);
+ }
return ret;
}
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* [PULL 35/35] vhost-vdpa: remove the unncessary queue_index assignment
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (33 preceding siblings ...)
2021-09-04 21:37 ` [PULL 34/35] vhost-vdpa: fix the wrong assertion in vhost_vdpa_init() Michael S. Tsirkin
@ 2021-09-04 21:37 ` Michael S. Tsirkin
2021-09-06 9:41 ` [PULL 00/35] pc,pci,virtio: fixes, cleanups Peter Maydell
35 siblings, 0 replies; 53+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 21:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Jason Wang, Stefano Garzarella
From: Jason Wang <jasowang@redhat.com>
The queue_index of NetClientState should be assigned in set_netdev()
afterwards, so trying to net_vhost_vdpa_init() is meaningless. This
patch removes this.
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20210903091031.47303-12-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/vhost-vdpa.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index d02cad9855..912686457c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -165,7 +165,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
assert(name);
nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
- nc->queue_index = 0;
s = DO_UPCAST(VhostVDPAState, nc, nc);
vdpa_device_fd = qemu_open_old(vhostdev, O_RDWR);
if (vdpa_device_fd == -1) {
--
MST
^ permalink raw reply related [flat|nested] 53+ messages in thread* Re: [PULL 00/35] pc,pci,virtio: fixes, cleanups
2021-09-04 21:35 [PULL 00/35] pc,pci,virtio: fixes, cleanups Michael S. Tsirkin
` (34 preceding siblings ...)
2021-09-04 21:37 ` [PULL 35/35] vhost-vdpa: remove the unncessary queue_index assignment Michael S. Tsirkin
@ 2021-09-06 9:41 ` Peter Maydell
35 siblings, 0 replies; 53+ messages in thread
From: Peter Maydell @ 2021-09-06 9:41 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit 8880cc4362fde4ecdac0b2092318893118206fcf:
>
> Merge remote-tracking branch 'remotes/cschoenebeck/tags/pull-9p-20210902' into staging (2021-09-03 08:27:38 +0100)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 499c8b4de96eecc554a03e452226f79f169a233b:
>
> vhost-vdpa: remove the unncessary queue_index assignment (2021-09-04 17:34:05 -0400)
>
> ----------------------------------------------------------------
> pc,pci,virtio: fixes, cleanups
>
> Fixes, cleanups all over the place.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 53+ messages in thread