* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> On Wed, Apr 22, 2020 at 09:13:57PM -0700, elena.ufimts...@oracle.com wrote:
> > diff --git a/hw/proxy/memory-sync.c b/hw/proxy/memory-sync.c
> > new file mode 100644
> > index 0000000000..b3f57747f3
> > --- /dev/null
> > +++ b/hw/proxy/memory-sync.c
> > @@ -0,0 +1,217 @@
> > +/*
> > + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <stdio.h>
> > +#include <string.h>
>
> These headers should already be included by "qemu/osdep.h".
>
> > +static void proxy_ml_region_addnop(MemoryListener *listener,
> > + MemoryRegionSection *section)
> > +{
> > + RemoteMemSync *sync = container_of(listener, RemoteMemSync, listener);
> > + bool need_add = true;
> > + uint64_t mrs_size, mrs_gpa, mrs_page;
> > + uintptr_t mrs_host;
> > + RAMBlock *mrs_rb;
> > + MemoryRegionSection *prev_sec;
> > +
> > + if (!(memory_region_is_ram(section->mr) &&
> > + !memory_region_is_rom(section->mr))) {
> > + return;
> > + }
> > +
> > + mrs_rb = section->mr->ram_block;
> > + mrs_page = (uint64_t)qemu_ram_pagesize(mrs_rb);
> > + mrs_size = int128_get64(section->size);
> > + mrs_gpa = section->offset_within_address_space;
> > + mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
> > + section->offset_within_region;
>
> These variables are only used in the if (sync->n_mr_sections) case. This
> function could be split into a something like this:
>
> static void proxy_ml_region_addnop(MemoryListener *listener,
> MemoryRegionSection *section)
> RemoteMemSync *sync = container_of(listener, RemoteMemSync, listener);
>
> if (!(memory_region_is_ram(section->mr) &&
> !memory_region_is_rom(section->mr))) {
> return;
> }
>
> if (try_merge(sync, section)) {
> return;
> }
>
> ...add new section...
> }
>
> And the try_merge() helper function has the rest of the code:
>
> /* Returns true if the section was merged */
> static bool try_merge(RemoteMemSync *sync, MemoryRegionSection *section)
> {
> if (sync->n_mr_sections == 0) {
> return false;
> }
>
> ...most of the code...
> }
>
> > +
> > + if (get_fd_from_hostaddr(mrs_host, NULL) <= 0) {
>
> 0 is a valid fd number, the comparison should probably be < 0?
>
> > + return;
> > + }
> > +
> > + mrs_host = mrs_host & ~(mrs_page - 1);
> > + mrs_gpa = mrs_gpa & ~(mrs_page - 1);
> > + mrs_size = ROUND_UP(mrs_size, mrs_page);
>
> Why is it necessary to align to the RAM block's page size?
>
> Can mrs_host and mrs_size be misaligned to the RAM block's page size?
>
> Why round the *guest* physical address down using the *host* page size?
That sounds like the type of magic we do for postcopy; where we can only
'place' pages atomically on a host page boundary.
Dave
> > +
> > + if (sync->n_mr_sections) {
> > + prev_sec = sync->mr_sections + (sync->n_mr_sections - 1);
> > + uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
> > + uint64_t prev_size = int128_get64(prev_sec->size);
> > + uint64_t prev_gpa_end = range_get_last(prev_gpa_start,
> > prev_size);
> > + uint64_t prev_host_start =
> > + (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
> > + prev_sec->offset_within_region;
> > + uint64_t prev_host_end = range_get_last(prev_host_start,
> > prev_size);
>
> Is it okay not to do the page alignment stuff for the previous
> MemoryRegionSection?
>
> > +void deconfigure_memory_sync(RemoteMemSync *sync)
> > +{
> > + memory_listener_unregister(&sync->listener);
> > +}
>
> This function is unused? It must be tied into the mpqemu_link lifecycle.
> It must be possible to hot plug/unplug proxy PCI devices without memory
> leaks or use-after-frees.
>
> > diff --git a/include/hw/proxy/memory-sync.h b/include/hw/proxy/memory-sync.h
> > new file mode 100644
> > index 0000000000..d8329c9b52
> > --- /dev/null
> > +++ b/include/hw/proxy/memory-sync.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Copyright © 2018, 2020 Oracle and/or its affiliates.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef MEMORY_SYNC_H
> > +#define MEMORY_SYNC_H
> > +
> > +#include <sys/types.h>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qom/object.h"
> > +#include "exec/memory.h"
> > +#include "io/mpqemu-link.h"
> > +
> > +#define TYPE_MEMORY_LISTENER "memory-listener"
>
> This name is too generic. There is already a C struct called
> MemoryListener. Please call this class "remote-memory-sync".
>
> I'm not sure if a QOM object is needed here. Can this just be a plain C
> struct? If you're not using QOM object-orientated features then there is
> no need to define a QOM object.
>
> > @@ -39,8 +40,13 @@ typedef struct ProxyMemoryRegion {
> > struct PCIProxyDev {
> > PCIDevice parent_dev;
> >
> > + int n_mr_sections;
> > + MemoryRegionSection *mr_sections;
>
> Is it necessary to duplicate these fields here since a RemoteMemSync
> field is also being added and it contains these same fields?
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
