Re: [PATCH v2 fixed 01/16] util: vfio-helpers: Factor out and fix processing of existing ram blocks

[Peter Xu]

On Wed, Feb 19, 2020 at 09:43:02AM +0100, David Hildenbrand wrote:
> On 18.02.20 23:00, Peter Xu wrote:
> > On Wed, Feb 12, 2020 at 02:42:39PM +0100, David Hildenbrand wrote:
> >> Factor it out into common code when a new notifier is registered, just
> >> as done with the memory region notifier. This allows us to have the
> >> logic about how to process existing ram blocks at a central place (which
> >> will be extended soon).
> >>
> >> Just like when adding a new ram block, we have to register the max_length
> >> for now. We don't have a way to get notified about resizes yet, and some
> >> memory would not be mapped when growing the ram block.
> >>
> >> Note: Currently, ram blocks are only "fake resized". All memory
> >> (max_length) is accessible.
> >>
> >> We can get rid of a bunch of functions in stubs/ram-block.c . Print the
> >> warning from inside qemu_vfio_ram_block_added().
> 
> [...]
> 
> >>  #include "exec/ramlist.h"
> >>  #include "exec/cpu-common.h"
> >>  
> >> -void *qemu_ram_get_host_addr(RAMBlock *rb)
> >> -{
> >> -    return 0;
> >> -}
> >> -
> >> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb)
> >> -{
> >> -    return 0;
> >> -}
> >> -
> >> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb)
> >> -{
> >> -    return 0;
> >> -}
> > 
> > Maybe put into another patch?
> > 
> > Actually I'm thinking whether it would worth to do...  They're still
> > declared in include/exec/cpu-common.h, so logically who includes the
> > header but linked against stubs can still call this function.  So
> > keeping them there still make sense to me.
> 
> Why keep dead code around? If you look closely, the stubs really only
> contain what's strictly necessary to make current code compile, not any
> available ramblock related function.

Seems correct.  Then it's fine.

> 
> I don't see a good reason for a separate patch either (after all, we're
> removing the last users in this patch), but if more people agree, I can
> move it to a separate patch.
> [...]
> 
> >> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> >> index 813f7ec564..71e02e7f35 100644
> >> --- a/util/vfio-helpers.c
> >> +++ b/util/vfio-helpers.c
> >> @@ -376,8 +376,13 @@ static void 
> >> qemu_vfio_ram_block_added(RAMBlockNotifier *n,
> >>                                        void *host, size_t size)
> >>  {
> >>      QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier);
> >> +    int ret;
> >> +
> >>      trace_qemu_vfio_ram_block_added(s, host, size);
> >> -    qemu_vfio_dma_map(s, host, size, false, NULL);
> >> +    ret = qemu_vfio_dma_map(s, host, size, false, NULL);
> >> +    if (ret) {
> >> +        error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, 
> >> ret);
> >> +    }
> > 
> > Irrelevant change (another patch)?
> 
> This is the error that was printed in qemu_vfio_init_ramblock(). Not
> moving it in this patch would mean we would stop printing the error.
> [...]
> 
> >> -
> >>  static void qemu_vfio_open_common(QEMUVFIOState *s)
> >>  {
> >>      qemu_mutex_init(&s->lock);
> >>      s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added;
> >>      s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed;
> >> -    ram_block_notifier_add(&s->ram_notifier);
> >>      s->low_water_mark = QEMU_VFIO_IOVA_MIN;
> >>      s->high_water_mark = QEMU_VFIO_IOVA_MAX;
> >> -    qemu_ram_foreach_block(qemu_vfio_init_ramblock, s);
> >> +    ram_block_notifier_add(&s->ram_notifier);
> > 
> > Pure question: this looks like a good improvement, however do you know
> > why HAX and SEV do not need to init ramblock?
> 
> They register very early (e.g., at accel init time), before any ram
> blocks are added.

That's what I thought but I did feel like it's tricky (not anything
about this patch, though).  Say, I don't see how that's guaranteed
that accel init will always happen before creating any ramblocks.

Anyway, your patch looks good from that point of view. :)

Thanks,

-- 
Peter Xu