Re: [Qemu-devel] [PATCH v4 03/11] dataplane: add host memory mapping code

[Michael S. Tsirkin]

On Thu, Nov 29, 2012 at 03:26:56PM +0100, Stefan Hajnoczi wrote:
> On Thu, Nov 29, 2012 at 03:54:25PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 22, 2012 at 04:16:44PM +0100, Stefan Hajnoczi wrote:
> > > The data plane thread needs to map guest physical addresses to host
> > > pointers.  Normally this is done with cpu_physical_memory_map() but the
> > > function assumes the global mutex is held.  The data plane thread does
> > > not touch the global mutex and therefore needs a thread-safe memory
> > > mapping mechanism.
> > > 
> > > Hostmem registers a MemoryListener similar to how vhost collects and
> > > pushes memory region information into the kernel.  There is a
> > > fine-grained lock on the regions list which is held during lookup and
> > > when installing a new regions list.
> > > 
> > > When the physical memory map changes the MemoryListener callbacks are
> > > invoked.  They build up a new list of memory regions which is finally
> > > installed when the list has been completed.
> > > 
> > > Note that this approach is not safe across memory hotplug because mapped
> > > pointers may still be in used across memory unplug.  However, this is
> > > currently a problem for QEMU in general and needs to be addressed in the
> > > future.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > 
> > Worth bothering with binary search?
> > vhost does a linear search over regions because
> > the number of ram regions is very small.
> 
> memory.c does binary search.  I did the same but in practice there are
> <20 regions for a simple VM.  It's probably not worth it but without
> performance results this is speculation.
> 
> I think there's no harm in using binary search to start with.
> 
> > > +static void hostmem_listener_append_region(MemoryListener *listener,
> > > +                                           MemoryRegionSection *section)
> > > +{
> > > +    Hostmem *hostmem = container_of(listener, Hostmem, listener);
> > > +
> > > +    if (memory_region_is_ram(section->mr)) {
> > > +        hostmem_append_new_region(hostmem, section);
> > > +    }
> > 
> > I think you also need to remove VGA region since you
> > don't mark these pages as dirty so access there won't work.
> 
> I don't understand.  If memory in the VGA region returns true from
> memory_region_is_ram(), why would there be a problem?

If you change this memory but you don't update the display.
Never happens with non buggy guests but we should catch and fail if it does.

-- 
MST