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

[David Hildenbrand]

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.

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.

Thanks for your review!

-- 
Thanks,

David / dhildenb