* [PULL 01/15] virtio-mem: unplug memory only during system resets, not device resets
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 02/15] s390x/s390-virtio-ccw: don't crash on weird RAM sizes David Hildenbrand
` (14 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand,
Juraj Marcin, Peter Maydell
We recently converted from the LegacyReset to the new reset framework
in commit c009a311e939 ("virtio-mem: Use new Resettable framework instead
of LegacyReset") to be able to use the ResetType to filter out wakeup
resets.
However, this change had an undesired implications: as we override the
Resettable interface methods in VirtIOMEMClass, the reset handler will
not only get called during system resets (i.e., qemu_devices_reset())
but also during any direct or indirect device rests (e.g.,
device_cold_reset()).
Further, we might now receive two reset callbacks during
qemu_devices_reset(), first when reset by a parent and later when reset
directly.
The memory state of virtio-mem devices is rather special: it's supposed to
be persistent/unchanged during most resets (similar to resetting a hard
disk will not destroy the data), unless actually cold-resetting the whole
system (different to a hard disk where a reboot will not destroy the data):
ripping out system RAM is something guest OSes don't particularly enjoy,
but we want to detect when rebooting to an OS that does not support
virtio-mem and wouldn't be able to detect+use the memory -- and we want
to force-defragment hotplugged memory to also shrink the usable device
memory region. So we rally want to catch system resets to do that.
On supported targets (e.g., x86), getting a cold reset on the
device/parent triggers is not that easy (but looks like PCI code
might trigger it), so this implication went unnoticed.
However, with upcoming s390x support it is problematic: during
kdump, s390x triggers a subsystem reset, ending up in
s390_machine_reset() and calling only subsystem_reset() instead of
qemu_devices_reset() -- because it's not a full system reset.
In subsystem_reset(), s390x performs a device_cold_reset() of any
TYPE_VIRTUAL_CSS_BRIDGE device, which ends up resetting all children,
including the virtio-mem device. Consequently, we wrongly detect a system
reset and unplug all device memory, resulting in hotplugged memory not
getting included in the crash dump -- undesired.
We really must not mess with hotplugged memory state during simple
device resets. To fix, create+register a new reset object that will only
get triggered during qemu_devices_reset() calls, but not during any other
resets as it is logically not the child of any other object.
Message-ID: <20241025104103.342188-1-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Juraj Marcin <jmarcin@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/virtio/virtio-mem.c | 103 +++++++++++++++++++++++----------
include/hw/virtio/virtio-mem.h | 13 ++++-
2 files changed, 84 insertions(+), 32 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 3f6f46fad7..a0dceaddec 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -956,6 +956,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
VirtIOMEM *vmem = VIRTIO_MEM(dev);
uint64_t page_size;
RAMBlock *rb;
+ Object *obj;
int ret;
if (!vmem->memdev) {
@@ -1121,7 +1122,28 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
vmstate_register_any(VMSTATE_IF(vmem),
&vmstate_virtio_mem_device_early, vmem);
}
- qemu_register_resettable(OBJECT(vmem));
+
+ /*
+ * We only want to unplug all memory to start with a clean slate when
+ * it is safe for the guest -- during system resets that call
+ * qemu_devices_reset().
+ *
+ * We'll filter out selected qemu_devices_reset() calls used for other
+ * purposes, like resetting all devices during wakeup from suspend on
+ * x86 based on the reset type passed to qemu_devices_reset().
+ *
+ * Unplugging all memory during simple device resets can result in the VM
+ * unexpectedly losing RAM, corrupting VM state.
+ *
+ * Simple device resets (or resets triggered by getting a parent device
+ * reset) must not change the state of plugged memory blocks. Therefore,
+ * we need a dedicated reset object that only gets called during
+ * qemu_devices_reset().
+ */
+ obj = object_new(TYPE_VIRTIO_MEM_SYSTEM_RESET);
+ vmem->system_reset = VIRTIO_MEM_SYSTEM_RESET(obj);
+ vmem->system_reset->vmem = vmem;
+ qemu_register_resettable(obj);
/*
* Set ourselves as RamDiscardManager before the plug handler maps the
@@ -1141,7 +1163,10 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
* found via an address space anymore. Unset ourselves.
*/
memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
- qemu_unregister_resettable(OBJECT(vmem));
+
+ qemu_unregister_resettable(OBJECT(vmem->system_reset));
+ object_unref(OBJECT(vmem->system_reset));
+
if (vmem->early_migration) {
vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
vmem);
@@ -1841,38 +1866,12 @@ static void virtio_mem_unplug_request_check(VirtIOMEM *vmem, Error **errp)
}
}
-static ResettableState *virtio_mem_get_reset_state(Object *obj)
-{
- VirtIOMEM *vmem = VIRTIO_MEM(obj);
- return &vmem->reset_state;
-}
-
-static void virtio_mem_system_reset_hold(Object *obj, ResetType type)
-{
- VirtIOMEM *vmem = VIRTIO_MEM(obj);
-
- /*
- * When waking up from standby/suspend-to-ram, do not unplug any memory.
- */
- if (type == RESET_TYPE_WAKEUP) {
- return;
- }
-
- /*
- * During usual resets, we will unplug all memory and shrink the usable
- * region size. This is, however, not possible in all scenarios. Then,
- * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
- */
- virtio_mem_unplug_all(vmem);
-}
-
static void virtio_mem_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
VirtIOMEMClass *vmc = VIRTIO_MEM_CLASS(klass);
RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_CLASS(klass);
- ResettableClass *rc = RESETTABLE_CLASS(klass);
device_class_set_props(dc, virtio_mem_properties);
dc->vmsd = &vmstate_virtio_mem;
@@ -1899,9 +1898,6 @@ static void virtio_mem_class_init(ObjectClass *klass, void *data)
rdmc->replay_discarded = virtio_mem_rdm_replay_discarded;
rdmc->register_listener = virtio_mem_rdm_register_listener;
rdmc->unregister_listener = virtio_mem_rdm_unregister_listener;
-
- rc->get_state = virtio_mem_get_reset_state;
- rc->phases.hold = virtio_mem_system_reset_hold;
}
static const TypeInfo virtio_mem_info = {
@@ -1924,3 +1920,48 @@ static void virtio_register_types(void)
}
type_init(virtio_register_types)
+
+OBJECT_DEFINE_SIMPLE_TYPE_WITH_INTERFACES(VirtioMemSystemReset, virtio_mem_system_reset, VIRTIO_MEM_SYSTEM_RESET, OBJECT, { TYPE_RESETTABLE_INTERFACE }, { })
+
+static void virtio_mem_system_reset_init(Object *obj)
+{
+}
+
+static void virtio_mem_system_reset_finalize(Object *obj)
+{
+}
+
+static ResettableState *virtio_mem_system_reset_get_state(Object *obj)
+{
+ VirtioMemSystemReset *vmem_reset = VIRTIO_MEM_SYSTEM_RESET(obj);
+
+ return &vmem_reset->reset_state;
+}
+
+static void virtio_mem_system_reset_hold(Object *obj, ResetType type)
+{
+ VirtioMemSystemReset *vmem_reset = VIRTIO_MEM_SYSTEM_RESET(obj);
+ VirtIOMEM *vmem = vmem_reset->vmem;
+
+ /*
+ * When waking up from standby/suspend-to-ram, do not unplug any memory.
+ */
+ if (type == RESET_TYPE_WAKEUP) {
+ return;
+ }
+
+ /*
+ * During usual resets, we will unplug all memory and shrink the usable
+ * region size. This is, however, not possible in all scenarios. Then,
+ * the guest has to deal with this manually (VIRTIO_MEM_REQ_UNPLUG_ALL).
+ */
+ virtio_mem_unplug_all(vmem);
+}
+
+static void virtio_mem_system_reset_class_init(ObjectClass *klass, void *data)
+{
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+ rc->get_state = virtio_mem_system_reset_get_state;
+ rc->phases.hold = virtio_mem_system_reset_hold;
+}
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index a1af144c28..550ce585b2 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -25,6 +25,10 @@
OBJECT_DECLARE_TYPE(VirtIOMEM, VirtIOMEMClass,
VIRTIO_MEM)
+#define TYPE_VIRTIO_MEM_SYSTEM_RESET "virtio-mem-system-reset"
+
+OBJECT_DECLARE_SIMPLE_TYPE(VirtioMemSystemReset, VIRTIO_MEM_SYSTEM_RESET)
+
#define VIRTIO_MEM_MEMDEV_PROP "memdev"
#define VIRTIO_MEM_NODE_PROP "node"
#define VIRTIO_MEM_SIZE_PROP "size"
@@ -117,8 +121,15 @@ struct VirtIOMEM {
/* listeners to notify on plug/unplug activity. */
QLIST_HEAD(, RamDiscardListener) rdl_list;
- /* State of the resettable container */
+ /* Catch system resets -> qemu_devices_reset() only. */
+ VirtioMemSystemReset *system_reset;
+};
+
+struct VirtioMemSystemReset {
+ Object parent;
+
ResettableState reset_state;
+ VirtIOMEM *vmem;
};
struct VirtIOMEMClass {
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 02/15] s390x/s390-virtio-ccw: don't crash on weird RAM sizes
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
2024-12-18 10:52 ` [PULL 01/15] virtio-mem: unplug memory only during system resets, not device resets David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 03/15] s390x/s390-virtio-hcall: remove hypercall registration mechanism David Hildenbrand
` (13 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand,
Janosch Frank
KVM is not happy when starting a VM with weird RAM sizes:
# qemu-system-s390x --enable-kvm --nographic -m 1234K
qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
failed, slot=0, start=0x0, size=0x244000: Invalid argument
kvm_set_phys_mem: error registering slot: Invalid argument
Aborted (core dumped)
Let's handle that in a better way by rejecting such weird RAM sizes
right from the start:
# qemu-system-s390x --enable-kvm --nographic -m 1234K
qemu-system-s390x: ram size must be multiples of 1 MiB
Message-ID: <20241008105455.2302628-2-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 67ae34aead..f2a17ecace 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
{
MemoryRegion *sysmem = get_system_memory();
+ if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
+ /*
+ * SCLP cannot possibly expose smaller granularity right now and KVM
+ * cannot handle smaller granularity. As we don't support NUMA, the
+ * region size directly corresponds to machine->ram_size, and the region
+ * is a single RAM memory region.
+ */
+ error_report("ram size must be multiples of 1 MiB");
+ exit(EXIT_FAILURE);
+ }
+
/* allocate RAM for core */
memory_region_add_subregion(sysmem, 0, ram);
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 03/15] s390x/s390-virtio-hcall: remove hypercall registration mechanism
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
2024-12-18 10:52 ` [PULL 01/15] virtio-mem: unplug memory only during system resets, not device resets David Hildenbrand
2024-12-18 10:52 ` [PULL 02/15] s390x/s390-virtio-ccw: don't crash on weird RAM sizes David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 04/15] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls David Hildenbrand
` (12 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Nowadays, we only have a single machine type in QEMU, everything is based
on virtio-ccw and the traditional virtio machine does no longer exist. No
need to dynamically register diag500 handlers. Move the two existing
handlers into s390-virtio-hcall.c.
Message-ID: <20241008105455.2302628-3-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 58 --------------------------------
hw/s390x/s390-virtio-hcall.c | 65 +++++++++++++++++++++++++++---------
hw/s390x/s390-virtio-hcall.h | 2 --
3 files changed, 49 insertions(+), 76 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f2a17ecace..b0edaa0872 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -16,11 +16,8 @@
#include "exec/ram_addr.h"
#include "exec/confidential-guest-support.h"
#include "hw/boards.h"
-#include "hw/s390x/s390-virtio-hcall.h"
#include "hw/s390x/sclp.h"
#include "hw/s390x/s390_flic.h"
-#include "hw/s390x/ioinst.h"
-#include "hw/s390x/css.h"
#include "virtio-ccw.h"
#include "qemu/config-file.h"
#include "qemu/ctype.h"
@@ -124,58 +121,6 @@ static void subsystem_reset(void)
}
}
-static int virtio_ccw_hcall_notify(const uint64_t *args)
-{
- uint64_t subch_id = args[0];
- uint64_t data = args[1];
- SubchDev *sch;
- VirtIODevice *vdev;
- int cssid, ssid, schid, m;
- uint16_t vq_idx = data;
-
- if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
- return -EINVAL;
- }
- sch = css_find_subch(m, cssid, ssid, schid);
- if (!sch || !css_subch_visible(sch)) {
- return -EINVAL;
- }
-
- vdev = virtio_ccw_get_vdev(sch);
- if (vq_idx >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, vq_idx)) {
- return -EINVAL;
- }
-
- if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
- virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev, vq_idx),
- (data >> 16) & 0xFFFF);
- }
-
- virtio_queue_notify(vdev, vq_idx);
- return 0;
-}
-
-static int virtio_ccw_hcall_early_printk(const uint64_t *args)
-{
- uint64_t mem = args[0];
- MachineState *ms = MACHINE(qdev_get_machine());
-
- if (mem < ms->ram_size) {
- /* Early printk */
- return 0;
- }
- return -EINVAL;
-}
-
-static void virtio_ccw_register_hcalls(void)
-{
- s390_register_virtio_hypercall(KVM_S390_VIRTIO_CCW_NOTIFY,
- virtio_ccw_hcall_notify);
- /* Tolerate early printk. */
- s390_register_virtio_hypercall(KVM_S390_VIRTIO_NOTIFY,
- virtio_ccw_hcall_early_printk);
-}
-
static void s390_memory_init(MemoryRegion *ram)
{
MemoryRegion *sysmem = get_system_memory();
@@ -296,9 +241,6 @@ static void ccw_init(MachineState *machine)
OBJECT(dev));
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
- /* register hypercalls */
- virtio_ccw_register_hcalls();
-
s390_enable_css_support(s390_cpu_addr2state(0));
ret = css_create_css_image(VIRTUAL_CSSID, true);
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index ec7cf8beb3..ca49e3cd22 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -11,31 +11,64 @@
#include "qemu/osdep.h"
#include "cpu.h"
+#include "hw/boards.h"
#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/ioinst.h"
+#include "hw/s390x/css.h"
+#include "virtio-ccw.h"
-#define MAX_DIAG_SUBCODES 255
+static int handle_virtio_notify(uint64_t mem)
+{
+ MachineState *ms = MACHINE(qdev_get_machine());
-static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
+ if (mem < ms->ram_size) {
+ /* Early printk */
+ return 0;
+ }
+ return -EINVAL;
+}
-void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
+static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
{
- assert(code < MAX_DIAG_SUBCODES);
- assert(!s390_diag500_table[code]);
+ SubchDev *sch;
+ VirtIODevice *vdev;
+ int cssid, ssid, schid, m;
+ uint16_t vq_idx = data;
+
+ if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
+ return -EINVAL;
+ }
+ sch = css_find_subch(m, cssid, ssid, schid);
+ if (!sch || !css_subch_visible(sch)) {
+ return -EINVAL;
+ }
- s390_diag500_table[code] = fn;
+ vdev = virtio_ccw_get_vdev(sch);
+ if (vq_idx >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, vq_idx)) {
+ return -EINVAL;
+ }
+
+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+ virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev, vq_idx),
+ (data >> 16) & 0xFFFF);
+ }
+
+ virtio_queue_notify(vdev, vq_idx);
+ return 0;
}
int s390_virtio_hypercall(CPUS390XState *env)
{
- s390_virtio_fn fn;
-
- if (env->regs[1] < MAX_DIAG_SUBCODES) {
- fn = s390_diag500_table[env->regs[1]];
- if (fn) {
- env->regs[2] = fn(&env->regs[2]);
- return 0;
- }
- }
+ const uint64_t subcode = env->regs[1];
- return -EINVAL;
+ switch (subcode) {
+ case KVM_S390_VIRTIO_NOTIFY:
+ env->regs[2] = handle_virtio_notify(env->regs[2]);
+ return 0;
+ case KVM_S390_VIRTIO_CCW_NOTIFY:
+ env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
+ return 0;
+ default:
+ return -EINVAL;
+ }
}
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 3ae6d6ae3a..3d9fe147d2 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -18,8 +18,6 @@
/* The only thing that we need from the old kvm_virtio.h file */
#define KVM_S390_VIRTIO_NOTIFY 0
-typedef int (*s390_virtio_fn)(const uint64_t *args);
-void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
int s390_virtio_hypercall(CPUS390XState *env);
#endif /* HW_S390_VIRTIO_HCALL_H */
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 04/15] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (2 preceding siblings ...)
2024-12-18 10:52 ` [PULL 03/15] s390x/s390-virtio-hcall: remove hypercall registration mechanism David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 05/15] s390x: rename s390-virtio-hcall* to s390-hypercall* David Hildenbrand
` (11 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's generalize, abstracting the virtio bits. diag500 is now a generic
hypercall to handle QEMU/KVM specific things. Explicitly specify all
already defined subcodes, including legacy ones (so we know what we can
use for new hypercalls).
Move the PGM_SPECIFICATION injection into the renamed function
handle_diag_500(), so we can turn it into a void function.
We'll rename the files separately, so git properly detects the rename.
Message-ID: <20241008105455.2302628-4-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-hcall.c | 15 ++++++++-------
hw/s390x/s390-virtio-hcall.h | 11 ++++++-----
target/s390x/kvm/kvm.c | 16 +---------------
target/s390x/tcg/misc_helper.c | 5 +++--
4 files changed, 18 insertions(+), 29 deletions(-)
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index ca49e3cd22..5fb78a719e 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -1,5 +1,5 @@
/*
- * Support for virtio hypercalls on s390
+ * Support for QEMU/KVM hypercalls on s390
*
* Copyright 2012 IBM Corp.
* Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -57,18 +57,19 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
return 0;
}
-int s390_virtio_hypercall(CPUS390XState *env)
+void handle_diag_500(S390CPU *cpu, uintptr_t ra)
{
+ CPUS390XState *env = &cpu->env;
const uint64_t subcode = env->regs[1];
switch (subcode) {
- case KVM_S390_VIRTIO_NOTIFY:
+ case DIAG500_VIRTIO_NOTIFY:
env->regs[2] = handle_virtio_notify(env->regs[2]);
- return 0;
- case KVM_S390_VIRTIO_CCW_NOTIFY:
+ break;
+ case DIAG500_VIRTIO_CCW_NOTIFY:
env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
- return 0;
+ break;
default:
- return -EINVAL;
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
}
}
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 3d9fe147d2..dca456b926 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -1,5 +1,5 @@
/*
- * Support for virtio hypercalls on s390x
+ * Support for QEMU/KVM hypercalls on s390x
*
* Copyright IBM Corp. 2012, 2017
* Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -12,12 +12,13 @@
#ifndef HW_S390_VIRTIO_HCALL_H
#define HW_S390_VIRTIO_HCALL_H
-#include "standard-headers/asm-s390/virtio-ccw.h"
#include "cpu.h"
-/* The only thing that we need from the old kvm_virtio.h file */
-#define KVM_S390_VIRTIO_NOTIFY 0
+#define DIAG500_VIRTIO_NOTIFY 0 /* legacy, implemented as a NOP */
+#define DIAG500_VIRTIO_RESET 1 /* legacy */
+#define DIAG500_VIRTIO_SET_STATUS 2 /* legacy */
+#define DIAG500_VIRTIO_CCW_NOTIFY 3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
-int s390_virtio_hypercall(CPUS390XState *env);
+void handle_diag_500(S390CPU *cpu, uintptr_t ra);
#endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index dd0322c43a..9fbb7db163 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -1491,20 +1491,6 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
return r;
}
-static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
-{
- CPUS390XState *env = &cpu->env;
- int ret;
-
- ret = s390_virtio_hypercall(env);
- if (ret == -EINVAL) {
- kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
- return 0;
- }
-
- return ret;
-}
-
static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
{
uint64_t r1, r3;
@@ -1601,7 +1587,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
handle_diag_318(cpu, run);
break;
case DIAG_KVM_HYPERCALL:
- r = handle_hypercall(cpu, run);
+ handle_diag_500(cpu, RA_IGNORED);
break;
case DIAG_KVM_BREAKPOINT:
r = handle_sw_breakpoint(cpu, run);
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 303f86d363..0ab2bf36cf 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -117,10 +117,11 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
switch (num) {
case 0x500:
- /* KVM hypercall */
+ /* QEMU/KVM hypercall */
bql_lock();
- r = s390_virtio_hypercall(env);
+ handle_diag_500(env_archcpu(env), GETPC());
bql_unlock();
+ r = 0;
break;
case 0x44:
/* yield */
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 05/15] s390x: rename s390-virtio-hcall* to s390-hypercall*
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (3 preceding siblings ...)
2024-12-18 10:52 ` [PULL 04/15] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 06/15] s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code David Hildenbrand
` (10 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's make it clearer that we are talking about general
QEMU/KVM-specific hypercalls.
Message-ID: <20241008105455.2302628-5-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/meson.build | 2 +-
hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} | 2 +-
hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} | 6 +++---
target/s390x/kvm/kvm.c | 2 +-
target/s390x/tcg/misc_helper.c | 2 +-
5 files changed, 7 insertions(+), 7 deletions(-)
rename hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} (97%)
rename hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} (86%)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 482fd13420..71ec747f4c 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -12,7 +12,7 @@ s390x_ss.add(files(
's390-pci-inst.c',
's390-skeys.c',
's390-stattrib.c',
- 's390-virtio-hcall.c',
+ 's390-hypercall.c',
'sclp.c',
'sclpcpu.c',
'sclpquiesce.c',
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-hypercall.c
similarity index 97%
rename from hw/s390x/s390-virtio-hcall.c
rename to hw/s390x/s390-hypercall.c
index 5fb78a719e..f816c2b1ef 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -12,7 +12,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "hw/boards.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
#include "hw/s390x/ioinst.h"
#include "hw/s390x/css.h"
#include "virtio-ccw.h"
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-hypercall.h
similarity index 86%
rename from hw/s390x/s390-virtio-hcall.h
rename to hw/s390x/s390-hypercall.h
index dca456b926..2fa81dbfdd 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-hypercall.h
@@ -9,8 +9,8 @@
* directory.
*/
-#ifndef HW_S390_VIRTIO_HCALL_H
-#define HW_S390_VIRTIO_HCALL_H
+#ifndef HW_S390_HYPERCALL_H
+#define HW_S390_HYPERCALL_H
#include "cpu.h"
@@ -21,4 +21,4 @@
void handle_diag_500(S390CPU *cpu, uintptr_t ra);
-#endif /* HW_S390_VIRTIO_HCALL_H */
+#endif /* HW_S390_HYPERCALL_H */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 9fbb7db163..7e4bce5337 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -49,7 +49,7 @@
#include "hw/s390x/ebcdic.h"
#include "exec/memattrs.h"
#include "hw/s390x/s390-virtio-ccw.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
#include "target/s390x/kvm/pv.h"
#define kvm_vm_check_mem_attr(s, attr) \
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 0ab2bf36cf..3732d79185 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -36,7 +36,7 @@
#include "sysemu/cpus.h"
#include "sysemu/sysemu.h"
#include "hw/s390x/ebcdic.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
#include "hw/s390x/sclp.h"
#include "hw/s390x/s390_flic.h"
#include "hw/s390x/ioinst.h"
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 06/15] s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (4 preceding siblings ...)
2024-12-18 10:52 ` [PULL 05/15] s390x: rename s390-virtio-hcall* to s390-hypercall* David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 07/15] s390x: introduce s390_get_memory_limit() David Hildenbrand
` (9 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Nowadays, it feels more natural to have that code located in
s390_memory_init(), where we also have direct access to the machine
object.
While at it, use the actual RAM size, not the maximum RAM size which
cannot currently be reached without support for any memory devices.
Consequently update s390_pv_vm_try_disable_async() to rely on the RAM size
as well, to avoid temporary issues while we further rework that
handling.
set_memory_limit() is temporary, we'll merge it with
s390_set_memory_limit() next.
Message-ID: <20241008105455.2302628-6-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++----
hw/s390x/sclp.c | 11 -----------
target/s390x/kvm/pv.c | 2 +-
3 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b0edaa0872..a28e615c5a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -121,11 +121,29 @@ static void subsystem_reset(void)
}
}
-static void s390_memory_init(MemoryRegion *ram)
+static void set_memory_limit(uint64_t new_limit)
+{
+ uint64_t hw_limit;
+ int ret;
+
+ ret = s390_set_memory_limit(new_limit, &hw_limit);
+ if (ret == -E2BIG) {
+ error_report("host supports a maximum of %" PRIu64 " GB",
+ hw_limit / GiB);
+ exit(EXIT_FAILURE);
+ } else if (ret) {
+ error_report("setting the guest size failed");
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void s390_memory_init(MachineState *machine)
{
MemoryRegion *sysmem = get_system_memory();
+ MemoryRegion *ram = machine->ram;
+ uint64_t ram_size = memory_region_size(ram);
- if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
+ if (!QEMU_IS_ALIGNED(ram_size, 1 * MiB)) {
/*
* SCLP cannot possibly expose smaller granularity right now and KVM
* cannot handle smaller granularity. As we don't support NUMA, the
@@ -136,7 +154,9 @@ static void s390_memory_init(MemoryRegion *ram)
exit(EXIT_FAILURE);
}
- /* allocate RAM for core */
+ set_memory_limit(ram_size);
+
+ /* Map the initial memory. Must happen after setting the memory limit. */
memory_region_add_subregion(sysmem, 0, ram);
/*
@@ -211,7 +231,7 @@ static void ccw_init(MachineState *machine)
qdev_realize_and_unref(DEVICE(ms->sclp), NULL, &error_fatal);
/* init memory + setup max page size. Required for the CPU model */
- s390_memory_init(machine->ram);
+ s390_memory_init(machine);
/* init CPUs (incl. CPU model) early so s390_has_feature() works */
s390_init_cpus(machine);
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 8757626b5c..73e88ab4eb 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -376,10 +376,7 @@ void sclp_service_interrupt(uint32_t sccb)
/* qemu object creation and initialization functions */
static void sclp_realize(DeviceState *dev, Error **errp)
{
- MachineState *machine = MACHINE(qdev_get_machine());
SCLPDevice *sclp = SCLP(dev);
- uint64_t hw_limit;
- int ret;
/*
* qdev_device_add searches the sysbus for TYPE_SCLP_EVENTS_BUS. As long
@@ -389,14 +386,6 @@ static void sclp_realize(DeviceState *dev, Error **errp)
if (!sysbus_realize(SYS_BUS_DEVICE(sclp->event_facility), errp)) {
return;
}
-
- ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
- if (ret == -E2BIG) {
- error_setg(errp, "host supports a maximum of %" PRIu64 " GB",
- hw_limit / GiB);
- } else if (ret) {
- error_setg(errp, "setting the guest size failed");
- }
}
static void sclp_memory_init(SCLPDevice *sclp)
diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index dde836d21a..424cce75ca 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
* If the feature is not present or if the VM is not larger than 2 GiB,
* KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
*/
- if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
+ if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
return false;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 07/15] s390x: introduce s390_get_memory_limit()
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (5 preceding siblings ...)
2024-12-18 10:52 ` [PULL 06/15] s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 08/15] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT David Hildenbrand
` (8 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's add s390_get_memory_limit(), to query what has been successfully
set via s390_set_memory_limit(). Allow setting the limit only once.
We'll remember the limit in the machine state. Move
s390_set_memory_limit() to machine code, merging it into
set_memory_limit(), because this really is a machine property.
Message-ID: <20241008105455.2302628-7-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 22 ++++++++++++++++++----
include/hw/s390x/s390-virtio-ccw.h | 3 +++
target/s390x/cpu-sysemu.c | 8 --------
target/s390x/cpu.h | 1 -
4 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index a28e615c5a..5a91e334f3 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -45,6 +45,7 @@
#include "migration/blocker.h"
#include "qapi/visitor.h"
#include "hw/s390x/cpu-topology.h"
+#include "kvm/kvm_s390x.h"
#include CONFIG_DEVICES
static Error *pv_mig_blocker;
@@ -121,12 +122,16 @@ static void subsystem_reset(void)
}
}
-static void set_memory_limit(uint64_t new_limit)
+static void s390_set_memory_limit(S390CcwMachineState *s390ms,
+ uint64_t new_limit)
{
uint64_t hw_limit;
- int ret;
+ int ret = 0;
- ret = s390_set_memory_limit(new_limit, &hw_limit);
+ assert(!s390ms->memory_limit && new_limit);
+ if (kvm_enabled()) {
+ ret = kvm_s390_set_mem_limit(new_limit, &hw_limit);
+ }
if (ret == -E2BIG) {
error_report("host supports a maximum of %" PRIu64 " GB",
hw_limit / GiB);
@@ -135,10 +140,19 @@ static void set_memory_limit(uint64_t new_limit)
error_report("setting the guest size failed");
exit(EXIT_FAILURE);
}
+ s390ms->memory_limit = new_limit;
+}
+
+uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
+{
+ /* We expect to be called only after the limit was set. */
+ assert(s390ms->memory_limit);
+ return s390ms->memory_limit;
}
static void s390_memory_init(MachineState *machine)
{
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(machine);
MemoryRegion *sysmem = get_system_memory();
MemoryRegion *ram = machine->ram;
uint64_t ram_size = memory_region_size(ram);
@@ -154,7 +168,7 @@ static void s390_memory_init(MachineState *machine)
exit(EXIT_FAILURE);
}
- set_memory_limit(ram_size);
+ s390_set_memory_limit(s390ms, ram_size);
/* Map the initial memory. Must happen after setting the memory limit. */
memory_region_add_subregion(sysmem, 0, ram);
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 996864a34e..eb04542979 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -29,10 +29,13 @@ struct S390CcwMachineState {
bool dea_key_wrap;
bool pv;
uint8_t loadparm[8];
+ uint64_t memory_limit;
SCLPDevice *sclp;
};
+uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
+
#define S390_PTF_REASON_NONE (0x00 << 8)
#define S390_PTF_REASON_DONE (0x01 << 8)
#define S390_PTF_REASON_BUSY (0x02 << 8)
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 1cd30c1d84..3118a25fee 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -255,14 +255,6 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
return s390_count_running_cpus();
}
-int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
-{
- if (kvm_enabled()) {
- return kvm_s390_set_mem_limit(new_limit, hw_limit);
- }
- return 0;
-}
-
void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
{
if (kvm_enabled()) {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 5ef61b1f75..b4506539f0 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -881,7 +881,6 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
/* cpu.c */
void s390_crypto_reset(void);
-int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
void s390_cmma_reset(void);
void s390_enable_css_support(S390CPU *cpu);
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 08/15] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (6 preceding siblings ...)
2024-12-18 10:52 ` [PULL 07/15] s390x: introduce s390_get_memory_limit() David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 09/15] s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts David Hildenbrand
` (7 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
A guest OS that supports memory hotplug / memory devices must during
boot be aware of the maximum possible physical memory address that it might
have to handle at a later stage during its runtime.
For example, the maximum possible memory address might be required to
prepare the kernel virtual address space accordingly (e.g., select page
table hierarchy depth).
On s390x there is currently no such mechanism that is compatible with
paravirtualized memory devices, because the whole SCLP interface was
designed around the idea of "storage increments" and "standby memory".
Paravirtualized memory devices we want to support, such as virtio-mem, have
no intersection with any of that, but could co-exist with them in the
future if ever needed.
In particular, a guest OS must never detect and use device memory
without the help of a proper device driver. Device memory must not be
exposed in any firmware-provided memory map (SCLP or diag260 on s390x).
For this reason, these memory devices will be places in memory *above*
the "maximum storage increment" exposed via SCLP.
Let's provide a new diag500 subcode to query the memory limit determined in
s390_memory_init().
Message-ID: <20241008105455.2302628-8-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-hypercall.c | 12 +++++++++++-
hw/s390x/s390-hypercall.h | 1 +
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/s390-hypercall.c b/hw/s390x/s390-hypercall.c
index f816c2b1ef..ac1b08b2cd 100644
--- a/hw/s390x/s390-hypercall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -11,7 +11,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
-#include "hw/boards.h"
+#include "hw/s390x/s390-virtio-ccw.h"
#include "hw/s390x/s390-hypercall.h"
#include "hw/s390x/ioinst.h"
#include "hw/s390x/css.h"
@@ -57,6 +57,13 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
return 0;
}
+static uint64_t handle_storage_limit(void)
+{
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(qdev_get_machine());
+
+ return s390_get_memory_limit(s390ms) - 1;
+}
+
void handle_diag_500(S390CPU *cpu, uintptr_t ra)
{
CPUS390XState *env = &cpu->env;
@@ -69,6 +76,9 @@ void handle_diag_500(S390CPU *cpu, uintptr_t ra)
case DIAG500_VIRTIO_CCW_NOTIFY:
env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
break;
+ case DIAG500_STORAGE_LIMIT:
+ env->regs[2] = handle_storage_limit();
+ break;
default:
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
}
diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
index 2fa81dbfdd..4f07209128 100644
--- a/hw/s390x/s390-hypercall.h
+++ b/hw/s390x/s390-hypercall.h
@@ -18,6 +18,7 @@
#define DIAG500_VIRTIO_RESET 1 /* legacy */
#define DIAG500_VIRTIO_SET_STATUS 2 /* legacy */
#define DIAG500_VIRTIO_CCW_NOTIFY 3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
+#define DIAG500_STORAGE_LIMIT 4
void handle_diag_500(S390CPU *cpu, uintptr_t ra);
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 09/15] s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (7 preceding siblings ...)
2024-12-18 10:52 ` [PULL 08/15] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 10/15] s390x/s390-skeys: prepare for memory devices David Hildenbrand
` (6 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
With memory devices, we will have storage attributes for memory that
exceeds the initial ram size. Further, we can easily have memory holes,
for which there (currently) are no storage attributes.
In particular, with memory holes, KVM_S390_SET_CMMA_BITS will fail to set
some storage attributes.
So let's do it like we handle storage keys migration, relying on
guest_phys_blocks_append(). However, in contrast to storage key
migration, we will handle it on the migration destination.
This is a preparation for virtio-mem support. Note that ever since the
"early migration" feature was added (x-early-migration), the state
of device blocks (plugged/unplugged) is migrated early such that
guest_phys_blocks_append() will properly consider all currently plugged
memory blocks and skip any unplugged ones.
In the future, we should try getting rid of the large temporary buffer
and also not send any attributes for any memory holes, just so they
get ignored on the destination.
Message-ID: <20241008105455.2302628-9-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-stattrib-kvm.c | 67 +++++++++++++++++++++++-------------
1 file changed, 43 insertions(+), 24 deletions(-)
diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index eeaa811098..33ec91422a 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -10,11 +10,12 @@
*/
#include "qemu/osdep.h"
-#include "hw/boards.h"
+#include "hw/s390x/s390-virtio-ccw.h"
#include "migration/qemu-file.h"
#include "hw/s390x/storage-attributes.h"
#include "qemu/error-report.h"
#include "sysemu/kvm.h"
+#include "sysemu/memory_mapping.h"
#include "exec/ram_addr.h"
#include "kvm/kvm_s390x.h"
#include "qapi/error.h"
@@ -84,8 +85,8 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
uint8_t *values)
{
KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
- MachineState *machine = MACHINE(qdev_get_machine());
- unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(qdev_get_machine());
+ unsigned long max = s390_get_memory_limit(s390ms) / TARGET_PAGE_SIZE;
if (start_gfn + count > max) {
error_report("Out of memory bounds when setting storage attributes");
@@ -103,39 +104,57 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
{
KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
- MachineState *machine = MACHINE(qdev_get_machine());
- unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
- /* We do not need to reach the maximum buffer size allowed */
- unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(qdev_get_machine());
+ unsigned long max = s390_get_memory_limit(s390ms) / TARGET_PAGE_SIZE;
+ unsigned long start_gfn, end_gfn, pages;
+ GuestPhysBlockList guest_phys_blocks;
+ GuestPhysBlock *block;
int r;
struct kvm_s390_cmma_log clog = {
.flags = 0,
.mask = ~0ULL,
};
- if (sas->incoming_buffer) {
- for (cx = 0; cx + len <= max; cx += len) {
- clog.start_gfn = cx;
- clog.count = len;
- clog.values = (uint64_t)(sas->incoming_buffer + cx);
- r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
- if (r) {
- error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
- return;
- }
- }
- if (cx < max) {
- clog.start_gfn = cx;
- clog.count = max - cx;
- clog.values = (uint64_t)(sas->incoming_buffer + cx);
+ if (!sas->incoming_buffer) {
+ return;
+ }
+ guest_phys_blocks_init(&guest_phys_blocks);
+ guest_phys_blocks_append(&guest_phys_blocks);
+
+ QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+ assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+ assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
+
+ start_gfn = block->target_start / TARGET_PAGE_SIZE;
+ end_gfn = block->target_end / TARGET_PAGE_SIZE;
+
+ while (start_gfn < end_gfn) {
+ /* Don't exceed the maximum buffer size. */
+ pages = MIN(end_gfn - start_gfn, KVM_S390_SKEYS_MAX / 2);
+
+ /*
+ * If we ever get guest physical memory beyond the configured
+ * memory limit, something went very wrong.
+ */
+ assert(start_gfn + pages <= max);
+
+ clog.start_gfn = start_gfn;
+ clog.count = pages;
+ clog.values = (uint64_t)(sas->incoming_buffer + start_gfn);
r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
if (r) {
error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
+ goto out;
}
+
+ start_gfn += pages;
}
- g_free(sas->incoming_buffer);
- sas->incoming_buffer = NULL;
}
+
+out:
+ guest_phys_blocks_free(&guest_phys_blocks);
+ g_free(sas->incoming_buffer);
+ sas->incoming_buffer = NULL;
}
static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val,
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 10/15] s390x/s390-skeys: prepare for memory devices
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (8 preceding siblings ...)
2024-12-18 10:52 ` [PULL 09/15] s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:52 ` [PULL 11/15] s390x/s390-virtio-ccw: " David Hildenbrand
` (5 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
With memory devices, we will have storage keys for memory that
exceeds the initial ram size.
The TODO already states that current handling is subopimal,
but we won't worry about improving that (TCG-only) thing for now.
Message-ID: <20241008105455.2302628-10-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-skeys.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 6d0a47ed73..6ea4d8c20e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -11,7 +11,7 @@
#include "qemu/osdep.h"
#include "qemu/units.h"
-#include "hw/boards.h"
+#include "hw/s390x/s390-virtio-ccw.h"
#include "hw/qdev-properties.h"
#include "hw/s390x/storage-keys.h"
#include "qapi/error.h"
@@ -251,9 +251,9 @@ static bool qemu_s390_enable_skeys(S390SKeysState *ss)
* g_once_init_enter() is good enough.
*/
if (g_once_init_enter(&initialized)) {
- MachineState *machine = MACHINE(qdev_get_machine());
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(qdev_get_machine());
- skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
+ skeys->key_count = s390_get_memory_limit(s390ms) / TARGET_PAGE_SIZE;
skeys->keydata = g_malloc0(skeys->key_count);
g_once_init_leave(&initialized, 1);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 11/15] s390x/s390-virtio-ccw: prepare for memory devices
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (9 preceding siblings ...)
2024-12-18 10:52 ` [PULL 10/15] s390x/s390-skeys: prepare for memory devices David Hildenbrand
@ 2024-12-18 10:52 ` David Hildenbrand
2024-12-18 10:53 ` [PULL 12/15] s390x/pv: " David Hildenbrand
` (4 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:52 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's prepare our address space for memory devices if enabled via
"maxmem" and if we have CONFIG_MEM_DEVICE enabled at all. Note that
CONFIG_MEM_DEVICE will be selected automatically once we add support
for devices.
Just like on other architectures, the region container for memory devices
is placed directly above our initial memory. For now, we only align the
start address of the region up to 1 GiB, but we won't add any additional
space to the region for internal alignment purposes; this can be done in
the future if really required.
The RAM size returned via SCLP is not modified, as this only
covers initial RAM (and standby memory we don't implement) and not memory
devices; clarify that in the docs of read_SCP_info(). Existing OSes without
support for memory devices will keep working as is, even when memory
devices would be attached the VM.
Guest OSs which support memory devices, such as virtio-mem, will
consult diag500(), to find out the maximum possible pfn. Guest OSes that
don't support memory devices, don't have to be changed and will continue
relying on information provided by SCLP.
There are no remaining maxram_size users in s390x code, and the remaining
ram_size users only care about initial RAM:
* hw/s390x/ipl.c
* hw/s390x/s390-hypercall.c
* hw/s390x/sclp.c
* target/s390x/kvm/pv.c
Message-ID: <20241008105455.2302628-11-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 23 ++++++++++++++++++++++-
hw/s390x/sclp.c | 6 +++++-
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5a91e334f3..aa06d07835 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -156,6 +156,7 @@ static void s390_memory_init(MachineState *machine)
MemoryRegion *sysmem = get_system_memory();
MemoryRegion *ram = machine->ram;
uint64_t ram_size = memory_region_size(ram);
+ uint64_t devmem_base, devmem_size;
if (!QEMU_IS_ALIGNED(ram_size, 1 * MiB)) {
/*
@@ -168,11 +169,31 @@ static void s390_memory_init(MachineState *machine)
exit(EXIT_FAILURE);
}
- s390_set_memory_limit(s390ms, ram_size);
+ devmem_size = 0;
+ devmem_base = ram_size;
+#ifdef CONFIG_MEM_DEVICE
+ if (machine->ram_size < machine->maxram_size) {
+
+ /*
+ * Make sure memory devices have a sane default alignment, even
+ * when weird initial memory sizes are specified.
+ */
+ devmem_base = QEMU_ALIGN_UP(devmem_base, 1 * GiB);
+ devmem_size = machine->maxram_size - machine->ram_size;
+ }
+#endif
+ s390_set_memory_limit(s390ms, devmem_base + devmem_size);
/* Map the initial memory. Must happen after setting the memory limit. */
memory_region_add_subregion(sysmem, 0, ram);
+ /* Initialize address space for memory devices. */
+#ifdef CONFIG_MEM_DEVICE
+ if (devmem_size) {
+ machine_memory_devices_init(machine, devmem_base, devmem_size);
+ }
+#endif /* CONFIG_MEM_DEVICE */
+
/*
* Configure the maximum page size. As no memory devices were created
* yet, this is the page size of initial memory only.
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 73e88ab4eb..5945c9b1d8 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -161,7 +161,11 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
read_info->rnsize2 = cpu_to_be32(rnsize);
}
- /* we don't support standby memory, maxram_size is never exposed */
+ /*
+ * We don't support standby memory. maxram_size is used for sizing the
+ * memory device region, which is not exposed through SCLP but through
+ * diag500.
+ */
rnmax = machine->ram_size >> sclp->increment_size;
if (rnmax < 0x10000) {
read_info->rnmax = cpu_to_be16(rnmax);
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 12/15] s390x/pv: prepare for memory devices
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (10 preceding siblings ...)
2024-12-18 10:52 ` [PULL 11/15] s390x/s390-virtio-ccw: " David Hildenbrand
@ 2024-12-18 10:53 ` David Hildenbrand
2024-12-18 10:53 ` [PULL 13/15] s390x: remember the maximum page size David Hildenbrand
` (3 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:53 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand,
Nina Schoetterl-Glausch
Let's avoid checking for the maxram_size, and instead rely on the memory
limit determined in s390_memory_init(), that might be larger than
maxram_size, for example due to alignment purposes.
This check now correctly mimics what the kernel will check in
kvm_s390_pv_set_aside(), whereby a VM <= 2 GiB VM would end up using
a segment type ASCE.
Message-ID: <20241008105455.2302628-12-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
target/s390x/kvm/pv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 424cce75ca..fa66607e7b 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
* If the feature is not present or if the VM is not larger than 2 GiB,
* KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
*/
- if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
+ if (s390_get_memory_limit(ms) <= 2 * GiB ||
!kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
return false;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 13/15] s390x: remember the maximum page size
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (11 preceding siblings ...)
2024-12-18 10:53 ` [PULL 12/15] s390x/pv: " David Hildenbrand
@ 2024-12-18 10:53 ` David Hildenbrand
2024-12-18 10:53 ` [PULL 14/15] s390x/virtio-ccw: add support for virtio based memory devices David Hildenbrand
` (2 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:53 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's remember the value (successfully) set via s390_set_max_pagesize().
This will be helpful to reject hotplugged memory devices that would exceed
this initially set page size.
Handle it just like how we handle s390_get_memory_limit(), storing it in
the machine, and moving the handling to machine code.
Message-ID: <20241008105455.2302628-13-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 12 +++++++++++-
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu-sysemu.c | 7 -------
target/s390x/cpu.h | 1 -
4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index aa06d07835..ef1bf32770 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -150,6 +150,16 @@ uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
return s390ms->memory_limit;
}
+static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
+ uint64_t pagesize)
+{
+ assert(!s390ms->max_pagesize && pagesize);
+ if (kvm_enabled()) {
+ kvm_s390_set_max_pagesize(pagesize, &error_fatal);
+ }
+ s390ms->max_pagesize = pagesize;
+}
+
static void s390_memory_init(MachineState *machine)
{
S390CcwMachineState *s390ms = S390_CCW_MACHINE(machine);
@@ -198,7 +208,7 @@ static void s390_memory_init(MachineState *machine)
* Configure the maximum page size. As no memory devices were created
* yet, this is the page size of initial memory only.
*/
- s390_set_max_pagesize(qemu_maxrampagesize(), &error_fatal);
+ s390_set_max_pagesize(s390ms, qemu_maxrampagesize());
/* Initialize storage key device */
s390_skeys_init();
/* Initialize storage attributes device */
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index eb04542979..5a730f5d07 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,7 @@ struct S390CcwMachineState {
bool pv;
uint8_t loadparm[8];
uint64_t memory_limit;
+ uint64_t max_pagesize;
SCLPDevice *sclp;
};
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 3118a25fee..706a5c53e2 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -255,13 +255,6 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
return s390_count_running_cpus();
}
-void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
-{
- if (kvm_enabled()) {
- kvm_s390_set_max_pagesize(pagesize, errp);
- }
-}
-
void s390_cmma_reset(void)
{
if (kvm_enabled()) {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index b4506539f0..5b7992deda 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -881,7 +881,6 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
/* cpu.c */
void s390_crypto_reset(void);
-void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
void s390_cmma_reset(void);
void s390_enable_css_support(S390CPU *cpu);
void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 14/15] s390x/virtio-ccw: add support for virtio based memory devices
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (12 preceding siblings ...)
2024-12-18 10:53 ` [PULL 13/15] s390x: remember the maximum page size David Hildenbrand
@ 2024-12-18 10:53 ` David Hildenbrand
2024-12-18 10:53 ` [PULL 15/15] s390x: virtio-mem support David Hildenbrand
2024-12-18 21:09 ` [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 Stefan Hajnoczi
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:53 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's implement support for abstract virtio based memory devices, using
the virtio-pci implementation as an orientation. Wire them up in the
machine hotplug handler, taking care of s390x page size limitations.
As we neither support virtio-mem or virtio-pmem yet, the code is
effectively unused. We'll implement support for virtio-mem based on this
next.
Note that we won't wire up the virtio-pci variant (should currently be
impossible due to lack of support for MSI-X), but we'll add a safety net
to reject plugging them in the pre-plug handler.
Message-ID: <20241008105455.2302628-14-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
MAINTAINERS | 3 +
hw/s390x/meson.build | 3 +
hw/s390x/s390-virtio-ccw.c | 47 +++++++++-
hw/s390x/virtio-ccw-md-stubs.c | 24 ++++++
hw/s390x/virtio-ccw-md.c | 153 +++++++++++++++++++++++++++++++++
hw/s390x/virtio-ccw-md.h | 44 ++++++++++
hw/virtio/Kconfig | 1 +
7 files changed, 274 insertions(+), 1 deletion(-)
create mode 100644 hw/s390x/virtio-ccw-md-stubs.c
create mode 100644 hw/s390x/virtio-ccw-md.c
create mode 100644 hw/s390x/virtio-ccw-md.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 822f34344b..f47fce0362 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2375,6 +2375,9 @@ F: include/hw/virtio/virtio-crypto.h
virtio based memory device
M: David Hildenbrand <david@redhat.com>
S: Supported
+F: hw/s390x/virtio-ccw-md.c
+F: hw/s390x/virtio-ccw-md.h
+F: hw/s390x/virtio-ccw-md-stubs.c
F: hw/virtio/virtio-md-pci.c
F: include/hw/virtio/virtio-md-pci.h
F: stubs/virtio-md-pci.c
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 71ec747f4c..28bbe4b06c 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -48,8 +48,11 @@ endif
virtio_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-ccw.c'))
virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'))
+virtio_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-ccw-md.c'))
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
+s390x_ss.add(when: 'CONFIG_VIRTIO_MD', if_false: files('virtio-ccw-md-stubs.c'))
+
hw_arch += {'s390x': s390x_ss}
hw_s390x_modules = {}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index ef1bf32770..248566f8dc 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -46,6 +46,8 @@
#include "qapi/visitor.h"
#include "hw/s390x/cpu-topology.h"
#include "kvm/kvm_s390x.h"
+#include "hw/virtio/virtio-md-pci.h"
+#include "hw/s390x/virtio-ccw-md.h"
#include CONFIG_DEVICES
static Error *pv_mig_blocker;
@@ -553,11 +555,39 @@ static void s390_machine_reset(MachineState *machine, ResetType type)
s390_ipl_clear_reset_request();
}
+static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+ virtio_ccw_md_pre_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
+ error_setg(errp,
+ "PCI-attached virtio based memory devices not supported");
+ }
+}
+
static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
+ S390CcwMachineState *s390ms = S390_CCW_MACHINE(hotplug_dev);
+
if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
s390_cpu_plug(hotplug_dev, dev, errp);
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+ /*
+ * At this point, the device is realized and set all memdevs mapped, so
+ * qemu_maxrampagesize() will pick up the page sizes of these memdevs
+ * as well. Before we plug the device and expose any RAM memory regions
+ * to the system, make sure we don't exceed the previously set max page
+ * size. While only relevant for KVM, there is not really any use case
+ * for this with TCG, so we'll unconditionally reject it.
+ */
+ if (qemu_maxrampagesize() != s390ms->max_pagesize) {
+ error_setg(errp, "Memory device uses a bigger page size than"
+ " initial memory");
+ return;
+ }
+ virtio_ccw_md_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
}
}
@@ -567,9 +597,20 @@ static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
error_setg(errp, "CPU hot unplug not supported on this machine");
return;
+ } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+ virtio_ccw_md_unplug_request(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev),
+ errp);
}
}
+static void s390_machine_device_unplug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+ virtio_ccw_md_unplug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
+ }
+ }
+
static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
unsigned cpu_index)
{
@@ -616,7 +657,9 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
DeviceState *dev)
{
- if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+ if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW) ||
+ object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
return HOTPLUG_HANDLER(machine);
}
return NULL;
@@ -776,8 +819,10 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
/* it is overridden with 'host' cpu *in kvm_arch_init* */
mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
+ hc->pre_plug = s390_machine_device_pre_plug;
hc->plug = s390_machine_device_plug;
hc->unplug_request = s390_machine_device_unplug_request;
+ hc->unplug = s390_machine_device_unplug;
nc->nmi_monitor_handler = s390_nmi;
mc->default_ram_id = "s390.ram";
mc->default_nic = "virtio-net-ccw";
diff --git a/hw/s390x/virtio-ccw-md-stubs.c b/hw/s390x/virtio-ccw-md-stubs.c
new file mode 100644
index 0000000000..e937865550
--- /dev/null
+++ b/hw/s390x/virtio-ccw-md-stubs.c
@@ -0,0 +1,24 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/s390x/virtio-ccw-md.h"
+
+void virtio_ccw_md_pre_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ error_setg(errp, "virtio based memory devices not supported");
+}
+
+void virtio_ccw_md_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ error_setg(errp, "virtio based memory devices not supported");
+}
+
+void virtio_ccw_md_unplug_request(VirtIOMDCcw *vmd, MachineState *ms,
+ Error **errp)
+{
+ error_setg(errp, "virtio based memory devices not supported");
+}
+
+void virtio_ccw_md_unplug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ error_setg(errp, "virtio based memory devices not supported");
+}
diff --git a/hw/s390x/virtio-ccw-md.c b/hw/s390x/virtio-ccw-md.c
new file mode 100644
index 0000000000..de333282df
--- /dev/null
+++ b/hw/s390x/virtio-ccw-md.c
@@ -0,0 +1,153 @@
+/*
+ * Virtio CCW support for abstract virtio based memory device
+ *
+ * Copyright (C) 2024 Red Hat, Inc.
+ *
+ * Authors:
+ * David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/s390x/virtio-ccw-md.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+
+void virtio_ccw_md_pre_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ DeviceState *dev = DEVICE(vmd);
+ HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+ MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+ Error *local_err = NULL;
+
+ if (!bus_handler && dev->hotplugged) {
+ /*
+ * Without a bus hotplug handler, we cannot control the plug/unplug
+ * order. We should never reach this point when hotplugging, but
+ * better add a safety net.
+ */
+ error_setg(errp, "hotplug of virtio based memory devices not supported"
+ " on this bus.");
+ return;
+ }
+
+ /*
+ * First, see if we can plug this memory device at all. If that
+ * succeeds, branch of to the actual hotplug handler.
+ */
+ memory_device_pre_plug(md, ms, &local_err);
+ if (!local_err && bus_handler) {
+ hotplug_handler_pre_plug(bus_handler, dev, &local_err);
+ }
+ error_propagate(errp, local_err);
+}
+
+void virtio_ccw_md_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ DeviceState *dev = DEVICE(vmd);
+ HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+ MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+ Error *local_err = NULL;
+
+ /*
+ * Plug the memory device first and then branch off to the actual
+ * hotplug handler. If that one fails, we can easily undo the memory
+ * device bits.
+ */
+ memory_device_plug(md, ms);
+ if (bus_handler) {
+ hotplug_handler_plug(bus_handler, dev, &local_err);
+ if (local_err) {
+ memory_device_unplug(md, ms);
+ }
+ }
+ error_propagate(errp, local_err);
+}
+
+void virtio_ccw_md_unplug_request(VirtIOMDCcw *vmd, MachineState *ms,
+ Error **errp)
+{
+ VirtIOMDCcwClass *vmdc = VIRTIO_MD_CCW_GET_CLASS(vmd);
+ DeviceState *dev = DEVICE(vmd);
+ HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+ HotplugHandlerClass *hdc;
+ Error *local_err = NULL;
+
+ if (!vmdc->unplug_request_check) {
+ error_setg(errp,
+ "this virtio based memory devices cannot be unplugged");
+ return;
+ }
+
+ if (!bus_handler) {
+ error_setg(errp, "hotunplug of virtio based memory devices not"
+ "supported on this bus");
+ return;
+ }
+
+ vmdc->unplug_request_check(vmd, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ /*
+ * Forward the async request or turn it into a sync request (handling it
+ * like qdev_unplug()).
+ */
+ hdc = HOTPLUG_HANDLER_GET_CLASS(bus_handler);
+ if (hdc->unplug_request) {
+ hotplug_handler_unplug_request(bus_handler, dev, &local_err);
+ } else {
+ virtio_ccw_md_unplug(vmd, ms, &local_err);
+ if (!local_err) {
+ object_unparent(OBJECT(dev));
+ }
+ }
+}
+
+void virtio_ccw_md_unplug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+ DeviceState *dev = DEVICE(vmd);
+ HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+ MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+ Error *local_err = NULL;
+
+ /* Unplug the memory device while it is still realized. */
+ memory_device_unplug(md, ms);
+
+ if (bus_handler) {
+ hotplug_handler_unplug(bus_handler, dev, &local_err);
+ if (local_err) {
+ /* Not expected to fail ... but still try to recover. */
+ memory_device_plug(md, ms);
+ error_propagate(errp, local_err);
+ return;
+ }
+ } else {
+ /* Very unexpected, but let's just try to do the right thing. */
+ warn_report("Unexpected unplug of virtio based memory device");
+ qdev_unrealize(dev);
+ }
+}
+
+static const TypeInfo virtio_ccw_md_info = {
+ .name = TYPE_VIRTIO_MD_CCW,
+ .parent = TYPE_VIRTIO_CCW_DEVICE,
+ .instance_size = sizeof(VirtIOMDCcw),
+ .class_size = sizeof(VirtIOMDCcwClass),
+ .abstract = true,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_MEMORY_DEVICE },
+ { }
+ },
+};
+
+static void virtio_ccw_md_register(void)
+{
+ type_register_static(&virtio_ccw_md_info);
+}
+type_init(virtio_ccw_md_register)
diff --git a/hw/s390x/virtio-ccw-md.h b/hw/s390x/virtio-ccw-md.h
new file mode 100644
index 0000000000..39ba864c92
--- /dev/null
+++ b/hw/s390x/virtio-ccw-md.h
@@ -0,0 +1,44 @@
+/*
+ * Virtio CCW support for abstract virtio based memory device
+ *
+ * Copyright (C) 2024 Red Hat, Inc.
+ *
+ * Authors:
+ * David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_S390X_VIRTIO_CCW_MD_H
+#define HW_S390X_VIRTIO_CCW_MD_H
+
+#include "virtio-ccw.h"
+#include "qom/object.h"
+
+/*
+ * virtio-md-ccw: This extends VirtioCcwDevice.
+ */
+#define TYPE_VIRTIO_MD_CCW "virtio-md-ccw"
+
+OBJECT_DECLARE_TYPE(VirtIOMDCcw, VirtIOMDCcwClass, VIRTIO_MD_CCW)
+
+struct VirtIOMDCcwClass {
+ /* private */
+ VirtIOCCWDeviceClass parent;
+
+ /* public */
+ void (*unplug_request_check)(VirtIOMDCcw *vmd, Error **errp);
+};
+
+struct VirtIOMDCcw {
+ VirtioCcwDevice parent_obj;
+};
+
+void virtio_ccw_md_pre_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+void virtio_ccw_md_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+void virtio_ccw_md_unplug_request(VirtIOMDCcw *vmd, MachineState *ms,
+ Error **errp);
+void virtio_ccw_md_unplug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+
+#endif /* HW_S390X_VIRTIO_CCW_MD_H */
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 70c77e183d..7648a2d68d 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -29,6 +29,7 @@ config VIRTIO_MMIO
config VIRTIO_CCW
bool
select VIRTIO
+ select VIRTIO_MD_SUPPORTED
config VIRTIO_BALLOON
bool
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PULL 15/15] s390x: virtio-mem support
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (13 preceding siblings ...)
2024-12-18 10:53 ` [PULL 14/15] s390x/virtio-ccw: add support for virtio based memory devices David Hildenbrand
@ 2024-12-18 10:53 ` David Hildenbrand
2024-12-18 21:09 ` [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 Stefan Hajnoczi
15 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-18 10:53 UTC (permalink / raw)
To: qemu-devel
Cc: Halil Pasic, Christian Borntraeger, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin, David Hildenbrand
Let's add our virtio-mem-ccw proxy device and wire it up. We should
be supporting everything (e.g., device unplug, "dynamic-memslots") that
we already support for the virtio-pci variant.
With a Linux guest that supports virtio-mem (and has automatic memory
onlining properly configured) the following example will work:
1. Start a VM with 4G initial memory and a virtio-mem device with a maximum
capacity of 16GB:
qemu/build/qemu-system-s390x \
--enable-kvm \
-m 4G,maxmem=20G \
-nographic \
-smp 8 \
-hda Fedora-Server-KVM-40-1.14.s390x.qcow2 \
-chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
-mon chardev=monitor,mode=readline \
-object memory-backend-ram,id=mem0,size=16G,reserve=off \
-device virtio-mem-ccw,id=vmem0,memdev=mem0,dynamic-memslots=on
2. Query the current size of virtio-mem device:
(qemu) info memory-devices
Memory device [virtio-mem]: "vmem0"
memaddr: 0x100000000
node: 0
requested-size: 0
size: 0
max-size: 17179869184
block-size: 1048576
memdev: /objects/mem0
3. Request to grow it to 8GB (hotplug 8GB):
(qemu) qom-set vmem0 requested-size 8G
(qemu) info memory-devices
Memory device [virtio-mem]: "vmem0"
memaddr: 0x100000000
node: 0
requested-size: 8589934592
size: 8589934592
max-size: 17179869184
block-size: 1048576
memdev: /objects/mem0
4. Request to grow to 16GB (hotplug another 8GB):
(qemu) qom-set vmem0 requested-size 16G
(qemu) info memory-devices
Memory device [virtio-mem]: "vmem0"
memaddr: 0x100000000
node: 0
requested-size: 17179869184
size: 17179869184
max-size: 17179869184
block-size: 1048576
memdev: /objects/mem0
5. Try to hotunplug all memory again, shrinking to 0GB:
(qemu) qom-set vmem0 requested-size 0G
(qemu) info memory-devices
Memory device [virtio-mem]: "vmem0"
memaddr: 0x100000000
node: 0
requested-size: 0
size: 0
max-size: 17179869184
block-size: 1048576
memdev: /objects/mem0
6. If it worked, unplug the device
(qemu) device_del vmem0
(qemu) info memory-devices
(qemu) object_del mem0
7. Hotplug a new device with a smaller capacity and directly size it to 1GB
(qemu) object_add memory-backend-ram,id=mem0,size=8G,reserve=off
(qemu) device_add virtio-mem-ccw,id=vmem0,memdev=mem0,\
dynamic-memslots=on,requested-size=1G
(qemu) info memory-devices
Memory device [virtio-mem]: "vmem0"
memaddr: 0x100000000
node: 0
requested-size: 1073741824
size: 1073741824
max-size: 8589934592
block-size: 1048576
memdev: /objects/mem0
Trying to use a virtio-mem device backed by hugetlb into a !hugetlb VM
correctly results in the error:
... Memory device uses a bigger page size than initial memory
Note that the virtio-mem driver in Linux will supports 1 MiB (pageblock)
granularity.
Message-ID: <20241008105455.2302628-15-david@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
MAINTAINERS | 2 +
hw/s390x/Kconfig | 1 +
hw/s390x/meson.build | 1 +
hw/s390x/virtio-ccw-mem.c | 226 ++++++++++++++++++++++++++++++++++++++
hw/s390x/virtio-ccw-mem.h | 34 ++++++
hw/virtio/virtio-mem.c | 4 +-
6 files changed, 267 insertions(+), 1 deletion(-)
create mode 100644 hw/s390x/virtio-ccw-mem.c
create mode 100644 hw/s390x/virtio-ccw-mem.h
diff --git a/MAINTAINERS b/MAINTAINERS
index f47fce0362..898097593e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2389,6 +2389,8 @@ W: https://virtio-mem.gitlab.io/
F: hw/virtio/virtio-mem.c
F: hw/virtio/virtio-mem-pci.h
F: hw/virtio/virtio-mem-pci.c
+F: hw/s390x/virtio-ccw-mem.c
+F: hw/s390x/virtio-ccw-mem.h
F: include/hw/virtio/virtio-mem.h
virtio-snd
diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
index 82afdaa9dc..02ea199701 100644
--- a/hw/s390x/Kconfig
+++ b/hw/s390x/Kconfig
@@ -16,3 +16,4 @@ config S390_CCW_VIRTIO
select SCLPCONSOLE
select VIRTIO_CCW
select MSI_NONBROKEN
+ select VIRTIO_MEM_SUPPORTED
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 28bbe4b06c..094435cd3b 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -49,6 +49,7 @@ virtio_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-ccw.c'))
virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-ccw-md.c'))
+virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-ccw-mem.c'))
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
s390x_ss.add(when: 'CONFIG_VIRTIO_MD', if_false: files('virtio-ccw-md-stubs.c'))
diff --git a/hw/s390x/virtio-ccw-mem.c b/hw/s390x/virtio-ccw-mem.c
new file mode 100644
index 0000000000..bee0d560cb
--- /dev/null
+++ b/hw/s390x/virtio-ccw-mem.c
@@ -0,0 +1,226 @@
+/*
+ * virtio-mem CCW implementation
+ *
+ * Copyright (C) 2024 Red Hat, Inc.
+ *
+ * Authors:
+ * David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "virtio-ccw-mem.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-misc.h"
+
+static void virtio_ccw_mem_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(ccw_dev);
+ DeviceState *vdev = DEVICE(&dev->vdev);
+
+ qdev_realize(vdev, BUS(&ccw_dev->bus), errp);
+}
+
+static void virtio_ccw_mem_set_addr(MemoryDeviceState *md, uint64_t addr,
+ Error **errp)
+{
+ object_property_set_uint(OBJECT(md), VIRTIO_MEM_ADDR_PROP, addr, errp);
+}
+
+static uint64_t virtio_ccw_mem_get_addr(const MemoryDeviceState *md)
+{
+ return object_property_get_uint(OBJECT(md), VIRTIO_MEM_ADDR_PROP,
+ &error_abort);
+}
+
+static MemoryRegion *virtio_ccw_mem_get_memory_region(MemoryDeviceState *md,
+ Error **errp)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+ VirtIOMEM *vmem = &dev->vdev;
+ VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+ return vmc->get_memory_region(vmem, errp);
+}
+
+static void virtio_ccw_mem_decide_memslots(MemoryDeviceState *md,
+ unsigned int limit)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+ VirtIOMEM *vmem = VIRTIO_MEM(&dev->vdev);
+ VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+ vmc->decide_memslots(vmem, limit);
+}
+
+static unsigned int virtio_ccw_mem_get_memslots(MemoryDeviceState *md)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+ VirtIOMEM *vmem = VIRTIO_MEM(&dev->vdev);
+ VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+ return vmc->get_memslots(vmem);
+}
+
+static uint64_t virtio_ccw_mem_get_plugged_size(const MemoryDeviceState *md,
+ Error **errp)
+{
+ return object_property_get_uint(OBJECT(md), VIRTIO_MEM_SIZE_PROP,
+ errp);
+}
+
+static void virtio_ccw_mem_fill_device_info(const MemoryDeviceState *md,
+ MemoryDeviceInfo *info)
+{
+ VirtioMEMDeviceInfo *vi = g_new0(VirtioMEMDeviceInfo, 1);
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+ VirtIOMEM *vmem = &dev->vdev;
+ VirtIOMEMClass *vpc = VIRTIO_MEM_GET_CLASS(vmem);
+ DeviceState *vdev = DEVICE(md);
+
+ if (vdev->id) {
+ vi->id = g_strdup(vdev->id);
+ }
+
+ /* let the real device handle everything else */
+ vpc->fill_device_info(vmem, vi);
+
+ info->u.virtio_mem.data = vi;
+ info->type = MEMORY_DEVICE_INFO_KIND_VIRTIO_MEM;
+}
+
+static uint64_t virtio_ccw_mem_get_min_alignment(const MemoryDeviceState *md)
+{
+ return object_property_get_uint(OBJECT(md), VIRTIO_MEM_BLOCK_SIZE_PROP,
+ &error_abort);
+}
+
+static void virtio_ccw_mem_size_change_notify(Notifier *notifier, void *data)
+{
+ VirtIOMEMCcw *dev = container_of(notifier, VirtIOMEMCcw,
+ size_change_notifier);
+ DeviceState *vdev = DEVICE(dev);
+ char *qom_path = object_get_canonical_path(OBJECT(dev));
+ const uint64_t * const size_p = data;
+
+ qapi_event_send_memory_device_size_change(vdev->id, *size_p, qom_path);
+ g_free(qom_path);
+}
+
+static void virtio_ccw_mem_unplug_request_check(VirtIOMDCcw *vmd, Error **errp)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(vmd);
+ VirtIOMEM *vmem = &dev->vdev;
+ VirtIOMEMClass *vpc = VIRTIO_MEM_GET_CLASS(vmem);
+
+ vpc->unplug_request_check(vmem, errp);
+}
+
+static void virtio_ccw_mem_get_requested_size(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+
+ object_property_get(OBJECT(&dev->vdev), name, v, errp);
+}
+
+static void virtio_ccw_mem_set_requested_size(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+ DeviceState *vdev = DEVICE(obj);
+
+ /*
+ * If we passed virtio_ccw_mem_unplug_request_check(), making sure that
+ * the requested size is 0, don't allow modifying the requested size
+ * anymore, otherwise the VM might end up hotplugging memory before
+ * handling the unplug request.
+ */
+ if (vdev->pending_deleted_event) {
+ error_setg(errp, "'%s' cannot be changed if the device is in the"
+ " process of unplug", name);
+ return;
+ }
+
+ object_property_set(OBJECT(&dev->vdev), name, v, errp);
+}
+
+static Property virtio_ccw_mem_properties[] = {
+ DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+ VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+ DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+ VIRTIO_CCW_MAX_REV),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_ccw_mem_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+ MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(klass);
+ VirtIOMDCcwClass *vmdc = VIRTIO_MD_CCW_CLASS(klass);
+
+ k->realize = virtio_ccw_mem_realize;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ device_class_set_props(dc, virtio_ccw_mem_properties);
+
+ mdc->get_addr = virtio_ccw_mem_get_addr;
+ mdc->set_addr = virtio_ccw_mem_set_addr;
+ mdc->get_plugged_size = virtio_ccw_mem_get_plugged_size;
+ mdc->get_memory_region = virtio_ccw_mem_get_memory_region;
+ mdc->decide_memslots = virtio_ccw_mem_decide_memslots;
+ mdc->get_memslots = virtio_ccw_mem_get_memslots;
+ mdc->fill_device_info = virtio_ccw_mem_fill_device_info;
+ mdc->get_min_alignment = virtio_ccw_mem_get_min_alignment;
+
+ vmdc->unplug_request_check = virtio_ccw_mem_unplug_request_check;
+}
+
+static void virtio_ccw_mem_instance_init(Object *obj)
+{
+ VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+ VirtIOMEMClass *vmc;
+ VirtIOMEM *vmem;
+
+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+ TYPE_VIRTIO_MEM);
+
+ dev->size_change_notifier.notify = virtio_ccw_mem_size_change_notify;
+ vmem = &dev->vdev;
+ vmc = VIRTIO_MEM_GET_CLASS(vmem);
+ /*
+ * We never remove the notifier again, as we expect both devices to
+ * disappear at the same time.
+ */
+ vmc->add_size_change_notifier(vmem, &dev->size_change_notifier);
+
+ object_property_add_alias(obj, VIRTIO_MEM_BLOCK_SIZE_PROP,
+ OBJECT(&dev->vdev), VIRTIO_MEM_BLOCK_SIZE_PROP);
+ object_property_add_alias(obj, VIRTIO_MEM_SIZE_PROP, OBJECT(&dev->vdev),
+ VIRTIO_MEM_SIZE_PROP);
+ object_property_add(obj, VIRTIO_MEM_REQUESTED_SIZE_PROP, "size",
+ virtio_ccw_mem_get_requested_size,
+ virtio_ccw_mem_set_requested_size, NULL, NULL);
+}
+
+static const TypeInfo virtio_ccw_mem = {
+ .name = TYPE_VIRTIO_MEM_CCW,
+ .parent = TYPE_VIRTIO_MD_CCW,
+ .instance_size = sizeof(VirtIOMEMCcw),
+ .instance_init = virtio_ccw_mem_instance_init,
+ .class_init = virtio_ccw_mem_class_init,
+};
+
+static void virtio_ccw_mem_register_types(void)
+{
+ type_register_static(&virtio_ccw_mem);
+}
+type_init(virtio_ccw_mem_register_types)
diff --git a/hw/s390x/virtio-ccw-mem.h b/hw/s390x/virtio-ccw-mem.h
new file mode 100644
index 0000000000..738ab2c744
--- /dev/null
+++ b/hw/s390x/virtio-ccw-mem.h
@@ -0,0 +1,34 @@
+/*
+ * Virtio MEM CCW device
+ *
+ * Copyright (C) 2024 Red Hat, Inc.
+ *
+ * Authors:
+ * David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_S390X_VIRTIO_CCW_MEM_H
+#define HW_S390X_VIRTIO_CCW_MEM_H
+
+#include "virtio-ccw-md.h"
+#include "hw/virtio/virtio-mem.h"
+#include "qom/object.h"
+
+typedef struct VirtIOMEMCcw VirtIOMEMCcw;
+
+/*
+ * virtio-mem-ccw: This extends VirtIOMDCcw
+ */
+#define TYPE_VIRTIO_MEM_CCW "virtio-mem-ccw"
+DECLARE_INSTANCE_CHECKER(VirtIOMEMCcw, VIRTIO_MEM_CCW, TYPE_VIRTIO_MEM_CCW)
+
+struct VirtIOMEMCcw {
+ VirtIOMDCcw parent_obj;
+ VirtIOMEM vdev;
+ Notifier size_change_notifier;
+};
+
+#endif /* HW_S390X_VIRTIO_CCW_MEM_H */
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index a0dceaddec..48e4a58239 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -61,6 +61,8 @@ static uint32_t virtio_mem_default_thp_size(void)
} else if (qemu_real_host_page_size() == 64 * KiB) {
default_thp_size = 512 * MiB;
}
+#elif defined(__s390x__)
+ default_thp_size = 1 * MiB;
#endif
return default_thp_size;
@@ -168,7 +170,7 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb)
* necessary (as the section size can change). But it's more likely that the
* section size will rather get smaller and not bigger over time.
*/
-#if defined(TARGET_X86_64) || defined(TARGET_I386)
+#if defined(TARGET_X86_64) || defined(TARGET_I386) || defined(TARGET_S390X)
#define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
#elif defined(TARGET_ARM)
#define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB))
--
2.47.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-18 10:52 [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 David Hildenbrand
` (14 preceding siblings ...)
2024-12-18 10:53 ` [PULL 15/15] s390x: virtio-mem support David Hildenbrand
@ 2024-12-18 21:09 ` Stefan Hajnoczi
2024-12-19 0:04 ` David Hildenbrand
15 siblings, 1 reply; 30+ messages in thread
From: Stefan Hajnoczi @ 2024-12-18 21:09 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com> wrote:
>
> The following changes since commit 8032c78e556cd0baec111740a6c636863f9bd7c8:
>
> Merge tag 'firmware-20241216-pull-request' of https://gitlab.com/kraxel/qemu into staging (2024-12-16 14:20:33 -0500)
>
> are available in the Git repository at:
>
> https://github.com/davidhildenbrand/qemu.git tags/mem-2024-12-18
>
> for you to fetch changes up to eb5c5f1ab479e9311d8e513e3eeafaf30f2b25b3:
>
> s390x: virtio-mem support (2024-12-18 09:50:05 +0100)
>
> ----------------------------------------------------------------
> Hi,
>
> "Host Memory Backends" and "Memory devices" queue ("mem"):
> - Fixup handling of virtio-mem unplug during system resets, as
> preparation for s390x support (especially kdump in the Linux guest)
> - virtio-mem support for s390x
>
> ----------------------------------------------------------------
> David Hildenbrand (15):
> virtio-mem: unplug memory only during system resets, not device resets
> s390x/s390-virtio-ccw: don't crash on weird RAM sizes
> s390x/s390-virtio-hcall: remove hypercall registration mechanism
> s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
> s390x: rename s390-virtio-hcall* to s390-hypercall*
> s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code
> s390x: introduce s390_get_memory_limit()
> s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
> s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts
> s390x/s390-skeys: prepare for memory devices
> s390x/s390-virtio-ccw: prepare for memory devices
> s390x/pv: prepare for memory devices
> s390x: remember the maximum page size
> s390x/virtio-ccw: add support for virtio based memory devices
> s390x: virtio-mem support
Please take a look at the following s390x-related CI failures:
https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
https://gitlab.com/qemu-project/qemu/-/jobs/8679972894
https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
If you find that this pull request caused the issue, please send a new
revision of the pull request. Thanks!
Stefan
>
> MAINTAINERS | 5 +
> hw/s390x/Kconfig | 1 +
> hw/s390x/meson.build | 6 +-
> hw/s390x/s390-hypercall.c | 85 ++++++++++++++
> hw/s390x/s390-hypercall.h | 25 ++++
> hw/s390x/s390-skeys.c | 6 +-
> hw/s390x/s390-stattrib-kvm.c | 67 +++++++----
> hw/s390x/s390-virtio-ccw.c | 165 ++++++++++++++++++---------
> hw/s390x/s390-virtio-hcall.c | 41 -------
> hw/s390x/s390-virtio-hcall.h | 25 ----
> hw/s390x/sclp.c | 17 +--
> hw/s390x/virtio-ccw-md-stubs.c | 24 ++++
> hw/s390x/virtio-ccw-md.c | 153 +++++++++++++++++++++++++
> hw/s390x/virtio-ccw-md.h | 44 ++++++++
> hw/s390x/virtio-ccw-mem.c | 226 +++++++++++++++++++++++++++++++++++++
> hw/s390x/virtio-ccw-mem.h | 34 ++++++
> hw/virtio/Kconfig | 1 +
> hw/virtio/virtio-mem.c | 107 ++++++++++++------
> include/hw/s390x/s390-virtio-ccw.h | 4 +
> include/hw/virtio/virtio-mem.h | 13 ++-
> target/s390x/cpu-sysemu.c | 15 ---
> target/s390x/cpu.h | 2 -
> target/s390x/kvm/kvm.c | 18 +--
> target/s390x/kvm/pv.c | 2 +-
> target/s390x/tcg/misc_helper.c | 7 +-
> 25 files changed, 866 insertions(+), 227 deletions(-)
> create mode 100644 hw/s390x/s390-hypercall.c
> create mode 100644 hw/s390x/s390-hypercall.h
> delete mode 100644 hw/s390x/s390-virtio-hcall.c
> delete mode 100644 hw/s390x/s390-virtio-hcall.h
> create mode 100644 hw/s390x/virtio-ccw-md-stubs.c
> create mode 100644 hw/s390x/virtio-ccw-md.c
> create mode 100644 hw/s390x/virtio-ccw-md.h
> create mode 100644 hw/s390x/virtio-ccw-mem.c
> create mode 100644 hw/s390x/virtio-ccw-mem.h
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-18 21:09 ` [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18 Stefan Hajnoczi
@ 2024-12-19 0:04 ` David Hildenbrand
2024-12-19 11:18 ` David Hildenbrand
0 siblings, 1 reply; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 0:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 18.12.24 22:09, Stefan Hajnoczi wrote:
> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com> wrote:
>>
>> The following changes since commit 8032c78e556cd0baec111740a6c636863f9bd7c8:
>>
>> Merge tag 'firmware-20241216-pull-request' of https://gitlab.com/kraxel/qemu into staging (2024-12-16 14:20:33 -0500)
>>
>> are available in the Git repository at:
>>
>> https://github.com/davidhildenbrand/qemu.git tags/mem-2024-12-18
>>
>> for you to fetch changes up to eb5c5f1ab479e9311d8e513e3eeafaf30f2b25b3:
>>
>> s390x: virtio-mem support (2024-12-18 09:50:05 +0100)
>>
>> ----------------------------------------------------------------
>> Hi,
>>
>> "Host Memory Backends" and "Memory devices" queue ("mem"):
>> - Fixup handling of virtio-mem unplug during system resets, as
>> preparation for s390x support (especially kdump in the Linux guest)
>> - virtio-mem support for s390x
>>
>> ----------------------------------------------------------------
>> David Hildenbrand (15):
>> virtio-mem: unplug memory only during system resets, not device resets
>> s390x/s390-virtio-ccw: don't crash on weird RAM sizes
>> s390x/s390-virtio-hcall: remove hypercall registration mechanism
>> s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
>> s390x: rename s390-virtio-hcall* to s390-hypercall*
>> s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code
>> s390x: introduce s390_get_memory_limit()
>> s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
>> s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts
>> s390x/s390-skeys: prepare for memory devices
>> s390x/s390-virtio-ccw: prepare for memory devices
>> s390x/pv: prepare for memory devices
>> s390x: remember the maximum page size
>> s390x/virtio-ccw: add support for virtio based memory devices
>> s390x: virtio-mem support
>
> Please take a look at the following s390x-related CI failures:
Thanks, most of them seem related to this PULL.
> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used uninitialized [-Werror=maybe-uninitialized]
138 | error_report("host supports a maximum of %" PRIu64 " GB",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
139 | hw_limit / GiB);
| ~~~~~~~~~~~~~~~
../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
130 | uint64_t hw_limit;
| ^~~~~~~~
Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
-E2BIG and consequently that code won't be executed.
Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>
> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
/usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in function `qemu_s390_enable_skeys':
/builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256: undefined reference to `s390_get_memory_limit'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_virtio_ccw_notify':
/builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46: undefined reference to `virtio_ccw_get_vdev'
/usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:47: undefined reference to `virtio_queue_get_num'
/usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:56: undefined reference to `virtio_queue_notify'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_storage_limit':
/builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64: undefined reference to `s390_get_memory_limit'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_virtio_ccw_notify':
/builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52: undefined reference to `virtio_get_queue'
/usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52: undefined reference to `virtio_queue_set_shadow_avail_idx'
We're building with "--without-default-devices' '--without-default-feature".
Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
So we won't compile s390-virtio-ccw.c, but we will compile things like s390-stattrib.c,
s390-hypercall.c, ... which to me is extremely odd.
Is this maybe a leftover from the time when we had the old machine type? What value
is it to compile all these files without even having a machine that could make use
of these?
So I wonder if most of these files should actually only be compiled with
CONFIG_S390_CCW_VIRTIO, which is the only machine we have.
> https://gitlab.com/qemu-project/qemu/-/jobs/8679972894
98/976 ERROR:../tests/qtest/migration-test.c:36:main: assertion failed (ret == 0): (1 == 0) ERROR
...
――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――
stderr:
Traceback (most recent call last):
File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 688, in <module>
dump.read(dump_memory = args.memory)
File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 625, in read
section.read()
File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 461, in read
field['data'] = reader(field, self.file)
File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 434, in __init__
for field in self.desc['struct']['fields']:
KeyError: 'fields'
warning: fd: migration to a file is deprecated. Use file: instead.
warning: fd: migration to a file is deprecated. Use file: instead.
**
ERROR:../tests/qtest/migration-test.c:36:main: assertion failed (ret == 0): (1 == 0)
(test program exited with status code -6)
Cannot reproduce it so far, will try again tomorrow.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 0:04 ` David Hildenbrand
@ 2024-12-19 11:18 ` David Hildenbrand
2024-12-19 11:43 ` Christian Borntraeger
2024-12-19 13:04 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 11:18 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19.12.24 01:04, David Hildenbrand wrote:
> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> The following changes since commit 8032c78e556cd0baec111740a6c636863f9bd7c8:
>>>
>>> Merge tag 'firmware-20241216-pull-request' of https://gitlab.com/kraxel/qemu into staging (2024-12-16 14:20:33 -0500)
>>>
>>> are available in the Git repository at:
>>>
>>> https://github.com/davidhildenbrand/qemu.git tags/mem-2024-12-18
>>>
>>> for you to fetch changes up to eb5c5f1ab479e9311d8e513e3eeafaf30f2b25b3:
>>>
>>> s390x: virtio-mem support (2024-12-18 09:50:05 +0100)
>>>
>>> ----------------------------------------------------------------
>>> Hi,
>>>
>>> "Host Memory Backends" and "Memory devices" queue ("mem"):
>>> - Fixup handling of virtio-mem unplug during system resets, as
>>> preparation for s390x support (especially kdump in the Linux guest)
>>> - virtio-mem support for s390x
>>>
>>> ----------------------------------------------------------------
>>> David Hildenbrand (15):
>>> virtio-mem: unplug memory only during system resets, not device resets
>>> s390x/s390-virtio-ccw: don't crash on weird RAM sizes
>>> s390x/s390-virtio-hcall: remove hypercall registration mechanism
>>> s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
>>> s390x: rename s390-virtio-hcall* to s390-hypercall*
>>> s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code
>>> s390x: introduce s390_get_memory_limit()
>>> s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
>>> s390x/s390-stattrib-kvm: prepare for memory devices and sparse memory layouts
>>> s390x/s390-skeys: prepare for memory devices
>>> s390x/s390-virtio-ccw: prepare for memory devices
>>> s390x/pv: prepare for memory devices
>>> s390x: remember the maximum page size
>>> s390x/virtio-ccw: add support for virtio based memory devices
>>> s390x: virtio-mem support
>>
>> Please take a look at the following s390x-related CI failures:
>
> Thanks, most of them seem related to this PULL.
>
>
>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>
> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used uninitialized [-Werror=maybe-uninitialized]
> 138 | error_report("host supports a maximum of %" PRIu64 " GB",
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 139 | hw_limit / GiB);
> | ~~~~~~~~~~~~~~~
> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
> 130 | uint64_t hw_limit;
> | ^~~~~~~~
>
> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
> -E2BIG and consequently that code won't be executed.
>
> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>
>
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in function `qemu_s390_enable_skeys':
> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256: undefined reference to `s390_get_memory_limit'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_virtio_ccw_notify':
> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46: undefined reference to `virtio_ccw_get_vdev'
> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:47: undefined reference to `virtio_queue_get_num'
> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:56: undefined reference to `virtio_queue_notify'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_storage_limit':
> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64: undefined reference to `s390_get_memory_limit'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in function `handle_virtio_ccw_notify':
> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52: undefined reference to `virtio_get_queue'
> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52: undefined reference to `virtio_queue_set_shadow_avail_idx'
>
> We're building with "--without-default-devices' '--without-default-feature".
> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>
> So we won't compile s390-virtio-ccw.c, but we will compile things like s390-stattrib.c,
> s390-hypercall.c, ... which to me is extremely odd.
>
> Is this maybe a leftover from the time when we had the old machine type? What value
> is it to compile all these files without even having a machine that could make use
> of these?
>
> So I wonder if most of these files should actually only be compiled with
> CONFIG_S390_CCW_VIRTIO, which is the only machine we have.
>
>
>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972894
>
> 98/976 ERROR:../tests/qtest/migration-test.c:36:main: assertion failed (ret == 0): (1 == 0) ERROR
> ...
> ――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――
> stderr:
> Traceback (most recent call last):
> File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 688, in <module>
> dump.read(dump_memory = args.memory)
> File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 625, in read
> section.read()
> File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 461, in read
> field['data'] = reader(field, self.file)
> File "/home/gitlab-runner/builds/4S3awx_3/0/qemu-project/qemu/build/scripts/analyze-migration.py", line 434, in __init__
> for field in self.desc['struct']['fields']:
> KeyError: 'fields'
> warning: fd: migration to a file is deprecated. Use file: instead.
> warning: fd: migration to a file is deprecated. Use file: instead.
> **
> ERROR:../tests/qtest/migration-test.c:36:main: assertion failed (ret == 0): (1 == 0)
> (test program exited with status code -6)
>
>
> Cannot reproduce it so far, will try again tomorrow.
The following on top seems to make everything happy. I wish the
CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
handle odd configs that don't really make sense.
I'll do some more testing, then squash the changes into the respective
patches and resend.
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 094435cd3b..3bbebfd817 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -12,7 +12,6 @@ s390x_ss.add(files(
's390-pci-inst.c',
's390-skeys.c',
's390-stattrib.c',
- 's390-hypercall.c',
'sclp.c',
'sclpcpu.c',
'sclpquiesce.c',
@@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
'tod-tcg.c',
))
-s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c'))
+s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
+ 's390-virtio-ccw.c',
+ 's390-hypercall.c',
+))
s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 248566f8dc..097ec78826 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -127,7 +127,7 @@ static void subsystem_reset(void)
static void s390_set_memory_limit(S390CcwMachineState *s390ms,
uint64_t new_limit)
{
- uint64_t hw_limit;
+ uint64_t hw_limit = 0;
int ret = 0;
assert(!s390ms->memory_limit && new_limit);
@@ -145,13 +145,6 @@ static void s390_set_memory_limit(S390CcwMachineState *s390ms,
s390ms->memory_limit = new_limit;
}
-uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
-{
- /* We expect to be called only after the limit was set. */
- assert(s390ms->memory_limit);
- return s390ms->memory_limit;
-}
-
static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
uint64_t pagesize)
{
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 5a730f5d07..599740a998 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -35,7 +35,12 @@ struct S390CcwMachineState {
SCLPDevice *sclp;
};
-uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
+static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
+{
+ /* We expect to be called only after the limit was set. */
+ assert(s390ms->memory_limit);
+ return s390ms->memory_limit;
+}
#define S390_PTF_REASON_NONE (0x00 << 8)
#define S390_PTF_REASON_DONE (0x01 << 8)
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 3732d79185..be1870b07d 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -44,6 +44,7 @@
#include "hw/boards.h"
#include "hw/s390x/tod.h"
#endif
+#include CONFIG_DEVICES
/* #define DEBUG_HELPER */
#ifdef DEBUG_HELPER
@@ -117,12 +118,14 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
switch (num) {
case 0x500:
+#ifdef CONFIG_S390_CCW_VIRTIO
/* QEMU/KVM hypercall */
bql_lock();
handle_diag_500(env_archcpu(env), GETPC());
bql_unlock();
r = 0;
break;
+#endif /* CONFIG_S390_CCW_VIRTIO */
case 0x44:
/* yield */
r = 0;
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 11:18 ` David Hildenbrand
@ 2024-12-19 11:43 ` Christian Borntraeger
2024-12-19 11:57 ` David Hildenbrand
2024-12-19 13:04 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 30+ messages in thread
From: Christian Borntraeger @ 2024-12-19 11:43 UTC (permalink / raw)
To: David Hildenbrand, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin
Am 19.12.24 um 12:18 schrieb David Hildenbrand:
> The following on top seems to make everything happy. I wish the
> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
> handle odd configs that don't really make sense.
WOuld it be possible to rid of this config?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 11:43 ` Christian Borntraeger
@ 2024-12-19 11:57 ` David Hildenbrand
2024-12-19 11:58 ` David Hildenbrand
2024-12-19 12:20 ` Thomas Huth
0 siblings, 2 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 11:57 UTC (permalink / raw)
To: Christian Borntraeger, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin
On 19.12.24 12:43, Christian Borntraeger wrote:
> Am 19.12.24 um 12:18 schrieb David Hildenbrand:
>> The following on top seems to make everything happy. I wish the
>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>> handle odd configs that don't really make sense.
>
> WOuld it be possible to rid of this config?
I was asking myself the same: when does it make sense to build for s390x
system without CONFIG_S390_CCW_VIRTIO. But other archs that also have a
single machine seem to be doing the same thing.
We wouldn't want to have the option to disable it, but "bool" gives you
the option to do that.
I suspect something that could work is:
diff --git a/target/s390x/Kconfig b/target/s390x/Kconfig
index 8a95f2bc3f..4c99b9cedd 100644
--- a/target/s390x/Kconfig
+++ b/target/s390x/Kconfig
@@ -1,5 +1,6 @@
config S390X
bool
+ select CONFIG_S390_CCW_VIRTIO
select PCI
select S390_FLIC
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 11:57 ` David Hildenbrand
@ 2024-12-19 11:58 ` David Hildenbrand
2024-12-19 12:20 ` Thomas Huth
1 sibling, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 11:58 UTC (permalink / raw)
To: Christian Borntraeger, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Eric Farman, Thomas Huth,
Richard Henderson, Michael S . Tsirkin
On 19.12.24 12:57, David Hildenbrand wrote:
> On 19.12.24 12:43, Christian Borntraeger wrote:
>> Am 19.12.24 um 12:18 schrieb David Hildenbrand:
>>> The following on top seems to make everything happy. I wish the
>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>> handle odd configs that don't really make sense.
>>
>> WOuld it be possible to rid of this config?
>
> I was asking myself the same: when does it make sense to build for s390x
> system without CONFIG_S390_CCW_VIRTIO. But other archs that also have a
> single machine seem to be doing the same thing.
>
> We wouldn't want to have the option to disable it, but "bool" gives you
> the option to do that.
>
> I suspect something that could work is:
>
> diff --git a/target/s390x/Kconfig b/target/s390x/Kconfig
> index 8a95f2bc3f..4c99b9cedd 100644
> --- a/target/s390x/Kconfig
> +++ b/target/s390x/Kconfig
> @@ -1,5 +1,6 @@
> config S390X
> bool
> + select CONFIG_S390_CCW_VIRTIO
> select PCI
> select S390_FLIC
(- CONFIG_ of course)
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 11:57 ` David Hildenbrand
2024-12-19 11:58 ` David Hildenbrand
@ 2024-12-19 12:20 ` Thomas Huth
2024-12-19 12:39 ` David Hildenbrand
1 sibling, 1 reply; 30+ messages in thread
From: Thomas Huth @ 2024-12-19 12:20 UTC (permalink / raw)
To: David Hildenbrand, Christian Borntraeger, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Eric Farman, Richard Henderson,
Michael S . Tsirkin
On 19/12/2024 12.57, David Hildenbrand wrote:
> On 19.12.24 12:43, Christian Borntraeger wrote:
>> Am 19.12.24 um 12:18 schrieb David Hildenbrand:
>>> The following on top seems to make everything happy. I wish the
>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>> handle odd configs that don't really make sense.
>>
>> WOuld it be possible to rid of this config?
>
> I was asking myself the same: when does it make sense to build for s390x
> system without CONFIG_S390_CCW_VIRTIO. But other archs that also have a
> single machine seem to be doing the same thing.
>
> We wouldn't want to have the option to disable it, but "bool" gives you the
> option to do that.
Since a while, (almost) all targets can be compiled without any machine
except for the "none" machine, so I think we should not diverge in the s390x
just for the sake of it.
Thomas
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 12:20 ` Thomas Huth
@ 2024-12-19 12:39 ` David Hildenbrand
0 siblings, 0 replies; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 12:39 UTC (permalink / raw)
To: Thomas Huth, Christian Borntraeger, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Eric Farman, Richard Henderson,
Michael S . Tsirkin
On 19.12.24 13:20, Thomas Huth wrote:
> On 19/12/2024 12.57, David Hildenbrand wrote:
>> On 19.12.24 12:43, Christian Borntraeger wrote:
>>> Am 19.12.24 um 12:18 schrieb David Hildenbrand:
>>>> The following on top seems to make everything happy. I wish the
>>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>>> handle odd configs that don't really make sense.
>>>
>>> WOuld it be possible to rid of this config?
>>
>> I was asking myself the same: when does it make sense to build for s390x
>> system without CONFIG_S390_CCW_VIRTIO. But other archs that also have a
>> single machine seem to be doing the same thing.
>>
>> We wouldn't want to have the option to disable it, but "bool" gives you the
>> option to do that.
>
> Since a while, (almost) all targets can be compiled without any machine
> except for the "none" machine, so I think we should not diverge in the s390x
> just for the sake of it.
Well, okay, although such a qemu-system-s390x is of questionable use. :)
At least hypercalls/skeys/stattrib only applies to the CONFIG_S390_CCW_VIRTIO
machine, so maybe more files can be moved under CONFIG_S390_CCW_VIRTIO later.
Anyhow, sounds like a bigger cleanup. The following on top seems to
make gitlab CI happy, without messing with config options:
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 094435cd3b..3bbebfd817 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -12,7 +12,6 @@ s390x_ss.add(files(
's390-pci-inst.c',
's390-skeys.c',
's390-stattrib.c',
- 's390-hypercall.c',
'sclp.c',
'sclpcpu.c',
'sclpquiesce.c',
@@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
'tod-tcg.c',
))
-s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-virtio-ccw.c'))
+s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
+ 's390-virtio-ccw.c',
+ 's390-hypercall.c',
+))
s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 248566f8dc..097ec78826 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -127,7 +127,7 @@ static void subsystem_reset(void)
static void s390_set_memory_limit(S390CcwMachineState *s390ms,
uint64_t new_limit)
{
- uint64_t hw_limit;
+ uint64_t hw_limit = 0;
int ret = 0;
assert(!s390ms->memory_limit && new_limit);
@@ -145,13 +145,6 @@ static void s390_set_memory_limit(S390CcwMachineState *s390ms,
s390ms->memory_limit = new_limit;
}
-uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
-{
- /* We expect to be called only after the limit was set. */
- assert(s390ms->memory_limit);
- return s390ms->memory_limit;
-}
-
static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
uint64_t pagesize)
{
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 5a730f5d07..599740a998 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -35,7 +35,12 @@ struct S390CcwMachineState {
SCLPDevice *sclp;
};
-uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
+static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
+{
+ /* We expect to be called only after the limit was set. */
+ assert(s390ms->memory_limit);
+ return s390ms->memory_limit;
+}
#define S390_PTF_REASON_NONE (0x00 << 8)
#define S390_PTF_REASON_DONE (0x01 << 8)
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 3732d79185..8002b1e2d0 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -43,6 +43,7 @@
#include "hw/s390x/s390-pci-inst.h"
#include "hw/boards.h"
#include "hw/s390x/tod.h"
+#include CONFIG_DEVICES
#endif
/* #define DEBUG_HELPER */
@@ -117,12 +118,14 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
switch (num) {
+#ifdef CONFIG_S390_CCW_VIRTIO
case 0x500:
/* QEMU/KVM hypercall */
bql_lock();
handle_diag_500(env_archcpu(env), GETPC());
bql_unlock();
r = 0;
break;
+#endif /* CONFIG_S390_CCW_VIRTIO */
case 0x44:
/* yield */
r = 0;
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 11:18 ` David Hildenbrand
2024-12-19 11:43 ` Christian Borntraeger
@ 2024-12-19 13:04 ` Philippe Mathieu-Daudé
2024-12-19 13:11 ` David Hildenbrand
1 sibling, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-19 13:04 UTC (permalink / raw)
To: David Hildenbrand, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
Hi,
On 19/12/24 12:18, David Hildenbrand wrote:
> On 19.12.24 01:04, David Hildenbrand wrote:
>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>> Please take a look at the following s390x-related CI failures:
>>
>> Thanks, most of them seem related to this PULL.
>>
>>
>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>
>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>> uninitialized [-Werror=maybe-uninitialized]
>> 138 | error_report("host supports a maximum of %" PRIu64 "
>> GB",
>> |
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 139 | hw_limit / GiB);
>> | ~~~~~~~~~~~~~~~
>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>> 130 | uint64_t hw_limit;
>> | ^~~~~~~~
>>
>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>> -E2BIG and consequently that code won't be executed.
>>
>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>
>>
>>>
>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>> function `qemu_s390_enable_skeys':
>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>> undefined reference to `s390_get_memory_limit'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>> function `handle_virtio_ccw_notify':
>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>> undefined reference to `virtio_ccw_get_vdev'
>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>> function `handle_storage_limit':
>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>> undefined reference to `s390_get_memory_limit'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>> function `handle_virtio_ccw_notify':
>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>> undefined reference to `virtio_get_queue'
>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>> hypercall.c:52: undefined reference to
>> `virtio_queue_set_shadow_avail_idx'
>>
>> We're building with "--without-default-devices' '--without-default-
>> feature".
>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>
>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>> s390-stattrib.c,
>> s390-hypercall.c, ... which to me is extremely odd.
>>
>> Is this maybe a leftover from the time when we had the old machine
>> type? What value
>> is it to compile all these files without even having a machine that
>> could make use
>> of these?
> The following on top seems to make everything happy. I wish the
> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
> handle odd configs that don't really make sense.
>
>
> I'll do some more testing, then squash the changes into the respective
> patches and resend.
>
>
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 094435cd3b..3bbebfd817 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -12,7 +12,6 @@ s390x_ss.add(files(
> 's390-pci-inst.c',
> 's390-skeys.c',
> 's390-stattrib.c',
> - 's390-hypercall.c',
> 'sclp.c',
> 'sclpcpu.c',
> 'sclpquiesce.c',
> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
> 'tod-tcg.c',
> ))
> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
> virtio-ccw.c'))
> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
> + 's390-virtio-ccw.c',
> + 's390-hypercall.c',
> +))
> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 248566f8dc..097ec78826 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
> uint64_t new_limit)
> {
> - uint64_t hw_limit;
> + uint64_t hw_limit = 0;
> int ret = 0;
>
> assert(!s390ms->memory_limit && new_limit);
> @@ -145,13 +145,6 @@ static void
> s390_set_memory_limit(S390CcwMachineState *s390ms,
> s390ms->memory_limit = new_limit;
> }
>
> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
> -{
> - /* We expect to be called only after the limit was set. */
> - assert(s390ms->memory_limit);
> - return s390ms->memory_limit;
> -}
> -
> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
> uint64_t pagesize)
> {
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-
> virtio-ccw.h
> index 5a730f5d07..599740a998 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
> SCLPDevice *sclp;
> };
>
> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
Pre-existing, I'm surprised this hw/ declaration is used
in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
but due to this call, KVM depends on S390_CCW_VIRTIO...
> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
> +{
> + /* We expect to be called only after the limit was set. */
> + assert(s390ms->memory_limit);
> + return s390ms->memory_limit;
> +}
Short term, no better suggestion than inlining :(
>
> #define S390_PTF_REASON_NONE (0x00 << 8)
> #define S390_PTF_REASON_DONE (0x01 << 8)
> diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/
> misc_helper.c
> index 3732d79185..be1870b07d 100644
> --- a/target/s390x/tcg/misc_helper.c
> +++ b/target/s390x/tcg/misc_helper.c
> @@ -44,6 +44,7 @@
> #include "hw/boards.h"
> #include "hw/s390x/tod.h"
> #endif
> +#include CONFIG_DEVICES
>
> /* #define DEBUG_HELPER */
> #ifdef DEBUG_HELPER
> @@ -117,12 +118,14 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1,
> uint32_t r3, uint32_t num)
>
> switch (num) {
> case 0x500:
> +#ifdef CONFIG_S390_CCW_VIRTIO
> /* QEMU/KVM hypercall */
> bql_lock();
> handle_diag_500(env_archcpu(env), GETPC());
> bql_unlock();
> r = 0;
> break;
> +#endif /* CONFIG_S390_CCW_VIRTIO */
> case 0x44:
> /* yield */
> r = 0;
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 13:04 ` Philippe Mathieu-Daudé
@ 2024-12-19 13:11 ` David Hildenbrand
2024-12-19 14:05 ` David Hildenbrand
0 siblings, 1 reply; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 13:11 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 19/12/24 12:18, David Hildenbrand wrote:
>> On 19.12.24 01:04, David Hildenbrand wrote:
>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>
>
>>>> Please take a look at the following s390x-related CI failures:
>>>
>>> Thanks, most of them seem related to this PULL.
>>>
>>>
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>
>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>> uninitialized [-Werror=maybe-uninitialized]
>>> 138 | error_report("host supports a maximum of %" PRIu64 "
>>> GB",
>>> |
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> 139 | hw_limit / GiB);
>>> | ~~~~~~~~~~~~~~~
>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>> 130 | uint64_t hw_limit;
>>> | ^~~~~~~~
>>>
>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>> -E2BIG and consequently that code won't be executed.
>>>
>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>
>>>
>>>>
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>> function `qemu_s390_enable_skeys':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>> undefined reference to `s390_get_memory_limit'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_virtio_ccw_notify':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>> undefined reference to `virtio_ccw_get_vdev'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_storage_limit':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>> undefined reference to `s390_get_memory_limit'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_virtio_ccw_notify':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>> undefined reference to `virtio_get_queue'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:52: undefined reference to
>>> `virtio_queue_set_shadow_avail_idx'
>>>
>>> We're building with "--without-default-devices' '--without-default-
>>> feature".
>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>
>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>> s390-stattrib.c,
>>> s390-hypercall.c, ... which to me is extremely odd.
>>>
>>> Is this maybe a leftover from the time when we had the old machine
>>> type? What value
>>> is it to compile all these files without even having a machine that
>>> could make use
>>> of these?
>
>
>> The following on top seems to make everything happy. I wish the
>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>> handle odd configs that don't really make sense.
>>
>>
>> I'll do some more testing, then squash the changes into the respective
>> patches and resend.
>>
>>
>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>> index 094435cd3b..3bbebfd817 100644
>> --- a/hw/s390x/meson.build
>> +++ b/hw/s390x/meson.build
>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>> 's390-pci-inst.c',
>> 's390-skeys.c',
>> 's390-stattrib.c',
>> - 's390-hypercall.c',
>> 'sclp.c',
>> 'sclpcpu.c',
>> 'sclpquiesce.c',
>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>> 'tod-tcg.c',
>> ))
>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>> virtio-ccw.c'))
>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>> + 's390-virtio-ccw.c',
>> + 's390-hypercall.c',
>> +))
>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 248566f8dc..097ec78826 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>> uint64_t new_limit)
>> {
>> - uint64_t hw_limit;
>> + uint64_t hw_limit = 0;
>> int ret = 0;
>>
>> assert(!s390ms->memory_limit && new_limit);
>> @@ -145,13 +145,6 @@ static void
>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>> s390ms->memory_limit = new_limit;
>> }
>>
>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>> -{
>> - /* We expect to be called only after the limit was set. */
>> - assert(s390ms->memory_limit);
>> - return s390ms->memory_limit;
>> -}
>> -
>> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>> uint64_t pagesize)
>> {
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-
>> virtio-ccw.h
>> index 5a730f5d07..599740a998 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>> SCLPDevice *sclp;
>> };
>>
>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>
> Pre-existing, I'm surprised this hw/ declaration is used
> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
That is added in patch #12, though.
>
>
> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
Right.
> but due to this call, KVM depends on S390_CCW_VIRTIO...
Right, that's why I opted for inlining for now.
>
>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>> +{
>> + /* We expect to be called only after the limit was set. */
>> + assert(s390ms->memory_limit);
>> + return s390ms->memory_limit;
>> +}
>
> Short term, no better suggestion than inlining :(
Yes. And I suspect we do have similar compilation problems, that simply
nobody noticed so far.
For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
but is called from target/s390x/kvm/kvm.c ...
So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make
the linker unhappy :/ :(
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 13:11 ` David Hildenbrand
@ 2024-12-19 14:05 ` David Hildenbrand
2024-12-19 15:41 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 14:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19.12.24 14:11, David Hildenbrand wrote:
> On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> On 19/12/24 12:18, David Hildenbrand wrote:
>>> On 19.12.24 01:04, David Hildenbrand wrote:
>>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>
>>
>>>>> Please take a look at the following s390x-related CI failures:
>>>>
>>>> Thanks, most of them seem related to this PULL.
>>>>
>>>>
>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>>
>>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>>> uninitialized [-Werror=maybe-uninitialized]
>>>> 138 | error_report("host supports a maximum of %" PRIu64 "
>>>> GB",
>>>> |
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> 139 | hw_limit / GiB);
>>>> | ~~~~~~~~~~~~~~~
>>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>> 130 | uint64_t hw_limit;
>>>> | ^~~~~~~~
>>>>
>>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>>> -E2BIG and consequently that code won't be executed.
>>>>
>>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>>
>>>>
>>>>>
>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>>
>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>>> function `qemu_s390_enable_skeys':
>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>>> undefined reference to `s390_get_memory_limit'
>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>> function `handle_virtio_ccw_notify':
>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>>> undefined reference to `virtio_ccw_get_vdev'
>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>> function `handle_storage_limit':
>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>>> undefined reference to `s390_get_memory_limit'
>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>> function `handle_virtio_ccw_notify':
>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>>> undefined reference to `virtio_get_queue'
>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>> hypercall.c:52: undefined reference to
>>>> `virtio_queue_set_shadow_avail_idx'
>>>>
>>>> We're building with "--without-default-devices' '--without-default-
>>>> feature".
>>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>>
>>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>>> s390-stattrib.c,
>>>> s390-hypercall.c, ... which to me is extremely odd.
>>>>
>>>> Is this maybe a leftover from the time when we had the old machine
>>>> type? What value
>>>> is it to compile all these files without even having a machine that
>>>> could make use
>>>> of these?
>>
>>
>>> The following on top seems to make everything happy. I wish the
>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>> handle odd configs that don't really make sense.
>>>
>>>
>>> I'll do some more testing, then squash the changes into the respective
>>> patches and resend.
>>>
>>>
>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>> index 094435cd3b..3bbebfd817 100644
>>> --- a/hw/s390x/meson.build
>>> +++ b/hw/s390x/meson.build
>>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>> 's390-pci-inst.c',
>>> 's390-skeys.c',
>>> 's390-stattrib.c',
>>> - 's390-hypercall.c',
>>> 'sclp.c',
>>> 'sclpcpu.c',
>>> 'sclpquiesce.c',
>>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>> 'tod-tcg.c',
>>> ))
>>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>>> virtio-ccw.c'))
>>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>> + 's390-virtio-ccw.c',
>>> + 's390-hypercall.c',
>>> +))
>>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 248566f8dc..097ec78826 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>> uint64_t new_limit)
>>> {
>>> - uint64_t hw_limit;
>>> + uint64_t hw_limit = 0;
>>> int ret = 0;
>>>
>>> assert(!s390ms->memory_limit && new_limit);
>>> @@ -145,13 +145,6 @@ static void
>>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>> s390ms->memory_limit = new_limit;
>>> }
>>>
>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>> -{
>>> - /* We expect to be called only after the limit was set. */
>>> - assert(s390ms->memory_limit);
>>> - return s390ms->memory_limit;
>>> -}
>>> -
>>> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>> uint64_t pagesize)
>>> {
>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-
>>> virtio-ccw.h
>>> index 5a730f5d07..599740a998 100644
>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>> SCLPDevice *sclp;
>>> };
>>>
>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>>
>> Pre-existing, I'm surprised this hw/ declaration is used
>> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
>
> That is added in patch #12, though.
>
>>
>>
>> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
>
> Right.
>
>> but due to this call, KVM depends on S390_CCW_VIRTIO...
>
> Right, that's why I opted for inlining for now.
>
>>
>>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>> +{
>>> + /* We expect to be called only after the limit was set. */
>>> + assert(s390ms->memory_limit);
>>> + return s390ms->memory_limit;
>>> +}
>>
>> Short term, no better suggestion than inlining :(
>
> Yes. And I suspect we do have similar compilation problems, that simply
> nobody noticed so far.
>
> For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
> but is called from target/s390x/kvm/kvm.c ...
>
> So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make
> the linker unhappy :/ :(
And indeed with KVM, what a mess.
/usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in function `kvm_s390_set_max_pagesize':
/home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:300: undefined reference to `hpage_1m_allowed'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in function `kvm_arch_init':
/home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:376: undefined reference to `ri_allowed'
/usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381: undefined reference to `cpu_model_allowed'
/usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391: undefined reference to `cpu_model_allowed'
/usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381: undefined reference to `cpu_model_allowed'
/usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391: undefined reference to `cpu_model_allowed'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in function `handle_diag':
/home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:1590: undefined reference to `handle_diag_500'
/usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in function `kvm_s390_cpu_models_supported':
/home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:2354: undefined reference to `cpu_model_allowed'
I can fix the handle_diag_500() similarly up here as done for TCG, although I think
we want to clean this up differently.
Most code doesn't make any sense without an actual s390x machine.
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index dd0322c43a..32cf70bb19 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -51,6 +51,7 @@
#include "hw/s390x/s390-virtio-ccw.h"
#include "hw/s390x/s390-virtio-hcall.h"
#include "target/s390x/kvm/pv.h"
+#include CONFIG_DEVICES
#define kvm_vm_check_mem_attr(s, attr) \
kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
@@ -1494,9 +1495,11 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
{
CPUS390XState *env = &cpu->env;
- int ret;
+ int ret = -EINVAL;
+#ifdef CONFIG_S390_CCW_VIRTIO
ret = s390_virtio_hypercall(env);
+#endif /* CONFIG_S390_CCW_VIRTIO */
if (ret == -EINVAL) {
kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
return 0;
stupid "none"-only configs that probably nobody needs ...
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 14:05 ` David Hildenbrand
@ 2024-12-19 15:41 ` Philippe Mathieu-Daudé
2024-12-19 15:45 ` David Hildenbrand
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-19 15:41 UTC (permalink / raw)
To: David Hildenbrand, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19/12/24 15:05, David Hildenbrand wrote:
> On 19.12.24 14:11, David Hildenbrand wrote:
>> On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
>>> Hi,
>>>
>>> On 19/12/24 12:18, David Hildenbrand wrote:
>>>> On 19.12.24 01:04, David Hildenbrand wrote:
>>>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>>
>>>
>>>>>> Please take a look at the following s390x-related CI failures:
>>>>>
>>>>> Thanks, most of them seem related to this PULL.
>>>>>
>>>>>
>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>>>
>>>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>> 138 | error_report("host supports a maximum of %"
>>>>> PRIu64 "
>>>>> GB",
>>>>> |
>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> 139 | hw_limit / GiB);
>>>>> | ~~~~~~~~~~~~~~~
>>>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>>> 130 | uint64_t hw_limit;
>>>>> | ^~~~~~~~
>>>>>
>>>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>>>> -E2BIG and consequently that code won't be executed.
>>>>>
>>>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>>>
>>>>>
>>>>>>
>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>>>
>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>>>> function `qemu_s390_enable_skeys':
>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>>>> undefined reference to `s390_get_memory_limit'
>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>> function `handle_virtio_ccw_notify':
>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>>>> undefined reference to `virtio_ccw_get_vdev'
>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>> function `handle_storage_limit':
>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>>>> undefined reference to `s390_get_memory_limit'
>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>> function `handle_virtio_ccw_notify':
>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>>>> undefined reference to `virtio_get_queue'
>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>> hypercall.c:52: undefined reference to
>>>>> `virtio_queue_set_shadow_avail_idx'
>>>>>
>>>>> We're building with "--without-default-devices' '--without-default-
>>>>> feature".
>>>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>>>
>>>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>>>> s390-stattrib.c,
>>>>> s390-hypercall.c, ... which to me is extremely odd.
>>>>>
>>>>> Is this maybe a leftover from the time when we had the old machine
>>>>> type? What value
>>>>> is it to compile all these files without even having a machine that
>>>>> could make use
>>>>> of these?
>>>
>>>
>>>> The following on top seems to make everything happy. I wish the
>>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>>> handle odd configs that don't really make sense.
>>>>
>>>>
>>>> I'll do some more testing, then squash the changes into the respective
>>>> patches and resend.
>>>>
>>>>
>>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>>> index 094435cd3b..3bbebfd817 100644
>>>> --- a/hw/s390x/meson.build
>>>> +++ b/hw/s390x/meson.build
>>>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>>> 's390-pci-inst.c',
>>>> 's390-skeys.c',
>>>> 's390-stattrib.c',
>>>> - 's390-hypercall.c',
>>>> 'sclp.c',
>>>> 'sclpcpu.c',
>>>> 'sclpquiesce.c',
>>>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>> 'tod-tcg.c',
>>>> ))
>>>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>>>> virtio-ccw.c'))
>>>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>>> + 's390-virtio-ccw.c',
>>>> + 's390-hypercall.c',
>>>> +))
>>>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-
>>>> ccw.c'))
>>>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-
>>>> vfio.c'))
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 248566f8dc..097ec78826 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>>> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>> uint64_t new_limit)
>>>> {
>>>> - uint64_t hw_limit;
>>>> + uint64_t hw_limit = 0;
>>>> int ret = 0;
>>>>
>>>> assert(!s390ms->memory_limit && new_limit);
>>>> @@ -145,13 +145,6 @@ static void
>>>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>> s390ms->memory_limit = new_limit;
>>>> }
>>>>
>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>>> -{
>>>> - /* We expect to be called only after the limit was set. */
>>>> - assert(s390ms->memory_limit);
>>>> - return s390ms->memory_limit;
>>>> -}
>>>> -
>>>> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>>> uint64_t pagesize)
>>>> {
>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/
>>>> s390-
>>>> virtio-ccw.h
>>>> index 5a730f5d07..599740a998 100644
>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>>> SCLPDevice *sclp;
>>>> };
>>>>
>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>>>
>>> Pre-existing, I'm surprised this hw/ declaration is used
>>> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
>>
>> That is added in patch #12, though.
>>
>>>
>>>
>>> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
>>
>> Right.
>>
>>> but due to this call, KVM depends on S390_CCW_VIRTIO...
>>
>> Right, that's why I opted for inlining for now.
>>
>>>
>>>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState
>>>> *s390ms)
>>>> +{
>>>> + /* We expect to be called only after the limit was set. */
>>>> + assert(s390ms->memory_limit);
>>>> + return s390ms->memory_limit;
>>>> +}
>>>
>>> Short term, no better suggestion than inlining :(
>>
>> Yes. And I suspect we do have similar compilation problems, that simply
>> nobody noticed so far.
>>
>> For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
>> but is called from target/s390x/kvm/kvm.c ...
>>
>> So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make
>> the linker unhappy :/ :(
>
> And indeed with KVM, what a mess.
>
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
> function `kvm_s390_set_max_pagesize':
> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:300: undefined
> reference to `hpage_1m_allowed'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
> function `kvm_arch_init':
> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:376: undefined
> reference to `ri_allowed'
> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
> undefined reference to `cpu_model_allowed'
> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
> undefined reference to `cpu_model_allowed'
> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
> undefined reference to `cpu_model_allowed'
> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
> undefined reference to `cpu_model_allowed'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
> function `handle_diag':
> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:1590: undefined
> reference to `handle_diag_500'
> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
> function `kvm_s390_cpu_models_supported':
> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:2354: undefined
> reference to `cpu_model_allowed'
>
> I can fix the handle_diag_500() similarly up here as done for TCG,
> although I think
> we want to clean this up differently.
>
> Most code doesn't make any sense without an actual s390x machine.
Agreed, don't waste time on that now, I'll likely have a look at
that in few months.
>
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index dd0322c43a..32cf70bb19 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -51,6 +51,7 @@
> #include "hw/s390x/s390-virtio-ccw.h"
> #include "hw/s390x/s390-virtio-hcall.h"
> #include "target/s390x/kvm/pv.h"
> +#include CONFIG_DEVICES
>
> #define kvm_vm_check_mem_attr(s, attr) \
> kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
> @@ -1494,9 +1495,11 @@ static int handle_e3(S390CPU *cpu, struct kvm_run
> *run, uint8_t ipbl)
> static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
> {
> CPUS390XState *env = &cpu->env;
> - int ret;
> + int ret = -EINVAL;
>
> +#ifdef CONFIG_S390_CCW_VIRTIO
> ret = s390_virtio_hypercall(env);
> +#endif /* CONFIG_S390_CCW_VIRTIO */
> if (ret == -EINVAL) {
> kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> return 0;
>
> stupid "none"-only configs that probably nobody needs ...
I'm using it as starting point for heterogeneous machines...
This is why I noticed your PR comment and jumped in =)
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 15:41 ` Philippe Mathieu-Daudé
@ 2024-12-19 15:45 ` David Hildenbrand
2024-12-19 15:48 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 30+ messages in thread
From: David Hildenbrand @ 2024-12-19 15:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19.12.24 16:41, Philippe Mathieu-Daudé wrote:
> On 19/12/24 15:05, David Hildenbrand wrote:
>> On 19.12.24 14:11, David Hildenbrand wrote:
>>> On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
>>>> Hi,
>>>>
>>>> On 19/12/24 12:18, David Hildenbrand wrote:
>>>>> On 19.12.24 01:04, David Hildenbrand wrote:
>>>>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>>>
>>>>
>>>>>>> Please take a look at the following s390x-related CI failures:
>>>>>>
>>>>>> Thanks, most of them seem related to this PULL.
>>>>>>
>>>>>>
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>>>>
>>>>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>>>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>>> 138 | error_report("host supports a maximum of %"
>>>>>> PRIu64 "
>>>>>> GB",
>>>>>> |
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> 139 | hw_limit / GiB);
>>>>>> | ~~~~~~~~~~~~~~~
>>>>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>>>> 130 | uint64_t hw_limit;
>>>>>> | ^~~~~~~~
>>>>>>
>>>>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>>>>> -E2BIG and consequently that code won't be executed.
>>>>>>
>>>>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>>>>
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>>>>> function `qemu_s390_enable_skeys':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_virtio_ccw_notify':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>>>>> undefined reference to `virtio_ccw_get_vdev'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_storage_limit':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_virtio_ccw_notify':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>>>>> undefined reference to `virtio_get_queue'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:52: undefined reference to
>>>>>> `virtio_queue_set_shadow_avail_idx'
>>>>>>
>>>>>> We're building with "--without-default-devices' '--without-default-
>>>>>> feature".
>>>>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>>>>
>>>>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>>>>> s390-stattrib.c,
>>>>>> s390-hypercall.c, ... which to me is extremely odd.
>>>>>>
>>>>>> Is this maybe a leftover from the time when we had the old machine
>>>>>> type? What value
>>>>>> is it to compile all these files without even having a machine that
>>>>>> could make use
>>>>>> of these?
>>>>
>>>>
>>>>> The following on top seems to make everything happy. I wish the
>>>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>>>> handle odd configs that don't really make sense.
>>>>>
>>>>>
>>>>> I'll do some more testing, then squash the changes into the respective
>>>>> patches and resend.
>>>>>
>>>>>
>>>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>>>> index 094435cd3b..3bbebfd817 100644
>>>>> --- a/hw/s390x/meson.build
>>>>> +++ b/hw/s390x/meson.build
>>>>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>>>> 's390-pci-inst.c',
>>>>> 's390-skeys.c',
>>>>> 's390-stattrib.c',
>>>>> - 's390-hypercall.c',
>>>>> 'sclp.c',
>>>>> 'sclpcpu.c',
>>>>> 'sclpquiesce.c',
>>>>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>>> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>>> 'tod-tcg.c',
>>>>> ))
>>>>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>>>>> virtio-ccw.c'))
>>>>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>>>> + 's390-virtio-ccw.c',
>>>>> + 's390-hypercall.c',
>>>>> +))
>>>>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-
>>>>> ccw.c'))
>>>>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-
>>>>> vfio.c'))
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index 248566f8dc..097ec78826 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>>>> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>> uint64_t new_limit)
>>>>> {
>>>>> - uint64_t hw_limit;
>>>>> + uint64_t hw_limit = 0;
>>>>> int ret = 0;
>>>>>
>>>>> assert(!s390ms->memory_limit && new_limit);
>>>>> @@ -145,13 +145,6 @@ static void
>>>>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>> s390ms->memory_limit = new_limit;
>>>>> }
>>>>>
>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>>>> -{
>>>>> - /* We expect to be called only after the limit was set. */
>>>>> - assert(s390ms->memory_limit);
>>>>> - return s390ms->memory_limit;
>>>>> -}
>>>>> -
>>>>> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>>>> uint64_t pagesize)
>>>>> {
>>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/
>>>>> s390-
>>>>> virtio-ccw.h
>>>>> index 5a730f5d07..599740a998 100644
>>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>>>> SCLPDevice *sclp;
>>>>> };
>>>>>
>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>>>>
>>>> Pre-existing, I'm surprised this hw/ declaration is used
>>>> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
>>>
>>> That is added in patch #12, though.
>>>
>>>>
>>>>
>>>> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
>>>
>>> Right.
>>>
>>>> but due to this call, KVM depends on S390_CCW_VIRTIO...
>>>
>>> Right, that's why I opted for inlining for now.
>>>
>>>>
>>>>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState
>>>>> *s390ms)
>>>>> +{
>>>>> + /* We expect to be called only after the limit was set. */
>>>>> + assert(s390ms->memory_limit);
>>>>> + return s390ms->memory_limit;
>>>>> +}
>>>>
>>>> Short term, no better suggestion than inlining :(
>>>
>>> Yes. And I suspect we do have similar compilation problems, that simply
>>> nobody noticed so far.
>>>
>>> For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
>>> but is called from target/s390x/kvm/kvm.c ...
>>>
>>> So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make
>>> the linker unhappy :/ :(
>>
>> And indeed with KVM, what a mess.
>>
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_s390_set_max_pagesize':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:300: undefined
>> reference to `hpage_1m_allowed'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_arch_init':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:376: undefined
>> reference to `ri_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `handle_diag':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:1590: undefined
>> reference to `handle_diag_500'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_s390_cpu_models_supported':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:2354: undefined
>> reference to `cpu_model_allowed'
>>
>> I can fix the handle_diag_500() similarly up here as done for TCG,
>> although I think
>> we want to clean this up differently.
>>
>> Most code doesn't make any sense without an actual s390x machine.
>
> Agreed, don't waste time on that now, I'll likely have a look at
> that in few months.
Great, thanks!
>
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index dd0322c43a..32cf70bb19 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -51,6 +51,7 @@
>> #include "hw/s390x/s390-virtio-ccw.h"
>> #include "hw/s390x/s390-virtio-hcall.h"
>> #include "target/s390x/kvm/pv.h"
>> +#include CONFIG_DEVICES
>>
>> #define kvm_vm_check_mem_attr(s, attr) \
>> kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
>> @@ -1494,9 +1495,11 @@ static int handle_e3(S390CPU *cpu, struct kvm_run
>> *run, uint8_t ipbl)
>> static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>> {
>> CPUS390XState *env = &cpu->env;
>> - int ret;
>> + int ret = -EINVAL;
>>
>> +#ifdef CONFIG_S390_CCW_VIRTIO
>> ret = s390_virtio_hypercall(env);
>> +#endif /* CONFIG_S390_CCW_VIRTIO */
>> if (ret == -EINVAL) {
>> kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
>> return 0;
>>
>> stupid "none"-only configs that probably nobody needs ...
>
> I'm using it as starting point for heterogeneous machines...
> This is why I noticed your PR comment and jumped in =)
I assume you mean not having multiple-machines per QEMU (meaning:
single-binary instead of per-arch binaries), but a single machine that
comprises multiple architectures? (like, having an arm and a riscv core)?
I see how that can be useful, but not necessarily with s390x in the
pitcure ... :)
In any case, thanks for your comments!
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
2024-12-19 15:45 ` David Hildenbrand
@ 2024-12-19 15:48 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-12-19 15:48 UTC (permalink / raw)
To: David Hildenbrand, Stefan Hajnoczi
Cc: qemu-devel, Halil Pasic, Christian Borntraeger, Eric Farman,
Thomas Huth, Richard Henderson, Michael S . Tsirkin
On 19/12/24 16:45, David Hildenbrand wrote:
> On 19.12.24 16:41, Philippe Mathieu-Daudé wrote:
>> On 19/12/24 15:05, David Hildenbrand wrote:
>>> On 19.12.24 14:11, David Hildenbrand wrote:
>>>> On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
>>>>> Hi,
>>>>>
>>>>> On 19/12/24 12:18, David Hildenbrand wrote:
>>>>>> On 19.12.24 01:04, David Hildenbrand wrote:
>>>>>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>>>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>>>>
>>>>>
>>>>>>>> Please take a look at the following s390x-related CI failures:
>>>>>>>
>>>>>>> Thanks, most of them seem related to this PULL.
>>>>>>>
>>>>>>>
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>>>>>
>>>>>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>>>>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>>>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>>>> 138 | error_report("host supports a maximum of %"
>>>>>>> PRIu64 "
>>>>>>> GB",
>>>>>>> |
>>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>> 139 | hw_limit / GiB);
>>>>>>> | ~~~~~~~~~~~~~~~
>>>>>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>>>>> 130 | uint64_t hw_limit;
>>>>>>> | ^~~~~~~~
>>>>>>>
>>>>>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>>>>>> -E2BIG and consequently that code won't be executed.
>>>>>>>
>>>>>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>>>>>
>>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>>>>>> function `qemu_s390_enable_skeys':
>>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-
>>>>>>> hypercall.c.o: in
>>>>>>> function `handle_virtio_ccw_notify':
>>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>>>>>> undefined reference to `virtio_ccw_get_vdev'
>>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-
>>>>>>> hypercall.c.o: in
>>>>>>> function `handle_storage_limit':
>>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-
>>>>>>> hypercall.c.o: in
>>>>>>> function `handle_virtio_ccw_notify':
>>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>>>>>> undefined reference to `virtio_get_queue'
>>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>>> hypercall.c:52: undefined reference to
>>>>>>> `virtio_queue_set_shadow_avail_idx'
>>>>>>>
>>>>>>> We're building with "--without-default-devices' '--without-default-
>>>>>>> feature".
>>>>>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>>>>>
>>>>>>> So we won't compile s390-virtio-ccw.c, but we will compile things
>>>>>>> like
>>>>>>> s390-stattrib.c,
>>>>>>> s390-hypercall.c, ... which to me is extremely odd.
>>>>>>>
>>>>>>> Is this maybe a leftover from the time when we had the old machine
>>>>>>> type? What value
>>>>>>> is it to compile all these files without even having a machine that
>>>>>>> could make use
>>>>>>> of these?
>>>>>
>>>>>
>>>>>> The following on top seems to make everything happy. I wish the
>>>>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated,
>>>>>> just to
>>>>>> handle odd configs that don't really make sense.
>>>>>>
>>>>>>
>>>>>> I'll do some more testing, then squash the changes into the
>>>>>> respective
>>>>>> patches and resend.
>>>>>>
>>>>>>
>>>>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>>>>> index 094435cd3b..3bbebfd817 100644
>>>>>> --- a/hw/s390x/meson.build
>>>>>> +++ b/hw/s390x/meson.build
>>>>>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>>>>> 's390-pci-inst.c',
>>>>>> 's390-skeys.c',
>>>>>> 's390-stattrib.c',
>>>>>> - 's390-hypercall.c',
>>>>>> 'sclp.c',
>>>>>> 'sclpcpu.c',
>>>>>> 'sclpquiesce.c',
>>>>>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>>>> s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>>>> 'tod-tcg.c',
>>>>>> ))
>>>>>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>>>>>> virtio-ccw.c'))
>>>>>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>>>>> + 's390-virtio-ccw.c',
>>>>>> + 's390-hypercall.c',
>>>>>> +))
>>>>>> s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-
>>>>>> ccw.c'))
>>>>>> s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-
>>>>>> vfio.c'))
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>> index 248566f8dc..097ec78826 100644
>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>>>>> static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>>> uint64_t new_limit)
>>>>>> {
>>>>>> - uint64_t hw_limit;
>>>>>> + uint64_t hw_limit = 0;
>>>>>> int ret = 0;
>>>>>>
>>>>>> assert(!s390ms->memory_limit && new_limit);
>>>>>> @@ -145,13 +145,6 @@ static void
>>>>>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>>> s390ms->memory_limit = new_limit;
>>>>>> }
>>>>>>
>>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>>>>> -{
>>>>>> - /* We expect to be called only after the limit was set. */
>>>>>> - assert(s390ms->memory_limit);
>>>>>> - return s390ms->memory_limit;
>>>>>> -}
>>>>>> -
>>>>>> static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>>>>> uint64_t pagesize)
>>>>>> {
>>>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/
>>>>>> s390-
>>>>>> virtio-ccw.h
>>>>>> index 5a730f5d07..599740a998 100644
>>>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>>>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>>>>> SCLPDevice *sclp;
>>>>>> };
>>>>>>
>>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>>>>>
>>>>> Pre-existing, I'm surprised this hw/ declaration is used
>>>>> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
>>>>
>>>> That is added in patch #12, though.
>>>>
>>>>>
>>>>>
>>>>> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
>>>>
>>>> Right.
>>>>
>>>>> but due to this call, KVM depends on S390_CCW_VIRTIO...
>>>>
>>>> Right, that's why I opted for inlining for now.
>>>>
>>>>>
>>>>>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState
>>>>>> *s390ms)
>>>>>> +{
>>>>>> + /* We expect to be called only after the limit was set. */
>>>>>> + assert(s390ms->memory_limit);
>>>>>> + return s390ms->memory_limit;
>>>>>> +}
>>>>>
>>>>> Short term, no better suggestion than inlining :(
>>>>
>>>> Yes. And I suspect we do have similar compilation problems, that simply
>>>> nobody noticed so far.
>>>>
>>>> For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
>>>> but is called from target/s390x/kvm/kvm.c ...
>>>>
>>>> So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should
>>>> make
>>>> the linker unhappy :/ :(
>>>
>>> And indeed with KVM, what a mess.
>>>
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>>> function `kvm_s390_set_max_pagesize':
>>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:300: undefined
>>> reference to `hpage_1m_allowed'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>>> function `kvm_arch_init':
>>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:376: undefined
>>> reference to `ri_allowed'
>>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>>> undefined reference to `cpu_model_allowed'
>>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>>> undefined reference to `cpu_model_allowed'
>>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>>> undefined reference to `cpu_model_allowed'
>>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>>> undefined reference to `cpu_model_allowed'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>>> function `handle_diag':
>>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:1590: undefined
>>> reference to `handle_diag_500'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>>> function `kvm_s390_cpu_models_supported':
>>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:2354: undefined
>>> reference to `cpu_model_allowed'
>>>
>>> I can fix the handle_diag_500() similarly up here as done for TCG,
>>> although I think
>>> we want to clean this up differently.
>>>
>>> Most code doesn't make any sense without an actual s390x machine.
>>
>> Agreed, don't waste time on that now, I'll likely have a look at
>> that in few months.
>
> Great, thanks!
>
>>
>>>
>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>> index dd0322c43a..32cf70bb19 100644
>>> --- a/target/s390x/kvm/kvm.c
>>> +++ b/target/s390x/kvm/kvm.c
>>> @@ -51,6 +51,7 @@
>>> #include "hw/s390x/s390-virtio-ccw.h"
>>> #include "hw/s390x/s390-virtio-hcall.h"
>>> #include "target/s390x/kvm/pv.h"
>>> +#include CONFIG_DEVICES
>>>
>>> #define kvm_vm_check_mem_attr(s, attr) \
>>> kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
>>> @@ -1494,9 +1495,11 @@ static int handle_e3(S390CPU *cpu, struct kvm_run
>>> *run, uint8_t ipbl)
>>> static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>>> {
>>> CPUS390XState *env = &cpu->env;
>>> - int ret;
>>> + int ret = -EINVAL;
>>>
>>> +#ifdef CONFIG_S390_CCW_VIRTIO
>>> ret = s390_virtio_hypercall(env);
>>> +#endif /* CONFIG_S390_CCW_VIRTIO */
>>> if (ret == -EINVAL) {
>>> kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
>>> return 0;
>>>
>>> stupid "none"-only configs that probably nobody needs ...
>>
>> I'm using it as starting point for heterogeneous machines...
>> This is why I noticed your PR comment and jumped in =)
>
> I assume you mean not having multiple-machines per QEMU (meaning:
> single-binary instead of per-arch binaries), but a single machine that
> comprises multiple architectures? (like, having an arm and a riscv core)?
Yes, mostly.
> I see how that can be useful, but not necessarily with s390x in the
> pitcure ... :)
You never know salesman creativity =)
> In any case, thanks for your comments!
:) Regards,
Phil.
^ permalink raw reply [flat|nested] 30+ messages in thread