Re: [PATCH] make vfio and DAX cache work together

[Alex Williamson]

On Mon, 26 Apr 2021 13:19:05 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Edge NFV (edgenfv@gmail.com) wrote:
> >  Signed-off-by: Edge NFV <edgenfv@gmail.com>  
> 
> Hi,
>   I take it that 'Edge NFV' isn't your real name; apologies if it is.
> It's unusual not to use a real name; I would be interested to know
> why you feel uncomfortable not doing.

The documentation noted by Laurent even goes so far as to request a
real name for the Sign-off.  Intentionally masking your identity will
only serve to raise suspicion and increase the chances that the patch
is ignored.  I think perhaps aside from one or two legacy contributors,
all QEMU contributions are signed-off by real names these days.

I also require a commit log describing the change.

> > ---
> >  hw/vfio/common.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ae5654fcdb..83e15bf7a3 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -668,6 +668,15 @@ static void vfio_listener_region_add(MemoryListener
> > *listener,
> >                  int128_get64(int128_sub(section->size, int128_one())));
> >          return;
> >      }
> > +
> > +    /* Do not add virtio fs cache section */
> > +    if (!strcmp(memory_region_name(section->mr), "virtio-fs-cache")) {  
> 
> So first, this is a patch that fixes something that isn't yet in qemu;
> the DAX mode of virtiofs.
> Secondly, hard coding the name like this is probably the wrong thing to
> do; we need a way for the cache to declare it wants to be omitted.
> Thirdly, shouldn't this actually be a change to
> vfio_listener_skip_section to add this test?

Agree on all points, there needs to be justification on why this region
cannot be a DMA target for the device, not simply wishing to skip it to
workaround a boot failure.  Thanks,

Alex

> > +        trace_vfio_listener_region_add_skip(
> > +                section->offset_within_address_space,
> > +                section->offset_within_address_space +
> > +                int128_get64(int128_sub(section->size, int128_one())));
> > +        return;
> > +    }
> > 
> >      if (unlikely((section->offset_within_address_space &
> >                    ~qemu_real_host_page_mask) !=
> > -- 
> > 2.25.1