qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Hildenbrand
Subject: Re: [PULL 00/15] Host Memory Backends and Memory devices queue 2024-12-18
Date: Thu, 19 Dec 2024 14:11:07 +0100
User-agent: Mozilla Thunderbird

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]