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

From: Cornelia Huck <cohuck@redhat.com>
To: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Janosch Frank <frankja@linux.vnet.ibm.com>,
	KVM <kvm@vger.kernel.org>, David Hildenbrand <david@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	Ulrich Weigand <Ulrich.Weigand@de.ibm.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [RFCv2 05/37] s390/mm: provide memory management functions for protected KVM guests
Date: Tue, 4 Feb 2020 13:15:37 +0100	[thread overview]
Message-ID: <20200204131537.3ec82487.cohuck@redhat.com> (raw)
In-Reply-To: <e2d208d0-e34c-c3d6-0c1b-57ee9a0c8458@de.ibm.com>

On Tue, 4 Feb 2020 12:56:10 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04.02.20 11:57, Cornelia Huck wrote:
> 
> >>  
> >>  /* 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...  
> 
> I will just use guest_handle as this guests assigned to and from such:
> 
> arch/s390/include/asm/gmap.h:   unsigned long se_handle;
> arch/s390/include/asm/uv.h:             .guest_handle = gmap->se_handle,
> arch/s390/kvm/pv.c:     WRITE_ONCE(kvm->arch.gmap->se_handle, 0);
> arch/s390/kvm/pv.c:     kvm->arch.gmap->se_handle = uvcb.guest_handle;
> arch/s390/mm/fault.c:           .guest_handle = gmap->se_handle,

Even better.

> 
> 
> >   
> >>  	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.  
> 
> I will use is "is_protected"

ok

> >   
> >>  	/*
> >>  	 * 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.  
> 
> Makes sense. I will wait a bit more but if you do not disagree I will remove 
> the extra host config and bind things to CONFIG_KVM or CONFIG_PGSTE depending
> on context

From the code I read so far, that makes sense.