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

[David Hildenbrand <david@redhat.com>]

On 19.12.24 14:04, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 19/12/24 12:18, David Hildenbrand wrote:
>> On 19.12.24 01:04, David Hildenbrand wrote:
>>> On 18.12.24 22:09, Stefan Hajnoczi wrote:
>>>> On Wed, 18 Dec 2024 at 05:55, David Hildenbrand <david@redhat.com>
> 
> 
>>>> Please take a look at the following s390x-related CI failures:
>>>
>>> Thanks, most of them seem related to this PULL.
>>>
>>>
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972912
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972809
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972931
>>>
>>> ../hw/s390x/s390-virtio-ccw.c: In function ‘s390_set_memory_limit’:
>>> ../hw/s390x/s390-virtio-ccw.c:138:9: error: ‘hw_limit’ may be used
>>> uninitialized [-Werror=maybe-uninitialized]
>>>      138 |         error_report("host supports a maximum of %" PRIu64 "
>>> GB",
>>>          |
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>      139 |                      hw_limit / GiB);
>>>          |                      ~~~~~~~~~~~~~~~
>>> ../hw/s390x/s390-virtio-ccw.c:130:14: note: ‘hw_limit’ declared here
>>>      130 |     uint64_t hw_limit;
>>>          |              ^~~~~~~~
>>>
>>> Looks weird. Without kvm_enabled() ret = 0, so ret cannot be
>>> -E2BIG and consequently that code won't be executed.
>>>
>>> Anyhow, I'll simply initialize hw_limit to 0 to silence the warning.
>>>
>>>
>>>>
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/8679972861
>>>
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-skeys.c.o: in
>>> function `qemu_s390_enable_skeys':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-skeys.c:256:
>>> undefined reference to `s390_get_memory_limit'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_virtio_ccw_notify':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:46:
>>> undefined reference to `virtio_ccw_get_vdev'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:47: undefined reference to `virtio_queue_get_num'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:56: undefined reference to `virtio_queue_notify'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_storage_limit':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:64:
>>> undefined reference to `s390_get_memory_limit'
>>> /usr/bin/ld: libqemu-s390x-softmmu.a.p/hw_s390x_s390-hypercall.c.o: in
>>> function `handle_virtio_ccw_notify':
>>> /builds/qemu-project/qemu/build/../hw/s390x/s390-hypercall.c:52:
>>> undefined reference to `virtio_get_queue'
>>> /usr/bin/ld: /builds/qemu-project/qemu/build/../hw/s390x/s390-
>>> hypercall.c:52: undefined reference to
>>> `virtio_queue_set_shadow_avail_idx'
>>>
>>> We're building with "--without-default-devices' '--without-default-
>>> feature".
>>> Consequently, we won't even have CONFIG_S390_CCW_VIRTIO
>>>
>>> So we won't compile s390-virtio-ccw.c, but we will compile things like
>>> s390-stattrib.c,
>>> s390-hypercall.c, ... which to me is extremely odd.
>>>
>>> Is this maybe a leftover from the time when we had the old machine
>>> type? What value
>>> is it to compile all these files without even having a machine that
>>> could make use
>>> of these?
> 
> 
>> The following on top seems to make everything happy. I wish the
>> CONFIG_S390_CCW_VIRTIO stuff would't have to be so complicated, just to
>> handle odd configs that don't really make sense.
>>
>>
>> I'll do some more testing, then squash the changes into the respective
>> patches and resend.
>>
>>
>> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
>> index 094435cd3b..3bbebfd817 100644
>> --- a/hw/s390x/meson.build
>> +++ b/hw/s390x/meson.build
>> @@ -12,7 +12,6 @@ s390x_ss.add(files(
>>      's390-pci-inst.c',
>>      's390-skeys.c',
>>      's390-stattrib.c',
>> -  's390-hypercall.c',
>>      'sclp.c',
>>      'sclpcpu.c',
>>      'sclpquiesce.c',
>> @@ -28,7 +27,10 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>>    s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>>      'tod-tcg.c',
>>    ))
>> -s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files('s390-
>> virtio-ccw.c'))
>> +s390x_ss.add(when: 'CONFIG_S390_CCW_VIRTIO', if_true: files(
>> +  's390-virtio-ccw.c',
>> +  's390-hypercall.c',
>> +))
>>    s390x_ss.add(when: 'CONFIG_TERMINAL3270', if_true: files('3270-ccw.c'))
>>    s390x_ss.add(when: 'CONFIG_VFIO', if_true: files('s390-pci-vfio.c'))
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 248566f8dc..097ec78826 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -127,7 +127,7 @@ static void subsystem_reset(void)
>>    static void s390_set_memory_limit(S390CcwMachineState *s390ms,
>>                                      uint64_t new_limit)
>>    {
>> -    uint64_t hw_limit;
>> +    uint64_t hw_limit = 0;
>>        int ret = 0;
>>
>>        assert(!s390ms->memory_limit && new_limit);
>> @@ -145,13 +145,6 @@ static void
>> s390_set_memory_limit(S390CcwMachineState *s390ms,
>>        s390ms->memory_limit = new_limit;
>>    }
>>
>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>> -{
>> -    /* We expect to be called only after the limit was set. */
>> -    assert(s390ms->memory_limit);
>> -    return s390ms->memory_limit;
>> -}
>> -
>>    static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
>>                                      uint64_t pagesize)
>>    {
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-
>> virtio-ccw.h
>> index 5a730f5d07..599740a998 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -35,7 +35,12 @@ struct S390CcwMachineState {
>>        SCLPDevice *sclp;
>>    };
>>
>> -uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms);
> 
> Pre-existing, I'm surprised this hw/ declaration is used
> in s390_pv_vm_try_disable_async() in target/s390x/kvm/pv.c.

That is added in patch #12, though.

> 
> 
> In hw/s390x/Kconfig, S390_CCW_VIRTIO doesn't depend on KVM,

Right.

> but due to this call, KVM depends on S390_CCW_VIRTIO...

Right, that's why I opted for inlining for now.

> 
>> +static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
>> +{
>> +    /* We expect to be called only after the limit was set. */
>> +    assert(s390ms->memory_limit);
>> +    return s390ms->memory_limit;
>> +}
> 
> Short term, no better suggestion than inlining :(

Yes. And I suspect we do have similar compilation problems, that simply 
nobody noticed so far.

For example, hpage_1m_allowed() resides in hw/s390x/s390-virtio-ccw.c, 
but is called from target/s390x/kvm/kvm.c ...

So building QEMU with KVM but without CONFIG_S390_CCW_VIRTIO should make 
the linker unhappy :/ :(

-- 
Cheers,

David / dhildenb