Re: [RFCv2 05/37] s390/mm: provide memory management functions for protected KVM guests

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

On Tue, 4 Feb 2020 11:57:38 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon,  3 Feb 2020 08:19:25 -0500
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > From: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > 
> > This provides the basic ultravisor calls and page table handling to
> > cope with secure guests.  
> 
> I'll leave reviewing the mm stuff to somebody actually familiar with
> mm; only some other comments from me.
> 
> > 
> > Co-authored-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  arch/s390/include/asm/gmap.h        |   2 +
> >  arch/s390/include/asm/mmu.h         |   2 +
> >  arch/s390/include/asm/mmu_context.h |   1 +
> >  arch/s390/include/asm/page.h        |   5 +
> >  arch/s390/include/asm/pgtable.h     |  34 +++++-
> >  arch/s390/include/asm/uv.h          |  59 ++++++++++
> >  arch/s390/kernel/uv.c               | 170
> > ++++++++++++++++++++++++++++ 7 files changed, 268 insertions(+), 5
> > deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/gmap.h
> > b/arch/s390/include/asm/gmap.h index 37f96b6f0e61..f2ab8b6d4b57
> > 100644 --- a/arch/s390/include/asm/gmap.h
> > +++ b/arch/s390/include/asm/gmap.h
> > @@ -9,6 +9,7 @@
> >  #ifndef _ASM_S390_GMAP_H
> >  #define _ASM_S390_GMAP_H
> >  
> > +#include <linux/radix-tree.h>
> >  #include <linux/refcount.h>
> >  
> >  /* Generic bits for GMAP notification on DAT table entry changes.
> > */ @@ -61,6 +62,7 @@ struct gmap {
> >  	spinlock_t shadow_lock;
> >  	struct gmap *parent;
> >  	unsigned long orig_asce;
> > +	unsigned long se_handle;  
> 
> This is a deviation from the "protected virtualization" naming scheme
> used in the previous patches, but I understand that the naming of this
> whole feature is still in flux :) (Would still be nice to have it
> consistent, though.)
> 
> However, I think I'd prefer something named based on protected
> virtualization: the se_* stuff here just tends to make me think of
> SELinux...
> 
> >  	int edat_level;
> >  	bool removed;
> >  	bool initialized;
> > diff --git a/arch/s390/include/asm/mmu.h
> > b/arch/s390/include/asm/mmu.h index bcfb6371086f..984026cb3608
> > 100644 --- a/arch/s390/include/asm/mmu.h
> > +++ b/arch/s390/include/asm/mmu.h
> > @@ -16,6 +16,8 @@ typedef struct {
> >  	unsigned long asce;
> >  	unsigned long asce_limit;
> >  	unsigned long vdso_base;
> > +	/* The mmu context belongs to a secure guest. */
> > +	atomic_t is_se;  
> 
> Here as well.
> 
> >  	/*
> >  	 * The following bitfields need a down_write on the mm
> >  	 * semaphore when they are written to. As they are only  
> 
> (...)
> 
> > @@ -520,6 +521,15 @@ static inline int mm_has_pgste(struct
> > mm_struct *mm) return 0;
> >  }
> >  
> > +static inline int mm_is_se(struct mm_struct *mm)
> > +{
> > +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> > +	if (unlikely(atomic_read(&mm->context.is_se)))
> > +		return 1;
> > +#endif
> > +	return 0;
> > +}  
> 
> I'm wondering how big of an overhead we actually have because we need
> to check the flag here if the feature is enabled. We have an extra
> check in a few functions anyway, even if protected virt is not
> configured in. Given that distributions would likely want to enable
> the feature in their kernels, I'm currently tending towards dropping
> the extra config option.
> 
> > +
> >  static inline int mm_alloc_pgste(struct mm_struct *mm)
> >  {
> >  #ifdef CONFIG_PGSTE  
> 
> (...)
> 
> > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> > index f7778493e829..136c60a8e3ca 100644
> > --- a/arch/s390/kernel/uv.c
> > +++ b/arch/s390/kernel/uv.c
> > @@ -9,6 +9,8 @@
> >  #include <linux/sizes.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/memblock.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/swap.h>
> >  #include <asm/facility.h>
> >  #include <asm/sections.h>
> >  #include <asm/uv.h>
> > @@ -98,4 +100,172 @@ void adjust_to_uv_max(unsigned long *vmax)
> >  	if (prot_virt_host && *vmax > uv_info.max_sec_stor_addr)
> >  		*vmax = uv_info.max_sec_stor_addr;
> >  }
> > +
> > +static int __uv_pin_shared(unsigned long paddr)
> > +{
> > +	struct uv_cb_cfs uvcb = {
> > +		.header.cmd	= UVC_CMD_PIN_PAGE_SHARED,
> > +		.header.len	= sizeof(uvcb),
> > +		.paddr		= paddr,
> > +	};
> > +
> > +	if (uv_call(0, (u64)&uvcb))
> > +		return -EINVAL;
> > +	return 0;  
> 
> I guess this call won't set an error rc in the control block, if it
> does not set a condition code != 0?

correct.
(cc == 0) ⇔ (rc == 1) which is success.
any rc other than 1 sets cc = 1