| Message ID | 20241218105303.1966303-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
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 > >
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.
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;
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?
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
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)
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
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;
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; >
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 :/ :(
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 ...
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 =)
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!
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.
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 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