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

[David Hildenbrand <david@redhat.com>]

On 19.12.24 16:41, Philippe Mathieu-Daudé wrote:
> On 19/12/24 15:05, David Hildenbrand wrote:
>> On 19.12.24 14:11, David Hildenbrand wrote:
>>> On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
>>>> Hi,
>>>>
>>>> On 19/12/24 12:18, David Hildenbrand wrote:
>>>>> On 19.12.24 01:04, David Hildenbrand wrote:
>>>>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
>>>>
>>>>
>>>>>>> Please take a look at the following s390x-related CI failures:
>>>>>>
>>>>>> Thanks, most of them seem related to this PULL.
>>>>>>
>>>>>>
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>>>>
>>>>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>>>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>>>>> uninitialized [-Werror=maybe-uninitialized]
>>>>>>        138 |         error_report("host supports a maximum of %"
>>>>>> PRIu64 "
>>>>>> GB",
>>>>>>            |
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>        139 |                      hw_limit / GiB);
>>>>>>            |                      ~~~~~~~~~~~~~~~
>>>>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>>>>        130 |     uint64_t hw_limit;
>>>>>>            |              ^~~~~~~~
>>>>>>
>>>>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>>>>> -E2BIG and consequently that code won't be executed.
>>>>>>
>>>>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>>>>
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>>>>> function `qemu_s390_enable_skeys':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_virtio_ccw_notify':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>>>>> undefined reference to `virtio_ccw_get_vdev'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_storage_limit':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>>>>> undefined reference to `s390_get_memory_limit'
>>>>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>>>>> function `handle_virtio_ccw_notify':
>>>>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>>>>> undefined reference to `virtio_get_queue'
>>>>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>>>>> hypercall.c:52: undefined reference to
>>>>>> `virtio_queue_set_shadow_avail_idx'
>>>>>>
>>>>>> We're building with "--without-default-devices' '--without-default-
>>>>>> feature".
>>>>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>>>>
>>>>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>>>>> s390-stattrib.c,
>>>>>> s390-hypercall.c, ... which to me is extremely odd.
>>>>>>
>>>>>> Is this maybe a leftover from the time when we had the old machine
>>>>>> type? What value
>>>>>> is it to compile all these files without even having a machine that
>>>>>> could make use
>>>>>> of these?
>>>>
>>>>
>>>>> The following on top seems to make everything happy. I wish the
>>>>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>>>>> handle odd configs that don't really make sense.
>>>>>
>>>>>
>>>>> I'll do some more testing, then squash the changes into the respective
>>>>> patches and resend.
>>>>>
>>>>>
>>>>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>>>>> index 094435cd3b..3bbebfd817 100644
>>>>> --- a/hw/s390x/meson.build
>>>>> +++ b/hw/s390x/meson.build
>>>>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>>>>        's390-pci-inst.c',
>>>>>        's390-skeys.c',
>>>>>        's390-stattrib.c',
>>>>> -  's390-hypercall.c',
>>>>>        'sclp.c',
>>>>>        'sclpcpu.c',
>>>>>        'sclpquiesce.c',
>>>>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>>>>      s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>>>>        'tod-tcg.c',
>>>>>      ))
>>>>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>>>>> virtio-ccw.c'))
>>>>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>>>>> +  's390-virtio-ccw.c',
>>>>> +  's390-hypercall.c',
>>>>> +))
>>>>>      s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-
>>>>> ccw.c'))
>>>>>      s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-
>>>>> vfio.c'))
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index 248566f8dc..097ec78826 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>>>>      static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>>                                        uint64_t new_limit)
>>>>>      {
>>>>> -    uint64_t hw_limit;
>>>>> +    uint64_t hw_limit = 0;
>>>>>          int ret = 0;
>>>>>
>>>>>          assert(!s390ms->memory_limit && new_limit);
>>>>> @@ -145,13 +145,6 @@ static void
>>>>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>>>>          s390ms->memory_limit = new_limit;
>>>>>      }
>>>>>
>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>>>>> -{
>>>>> -    /* We expect to be called only after the limit was set. */
>>>>> -    assert(s390ms->memory_limit);
>>>>> -    return s390ms->memory_limit;
>>>>> -}
>>>>> -
>>>>>      static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>>>>                                        uint64_t pagesize)
>>>>>      {
>>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/
>>>>> s390-
>>>>> virtio-ccw.h
>>>>> index 5a730f5d07..599740a998 100644
>>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>>>>          SCLPDevice *sclp;
>>>>>      };
>>>>>
>>>>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
>>>>
>>>> Pre-existing, I'm surprised this hw/ declaration is used
>>>> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.
>>>
>>> That is added in patch #12, though.
>>>
>>>>
>>>>
>>>> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,
>>>
>>> Right.
>>>
>>>> but due to this call, KVM depends on S390_CCW_VIRTIO...
>>>
>>> Right, that's why I opted for inlining for now.
>>>
>>>>
>>>>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState
>>>>> *s390ms)
>>>>> +{
>>>>> +    /* We expect to be called only after the limit was set. */
>>>>> +    assert(s390ms->memory_limit);
>>>>> +    return s390ms->memory_limit;
>>>>> +}
>>>>
>>>> Short term, no better suggestion than inlining :(
>>>
>>> Yes. And I suspect we do have similar compilation problems, that simply
>>> nobody noticed so far.
>>>
>>> For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c,
>>> but is called from target/s390x/kvm/kvm.c ...
>>>
>>> So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make
>>> the linker unhappy :/ :(
>>
>> And indeed with KVM, what a mess.
>>
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_s390_set_max_pagesize':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:300: undefined
>> reference to `hpage_1m_allowed'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_arch_init':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:376: undefined
>> reference to `ri_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:381:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:391:
>> undefined reference to `cpu_model_allowed'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `handle_diag':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:1590: undefined
>> reference to `handle_diag_500'
>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/target_s390x_kvm_kvm.c.o: in
>> function `kvm_s390_cpu_models_supported':
>> /home/dhildenb/qemu/build/../target/s390x/kvm/kvm.c:2354: undefined
>> reference to `cpu_model_allowed'
>>
>> I can fix the handle_diag_500() similarly up here as done for TCG,
>> although I think
>> we want to clean this up differently.
>>
>> Most code doesn't make any sense without an actual s390x machine.
> 
> Agreed, don't waste time on that now, I'll likely have a look at
> that in few months.

Great, thanks!

> 
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index dd0322c43a..32cf70bb19 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -51,6 +51,7 @@
>>    #include "hw/s390x/s390-virtio-ccw.h"
>>    #include "hw/s390x/s390-virtio-hcall.h"
>>    #include "target/s390x/kvm/pv.h"
>> +#include CONFIG_DEVICES
>>
>>    #define kvm_vm_check_mem_attr(s, attr) \
>>        kvm_vm_check_attr(s, KVM_S390_VM_MEM_CTRL, attr)
>> @@ -1494,9 +1495,11 @@ static int handle_e3(S390CPU *cpu, struct kvm_run
>> *run, uint8_t ipbl)
>>    static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>>    {
>>        CPUS390XState *env = &cpu->env;
>> -    int ret;
>> +    int ret = -EINVAL;
>>
>> +#ifdef CONFIG_S390_CCW_VIRTIO
>>        ret = s390_virtio_hypercall(env);
>> +#endif /* CONFIG_S390_CCW_VIRTIO */
>>        if (ret == -EINVAL) {
>>            kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
>>            return 0;
>>
>> stupid "none"-only configs that probably nobody needs ...
> 
> I'm using it as starting point for heterogeneous machines...
> This is why I noticed your PR comment and jumped in =)

I assume you mean not having multiple-machines per QEMU (meaning: 
single-binary instead of per-arch binaries), but a single machine that 
comprises multiple architectures? (like, having an arm and a riscv core)?

I see how that can be useful, but not necessarily with s390x in the 
pitcure ... :)

In any case, thanks for your comments!

-- 
Cheers,

David / dhildenb