Re: [PATCH v1 1/1] KVM: s390: pv: fix asynchronous teardown for small VMs

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 20 Apr 2023 18:15:36 +0200
"Marc Hartmayer" <mhartmay@linux.ibm.com> wrote:

> Claudio Imbrenda <imbrenda@linux.ibm.com> writes:
> 
> > On machines without the Destroy Secure Configuration Fast UVC, the
> > topmost level of page tables is set aside and freed asynchronously
> > as last step of the asynchronous teardown.
> >
> > Each gmap has a host_to_guest radix tree mapping host (userspace)
> > addresses (with 1M granularity) to gmap segment table entries (pmds).
> >
> > If a guest is smaller than 2GB, the topmost level of page tables is the
> > segment table (i.e. there are only 2 levels). Replacing it means that
> > the pointers in the host_to_guest mapping would become stale and cause
> > all kinds of nasty issues.
> >
> > This patch fixes the issue by synchronously destroying all guests with
> > only 2 levels of page tables in kvm_s390_pv_set_aside. This will
> > speed up the process and avoid the issue altogether.
> >
> > Update s390_replace_asce so it refuses to replace segment type ASCEs.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Fixes: fb491d5500a7 ("KVM: s390: pv: asynchronous destroy for reboot")
> > ---
> >  arch/s390/kvm/pv.c  | 35 ++++++++++++++++++++---------------
> >  arch/s390/mm/gmap.c |  7 +++++++
> >  2 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> > index e032ebbf51b9..ceb8cb628d62 100644
> > --- a/arch/s390/kvm/pv.c
> > +++ b/arch/s390/kvm/pv.c
> > @@ -39,6 +39,7 @@ struct pv_vm_to_be_destroyed {
> >  	u64 handle;
> >  	void *stor_var;
> >  	unsigned long stor_base;
> > +	bool small;  
> 
> I would rather use a more verbose variable name (maybe ‘lower_2g‘

lower_2g would mean that there is more above 2g, but in this case the VM
is smaller than 2g (which is the whole point)

> accordingly to the `kvm_s390_destroy_lower_2g` function name or even
> better something indicating the actual implications).
> 
> At least, I would add a comment what ‘small‘ means and the implications

I'll add a comment

> of it.
> 
> […snip]
>