Re: [PATCH v2 1/3] RISC-V: KVM: Refactor kvm_arch_commit_memory_region()

[wang.yechao255 at zte.com.cn]

> > From: Wang Yechao <wang.yechao255 at zte.com.cn>
> >
> > 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 arm64 and x86 counterparts.
> 
> This claim of making it similar to arm64 and x86 is incorrect. See below.
> 
> >
> > Signed-off-by: Wang Yechao <wang.yechao255 at zte.com.cn>
> > ---
> >  arch/riscv/kvm/mmu.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> > index 2d3def024270..dc6a8efde99e 100644
> > --- a/arch/riscv/kvm/mmu.c
> > +++ b/arch/riscv/kvm/mmu.c
> > @@ -141,12 +141,22 @@ 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;
> > +       bool read_only = new && new->flags & KVM_MEM_READONLY;
> > +
> > +       /*
> > +        * Nothing more to do for RO slots (which can't be dirtied and can't be
> > +        * made writable) or CREATE/MOVE/DELETE of a slot.
> > +        */
> > +       if ((change != KVM_MR_FLAGS_ONLY) || read_only)
> > +               return;
> > +
> 
> I don't see how this is same as what x86 and arm64 does. There is
> no special casing for KVM_MR_FLAGS_ONLY in x86 and arm64.
> 
This is closer to the `kvm_mmu_slot_apply_flags` function on x86, so perhaps
it would be better to refer to the arm64 implementation to modify this function.

> >         /*
> >          * 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) {
> 
> Why drop "change != KVM_MR_DELETE" ?

The above condition "change != KVM_MR_FLAGS_ONLY" already filters out the
condition "change != KVM_MR_DELETE".

Best regards,
Yechao
> 
> >                 if (kvm_dirty_log_manual_protect_and_init_set(kvm))
> >                         return;
> >                 mmu_wp_memory_region(kvm, new->id);
> > --
> > 2.43.5
> 
> Regards,
> Anup