Re: [PATCH RESEND v6 22/36] multi-process: Synchronize remote memory

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:57PM -0700, address@hidden 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