| Message ID | 20200703155914.40262-3-ldufour@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Rework secure memslot dropping | expand |
On Fri, Jul 03, 2020 at 05:59:14PM +0200, Laurent Dufour wrote: > When a secure memslot is dropped, all the pages backed in the secure device > (aka really backed by secure memory by the Ultravisor) should be paged out > to a normal page. Previously, this was achieved by triggering the page > fault mechanism which is calling kvmppc_svm_page_out() on each pages. > > This can't work when hot unplugging a memory slot because the memory slot > is flagged as invalid and gfn_to_pfn() is then not trying to access the > page, so the page fault mechanism is not triggered. > > Since the final goal is to make a call to kvmppc_svm_page_out() it seems > simpler to directly calling it instead of triggering such a mechanism. This > way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a > memslot. Yes, this appears much simpler. > > Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock, > the call to __kvmppc_svm_page_out() is made. > As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the > VMA is fetched in a lazy way, to not trigger find_vma() all the time. In > addition, the mmap_sem is help in read mode during that time, not in write > mode since the virual memory layout is not impacted, and > kvm->arch.uvmem_lock prevents concurrent operation on the secure device. > > Cc: Ram Pai <linuxram@us.ibm.com> > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++---------- > 1 file changed, 37 insertions(+), 17 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 852cc9ae6a0b..479ddf16d18c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma, > * fault on them, do fault time migration to replace the device PTEs in > * QEMU page table with normal PTEs from newly allocated pages. > */ > -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, > +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot, > struct kvm *kvm, bool skip_page_out) > { > int i; > struct kvmppc_uvmem_page_pvt *pvt; > - unsigned long pfn, uvmem_pfn; > - unsigned long gfn = free->base_gfn; > + struct page *uvmem_page; > + struct vm_area_struct *vma = NULL; > + unsigned long uvmem_pfn, gfn; > + unsigned long addr, end; > + > + down_read(&kvm->mm->mmap_sem); You should be using mmap_read_lock(kvm->mm) with recent kernels. > + > + addr = slot->userspace_addr; > + end = addr + (slot->npages * PAGE_SIZE); > > - for (i = free->npages; i; --i, ++gfn) { > - struct page *uvmem_page; > + gfn = slot->base_gfn; > + for (i = slot->npages; i; --i, ++gfn, addr += PAGE_SIZE) { > + > + /* Fetch the VMA if addr is not in the latest fetched one */ > + if (!vma || (addr < vma->vm_start || addr >= vma->vm_end)) { > + vma = find_vma_intersection(kvm->mm, addr, end); > + if (!vma || > + vma->vm_start > addr || vma->vm_end < end) { > + pr_err("Can't find VMA for gfn:0x%lx\n", gfn); > + break; > + } > + } The first find_vma_intersection() was called for the range spanning the entire memslot, but you have code to check if vma remains valid for the new addr in each iteration. Guess you wanted to get vma for one page at a time and use it for subsequent pages until it covers the range? Regards, Bharata.
Le 08/07/2020 à 13:25, Bharata B Rao a écrit : > On Fri, Jul 03, 2020 at 05:59:14PM +0200, Laurent Dufour wrote: >> When a secure memslot is dropped, all the pages backed in the secure device >> (aka really backed by secure memory by the Ultravisor) should be paged out >> to a normal page. Previously, this was achieved by triggering the page >> fault mechanism which is calling kvmppc_svm_page_out() on each pages. >> >> This can't work when hot unplugging a memory slot because the memory slot >> is flagged as invalid and gfn_to_pfn() is then not trying to access the >> page, so the page fault mechanism is not triggered. >> >> Since the final goal is to make a call to kvmppc_svm_page_out() it seems >> simpler to directly calling it instead of triggering such a mechanism. This >> way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a >> memslot. > > Yes, this appears much simpler. Thanks Bharata for reviewing this. > >> >> Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock, >> the call to __kvmppc_svm_page_out() is made. >> As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the >> VMA is fetched in a lazy way, to not trigger find_vma() all the time. In >> addition, the mmap_sem is help in read mode during that time, not in write >> mode since the virual memory layout is not impacted, and >> kvm->arch.uvmem_lock prevents concurrent operation on the secure device. >> >> Cc: Ram Pai <linuxram@us.ibm.com> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++---------- >> 1 file changed, 37 insertions(+), 17 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c >> index 852cc9ae6a0b..479ddf16d18c 100644 >> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c >> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c >> @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma, >> * fault on them, do fault time migration to replace the device PTEs in >> * QEMU page table with normal PTEs from newly allocated pages. >> */ >> -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, >> +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot, >> struct kvm *kvm, bool skip_page_out) >> { >> int i; >> struct kvmppc_uvmem_page_pvt *pvt; >> - unsigned long pfn, uvmem_pfn; >> - unsigned long gfn = free->base_gfn; >> + struct page *uvmem_page; >> + struct vm_area_struct *vma = NULL; >> + unsigned long uvmem_pfn, gfn; >> + unsigned long addr, end; >> + >> + down_read(&kvm->mm->mmap_sem); > > You should be using mmap_read_lock(kvm->mm) with recent kernels. Absolutely, shame on me, I reviewed Michel's series about that! Paul, Michael, could you fix that when pulling this patch or should I sent a whole new series? > >> + >> + addr = slot->userspace_addr; >> + end = addr + (slot->npages * PAGE_SIZE); >> >> - for (i = free->npages; i; --i, ++gfn) { >> - struct page *uvmem_page; >> + gfn = slot->base_gfn; >> + for (i = slot->npages; i; --i, ++gfn, addr += PAGE_SIZE) { >> + >> + /* Fetch the VMA if addr is not in the latest fetched one */ >> + if (!vma || (addr < vma->vm_start || addr >= vma->vm_end)) { >> + vma = find_vma_intersection(kvm->mm, addr, end); >> + if (!vma || >> + vma->vm_start > addr || vma->vm_end < end) { >> + pr_err("Can't find VMA for gfn:0x%lx\n", gfn); >> + break; >> + } >> + } > > The first find_vma_intersection() was called for the range spanning the > entire memslot, but you have code to check if vma remains valid for the > new addr in each iteration. Guess you wanted to get vma for one page at > a time and use it for subsequent pages until it covers the range? That's the goal, fetch the VMA once and no more until we reach its end boundary.
Laurent Dufour <ldufour@linux.ibm.com> writes: > Le 08/07/2020 à 13:25, Bharata B Rao a écrit : >> On Fri, Jul 03, 2020 at 05:59:14PM +0200, Laurent Dufour wrote: >>> When a secure memslot is dropped, all the pages backed in the secure device >>> (aka really backed by secure memory by the Ultravisor) should be paged out >>> to a normal page. Previously, this was achieved by triggering the page >>> fault mechanism which is calling kvmppc_svm_page_out() on each pages. >>> >>> This can't work when hot unplugging a memory slot because the memory slot >>> is flagged as invalid and gfn_to_pfn() is then not trying to access the >>> page, so the page fault mechanism is not triggered. >>> >>> Since the final goal is to make a call to kvmppc_svm_page_out() it seems >>> simpler to directly calling it instead of triggering such a mechanism. This >>> way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a >>> memslot. >> >> Yes, this appears much simpler. > > Thanks Bharata for reviewing this. > >> >>> >>> Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock, >>> the call to __kvmppc_svm_page_out() is made. >>> As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the >>> VMA is fetched in a lazy way, to not trigger find_vma() all the time. In >>> addition, the mmap_sem is help in read mode during that time, not in write >>> mode since the virual memory layout is not impacted, and >>> kvm->arch.uvmem_lock prevents concurrent operation on the secure device. >>> >>> Cc: Ram Pai <linuxram@us.ibm.com> >>> Cc: Bharata B Rao <bharata@linux.ibm.com> >>> Cc: Paul Mackerras <paulus@ozlabs.org> >>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >>> --- >>> arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++---------- >>> 1 file changed, 37 insertions(+), 17 deletions(-) >>> >>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c >>> index 852cc9ae6a0b..479ddf16d18c 100644 >>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c >>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c >>> @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma, >>> * fault on them, do fault time migration to replace the device PTEs in >>> * QEMU page table with normal PTEs from newly allocated pages. >>> */ >>> -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, >>> +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot, >>> struct kvm *kvm, bool skip_page_out) >>> { >>> int i; >>> struct kvmppc_uvmem_page_pvt *pvt; >>> - unsigned long pfn, uvmem_pfn; >>> - unsigned long gfn = free->base_gfn; >>> + struct page *uvmem_page; >>> + struct vm_area_struct *vma = NULL; >>> + unsigned long uvmem_pfn, gfn; >>> + unsigned long addr, end; >>> + >>> + down_read(&kvm->mm->mmap_sem); >> >> You should be using mmap_read_lock(kvm->mm) with recent kernels. > > Absolutely, shame on me, I reviewed Michel's series about that! > > Paul, Michael, could you fix that when pulling this patch or should I sent a > whole new series? Paul will take this series, so up to him. cheers
On Wed, Jul 08, 2020 at 02:16:36PM +0200, Laurent Dufour wrote: > Le 08/07/2020 à 13:25, Bharata B Rao a écrit : > > On Fri, Jul 03, 2020 at 05:59:14PM +0200, Laurent Dufour wrote: > > > When a secure memslot is dropped, all the pages backed in the secure device > > > (aka really backed by secure memory by the Ultravisor) should be paged out > > > to a normal page. Previously, this was achieved by triggering the page > > > fault mechanism which is calling kvmppc_svm_page_out() on each pages. > > > > > > This can't work when hot unplugging a memory slot because the memory slot > > > is flagged as invalid and gfn_to_pfn() is then not trying to access the > > > page, so the page fault mechanism is not triggered. > > > > > > Since the final goal is to make a call to kvmppc_svm_page_out() it seems > > > simpler to directly calling it instead of triggering such a mechanism. This > > > way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a > > > memslot. > > > > Yes, this appears much simpler. > > Thanks Bharata for reviewing this. > > > > > > > > > Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock, > > > the call to __kvmppc_svm_page_out() is made. > > > As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the > > > VMA is fetched in a lazy way, to not trigger find_vma() all the time. In > > > addition, the mmap_sem is help in read mode during that time, not in write > > > mode since the virual memory layout is not impacted, and > > > kvm->arch.uvmem_lock prevents concurrent operation on the secure device. > > > > > > Cc: Ram Pai <linuxram@us.ibm.com> > > > Cc: Bharata B Rao <bharata@linux.ibm.com> > > > Cc: Paul Mackerras <paulus@ozlabs.org> > > > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > > > --- > > > arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++---------- > > > 1 file changed, 37 insertions(+), 17 deletions(-) > > > > > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > index 852cc9ae6a0b..479ddf16d18c 100644 > > > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > > > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma, > > > * fault on them, do fault time migration to replace the device PTEs in > > > * QEMU page table with normal PTEs from newly allocated pages. > > > */ > > > -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, > > > +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot, > > > struct kvm *kvm, bool skip_page_out) > > > { > > > int i; > > > struct kvmppc_uvmem_page_pvt *pvt; > > > - unsigned long pfn, uvmem_pfn; > > > - unsigned long gfn = free->base_gfn; > > > + struct page *uvmem_page; > > > + struct vm_area_struct *vma = NULL; > > > + unsigned long uvmem_pfn, gfn; > > > + unsigned long addr, end; > > > + > > > + down_read(&kvm->mm->mmap_sem); > > > > You should be using mmap_read_lock(kvm->mm) with recent kernels. > > Absolutely, shame on me, I reviewed Michel's series about that! > > Paul, Michael, could you fix that when pulling this patch or should I sent a > whole new series? Given that Ram has reworked his series, I think it would be best if you rebase on top of his new series and re-send your series. Thanks, Paul.
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index 852cc9ae6a0b..479ddf16d18c 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -533,35 +533,55 @@ static inline int kvmppc_svm_page_out(struct vm_area_struct *vma, * fault on them, do fault time migration to replace the device PTEs in * QEMU page table with normal PTEs from newly allocated pages. */ -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot, struct kvm *kvm, bool skip_page_out) { int i; struct kvmppc_uvmem_page_pvt *pvt; - unsigned long pfn, uvmem_pfn; - unsigned long gfn = free->base_gfn; + struct page *uvmem_page; + struct vm_area_struct *vma = NULL; + unsigned long uvmem_pfn, gfn; + unsigned long addr, end; + + down_read(&kvm->mm->mmap_sem); + + addr = slot->userspace_addr; + end = addr + (slot->npages * PAGE_SIZE); - for (i = free->npages; i; --i, ++gfn) { - struct page *uvmem_page; + gfn = slot->base_gfn; + for (i = slot->npages; i; --i, ++gfn, addr += PAGE_SIZE) { + + /* Fetch the VMA if addr is not in the latest fetched one */ + if (!vma || (addr < vma->vm_start || addr >= vma->vm_end)) { + vma = find_vma_intersection(kvm->mm, addr, end); + if (!vma || + vma->vm_start > addr || vma->vm_end < end) { + pr_err("Can't find VMA for gfn:0x%lx\n", gfn); + break; + } + } mutex_lock(&kvm->arch.uvmem_lock); - if (!kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) { + + if (kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) { + uvmem_page = pfn_to_page(uvmem_pfn); + pvt = uvmem_page->zone_device_data; + pvt->skip_page_out = skip_page_out; + pvt->remove_gfn = true; + + if (__kvmppc_svm_page_out(vma, addr, addr + PAGE_SIZE, + PAGE_SHIFT, kvm, pvt->gpa)) + pr_err("Can't page out gpa:0x%lx addr:0x%lx\n", + pvt->gpa, addr); + } else { + /* Remove the shared flag if any */ kvmppc_gfn_remove(gfn, kvm); - mutex_unlock(&kvm->arch.uvmem_lock); - continue; } - uvmem_page = pfn_to_page(uvmem_pfn); - pvt = uvmem_page->zone_device_data; - pvt->skip_page_out = skip_page_out; - pvt->remove_gfn = true; mutex_unlock(&kvm->arch.uvmem_lock); - - pfn = gfn_to_pfn(kvm, gfn); - if (is_error_noslot_pfn(pfn)) - continue; - kvm_release_pfn_clean(pfn); } + + up_read(&kvm->mm->mmap_sem); } unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm)
When a secure memslot is dropped, all the pages backed in the secure device (aka really backed by secure memory by the Ultravisor) should be paged out to a normal page. Previously, this was achieved by triggering the page fault mechanism which is calling kvmppc_svm_page_out() on each pages. This can't work when hot unplugging a memory slot because the memory slot is flagged as invalid and gfn_to_pfn() is then not trying to access the page, so the page fault mechanism is not triggered. Since the final goal is to make a call to kvmppc_svm_page_out() it seems simpler to directly calling it instead of triggering such a mechanism. This way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a memslot. Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock, the call to __kvmppc_svm_page_out() is made. As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the VMA is fetched in a lazy way, to not trigger find_vma() all the time. In addition, the mmap_sem is help in read mode during that time, not in write mode since the virual memory layout is not impacted, and kvm->arch.uvmem_lock prevents concurrent operation on the secure device. Cc: Ram Pai <linuxram@us.ibm.com> Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 17 deletions(-)