| Message ID | 20210213005015.1651772-11-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Dirty logging fixes and improvements | expand |
On 13/02/21 01:50, Sean Christopherson wrote: > > - * Nothing to do for RO slots or CREATE/MOVE/DELETE of a slot. > - * See comments below. > + * Nothing to do for RO slots (which can't be dirtied and can't be made > + * writable) or CREATE/MOVE/DELETE of a slot. See comments below. > */ > if ((change != KVM_MR_FLAGS_ONLY) || (new->flags & KVM_MEM_READONLY)) > return; > > + /* > + * READONLY and non-flags changes were filtered out above, and the only > + * other flag is LOG_DIRTY_PAGES, i.e. something is wrong if dirty > + * logging isn't being toggled on or off. > + */ > + if (WARN_ON_ONCE(!((old->flags ^ new->flags) & KVM_MEM_LOG_DIRTY_PAGES))) > + return; > + What about readonly -> readwrite changes? Paolo
On Thu, Feb 18, 2021, Paolo Bonzini wrote: > On 13/02/21 01:50, Sean Christopherson wrote: > > > > - * Nothing to do for RO slots or CREATE/MOVE/DELETE of a slot. > > - * See comments below. > > + * Nothing to do for RO slots (which can't be dirtied and can't be made > > + * writable) or CREATE/MOVE/DELETE of a slot. See comments below. > > */ > > if ((change != KVM_MR_FLAGS_ONLY) || (new->flags & KVM_MEM_READONLY)) > > return; > > + /* > > + * READONLY and non-flags changes were filtered out above, and the only > > + * other flag is LOG_DIRTY_PAGES, i.e. something is wrong if dirty > > + * logging isn't being toggled on or off. > > + */ > > + if (WARN_ON_ONCE(!((old->flags ^ new->flags) & KVM_MEM_LOG_DIRTY_PAGES))) > > + return; > > + > > What about readonly -> readwrite changes? Not allowed without first deleting the memslot. See commit 75d61fbcf563 ("KVM: set_memory_region: Disallow changing read-only attribute later"). RW->RO is also not supported. if (!old.npages) { change = KVM_MR_CREATE; new.dirty_bitmap = NULL; memset(&new.arch, 0, sizeof(new.arch)); } else { /* Modify an existing slot. */ if ((new.userspace_addr != old.userspace_addr) || (new.npages != old.npages) || ((new.flags ^ old.flags) & KVM_MEM_READONLY)) return -EINVAL;
On 18/02/21 17:15, Sean Christopherson wrote: > On Thu, Feb 18, 2021, Paolo Bonzini wrote: >> On 13/02/21 01:50, Sean Christopherson wrote: >>> >>> - * Nothing to do for RO slots or CREATE/MOVE/DELETE of a slot. >>> - * See comments below. >>> + * Nothing to do for RO slots (which can't be dirtied and can't be made >>> + * writable) or CREATE/MOVE/DELETE of a slot. See comments below. >>> */ >>> if ((change != KVM_MR_FLAGS_ONLY) || (new->flags & KVM_MEM_READONLY)) >>> return; >>> + /* >>> + * READONLY and non-flags changes were filtered out above, and the only >>> + * other flag is LOG_DIRTY_PAGES, i.e. something is wrong if dirty >>> + * logging isn't being toggled on or off. >>> + */ >>> + if (WARN_ON_ONCE(!((old->flags ^ new->flags) & KVM_MEM_LOG_DIRTY_PAGES))) >>> + return; >>> + >> >> What about readonly -> readwrite changes? > > Not allowed without first deleting the memslot. See commit 75d61fbcf563 ("KVM: > set_memory_region: Disallow changing read-only attribute later"). RW->RO is > also not supported. > > if (!old.npages) { > change = KVM_MR_CREATE; > new.dirty_bitmap = NULL; > memset(&new.arch, 0, sizeof(new.arch)); > } else { /* Modify an existing slot. */ > if ((new.userspace_addr != old.userspace_addr) || > (new.npages != old.npages) || > ((new.flags ^ old.flags) & KVM_MEM_READONLY)) > return -EINVAL; > Right you are, thanks. Then the warning would catch this quick. I queued 10 and 11 too, and will reply on 12 now that I looked at it more closely. Paolo
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e89fe98a0099..c0d22f19aed0 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10761,12 +10761,20 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm, enum kvm_mr_change change) { /* - * Nothing to do for RO slots or CREATE/MOVE/DELETE of a slot. - * See comments below. + * Nothing to do for RO slots (which can't be dirtied and can't be made + * writable) or CREATE/MOVE/DELETE of a slot. See comments below. */ if ((change != KVM_MR_FLAGS_ONLY) || (new->flags & KVM_MEM_READONLY)) return; + /* + * READONLY and non-flags changes were filtered out above, and the only + * other flag is LOG_DIRTY_PAGES, i.e. something is wrong if dirty + * logging isn't being toggled on or off. + */ + if (WARN_ON_ONCE(!((old->flags ^ new->flags) & KVM_MEM_LOG_DIRTY_PAGES))) + return; + /* * Dirty logging tracks sptes in 4k granularity, meaning that large * sptes have to be split. If live migration is successful, the guest @@ -10784,8 +10792,7 @@ static void kvm_mmu_slot_apply_flags(struct kvm *kvm, * MOVE/DELETE: The old mappings will already have been cleaned up by * kvm_arch_flush_shadow_memslot() */ - if ((old->flags & KVM_MEM_LOG_DIRTY_PAGES) && - !(new->flags & KVM_MEM_LOG_DIRTY_PAGES)) + if (!(new->flags & KVM_MEM_LOG_DIRTY_PAGES)) kvm_mmu_zap_collapsible_sptes(kvm, new); /*
Add a sanity check in kvm_mmu_slot_apply_flags to assert that the LOG_DIRTY_PAGES flag is indeed being toggled, and explicitly rely on that holding true when zapping collapsible SPTEs. Manipulating the CPU dirty log (PML) and write-protection also relies on this assertion, but that's not obvious in the current code. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)