Re: [Qemu-devel] [PATCH] vl: Delay initialization of memory backends

[Bandan Das <bsd@redhat.com>]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Sep 01, 2016 at 02:39:37PM -0300, Eduardo Habkost wrote:
>> On Thu, Sep 01, 2016 at 05:41:52PM +0200, Paolo Bonzini wrote:
>> > On 01/09/2016 17:10, Eduardo Habkost wrote:
>> > > Ouch. It looks like the ordering requirements are messier than I
>> > > thought. vhost-user depends on the memory backends to be already
>> > > initialized.
>> > 
>> > You could also look at delaying initialization of vhost-user, not
>> > sending anything on the wire until after machine creation.
>> 
>> I was wishing the bug could be fixed without the need to touch
>> vhost, but I will take a look.
>> 
>> BTW, the vhost error is actually happening inside a VCPU thread,
>> after everything was supposed to be fully initialized.  Maybe the
>> memory listener logic in vhost.c is broken somehow?
>
> This is getting hairier.

Just started looking... I understand that chardev init can't be moved
up because it might depend on object init but why can't we delay
vhost-user initialization ?

> Summary:
>
> 1) vhost_user_set_mem_table() fails because dev->mem->nregions is 0
> 2) dev->mem->nregions is supposed to get new entries based on the
>    memory listener callbacks
> 3) vhost_region_add() gets called properly, and calls
>    vhost_set_memory(), but:
> 4) vhost_set_memory() forces add=false if
>    memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION)
>    (I have no idea why)
> 5) memory_region_init_ram_from_file() sets:
>    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;

The commit message that added it says that DIRTY_MEMORY_CODE is only used
with TCG. However, the above check is saying that as long as any bit
except DIRTY_MEMORY_MIGRATION is set, don't add the region (because that is
handled else where). Should this apply to DIRTY_MEMORY_CODE too ?
This check is at some other places too but it seems none of them can have
DIRTY_MEMORY_CODE set since they will always be called in non-tcg context.

>    (I don't understand what are the consequences of this)
> 6) The tcg_enabled() check above is broken if the memory region
>    is created before configure_accelerator() is called. My patch
>    moves memory backend initialization after
>    configure_accelerator()

Indeed, this seems broken and will always add the region.
I am not sure, however, whether DIRTY_MEMORY_CODE is something
that needs to be tracked.

> I'm very confused. My patch seems to fix the dirty_log_mask
> initialization at (5) by accident? But for some reason this
> breaks vhost-user and makes it ignore all memory regions if using
> TCG? (vhost-user-test forces accel=tcg, BTW)

Right, because this bit will be set for all memory regions in the
tcg case. From what I understand, either this shouldn't be set for
all memory regions universally or vhost can simply ignore checking
for this bit in the mask ?

bandan

> As I have no idea why vhost_set_memory() ignores the memory
> region based on memory_region_get_dirty_log_mask(), I don't know
> what's supposed to be happening here. Any help would be
> appreciated.