On Wed, 17 Jan 2018 10:17:56 +0100
Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx> wrote:
> On 15.01.2018 18:03, 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.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxxxxxxx>
> > Fixes: 190df4a212a ("KVM: s390: CMMA tracking, ESSA emulation,
> > migration mode") Fixes: 4036e3874a1 ("KVM: s390: ioctls to get and
> > set guest storage attributes")
>
> Did Christians patches go into stable, and should yours too?
I don't know, and I don't know, but I'd bet probably yes
> > ---
>
> > static int kvm_s390_vm_start_migration(struct kvm *kvm)
> > {
> [...]
> > if (kvm->arch.use_cmma) {
> [...]
> > -
> > - mgs->bitmap_size = ram_pages;
> > - atomic64_set(&mgs->dirty_pages, ram_pages);
> > /* mark all the pages in active slots as dirty */
> > for (slotnr = 0; slotnr < slots->used_slots;
> > slotnr++) { ms = slots->memslots + slotnr;
> > - bitmap_set(mgs->pgste_bitmap,
> > ms->base_gfn, ms->npages);
> > + /*
> > + * The second half of the bitmap is only
> > used on x86,
> > + * and would be wasted otherwise, so we
> > put it to good
> > + * use here to keep track of the state of
> > the storage
> > + * attributes.
> > + */
> > + memset(_cmma_bitmap(ms), 0xFF,
> > + kvm_dirty_bitmap_bytes(ms));
>
> On your branch, the second line is not indented properly.
> Also s/0xFF/0xff/
will fix
> > + ram_pages += ms->npages;
> > }
> >
> > + 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);
> > + } else {
> > + atomic_set(&kvm->arch.migration_mode, 1);
>
> For stop migration, you do that unconditionally, why don't you move
> this in front of the if?
I want the bitmaps and the count of pages to be properly set up before
we enable migration mode.
we can intercept essa also during normal operation, and I don't want
migration_mode to be set before we're done setting up bitmaps and page
count, because the code in the essa handler could access both before
they're properly set up.
thread1: migration_mode=1
thread2: essa handler: migration_mode==1 -> set bit in bitmap
thread1: fill bitmap, cmma_dirty_pages=N
thread2: cmma_dirty_pages=N+1
now the number of dirty pages is one more than the maximum number of
pages, and could be a problem for userspace. this should not happen in
normal operation.
it can still happen if the process maliciously calls
kvm_s390_vm_start_migration several times in parallel, but this is not
our problem any longer.
in any case the race won't cause illegal memory
accessees, crashes, leaks or security issues, so I think we can ignore
the issue
> > }
> > return 0;
> > }
> > @@ -834,19 +820,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;
> > }
> >
> The rest below will take me some time.
> However, it is much more readable than before!
>
