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?
> +
> + 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?
signature.asc
Description: PGP signature
