Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18

[David Hildenbrand]

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