On 01/18/2018 01:56 PM, Claudio Imbrenda wrote:
> This is a fix for several issues that were found in the original code
> for storage attributes migration.
>
> Now no bitmap is allocated to keep track of dirty storage attributes;
> the extra bits of the per-memslot bitmap that are always present anyway
> are now used for this purpose.
>
> The code has also been refactored a little to improve readability.
>
> Fixes: 190df4a212a ("KVM: s390: CMMA tracking, ESSA emulation, migration mode")
> Fixes: 4036e3874a1 ("KVM: s390: ioctls to get and set guest storage attributes")
> Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxxxxxxx>
> ---
> arch/s390/include/asm/kvm_host.h | 9 +-
> arch/s390/kvm/kvm-s390.c | 265 ++++++++++++++++++++++-----------------
> arch/s390/kvm/priv.c | 26 ++--
> 3 files changed, 168 insertions(+), 132 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 819838c..2ca73b0 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -763,12 +763,6 @@ struct kvm_s390_vsie {
> struct page *pages[KVM_MAX_VCPUS];
> };
>
> -struct kvm_s390_migration_state {
> - unsigned long bitmap_size; /* in bits (number of guest pages) */
> - atomic64_t dirty_pages; /* number of dirty pages */
> - unsigned long *pgste_bitmap;
> -};
> -
> struct kvm_arch{
> void *sca;
> int use_esca;
> @@ -796,7 +790,8 @@ struct kvm_arch{
> struct kvm_s390_vsie vsie;
> u8 epdx;
> u64 epoch;
> - struct kvm_s390_migration_state *migration_state;
> + atomic_t migration_mode;
> + atomic64_t cmma_dirty_pages;
> /* subset of available cpu features enabled by user space */
> DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
> struct kvm_s390_gisa *gisa;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5a69334..85448a2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -770,53 +770,37 @@ static void kvm_s390_sync_request_broadcast(struct kvm *kvm, int req)
> */
> static int kvm_s390_vm_start_migration(struct kvm *kvm)
> {
> - struct kvm_s390_migration_state *mgs;
> - struct kvm_memory_slot *ms;
> - /* should be the only one */
> struct kvm_memslots *slots;
> - unsigned long ram_pages;
> + struct kvm_memory_slot *sl;
> + unsigned long ram_pages = 0;
> int slotnr;
>
> /* migration mode already enabled */
> - if (kvm->arch.migration_state)
> + if (atomic_read(&kvm->arch.migration_mode))
> return 0;
Being atomic does not help here, because you just use read and write
(and not an atomic op). We are protected by a mutex, though. So maybe
just use a normal bool variable and add a comment.
Please also have a look at
KVM: s390: add proper locking for CMMA migration bitmap
which changes the locking. Do we want to keep some of these
changes (e.g. use the slots_lock)? Or do we want to schedule that fix
for stable and use your patches only for 4.16 and later?
[...]
> + atomic64_set(&kvm->arch.cmma_dirty_pages, ram_pages);
> + atomic_set(&kvm->arch.migration_mode, 1);
> + kvm_s390_sync_request_broadcast(kvm, KVM_REQ_START_MIGRATION);
> return 0;
> }
>
> @@ -826,19 +810,12 @@ static int kvm_s390_vm_start_migration(struct kvm *kvm)
> */
> static int kvm_s390_vm_stop_migration(struct kvm *kvm)
> {
> - struct kvm_s390_migration_state *mgs;
> -
> /* migration mode already disabled */
> - if (!kvm->arch.migration_state)
> + if (!atomic_read(&kvm->arch.migration_mode))
> return 0;
> - mgs = kvm->arch.migration_state;
> - kvm->arch.migration_state = NULL;
> -
> - if (kvm->arch.use_cmma) {
> + atomic_set(&kvm->arch.migration_mode, 0);
> + if (kvm->arch.use_cmma)
> kvm_s390_sync_request_broadcast(kvm, KVM_REQ_STOP_MIGRATION);
> - vfree(mgs->pgste_bitmap);
> - }
> - kfree(mgs);
> return 0;
> }
>
> @@ -868,7 +845,7 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
> static int kvm_s390_vm_get_migration(struct kvm *kvm,
> struct kvm_device_attr *attr)
> {
> - u64 mig = (kvm->arch.migration_state != NULL);
> + u64 mig = atomic_read(&kvm->arch.migration_mode);
>
> if (attr->attr != KVM_S390_VM_MIGRATION_STATUS)
> return -ENXIO;
> @@ -1543,6 +1520,108 @@ static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn)
> return start;
> }
>
> +static int kvm_s390_peek_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
> + u8 *res, unsigned long bufsize)
> +{
> + unsigned long pgstev, cur_gfn, hva;
> +
> + cur_gfn = args->start_gfn;
> + args->count = 0;
> + while (args->count < bufsize) {
> + hva = gfn_to_hva(kvm, cur_gfn);
> + /*
> + * We return an error if the first value was invalid, but we
> + * return successfully if at least one value was copied.
> + */
> + if (kvm_is_error_hva(hva))
> + return args->count ? 0 : -EFAULT;
> + if (get_pgste(kvm->mm, hva, &pgstev) < 0)
> + pgstev = 0;
> + res[args->count++] = (pgstev >> 24) & 0x43;
> + cur_gfn++;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned long kvm_s390_next_dirty_cmma(struct kvm *kvm,
> + unsigned long cur)
> +{
> + struct kvm_memslots *slots = kvm_memslots(kvm);
> + struct kvm_memory_slot *sl;
> + unsigned long ofs;
> + int slotidx;
> +
> + slotidx = gfn_to_memslot_approx(kvm, cur);
> + sl = slots->memslots + slotidx;
> +
> + if (sl->base_gfn + sl->npages <= cur) {
> + slotidx--;
> + /* If we are above the highest slot, wrap around */
> + if (slotidx < 0)
> + slotidx = slots->used_slots - 1;
> +
> + sl = slots->memslots + slotidx;
> + cur = sl->base_gfn;
> + }
> + ofs = find_next_bit(cmma_bitmap(sl), sl->npages, cur - sl->base_gfn);
> + while ((slotidx > 0) && (ofs >= sl->npages)) {
> + slotidx--;
> + sl = slots->memslots + slotidx;
> + ofs = find_next_bit(cmma_bitmap(sl), sl->npages, 0);
> + }
> + return sl->base_gfn + ofs;
> +}
> +
> +static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
> + u8 *res, unsigned long bufsize)
> +{
> + unsigned long mem_end, cur_gfn, next_gfn, hva, pgstev;
> + struct kvm_memslots *slots = kvm_memslots(kvm);
> + struct kvm_memory_slot *sl;
> +
> + cur_gfn = kvm_s390_next_dirty_cmma(kvm, args->start_gfn);
> + sl = gfn_to_memslot(kvm, cur_gfn);
> + args->count = 0;
> + args->start_gfn = cur_gfn;
> + if (!sl)
> + return 0;
> + next_gfn = kvm_s390_next_dirty_cmma(kvm, cur_gfn + 1);
> + mem_end = slots->memslots[0].base_gfn + slots->memslots[0].npages;
> +
> + while (args->count < bufsize) {
> + hva = gfn_to_hva(kvm, cur_gfn);
> + if (kvm_is_error_hva(hva))
> + break;
> + /* decrement only if we actually flipped the bit to 0 */
> + if (test_and_clear_bit(cur_gfn - sl->base_gfn, cmma_bitmap(sl)))
> + atomic64_dec(&kvm->arch.cmma_dirty_pages);
> + if (get_pgste(kvm->mm, hva, &pgstev) < 0)
> + pgstev = 0;
> + /* save the value */
> + res[args->count++] = (pgstev >> 24) & 0x43;
> + /*
> + * if the next bit is too far away, stop.
> + * if we reached the previous "next", find the next one
> + */
> + if (next_gfn > cur_gfn + KVM_S390_MAX_BIT_DISTANCE)
> + break;
> + if (cur_gfn == next_gfn)
> + next_gfn = kvm_s390_next_dirty_cmma(kvm, cur_gfn + 1);
> + /* reached the end of memory or of the buffer, stop */
> + if ((next_gfn >= mem_end) ||
> + (next_gfn - args->start_gfn >= bufsize))
> + break;
> + cur_gfn++;
> + if (cur_gfn - sl->base_gfn >= sl->npages) {
> + sl = gfn_to_memslot(kvm, cur_gfn);
> + if (!sl)
> + break;
> + }
> + }
> + return 0;
> +}
> +
> /*
> * This function searches for the next page with dirty CMMA attributes, and
> * saves the attributes in the buffer up to either the end of the buffer or
> @@ -1554,97 +1633,53 @@ static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn)
> static int kvm_s390_get_cmma_bits(struct kvm *kvm,
> struct kvm_s390_cmma_log *args)
> {
> - struct kvm_s390_migration_state *s = kvm->arch.migration_state;
> - unsigned long bufsize, hva, pgstev, i, next, cur;
> - int srcu_idx, peek, r = 0, rr;
> - u8 *res;
> + unsigned long bufsize;
> + int srcu_idx, peek, migr, ret;
> + u8 *values;
>
> - cur = args->start_gfn;
> - i = next = pgstev = 0;
> + migr = atomic_read(&kvm->arch.migration_mode);
>
> if (unlikely(!kvm->arch.use_cmma))
> return -ENXIO;
> /* Invalid/unsupported flags were specified */
> - if (args->flags & ~KVM_S390_CMMA_PEEK)
> + if (unlikely(args->flags & ~KVM_S390_CMMA_PEEK))
> return -EINVAL;
> /* Migration mode query, and we are not doing a migration */
> peek = !!(args->flags & KVM_S390_CMMA_PEEK);
> - if (!peek && !s)
> + if (unlikely(!peek && !migr))
> return -EINVAL;
> /* CMMA is disabled or was not used, or the buffer has length zero */
> bufsize = min(args->count, KVM_S390_CMMA_SIZE_MAX);
> - if (!bufsize || !kvm->mm->context.use_cmma) {
> + if (unlikely(!bufsize || !kvm->mm->context.use_cmma)) {
> memset(args, 0, sizeof(*args));
> return 0;
> }
> -
> - if (!peek) {
> - /* We are not peeking, and there are no dirty pages */
> - if (!atomic64_read(&s->dirty_pages)) {
> - memset(args, 0, sizeof(*args));
> - return 0;
> - }
> - cur = find_next_bit(s->pgste_bitmap, s->bitmap_size,
> - args->start_gfn);
> - if (cur >= s->bitmap_size) /* nothing found, loop back */
> - cur = find_next_bit(s->pgste_bitmap, s->bitmap_size, 0);
> - if (cur >= s->bitmap_size) { /* again! (very unlikely) */
> - memset(args, 0, sizeof(*args));
> - return 0;
> - }
> - next = find_next_bit(s->pgste_bitmap, s->bitmap_size, cur + 1);
> + /* We are not peeking, and there are no dirty pages */
> + if (!peek && !atomic64_read(&kvm->arch.cmma_dirty_pages)) {
> + memset(args, 0, sizeof(*args));
> + return 0;
> }
>
> - res = vmalloc(bufsize);
> - if (!res)
> + values = vmalloc(bufsize);
> + if (!values)
> return -ENOMEM;
>
> - args->start_gfn = cur;
> -
> down_read(&kvm->mm->mmap_sem);
> srcu_idx = srcu_read_lock(&kvm->srcu);
> - while (i < bufsize) {
> - hva = gfn_to_hva(kvm, cur);
> - if (kvm_is_error_hva(hva)) {
> - r = -EFAULT;
> - break;
> - }
> - /* decrement only if we actually flipped the bit to 0 */
> - if (!peek && test_and_clear_bit(cur, s->pgste_bitmap))
> - atomic64_dec(&s->dirty_pages);
> - r = get_pgste(kvm->mm, hva, &pgstev);
> - if (r < 0)
> - pgstev = 0;
> - /* save the value */
> - res[i++] = (pgstev >> 24) & 0x43;
> - /*
> - * if the next bit is too far away, stop.
> - * if we reached the previous "next", find the next one
> - */
> - if (!peek) {
> - if (next > cur + KVM_S390_MAX_BIT_DISTANCE)
> - break;
> - if (cur == next)
> - next = find_next_bit(s->pgste_bitmap,
> - s->bitmap_size, cur + 1);
> - /* reached the end of the bitmap or of the buffer, stop */
> - if ((next >= s->bitmap_size) ||
> - (next >= args->start_gfn + bufsize))
> - break;
> - }
> - cur++;
> - }
> + if (peek)
> + ret = kvm_s390_peek_cmma(kvm, args, values, bufsize);
> + else
> + ret = kvm_s390_get_cmma(kvm, args, values, bufsize);
> srcu_read_unlock(&kvm->srcu, srcu_idx);
> up_read(&kvm->mm->mmap_sem);
> - args->count = i;
> - args->remaining = s ? atomic64_read(&s->dirty_pages) : 0;
>
> - rr = copy_to_user((void __user *)args->values, res, args->count);
> - if (rr)
> - r = -EFAULT;
> + args->remaining = migr ? atomic64_read(&kvm->arch.cmma_dirty_pages) : 0;
>
> - vfree(res);
> - return r;
> + if (copy_to_user((void __user *)args->values, values, args->count))
> + ret = -EFAULT;
> +
> + vfree(values);
> + return ret;
> }
>
> /*
> @@ -2088,10 +2123,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> kvm_s390_destroy_adapters(kvm);
> kvm_s390_clear_float_irqs(kvm);
> kvm_s390_vsie_destroy(kvm);
> - if (kvm->arch.migration_state) {
> - vfree(kvm->arch.migration_state->pgste_bitmap);
> - kfree(kvm->arch.migration_state);
> - }
> KVM_EVENT(3, "vm 0x%pK destroyed", kvm);
> }
>
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 572496c..7ae4893 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -954,9 +954,11 @@ static int handle_pfmf(struct kvm_vcpu *vcpu)
> return 0;
> }
>
> -static inline int do_essa(struct kvm_vcpu *vcpu, const int orc)
> +/*
> + * Must be called with relevant read locks held (kvm->mm->mmap_sem, kvm->srcu)
> + */
> +static inline int __do_essa(struct kvm_vcpu *vcpu, const int orc)
> {
> - struct kvm_s390_migration_state *ms = vcpu->kvm->arch.migration_state;
> int r1, r2, nappended, entries;
> unsigned long gfn, hva, res, pgstev, ptev;
> unsigned long *cbrlo;
> @@ -1007,9 +1009,11 @@ static inline int do_essa(struct kvm_vcpu *vcpu, const int orc)
> }
>
> if (orc) {
> - /* increment only if we are really flipping the bit to 1 */
> - if (!test_and_set_bit(gfn, ms->pgste_bitmap))
> - atomic64_inc(&ms->dirty_pages);
> + struct kvm_memory_slot *s = gfn_to_memslot(vcpu->kvm, gfn);
> +
> + /* Increment only if we are really flipping the bit */
> + if (s && !test_and_set_bit(gfn - s->base_gfn, cmma_bitmap(s)))
> + atomic64_inc(&vcpu->kvm->arch.cmma_dirty_pages);
> }
>
> return nappended;
> @@ -1038,7 +1042,7 @@ static int handle_essa(struct kvm_vcpu *vcpu)
> : ESSA_SET_STABLE_IF_RESIDENT))
> return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>
> - if (likely(!vcpu->kvm->arch.migration_state)) {
> + if (likely(!atomic_read(&vcpu->kvm->arch.migration_mode))) {
> /*
> * CMMA is enabled in the KVM settings, but is disabled in
> * the SIE block and in the mm_context, and we are not doing
> @@ -1066,10 +1070,16 @@ static int handle_essa(struct kvm_vcpu *vcpu)
> /* Retry the ESSA instruction */
> kvm_s390_retry_instr(vcpu);
> } else {
> - /* Account for the possible extra cbrl entry */
> - i = do_essa(vcpu, orc);
> + int srcu_idx;
> +
> + down_read(&vcpu->kvm->mm->mmap_sem);
> + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> + i = __do_essa(vcpu, orc);
> + srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx);
> + up_read(&vcpu->kvm->mm->mmap_sem);
> if (i < 0)
> return i;
> + /* Account for the possible extra cbrl entry */
> entries += i;
> }
> vcpu->arch.sie_block->cbrlo &= PAGE_MASK; /* reset nceo */
>
