| Message ID | 20191025082446.754-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] KVM: s390: protvirt: Secure memory is not mergeable | expand |
On 25.10.19 10:24, 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> v2 Looks better. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/gmap.h | 1 + > arch/s390/kvm/kvm-s390.c | 6 ++++++ > arch/s390/mm/gmap.c | 32 +++++++++++++++++++++----------- > 3 files changed, 28 insertions(+), 11 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(¤t->mm->mmap_sem); > + r = gmap_mark_unmergeable(); > + up_write(¤t->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..faecdf81abdb 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)) > + 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,24 +2610,17 @@ 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); > if (mm_uses_skeys(mm)) > 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; > + rc = gmap_mark_unmergeable(); > + if (rc) > + goto out_up; > > + mm->context.uses_skeys = 1; > walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL); > > out_up: >
On 25.10.19 10:24, 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. Patch itself looks good to me, but I do wonder: Is this really needed when pinning all encrypted pages currently? Not sure about races between KSM and the pinning/encrypting thread, similar to paging, though ...
On 11/4/19 3:32 PM, David Hildenbrand wrote: > On 25.10.19 10:24, 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. > > Patch itself looks good to me, but I do wonder: Is this really needed > when pinning all encrypted pages currently? > > Not sure about races between KSM and the pinning/encrypting thread, > similar to paging, though ... > The pinning was added several months after I wrote the patch. Now that we have it, we really need to have another proper look at the whole topic. Thanks for your review :-)
On 04.11.19 15:36, Janosch Frank wrote: > On 11/4/19 3:32 PM, David Hildenbrand wrote: >> On 25.10.19 10:24, 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. >> >> Patch itself looks good to me, but I do wonder: Is this really needed >> when pinning all encrypted pages currently? >> >> Not sure about races between KSM and the pinning/encrypting thread, >> similar to paging, though ... >> > > The pinning was added several months after I wrote the patch. > Now that we have it, we really need to have another proper look at the > whole topic. > > Thanks for your review :-) I'd certainly prefer this patch (+some way to mlock) over pinning ;) You can have Reviewed-by: David Hildenbrand <david@redhat.com> For this patch, if you end up needing it :)
On 25/10/2019 10.24, 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> > --- [...] > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index edcdca97e85e..faecdf81abdb 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; > + > + Please remove one of the two empty lines. > + for (vma = mm->mmap; vma; vma = vma->vm_next) { > + if (ksm_madvise(vma, vma->vm_start, vma->vm_end, > + MADV_UNMERGEABLE, &vma->vm_flags)) > + return -ENOMEM; > + } > + mm->def_flags &= ~VM_MERGEABLE; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable); > + [...] Apart from the cosmetic nit, the patch looks fine to me. Reviewed-by: Thomas Huth <thuth@redhat.com>
On 11/13/19 1:23 PM, Thomas Huth wrote: > On 25/10/2019 10.24, 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> >> --- > [...] >> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c >> index edcdca97e85e..faecdf81abdb 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; >> + >> + > > Please remove one of the two empty lines. Already gone > >> + for (vma = mm->mmap; vma; vma = vma->vm_next) { >> + if (ksm_madvise(vma, vma->vm_start, vma->vm_end, >> + MADV_UNMERGEABLE, &vma->vm_flags)) >> + return -ENOMEM; >> + } >> + mm->def_flags &= ~VM_MERGEABLE; >> + Also this one was removed >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(gmap_mark_unmergeable); >> + > [...] > > Apart from the cosmetic nit, the patch looks fine to me. > > Reviewed-by: Thomas Huth <thuth@redhat.com> > Thanks!
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(¤t->mm->mmap_sem); + r = gmap_mark_unmergeable(); + up_write(¤t->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..faecdf81abdb 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)) + 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,24 +2610,17 @@ 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); if (mm_uses_skeys(mm)) 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; + rc = gmap_mark_unmergeable(); + if (rc) + goto out_up; + mm->context.uses_skeys = 1; walk_page_range(mm, 0, TASK_SIZE, &enable_skey_walk_ops, NULL); out_up:
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 | 32 +++++++++++++++++++++----------- 3 files changed, 28 insertions(+), 11 deletions(-)