[RFC,07/37] KVM: s390: protvirt: Secure memory is not mergeable
diff mbox series

Message ID 20191024114059.102802-8-frankja@linux.ibm.com
State New
Headers show
Series
  • KVM: s390: Add support for protected VMs
Related show

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC
KSM will not work on secure pages, because when the kernel reads a
secure page, it will be encrypted and hence no two pages will look the
same.

Let's mark the guest pages as unmergeable when we transition to secure
mode.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/asm/gmap.h |  1 +
 arch/s390/kvm/kvm-s390.c     |  6 ++++++
 arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
 3 files changed, 25 insertions(+), 10 deletions(-)

Comments

David Hildenbrand Oct. 24, 2019, 4:07 p.m. UTC | #1
On 24.10.19 13:40, Janosch Frank wrote:
> KSM will not work on secure pages, because when the kernel reads a
> secure page, it will be encrypted and hence no two pages will look the
> same.
> 
> Let's mark the guest pages as unmergeable when we transition to secure
> mode.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   arch/s390/include/asm/gmap.h |  1 +
>   arch/s390/kvm/kvm-s390.c     |  6 ++++++
>   arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>   3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
> index 6efc0b501227..eab6a2ec3599 100644
> --- a/arch/s390/include/asm/gmap.h
> +++ b/arch/s390/include/asm/gmap.h
> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>   
>   void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>   			     unsigned long gaddr, unsigned long vmaddr);
> +int gmap_mark_unmergeable(void);
>   #endif /* _ASM_S390_GMAP_H */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 924132d92782..d1ba12f857e7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   		if (r)
>   			break;
>   
> +		down_write(&current->mm->mmap_sem);
> +		r = gmap_mark_unmergeable();
> +		up_write(&current->mm->mmap_sem);
> +		if (r)
> +			break;
> +
>   		mutex_lock(&kvm->lock);
>   		kvm_s390_vcpu_block_all(kvm);
>   		/* FMT 4 SIE needs esca */
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index edcdca97e85e..bf365a09f900 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>   }
>   EXPORT_SYMBOL_GPL(s390_enable_sie);
>   
> +int gmap_mark_unmergeable(void)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> +				MADV_UNMERGEABLE, &vma->vm_flags)) {
> +			mm->context.uses_skeys = 0;

That skey setting does not make too much sense when coming via 
kvm_s390_handle_pv(). handle that in the caller?

> +			return -ENOMEM;
> +		}
> +	}
> +	mm->def_flags &= ~VM_MERGEABLE;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
> +
>   /*
>    * Enable storage key handling from now on and initialize the storage
>    * keys with the default key.
> @@ -2593,7 +2610,6 @@ static const struct mm_walk_ops enable_skey_walk_ops = {
>   int s390_enable_skey(void)
>   {
>   	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
>   	int rc = 0;
>   
>   	down_write(&mm->mmap_sem);
> @@ -2601,15 +2617,7 @@ int s390_enable_skey(void)
>   		goto out_up;
>   
>   	mm->context.uses_skeys = 1;
> -	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> -		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -				MADV_UNMERGEABLE, &vma->vm_flags)) {
> -			mm->context.uses_skeys = 0;
> -			rc = -ENOMEM;
> -			goto out_up;
> -		}
> -	}
> -	mm->def_flags &= ~VM_MERGEABLE;
> +	gmap_mark_unmergeable();
>   
>   	walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL);
>   
>
Claudio Imbrenda Oct. 24, 2019, 4:33 p.m. UTC | #2
On Thu, 24 Oct 2019 18:07:14 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 24.10.19 13:40, Janosch Frank wrote:
> > KSM will not work on secure pages, because when the kernel reads a
> > secure page, it will be encrypted and hence no two pages will look
> > the same.
> > 
> > Let's mark the guest pages as unmergeable when we transition to
> > secure mode.
> > 
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   arch/s390/include/asm/gmap.h |  1 +
> >   arch/s390/kvm/kvm-s390.c     |  6 ++++++
> >   arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
> >   3 files changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/gmap.h
> > b/arch/s390/include/asm/gmap.h index 6efc0b501227..eab6a2ec3599
> > 100644 --- a/arch/s390/include/asm/gmap.h
> > +++ b/arch/s390/include/asm/gmap.h
> > @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *,
> > unsigned long start, 
> >   void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long
> > dirty_bitmap[4], unsigned long gaddr, unsigned long vmaddr);
> > +int gmap_mark_unmergeable(void);
> >   #endif /* _ASM_S390_GMAP_H */
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 924132d92782..d1ba12f857e7 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm
> > *kvm, struct kvm_pv_cmd *cmd) if (r)
> >   			break;
> >   
> > +		down_write(&current->mm->mmap_sem);
> > +		r = gmap_mark_unmergeable();
> > +		up_write(&current->mm->mmap_sem);
> > +		if (r)
> > +			break;
> > +
> >   		mutex_lock(&kvm->lock);
> >   		kvm_s390_vcpu_block_all(kvm);
> >   		/* FMT 4 SIE needs esca */
> > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> > index edcdca97e85e..bf365a09f900 100644
> > --- a/arch/s390/mm/gmap.c
> > +++ b/arch/s390/mm/gmap.c
> > @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
> >   }
> >   EXPORT_SYMBOL_GPL(s390_enable_sie);
> >   
> > +int gmap_mark_unmergeable(void)
> > +{
> > +	struct mm_struct *mm = current->mm;
> > +	struct vm_area_struct *vma;
> > +
> > +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> > +				MADV_UNMERGEABLE, &vma->vm_flags))
> > {
> > +			mm->context.uses_skeys = 0;  
> 
> That skey setting does not make too much sense when coming via 
> kvm_s390_handle_pv(). handle that in the caller?

protected guests run keyless; any attempt to use keys in the guest will
result in an exception in the guest.

> 
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +	mm->def_flags &= ~VM_MERGEABLE;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
> > +
> >   /*
> >    * Enable storage key handling from now on and initialize the
> > storage
> >    * keys with the default key.
> > @@ -2593,7 +2610,6 @@ static const struct mm_walk_ops
> > enable_skey_walk_ops = { int s390_enable_skey(void)
> >   {
> >   	struct mm_struct *mm = current->mm;
> > -	struct vm_area_struct *vma;
> >   	int rc = 0;
> >   
> >   	down_write(&mm->mmap_sem);
> > @@ -2601,15 +2617,7 @@ int s390_enable_skey(void)
> >   		goto out_up;
> >   
> >   	mm->context.uses_skeys = 1;
> > -	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > -		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> > -				MADV_UNMERGEABLE, &vma->vm_flags))
> > {
> > -			mm->context.uses_skeys = 0;
> > -			rc = -ENOMEM;
> > -			goto out_up;
> > -		}
> > -	}
> > -	mm->def_flags &= ~VM_MERGEABLE;
> > +	gmap_mark_unmergeable();
> >   
> >   	walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops,
> > NULL); 
> >   
> 
>
David Hildenbrand Oct. 24, 2019, 4:49 p.m. UTC | #3
On 24.10.19 18:33, Claudio Imbrenda wrote:
> On Thu, 24 Oct 2019 18:07:14 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 24.10.19 13:40, Janosch Frank wrote:
>>> KSM will not work on secure pages, because when the kernel reads a
>>> secure page, it will be encrypted and hence no two pages will look
>>> the same.
>>>
>>> Let's mark the guest pages as unmergeable when we transition to
>>> secure mode.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/gmap.h |  1 +
>>>    arch/s390/kvm/kvm-s390.c     |  6 ++++++
>>>    arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>>>    3 files changed, 25 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/gmap.h
>>> b/arch/s390/include/asm/gmap.h index 6efc0b501227..eab6a2ec3599
>>> 100644 --- a/arch/s390/include/asm/gmap.h
>>> +++ b/arch/s390/include/asm/gmap.h
>>> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *,
>>> unsigned long start,
>>>    void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long
>>> dirty_bitmap[4], unsigned long gaddr, unsigned long vmaddr);
>>> +int gmap_mark_unmergeable(void);
>>>    #endif /* _ASM_S390_GMAP_H */
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 924132d92782..d1ba12f857e7 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm
>>> *kvm, struct kvm_pv_cmd *cmd) if (r)
>>>    			break;
>>>    
>>> +		down_write(&current->mm->mmap_sem);
>>> +		r = gmap_mark_unmergeable();
>>> +		up_write(&current->mm->mmap_sem);
>>> +		if (r)
>>> +			break;
>>> +
>>>    		mutex_lock(&kvm->lock);
>>>    		kvm_s390_vcpu_block_all(kvm);
>>>    		/* FMT 4 SIE needs esca */
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index edcdca97e85e..bf365a09f900 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>>    }
>>>    EXPORT_SYMBOL_GPL(s390_enable_sie);
>>>    
>>> +int gmap_mark_unmergeable(void)
>>> +{
>>> +	struct mm_struct *mm = current->mm;
>>> +	struct vm_area_struct *vma;
>>> +
>>> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>> +				MADV_UNMERGEABLE, &vma->vm_flags))
>>> {
>>> +			mm->context.uses_skeys = 0;
>>
>> That skey setting does not make too much sense when coming via
>> kvm_s390_handle_pv(). handle that in the caller?
> 
> protected guests run keyless; any attempt to use keys in the guest will
> result in an exception in the guest.

still, this is the recovery path for the "mm->context.uses_skeys = 1;" 
in enable_skey_walk_ops() and confuses reader (like me).
Janosch Frank Oct. 25, 2019, 7:18 a.m. UTC | #4
On 10/24/19 6:07 PM, David Hildenbrand wrote:
> On 24.10.19 13:40, Janosch Frank wrote:
>> KSM will not work on secure pages, because when the kernel reads a
>> secure page, it will be encrypted and hence no two pages will look the
>> same.
>>
>> Let's mark the guest pages as unmergeable when we transition to secure
>> mode.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/gmap.h |  1 +
>>   arch/s390/kvm/kvm-s390.c     |  6 ++++++
>>   arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>>   3 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
>> index 6efc0b501227..eab6a2ec3599 100644
>> --- a/arch/s390/include/asm/gmap.h
>> +++ b/arch/s390/include/asm/gmap.h
>> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>>   
>>   void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>>   			     unsigned long gaddr, unsigned long vmaddr);
>> +int gmap_mark_unmergeable(void);
>>   #endif /* _ASM_S390_GMAP_H */
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 924132d92782..d1ba12f857e7 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>   		if (r)
>>   			break;
>>   
>> +		down_write(&current->mm->mmap_sem);
>> +		r = gmap_mark_unmergeable();
>> +		up_write(&current->mm->mmap_sem);
>> +		if (r)
>> +			break;
>> +
>>   		mutex_lock(&kvm->lock);
>>   		kvm_s390_vcpu_block_all(kvm);
>>   		/* FMT 4 SIE needs esca */
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index edcdca97e85e..bf365a09f900 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>   }
>>   EXPORT_SYMBOL_GPL(s390_enable_sie);
>>   
>> +int gmap_mark_unmergeable(void)
>> +{
>> +	struct mm_struct *mm = current->mm;
>> +	struct vm_area_struct *vma;
>> +
>> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>> +				MADV_UNMERGEABLE, &vma->vm_flags)) {
>> +			mm->context.uses_skeys = 0;
> 
> That skey setting does not make too much sense when coming via 
> kvm_s390_handle_pv(). handle that in the caller?

Hmm, I think the name of that variable is just plain wrong.
It should be "can_use_skeys" or "uses_unmergeable" (which would fit
better into the mm context anyway) and then we could add a
kvm->arch.uses_skeys to tell that we actually used them for migration
checks, etc..

I had long discussions with Martin over these variable names a long time
ago..
David Hildenbrand Oct. 25, 2019, 7:46 a.m. UTC | #5
On 24.10.19 13:40, Janosch Frank wrote:
> KSM will not work on secure pages, because when the kernel reads a
> secure page, it will be encrypted and hence no two pages will look the
> same.
> 
> Let's mark the guest pages as unmergeable when we transition to secure
> mode.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   arch/s390/include/asm/gmap.h |  1 +
>   arch/s390/kvm/kvm-s390.c     |  6 ++++++
>   arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>   3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
> index 6efc0b501227..eab6a2ec3599 100644
> --- a/arch/s390/include/asm/gmap.h
> +++ b/arch/s390/include/asm/gmap.h
> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>   
>   void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>   			     unsigned long gaddr, unsigned long vmaddr);
> +int gmap_mark_unmergeable(void);
>   #endif /* _ASM_S390_GMAP_H */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 924132d92782..d1ba12f857e7 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>   		if (r)
>   			break;
>   
> +		down_write(&current->mm->mmap_sem);
> +		r = gmap_mark_unmergeable();
> +		up_write(&current->mm->mmap_sem);
> +		if (r)
> +			break;
> +
>   		mutex_lock(&kvm->lock);
>   		kvm_s390_vcpu_block_all(kvm);
>   		/* FMT 4 SIE needs esca */
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index edcdca97e85e..bf365a09f900 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>   }
>   EXPORT_SYMBOL_GPL(s390_enable_sie);
>   
> +int gmap_mark_unmergeable(void)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> +				MADV_UNMERGEABLE, &vma->vm_flags)) {
> +			mm->context.uses_skeys = 0;
> +			return -ENOMEM;
> +		}
> +	}
> +	mm->def_flags &= ~VM_MERGEABLE;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
> +
>   /*
>    * Enable storage key handling from now on and initialize the storage
>    * keys with the default key.
> @@ -2593,7 +2610,6 @@ static const struct mm_walk_ops enable_skey_walk_ops = {
>   int s390_enable_skey(void)
>   {
>   	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
>   	int rc = 0;
>   
>   	down_write(&mm->mmap_sem);
> @@ -2601,15 +2617,7 @@ int s390_enable_skey(void)
>   		goto out_up;
>   
>   	mm->context.uses_skeys = 1;
> -	for (vma = mm->mmap; vma; vma = vma->vm_next) {
> -		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -				MADV_UNMERGEABLE, &vma->vm_flags)) {
> -			mm->context.uses_skeys = 0;
> -			rc = -ENOMEM;
> -			goto out_up;
> -		}
> -	}
> -	mm->def_flags &= ~VM_MERGEABLE;
> +	gmap_mark_unmergeable();

also, here we are now ignoring errors?

This looks broken.
David Hildenbrand Oct. 25, 2019, 8:04 a.m. UTC | #6
On 25.10.19 09:18, Janosch Frank wrote:
> On 10/24/19 6:07 PM, David Hildenbrand wrote:
>> On 24.10.19 13:40, Janosch Frank wrote:
>>> KSM will not work on secure pages, because when the kernel reads a
>>> secure page, it will be encrypted and hence no two pages will look the
>>> same.
>>>
>>> Let's mark the guest pages as unmergeable when we transition to secure
>>> mode.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/gmap.h |  1 +
>>>    arch/s390/kvm/kvm-s390.c     |  6 ++++++
>>>    arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>>>    3 files changed, 25 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
>>> index 6efc0b501227..eab6a2ec3599 100644
>>> --- a/arch/s390/include/asm/gmap.h
>>> +++ b/arch/s390/include/asm/gmap.h
>>> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>>>    
>>>    void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>>>    			     unsigned long gaddr, unsigned long vmaddr);
>>> +int gmap_mark_unmergeable(void);
>>>    #endif /* _ASM_S390_GMAP_H */
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 924132d92782..d1ba12f857e7 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>>    		if (r)
>>>    			break;
>>>    
>>> +		down_write(&current->mm->mmap_sem);
>>> +		r = gmap_mark_unmergeable();
>>> +		up_write(&current->mm->mmap_sem);
>>> +		if (r)
>>> +			break;
>>> +
>>>    		mutex_lock(&kvm->lock);
>>>    		kvm_s390_vcpu_block_all(kvm);
>>>    		/* FMT 4 SIE needs esca */
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index edcdca97e85e..bf365a09f900 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>>    }
>>>    EXPORT_SYMBOL_GPL(s390_enable_sie);
>>>    
>>> +int gmap_mark_unmergeable(void)
>>> +{
>>> +	struct mm_struct *mm = current->mm;
>>> +	struct vm_area_struct *vma;
>>> +
>>> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>> +				MADV_UNMERGEABLE, &vma->vm_flags)) {
>>> +			mm->context.uses_skeys = 0;
>>
>> That skey setting does not make too much sense when coming via
>> kvm_s390_handle_pv(). handle that in the caller?
> 
> Hmm, I think the name of that variable is just plain wrong.
> It should be "can_use_skeys" or "uses_unmergeable" (which would fit
> better into the mm context anyway) and then we could add a
> kvm->arch.uses_skeys to tell that we actually used them for migration
> checks, etc..
> 
> I had long discussions with Martin over these variable names a long time
> ago..

uses_skeys is set during s390_enable_skey(). that is used when we

a) Call an skey instruction
b) Migrate skeys

So it should match "uses" or what am I missing?

If you look at the users of "mm_uses_skeys(mm)" I think 
"uses_unmergeable" would actually be misleading. (e.g., 
pgste_set_key()). it really means "somebody used skeys". The unmergable 
is just a required side effect.
Janosch Frank Oct. 25, 2019, 8:20 a.m. UTC | #7
On 10/25/19 10:04 AM, David Hildenbrand wrote:
> On 25.10.19 09:18, Janosch Frank wrote:
>> On 10/24/19 6:07 PM, David Hildenbrand wrote:
>>> On 24.10.19 13:40, Janosch Frank wrote:
>>>> KSM will not work on secure pages, because when the kernel reads a
>>>> secure page, it will be encrypted and hence no two pages will look the
>>>> same.
>>>>
>>>> Let's mark the guest pages as unmergeable when we transition to secure
>>>> mode.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>    arch/s390/include/asm/gmap.h |  1 +
>>>>    arch/s390/kvm/kvm-s390.c     |  6 ++++++
>>>>    arch/s390/mm/gmap.c          | 28 ++++++++++++++++++----------
>>>>    3 files changed, 25 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
>>>> index 6efc0b501227..eab6a2ec3599 100644
>>>> --- a/arch/s390/include/asm/gmap.h
>>>> +++ b/arch/s390/include/asm/gmap.h
>>>> @@ -145,4 +145,5 @@ int gmap_mprotect_notify(struct gmap *, unsigned long start,
>>>>    
>>>>    void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
>>>>    			     unsigned long gaddr, unsigned long vmaddr);
>>>> +int gmap_mark_unmergeable(void);
>>>>    #endif /* _ASM_S390_GMAP_H */
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 924132d92782..d1ba12f857e7 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2176,6 +2176,12 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>>>>    		if (r)
>>>>    			break;
>>>>    
>>>> +		down_write(&current->mm->mmap_sem);
>>>> +		r = gmap_mark_unmergeable();
>>>> +		up_write(&current->mm->mmap_sem);
>>>> +		if (r)
>>>> +			break;
>>>> +
>>>>    		mutex_lock(&kvm->lock);
>>>>    		kvm_s390_vcpu_block_all(kvm);
>>>>    		/* FMT 4 SIE needs esca */
>>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>>> index edcdca97e85e..bf365a09f900 100644
>>>> --- a/arch/s390/mm/gmap.c
>>>> +++ b/arch/s390/mm/gmap.c
>>>> @@ -2548,6 +2548,23 @@ int s390_enable_sie(void)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(s390_enable_sie);
>>>>    
>>>> +int gmap_mark_unmergeable(void)
>>>> +{
>>>> +	struct mm_struct *mm = current->mm;
>>>> +	struct vm_area_struct *vma;
>>>> +
>>>> +	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>>> +		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>>> +				MADV_UNMERGEABLE, &vma->vm_flags)) {
>>>> +			mm->context.uses_skeys = 0;
>>>
>>> That skey setting does not make too much sense when coming via
>>> kvm_s390_handle_pv(). handle that in the caller?
>>
>> Hmm, I think the name of that variable is just plain wrong.
>> It should be "can_use_skeys" or "uses_unmergeable" (which would fit
>> better into the mm context anyway) and then we could add a
>> kvm->arch.uses_skeys to tell that we actually used them for migration
>> checks, etc..
>>
>> I had long discussions with Martin over these variable names a long time
>> ago..
> 
> uses_skeys is set during s390_enable_skey(). that is used when we
> 
> a) Call an skey instruction
> b) Migrate skeys
> 
> So it should match "uses" or what am I missing?
> 
> If you look at the users of "mm_uses_skeys(mm)" I think 
> "uses_unmergeable" would actually be misleading. (e.g., 
> pgste_set_key()). it really means "somebody used skeys". The unmergable 
> is just a required side effect.

Hmm, we couldn't check struct kvm from pgtable.c anyway.
Oh well, I still don't like it very much but your arguments are better
:-) Let's fix this.

Patch
diff mbox series

diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
index 6efc0b501227..eab6a2ec3599 100644
--- a/arch/s390/include/asm/gmap.h
+++ b/arch/s390/include/asm/gmap.h
@@ -145,4 +145,5 @@  int gmap_mprotect_notify(struct gmap *, unsigned long start,
 
 void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4],
 			     unsigned long gaddr, unsigned long vmaddr);
+int gmap_mark_unmergeable(void);
 #endif /* _ASM_S390_GMAP_H */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 924132d92782..d1ba12f857e7 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2176,6 +2176,12 @@  static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
 		if (r)
 			break;
 
+		down_write(&current->mm->mmap_sem);
+		r = gmap_mark_unmergeable();
+		up_write(&current->mm->mmap_sem);
+		if (r)
+			break;
+
 		mutex_lock(&kvm->lock);
 		kvm_s390_vcpu_block_all(kvm);
 		/* FMT 4 SIE needs esca */
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index edcdca97e85e..bf365a09f900 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2548,6 +2548,23 @@  int s390_enable_sie(void)
 }
 EXPORT_SYMBOL_GPL(s390_enable_sie);
 
+int gmap_mark_unmergeable(void)
+{
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+
+	for (vma = mm->mmap; vma; vma = vma->vm_next) {
+		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
+				MADV_UNMERGEABLE, &vma->vm_flags)) {
+			mm->context.uses_skeys = 0;
+			return -ENOMEM;
+		}
+	}
+	mm->def_flags &= ~VM_MERGEABLE;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
+
 /*
  * Enable storage key handling from now on and initialize the storage
  * keys with the default key.
@@ -2593,7 +2610,6 @@  static const struct mm_walk_ops enable_skey_walk_ops = {
 int s390_enable_skey(void)
 {
 	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma;
 	int rc = 0;
 
 	down_write(&mm->mmap_sem);
@@ -2601,15 +2617,7 @@  int s390_enable_skey(void)
 		goto out_up;
 
 	mm->context.uses_skeys = 1;
-	for (vma = mm->mmap; vma; vma = vma->vm_next) {
-		if (ksm_madvise(vma, vma->vm_start, vma->vm_end,
-				MADV_UNMERGEABLE, &vma->vm_flags)) {
-			mm->context.uses_skeys = 0;
-			rc = -ENOMEM;
-			goto out_up;
-		}
-	}
-	mm->def_flags &= ~VM_MERGEABLE;
+	gmap_mark_unmergeable();
 
 	walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL);