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

[Claudio Imbrenda]

On Mon, 30 Sep 2024 15:14:52 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 30.09.24 13:37, Claudio Imbrenda wrote:
> > On Mon, 30 Sep 2024 13:15:52 +0200
> > Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> >   
> >> Am 24.09.24 um 22:17 schrieb David Hildenbrand:  
> >>> 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.  
> >>
> >> IIRC there have been functional issues with small guests and asnyc. 
> >> Claudio, do you remember?  
> > 
> > if we are <2G, KVM allocates a segment table as the highest level table
> > for the gmap ASCE. there are pointers lurking around in the reverse
> > mapping prefix_tree, which point directly into segment tables.
> > 
> > if the ASCE is region3 or higher, that's not an issue. if it's a
> > segment table, then it's an issue, because those pointers will end up
> > pointing into freed memory, once the old asce is freed.
> > 
> > in short, we have to guarantee that we will never set aside a gmap ASCE
> > if it is a segment table.  
> 
> Thanks for the details, the kernel seems to properly safeguard against 

it does, but it returns an error if userspace tries; the check there is
to prevent qemu from emitting confusing error messages.

> that. For now, I'll turn it into a check against the memory limit, which 
> directly translates to the gmap ASCE used.

sounds good

> 
> Thanks!

no problem :)