| Message ID | 20230206165851.3106338-6-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Eager Page Splitting for ARM. | expand |
Hi Ricardo, On 2/7/23 3:58 AM, Ricardo Koller wrote: > Refactor kvm_arch_commit_memory_region() as a preparation for a future > commit to look cleaner and more understandable. Also, it looks more > like its x86 counterpart (in kvm_mmu_slot_apply_flags()). > > No functional change intended. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arch/arm64/kvm/mmu.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 9bd3c2cfb476..d2c5e6992459 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1761,20 +1761,27 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, > const struct kvm_memory_slot *new, > enum kvm_mr_change change) > { > + bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES; > + > /* > * At this point memslot has been committed and there is an > * allocated dirty_bitmap[], dirty pages will be tracked while the > * memory slot is write protected. > */ > - if (change != KVM_MR_DELETE && new->flags & KVM_MEM_LOG_DIRTY_PAGES) { > + if (log_dirty_pages) { > + > + if (change == KVM_MR_DELETE) > + return; > + When @change is KVM_MR_DELETE, @new should be NULL. It means this check isn't needed? > /* > * If we're with initial-all-set, we don't need to write > * protect any pages because they're all reported as dirty. > * Huge pages and normal pages will be write protect gradually. > */ > - if (!kvm_dirty_log_manual_protect_and_init_set(kvm)) { > - kvm_mmu_wp_memory_region(kvm, new->id); > - } > + if (kvm_dirty_log_manual_protect_and_init_set(kvm)) > + return; > + > + kvm_mmu_wp_memory_region(kvm, new->id); > } > } > Thanks, Gavin
On Wed, Feb 8, 2023 at 10:02 PM Gavin Shan <gshan@redhat.com> wrote: > > Hi Ricardo, > > On 2/7/23 3:58 AM, Ricardo Koller wrote: > > Refactor kvm_arch_commit_memory_region() as a preparation for a future > > commit to look cleaner and more understandable. Also, it looks more > > like its x86 counterpart (in kvm_mmu_slot_apply_flags()). > > > > No functional change intended. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arch/arm64/kvm/mmu.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > > index 9bd3c2cfb476..d2c5e6992459 100644 > > --- a/arch/arm64/kvm/mmu.c > > +++ b/arch/arm64/kvm/mmu.c > > @@ -1761,20 +1761,27 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, > > const struct kvm_memory_slot *new, > > enum kvm_mr_change change) > > { > > + bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES; > > + > > /* > > * At this point memslot has been committed and there is an > > * allocated dirty_bitmap[], dirty pages will be tracked while the > > * memory slot is write protected. > > */ > > - if (change != KVM_MR_DELETE && new->flags & KVM_MEM_LOG_DIRTY_PAGES) { > > + if (log_dirty_pages) { > > + > > + if (change == KVM_MR_DELETE) > > + return; > > + > > When @change is KVM_MR_DELETE, @new should be NULL. It means this check > isn't needed? If you don't mind, I prefer not risking making this commit change some functionality. > > > /* > > * If we're with initial-all-set, we don't need to write > > * protect any pages because they're all reported as dirty. > > * Huge pages and normal pages will be write protect gradually. > > */ > > - if (!kvm_dirty_log_manual_protect_and_init_set(kvm)) { > > - kvm_mmu_wp_memory_region(kvm, new->id); > > - } > > + if (kvm_dirty_log_manual_protect_and_init_set(kvm)) > > + return; > > + > > + kvm_mmu_wp_memory_region(kvm, new->id); > > } > > } > > > > Thanks, > Gavin > > >
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 9bd3c2cfb476..d2c5e6992459 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1761,20 +1761,27 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, const struct kvm_memory_slot *new, enum kvm_mr_change change) { + bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES; + /* * At this point memslot has been committed and there is an * allocated dirty_bitmap[], dirty pages will be tracked while the * memory slot is write protected. */ - if (change != KVM_MR_DELETE && new->flags & KVM_MEM_LOG_DIRTY_PAGES) { + if (log_dirty_pages) { + + if (change == KVM_MR_DELETE) + return; + /* * If we're with initial-all-set, we don't need to write * protect any pages because they're all reported as dirty. * Huge pages and normal pages will be write protect gradually. */ - if (!kvm_dirty_log_manual_protect_and_init_set(kvm)) { - kvm_mmu_wp_memory_region(kvm, new->id); - } + if (kvm_dirty_log_manual_protect_and_init_set(kvm)) + return; + + kvm_mmu_wp_memory_region(kvm, new->id); } }
Refactor kvm_arch_commit_memory_region() as a preparation for a future commit to look cleaner and more understandable. Also, it looks more like its x86 counterpart (in kvm_mmu_slot_apply_flags()). No functional change intended. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arch/arm64/kvm/mmu.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)