Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[David Hildenbrand <david@redhat.com>]

On 24.09.24 22:17, David Hildenbrand wrote:
> On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:
>> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>>> We actually want to check the available RAM, not the maximum RAM size.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> Nit below.
>>> ---
>>>    target/s390x/kvm/pv.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>>> index dde836d21a..424cce75ca 100644
>>> --- a/target/s390x/kvm/pv.c
>>> +++ b/target/s390x/kvm/pv.c
>>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>>>         * If the feature is not present or if the VM is not larger than 2 GiB,
>>>         * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>>>         */
>>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
>>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>>>            !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>>>            return false;
>>>        }
>>
>> If I understood the kernel code right, the decision is made wrt
>> the size of the gmap address space, which is the same as the
>> limit set for the VM. So using s390_get_memory_limit would be
>> semantically cleaner.
> 
> I wonder if we should just drop the RAM size check. Not convinced the
> slightly faster reboot for such small VMs is really relevant? Makes the
> code more complicated than really necessary.

Thinking about it,

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 424cce75ca..bbb2108546 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
       * If the feature is not present or if the VM is not larger than 2 GiB,
       * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
       */
-    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
+    if (s390_get_memory_limit(ms) < 2 * GiB ||
          !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
          return false;
      }

Is probably the easiest change, thanks!


-- 
Cheers,

David / dhildenb