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 15:05:47 +0100
User-agent: Mozilla Thunderbird

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




reply via email to

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