Re: [PATCH v4 02/66] RAMBlock: Add support of KVM private guest memfd

[Xiaoyao Li]

On 1/26/2024 9:57 PM, David Hildenbrand wrote:
> >   uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
> >   {
> >       uint8_t mask = mr->dirty_log_mask;
> > diff --git a/system/physmem.c b/system/physmem.c
> > index c1b22bac77c2..4735b0462ed9 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -1841,6 +1841,17 @@ static void ram_block_add(RAMBlock *new_block, 
> > Error **errp)
> >           }
> >       }
> > +    if (kvm_enabled() && (new_block->flags & RAM_GUEST_MEMFD) &&
> > +        new_block->guest_memfd < 0) {
>
> How could we have a guest_memfd already at this point? Smells more like 
> an assert(new_block->guest_memfd < 0);

you are right. I will change it to the assert()

> > +        /* TODO: to decide if KVM_GUEST_MEMFD_ALLOW_HUGEPAGE is 
> > supported */
>
> I suggest dropping that completely. As long as it's not upstream, not 
> even the name of that thing is stable.

OK

> > +        new_block->guest_memfd = 
> > kvm_create_guest_memfd(new_block->max_length,
> > +                                                        0, errp);
> > +        if (new_block->guest_memfd < 0) {
> > +            qemu_mutex_unlock_ramlist();
> > +            return;
> > +        }
> > +    }
> > +
>
>
> In general, LGTM. With the two nits above:
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!