lkml.org 
[lkml]   [2022]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 04/17] KVM: s390: pv: refactor s390_reset_acc
    On Mon, 7 Feb 2022 11:56:11 +0100
    Janosch Frank <frankja@linux.ibm.com> wrote:

    > On 2/7/22 11:47, Claudio Imbrenda wrote:
    > > On Mon, 7 Feb 2022 11:02:28 +0100
    > > Janosch Frank <frankja@linux.ibm.com> wrote:
    > >
    > >> On 2/4/22 16:53, Claudio Imbrenda wrote:
    > >>> Refactor s390_reset_acc so that it can be reused in upcoming patches.
    > >>>
    > >>> We don't want to hold all the locks used in a walk_page_range for too
    > >>> long, and the destroy page UVC does take some time to complete.
    > >>> Therefore we quickly gather the pages to destroy, and then destroy them
    > >>> without holding all the locks.
    > >>>
    > >>> The new refactored function optionally allows to return early without
    > >>> completing if a fatal signal is pending (and return and appropriate
    > >>> error code). Two wrappers are provided to call the new function.
    > >>>
    > >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
    > >>> (dropping Janosch's Ack because of major changes to the patch)
    > >>> ---
    > >>> arch/s390/include/asm/gmap.h | 36 +++++++++++++-
    > >>> arch/s390/kvm/pv.c | 12 ++++-
    > >>> arch/s390/mm/gmap.c | 95 +++++++++++++++++++++++++-----------
    > >>> 3 files changed, 111 insertions(+), 32 deletions(-)
    > >>>
    > >>> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
    > >>> index 746e18bf8984..2a913014f63c 100644
    > >>> --- a/arch/s390/include/asm/gmap.h
    > >>> +++ b/arch/s390/include/asm/gmap.h
    > >>> @@ -147,7 +147,41 @@ 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);
    > >>> -void s390_reset_acc(struct mm_struct *mm);
    > >>> void s390_remove_old_asce(struct gmap *gmap);
    > >>> int s390_replace_asce(struct gmap *gmap);
    > >>> +void s390_uv_destroy_pfns(unsigned long count, unsigned long *pfns);
    > >>> +int __s390_uv_destroy_range(struct mm_struct *mm, unsigned long start,
    > >>> + unsigned long end, int interruptible);
    > >>
    > >> s/int/bool/ ?
    > >
    > > I don't know, I like the fact that the function could return
    > > different error codes in the future if needed.
    >
    > I meant the interruptible parameter or do you expect to have various
    > levels of interruptibility (is that even a word)?

    ooohhh, that one.

    I guess it could be a bool, yes

    >
    > >
    > >>
    > >>> +
    > >>> +/**
    > >>> + * s390_uv_destroy_range - Destroy a range of pages in the given mm.
    > >>> + * @mm the mm on which to operate on
    > >>> + * @start the start of the range
    > >>> + * @end the end of the range
    > >>> + *
    > >>> + * This call will call cond_sched, so it should not generate stalls, but it
    > >>
    > >> This function will call ?
    > >
    > > will fix
    > >
    > >>
    > >>> + * will otherwise only return when it completed.
    > >>> + */
    > >>> +static inline void s390_uv_destroy_range(struct mm_struct *mm, unsigned long start,
    > >>> + unsigned long end)
    > >>> +{
    > >>> + (void)__s390_uv_destroy_range(mm, start, end, 0);
    > >>> +}
    > >>> +
    > >>> +/**
    > >>> + * s390_uv_destroy_range_interruptibe - Destroy a range of pages in the
    > >>
    > >> interruptible
    > >
    > > will fix
    > >
    > >>
    > >>> + * given mm, but stop when a fatal signal is received.
    > >>> + * @mm the mm on which to operate on
    > >>> + * @start the start of the range
    > >>> + * @end the end of the range
    > >>> + *
    > >>> + * This call will call cond_sched, so it should not generate stalls. It
    > >>> + * will return -EINTR if a fatal signal is received, or 0 if the whole range
    > >>> + * has been destroyed.
    > >>> + */
    > >>> +static inline int s390_uv_destroy_range_interruptible(struct mm_struct *mm, unsigned long start,
    > >>> + unsigned long end)
    > >>> +{
    > >>> + return __s390_uv_destroy_range(mm, start, end, 1);
    > >>> +}
    > >>> #endif /* _ASM_S390_GMAP_H */
    > >>> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
    > >>> index 3c59ef763dde..2ab22500e092 100644
    > >>> --- a/arch/s390/kvm/pv.c
    > >>> +++ b/arch/s390/kvm/pv.c
    > >>> @@ -12,6 +12,8 @@
    > >>> #include <asm/gmap.h>
    > >>> #include <asm/uv.h>
    > >>> #include <asm/mman.h>
    > >>> +#include <linux/pagewalk.h>
    > >>> +#include <linux/sched/mm.h>
    > >>> #include "kvm-s390.h"
    > >>>
    > >>> int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
    > >>> @@ -157,8 +159,14 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
    > >>> {
    > >>> int cc;
    > >>>
    > >>> - /* make all pages accessible before destroying the guest */
    > >>> - s390_reset_acc(kvm->mm);
    > >>> + /*
    > >>> + * if the mm still has a mapping, make all its pages accessible
    > >>> + * before destroying the guest
    > >>> + */
    > >>> + if (mmget_not_zero(kvm->mm)) {
    > >>> + s390_uv_destroy_range(kvm->mm, 0, TASK_SIZE);
    > >>> + mmput(kvm->mm);
    > >>> + }
    > >>>
    > >>> cc = uv_cmd_nodata(kvm_s390_pv_get_handle(kvm),
    > >>> UVC_CMD_DESTROY_SEC_CONF, rc, rrc);
    > >>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
    > >>> index ce6cac4463f2..6eb5acb4be3d 100644
    > >>> --- a/arch/s390/mm/gmap.c
    > >>> +++ b/arch/s390/mm/gmap.c
    > >>> @@ -2676,44 +2676,81 @@ void s390_reset_cmma(struct mm_struct *mm)
    > >>> }
    > >>> EXPORT_SYMBOL_GPL(s390_reset_cmma);
    > >>>
    > >>> -/*
    > >>> - * make inaccessible pages accessible again
    > >>> - */
    > >>> -static int __s390_reset_acc(pte_t *ptep, unsigned long addr,
    > >>> - unsigned long next, struct mm_walk *walk)
    > >>> +#define DESTROY_LOOP_THRESHOLD 32
    > >>> +
    > >>> +struct reset_walk_state {
    > >>> + unsigned long next;
    > >>> + unsigned long count;
    > >>> + unsigned long pfns[DESTROY_LOOP_THRESHOLD];
    > >>> +};
    > >>> +
    > >>> +static int s390_gather_pages(pte_t *ptep, unsigned long addr,
    > >>> + unsigned long next, struct mm_walk *walk)
    > >>> {
    > >>> + struct reset_walk_state *p = walk->private;
    > >>> pte_t pte = READ_ONCE(*ptep);
    > >>>
    > >>> - /* There is a reference through the mapping */
    > >>> - if (pte_present(pte))
    > >>> - WARN_ON_ONCE(uv_destroy_owned_page(pte_val(pte) & PAGE_MASK));
    > >>> -
    > >>> - return 0;
    > >>> + if (pte_present(pte)) {
    > >>> + /* we have a reference from the mapping, take an extra one */
    > >>> + get_page(phys_to_page(pte_val(pte)));
    > >>> + p->pfns[p->count] = phys_to_pfn(pte_val(pte));
    > >>> + p->next = next;
    > >>> + p->count++;
    > >>> + }
    > >>> + return p->count >= DESTROY_LOOP_THRESHOLD;
    > >>> }
    > >>>
    > >>> -static const struct mm_walk_ops reset_acc_walk_ops = {
    > >>> - .pte_entry = __s390_reset_acc,
    > >>> +static const struct mm_walk_ops gather_pages_ops = {
    > >>> + .pte_entry = s390_gather_pages,
    > >>> };
    > >>>
    > >>> -#include <linux/sched/mm.h>
    > >>> -void s390_reset_acc(struct mm_struct *mm)
    > >>> +/*
    > >>> + * Call the Destroy secure page UVC on each page in the given array of PFNs.
    > >>> + * Each page needs to have an extra reference, which will be released here.
    > >>> + */
    > >>> +void s390_uv_destroy_pfns(unsigned long count, unsigned long *pfns)
    > >>> {
    > >>> - if (!mm_is_protected(mm))
    > >>> - return;
    > >>> - /*
    > >>> - * we might be called during
    > >>> - * reset: we walk the pages and clear
    > >>> - * close of all kvm file descriptors: we walk the pages and clear
    > >>> - * exit of process on fd closure: vma already gone, do nothing
    > >>> - */
    > >>> - if (!mmget_not_zero(mm))
    > >>> - return;
    > >>> - mmap_read_lock(mm);
    > >>> - walk_page_range(mm, 0, TASK_SIZE, &reset_acc_walk_ops, NULL);
    > >>> - mmap_read_unlock(mm);
    > >>> - mmput(mm);
    > >>> + unsigned long i;
    > >>> +
    > >>> + for (i = 0; i < count; i++) {
    > >>> + /* we always have an extra reference */
    > >>> + uv_destroy_owned_page(pfn_to_phys(pfns[i]));
    > >>> + /* get rid of the extra reference */
    > >>> + put_page(pfn_to_page(pfns[i]));
    > >>> + cond_resched();
    > >>> + }
    > >>> +}
    > >>> +EXPORT_SYMBOL_GPL(s390_uv_destroy_pfns);
    > >>> +
    > >>> +/**
    > >>> + * __s390_uv_destroy_range - Walk the given range of the given address
    > >>> + * space, and call the destroy secure page UVC on each page.
    > >>> + * Optionally exit early if a fatal signal is pending.
    > >>> + * @mm the mm to operate on
    > >>> + * @start the start of the range
    > >>> + * @end the end of the range
    > >>> + * @interruptible if not 0, stop when a fatal signal is received
    > >>> + * Return: 0 on success, -EINTR if the function stopped before completing
    > >>> + */
    > >>> +int __s390_uv_destroy_range(struct mm_struct *mm, unsigned long start,
    > >>> + unsigned long end, int interruptible)
    > >>> +{
    > >>> + struct reset_walk_state state = { .next = start };
    > >>> + int r = 1;
    > >>> +
    > >>> + while (r > 0) {
    > >>> + state.count = 0;
    > >>> + mmap_read_lock(mm);
    > >>> + r = walk_page_range(mm, state.next, end, &gather_pages_ops, &state);
    > >>> + mmap_read_unlock(mm);
    > >>> + cond_resched();
    > >>> + s390_uv_destroy_pfns(state.count, state.pfns);
    > >>> + if (interruptible && fatal_signal_pending(current))
    > >>> + return -EINTR;
    > >>> + }
    > >>> + return 0;
    > >>> }
    > >>> -EXPORT_SYMBOL_GPL(s390_reset_acc);
    > >>> +EXPORT_SYMBOL_GPL(__s390_uv_destroy_range);
    > >>>
    > >>> /**
    > >>> * s390_remove_old_asce - Remove the topmost level of page tables from the
    > >>>
    > >>
    > >
    >

    \
     
     \ /
      Last update: 2022-02-07 12:09    [W:8.402 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site