[Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[David Gibson]

I posted some RFCs for this back in December, but didn't wrap it up in
time for 3.1.  Posting again for inclusion in 4.0.

The virtio-balloon devices was never really thought out for cases
other than 4kiB pagesize on both guest and host.  It works in some
cases, but in others can be ineffectual or even cause guest memory
corruption.

This series makes a handful of preliminary cleanups, then makes a
change to safely, though not perfectly, handle cases with non 4kiB
pagesizes.

Changes since RFC:
 * Further refinement of when to issue warnings in 5/5

David Gibson (5):
  virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
  virtio-balloon: Corrections to address verification
  virtio-balloon: Rework ballon_page() interface
  virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
  virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

 hw/virtio/virtio-balloon.c         | 102 ++++++++++++++++++++++++-----
 include/hw/virtio/virtio-balloon.h |   3 +
 2 files changed, 89 insertions(+), 16 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

When the balloon is inflated, we discard memory place in it using madvise()
with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
sounds like it makes sense but is actually unnecessary.

The misleadingly named MADV_DONTNEED just discards the memory in question,
it doesn't set any persistent state on it in-kernel; all that's necessary
to bring the memory back is to touch it.  MADV_WILLNEED in contrast
specifically says that the memory will be used soon and faults it in.

This patch simplify's the balloon operation by dropping the madvise()
on deflate.  This might have an impact on performance - it will move a
delay at deflate time until that memory is actually touched, which
might be more latency sensitive.  However:

  * Memory that's being given back to the guest by deflating the
    balloon *might* be used soon, but it equally could just sit around
    in the guest's pools until needed (or even be faulted out again if
    the host is under memory pressure).

  * Usually, the timescale over which you'll be adjusting the balloon
    is long enough that a few extra faults after deflation aren't
    going to make a difference.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a12677d4d5..43af521884 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -35,9 +35,8 @@
 
 static void balloon_page(void *addr, int deflate)
 {
-    if (!qemu_balloon_is_inhibited()) {
-        qemu_madvise(addr, BALLOON_PAGE_SIZE,
-                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
+    if (!qemu_balloon_is_inhibited() && !deflate) {
+        qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
     }
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH 2/5] virtio-balloon: Corrections to address verification

[David Gibson]

The virtio-balloon device's verification of the address given to it by the
guest has a number of faults:
    * The addresses here are guest physical addresses, which should be
      'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
      pretty subtle and confusing)
    * We don't check for section.mr being NULL, which is the main way that
      memory_region_find() reports basic failures.  We really need to check
      that before looking at any other section fields, because
      memory_region_find() doesn't initialize them on the failure path
    * We're passing a length of '1' to memory_region_find(), but really the
      guest is requesting that we put the entire page into the balloon,
      so it makes more sense to call it with BALLOON_PAGE_SIZE

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 43af521884..eb357824d8 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         }
 
         while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
-            ram_addr_t pa;
-            ram_addr_t addr;
+            hwaddr pa;
+            hwaddr addr;
             int p = virtio_ldl_p(vdev, &pfn);
 
-            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
+            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
             offset += 4;
 
-            /* FIXME: remove get_system_memory(), but how? */
-            section = memory_region_find(get_system_memory(), pa, 1);
-            if (!int128_nz(section.size) ||
-                !memory_region_is_ram(section.mr) ||
+            section = memory_region_find(get_system_memory(), pa,
+                                         BALLOON_PAGE_SIZE);
+            if (!section.mr) {
+                trace_virtio_balloon_bad_addr(pa);
+                continue;
+            }
+            if (!memory_region_is_ram(section.mr) ||
                 memory_region_is_rom(section.mr) ||
                 memory_region_is_romd(section.mr)) {
                 trace_virtio_balloon_bad_addr(pa);
-- 
2.20.1

[Qemu-devel] [PATCH 3/5] virtio-balloon: Rework ballon_page() interface

[David Gibson]

This replaces the balloon_page() internal interface with
ballon_inflate_page(), with a slightly different interface.  The new
interface will make future alterations simpler.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index eb357824d8..bf93148486 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -33,11 +33,12 @@
 
 #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
 
-static void balloon_page(void *addr, int deflate)
+static void balloon_inflate_page(VirtIOBalloon *balloon,
+                                 MemoryRegion *mr, hwaddr offset)
 {
-    if (!qemu_balloon_is_inhibited() && !deflate) {
-        qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
-    }
+    void *addr = memory_region_get_ram_ptr(mr) + offset;
+
+    qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
 }
 
 static const char *balloon_stat_names[] = {
@@ -222,7 +223,6 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
         while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
             hwaddr pa;
-            hwaddr addr;
             int p = virtio_ldl_p(vdev, &pfn);
 
             pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
@@ -244,11 +244,9 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
             trace_virtio_balloon_handle_output(memory_region_name(section.mr),
                                                pa);
-            /* Using memory_region_get_ram_ptr is bending the rules a bit, but
-               should be OK because we only want a single page.  */
-            addr = section.offset_within_region;
-            balloon_page(memory_region_get_ram_ptr(section.mr) + addr,
-                         !!(vq == s->dvq));
+            if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
+                balloon_inflate_page(s, section.mr, section.offset_within_region);
+            }
             memory_region_unref(section.mr);
         }
 
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] virtio-balloon: Use ram_block_discard_range() instead of raw madvise()

[David Gibson]

Currently, virtio-balloon uses madvise() with MADV_DONTNEED to actually
discard RAM pages inserted into the balloon.  This is basically a Linux
only interface (MADV_DONTNEED exists on some other platforms, but doesn't
always have the same semantics).  It also doesn't work on hugepages and has
some other limitations.

It turns out that postcopy also needs to discard chunks of memory, and uses
a better interface for it: ram_block_discard_range().  It doesn't cover
every case, but it covers more than going direct to madvise() and this
gives us a single place to update for more possibilities in future.

There are some subtleties here to maintain the current balloon behaviour:

* For now, we just ignore requests to balloon in a hugepage backed region.
  That matches current behaviour, because MADV_DONTNEED on a hugepage would
  simply fail, and we ignore the error.

* If host page size is > BALLOON_PAGE_SIZE we can frequently call this on
  non-host-page-aligned addresses.  These would also fail in madvise(),
  which we then ignored.  ram_block_discard_range() error_report()s calls
  on unaligned addresses, so we explicitly check that case to avoid
  spamming the logs.

* We now call ram_block_discard_range() with the *host* page size, whereas
  we previously called madvise() with BALLOON_PAGE_SIZE.  Surprisingly,
  this also matches existing behaviour.  Although the kernel fails madvise
  on unaligned addresses, it will round unaligned sizes *up* to the host
  page size.  Yes, this means that if BALLOON_PAGE_SIZE < guest page size
  we can incorrectly discard more memory than the guest asked us to.  I'm
  planning to address that soon.

Errors other than the ones discussed above, will now be reported by
ram_block_discard_range(), rather than silently ignored, which means we
have a much better chance of seeing when something is going wrong.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index bf93148486..e4cd8d566b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -37,8 +37,29 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
                                  MemoryRegion *mr, hwaddr offset)
 {
     void *addr = memory_region_get_ram_ptr(mr) + offset;
+    RAMBlock *rb;
+    size_t rb_page_size;
+    ram_addr_t ram_offset;
 
-    qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
+    /* XXX is there a better way to get to the RAMBlock than via a
+     * host address? */
+    rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+    rb_page_size = qemu_ram_pagesize(rb);
+
+    /* Silently ignore hugepage RAM blocks */
+    if (rb_page_size != getpagesize()) {
+        return;
+    }
+
+    /* Silently ignore unaligned requests */
+    if (ram_offset & (rb_page_size - 1)) {
+        return;
+    }
+
+    ram_block_discard_range(rb, ram_offset, rb_page_size);
+    /* We ignore errors from ram_block_discard_range(), because it has
+     * already reported them, and failing to discard a balloon page is
+     * not fatal */
 }
 
 static const char *balloon_stat_names[] = {
-- 
2.20.1

[Qemu-devel] [PATCH 5/5] virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

[David Gibson]

The virtio-balloon always works in units of 4kiB (BALLOON_PAGE_SIZE), but
we can only actually discard memory in units of the host page size.

Now, we handle this very badly: we silently ignore balloon requests that
aren't host page aligned, and for requests that are host page aligned we
discard the entire host page.  The latter can corrupt guest memory if its
page size is smaller than the host's.

The obvious choice would be to disable the balloon if the host page size is
not 4kiB.  However, that would break the special case where host and guest
have the same page size, but that's larger than 4kiB.  That case currently
works by accident[1] - and is used in practice on many production POWER
systems where 64kiB has long been the Linux default page size on both host
and guest.

To make the balloon safe, without breaking that useful special case, we
need to accumulate 4kiB balloon requests until we have a whole contiguous
host page to discard.

We could in principle do that across all guest memory, but it would require
a large bitmap to track.  This patch represents a compromise: we track
ballooned subpages for a single contiguous host page at a time.  This means
that if the guest discards all 4kiB chunks of a host page in succession,
we will discard it.  This is the expected behaviour in the (host page) ==
(guest page) != 4kiB case we want to support.

If the guest scatters 4kiB requests across different host pages, we don't
discard anything, and issue a warning.  Not ideal, but at least we don't
corrupt guest memory as the previous version could.

Warning reporting is kind of a compromise here.  Determining whether we're
in a problematic state at realize() time is tricky, because we'd have to
look at the host pagesizes of all memory backends, but we can't really know
if some of those backends could be for special purpose memory that's not
subject to ballooning.

Reporting only when the guest tries to balloon a partial page also isn't
great because if the guest page size happens to line up it won't indicate
that we're in a non ideal situation.  It could also cause alarming repeated
warnings whenever a migration is attempted.

So, what we do is warn the first time the guest attempts balloon a partial
host page, whether or not it will end up ballooning the rest of the page
immediately afterwards.

[1] Because when the guest attempts to balloon a page, it will submit
    requests for each 4kiB subpage.  Most will be ignored, but the one
    which happens to be host page aligned will discard the whole lot.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio/virtio-balloon.c         | 69 +++++++++++++++++++++++++-----
 include/hw/virtio/virtio-balloon.h |  3 ++
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index e4cd8d566b..65f861cbef 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -33,33 +33,82 @@
 
 #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
 
+typedef struct PartiallyBalloonedPage {
+    RAMBlock *rb;
+    ram_addr_t base;
+    unsigned long bitmap[];
+} PartiallyBalloonedPage;
+
 static void balloon_inflate_page(VirtIOBalloon *balloon,
                                  MemoryRegion *mr, hwaddr offset)
 {
     void *addr = memory_region_get_ram_ptr(mr) + offset;
     RAMBlock *rb;
     size_t rb_page_size;
-    ram_addr_t ram_offset;
+    int subpages;
+    ram_addr_t ram_offset, host_page_base;
 
     /* XXX is there a better way to get to the RAMBlock than via a
      * host address? */
     rb = qemu_ram_block_from_host(addr, false, &ram_offset);
     rb_page_size = qemu_ram_pagesize(rb);
+    host_page_base = ram_offset & ~(rb_page_size - 1);
+
+    if (rb_page_size == BALLOON_PAGE_SIZE) {
+        /* Easy case */
 
-    /* Silently ignore hugepage RAM blocks */
-    if (rb_page_size != getpagesize()) {
+        ram_block_discard_range(rb, ram_offset, rb_page_size);
+        /* We ignore errors from ram_block_discard_range(), because it
+         * has already reported them, and failing to discard a balloon
+         * page is not fatal */
         return;
     }
 
-    /* Silently ignore unaligned requests */
-    if (ram_offset & (rb_page_size - 1)) {
-        return;
+    /* Hard case
+     *
+     * We've put a piece of a larger host page into the balloon - we
+     * need to keep track until we have a whole host page to
+     * discard
+     */
+    warn_report_once(
+"Balloon used with backing page size > 4kiB, this may not be reliable");
+
+    subpages = rb_page_size / BALLOON_PAGE_SIZE;
+
+    if (balloon->pbp
+        && (rb != balloon->pbp->rb
+            || host_page_base != balloon->pbp->base)) {
+        /* We've partially ballooned part of a host page, but now
+         * we're trying to balloon part of a different one.  Too hard,
+         * give up on the old partial page */
+        free(balloon->pbp);
+        balloon->pbp = NULL;
     }
 
-    ram_block_discard_range(rb, ram_offset, rb_page_size);
-    /* We ignore errors from ram_block_discard_range(), because it has
-     * already reported them, and failing to discard a balloon page is
-     * not fatal */
+    if (!balloon->pbp) {
+        /* Starting on a new host page */
+        size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
+        balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);
+        balloon->pbp->rb = rb;
+        balloon->pbp->base = host_page_base;
+    }
+
+    bitmap_set(balloon->pbp->bitmap,
+               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+               subpages);
+
+    if (bitmap_full(balloon->pbp->bitmap, subpages)) {
+        /* We've accumulated a full host page, we can actually discard
+         * it now */
+
+        ram_block_discard_range(rb, balloon->pbp->base, rb_page_size);
+        /* We ignore errors from ram_block_discard_range(), because it
+         * has already reported them, and failing to discard a balloon
+         * page is not fatal */
+
+        free(balloon->pbp);
+        balloon->pbp = NULL;
+    }
 }
 
 static const char *balloon_stat_names[] = {
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index e0df3528c8..99dcd6d105 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -30,6 +30,8 @@ typedef struct virtio_balloon_stat_modern {
        uint64_t val;
 } VirtIOBalloonStatModern;
 
+typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
+
 typedef struct VirtIOBalloon {
     VirtIODevice parent_obj;
     VirtQueue *ivq, *dvq, *svq;
@@ -42,6 +44,7 @@ typedef struct VirtIOBalloon {
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
+    PartiallyBalloonedPage *pbp;
 } VirtIOBalloon;
 
 #endif
-- 
2.20.1

[Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Michael S. Tsirkin]

The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:

  Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 1f8c04f18d2ee2f6ec88217dfd547ab38d2be5c5:

  pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)

----------------------------------------------------------------
pci, pc, virtio: fixes, cleanups, tests

Lots of work on tests: BiosTablesTest UEFI app,
vhost-user testing for non-Linux hosts.
Misc cleanups and fixes all over the place

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Alex Williamson (1):
      pci: Sanity test minimum downstream LNKSTA

Alexey Kardashevskiy (1):
      pci: Move NVIDIA vendor id to the rest of ids

Changpeng Liu (1):
      contrib/vhost-user-blk: fix the compilation issue

Daniel P. Berrangé (1):
      hw/smbios: fix offset of type 3 sku field

David Gibson (5):
      virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
      virtio-balloon: Corrections to address verification
      virtio-balloon: Rework ballon_page() interface
      virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
      virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

Laszlo Ersek (5):
      roms: add the edk2 project as a git submodule
      roms: build the EfiRom utility from the roms/edk2 submodule
      tests: introduce "uefi-test-tools" with the BiosTablesTest UEFI app
      tests/uefi-test-tools: add build scripts
      tests/data: introduce "uefi-boot-images" with the "bios-tables-test" ISOs

Paolo Bonzini (9):
      vhost-net: move stubs to a separate file
      vhost-net-user: add stubs for when no virtio-net device is present
      vhost: restrict Linux dependency to kernel vhost
      vhost-user: support cross-endian vnet headers
      vhost-net: compile it on all targets that have virtio-net.
      vhost-net: revamp configure logic
      vhost-user-test: create a main loop per TestServer
      vhost-user-test: small changes to init_hugepagefs
      vhost-user-test: create a temporary directory per TestServer

Peter Xu (1):
      i386/kvm: ignore masked irqs when update msi routes

Philippe Mathieu-Daudé (1):
      Revert "contrib/vhost-user-blk: fix the compilation issue"

Wei Yang (1):
      pc-dimm: use same mechanism for [get|set]_addr

 configure                                          | 102 ++++++++-----
 Makefile                                           |   6 +-
 default-configs/virtio.mak                         |   4 +-
 include/exec/poison.h                              |   1 -
 include/hw/firmware/smbios.h                       |   1 +
 include/hw/pci/pci_ids.h                           |   2 +
 include/hw/virtio/virtio-balloon.h                 |   3 +
 .../UefiTestToolsPkg/Include/Guid/BiosTablesTest.h |  67 +++++++++
 hw/mem/pc-dimm.c                                   |   4 +-
 hw/net/vhost_net-stub.c                            |  92 ++++++++++++
 hw/net/vhost_net.c                                 |  85 +----------
 hw/pci/pcie.c                                      |  13 +-
 hw/smbios/smbios.c                                 |   1 +
 hw/vfio/pci-quirks.c                               |   2 -
 hw/virtio/vhost-backend.c                          |  12 +-
 hw/virtio/vhost-user.c                             |  13 +-
 hw/virtio/vhost.c                                  |   2 +-
 hw/virtio/virtio-balloon.c                         | 102 ++++++++++---
 net/net.c                                          |   2 +-
 net/vhost-user-stub.c                              |  23 +++
 net/vhost-user.c                                   |  13 ++
 .../BiosTablesTest/BiosTablesTest.c                | 130 +++++++++++++++++
 tests/vhost-user-test.c                            | 160 +++++++++++----------
 .gitmodules                                        |   3 +
 backends/Makefile.objs                             |   5 +-
 hw/net/Makefile.objs                               |   4 +-
 hw/virtio/Makefile.objs                            |   8 +-
 net/Makefile.objs                                  |   4 +-
 roms/Makefile                                      |  13 +-
 roms/edk2                                          |   1 +
 tests/Makefile.include                             |   5 +-
 .../bios-tables-test.aarch64.iso.qcow2             | Bin 0 -> 11776 bytes
 .../bios-tables-test.arm.iso.qcow2                 | Bin 0 -> 11776 bytes
 .../bios-tables-test.i386.iso.qcow2                | Bin 0 -> 12800 bytes
 .../bios-tables-test.x86_64.iso.qcow2              | Bin 0 -> 13312 bytes
 tests/uefi-test-tools/.gitignore                   |   3 +
 tests/uefi-test-tools/LICENSE                      |  25 ++++
 tests/uefi-test-tools/Makefile                     | 106 ++++++++++++++
 .../BiosTablesTest/BiosTablesTest.inf              |  41 ++++++
 .../UefiTestToolsPkg/UefiTestToolsPkg.dec          |  27 ++++
 .../UefiTestToolsPkg/UefiTestToolsPkg.dsc          |  69 +++++++++
 tests/uefi-test-tools/build.sh                     | 145 +++++++++++++++++++
 42 files changed, 1051 insertions(+), 248 deletions(-)
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h
 create mode 100644 hw/net/vhost_net-stub.c
 create mode 100644 net/vhost-user-stub.c
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
 create mode 160000 roms/edk2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.arm.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.i386.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.x86_64.iso.qcow2
 create mode 100644 tests/uefi-test-tools/.gitignore
 create mode 100644 tests/uefi-test-tools/LICENSE
 create mode 100644 tests/uefi-test-tools/Makefile
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.inf
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dec
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dsc
 create mode 100755 tests/uefi-test-tools/build.sh

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification

[Greg Kurz]

On Thu, 14 Feb 2019 15:39:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> The virtio-balloon device's verification of the address given to it by the
> guest has a number of faults:
>     * The addresses here are guest physical addresses, which should be
>       'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
>       pretty subtle and confusing)
>     * We don't check for section.mr being NULL, which is the main way that
>       memory_region_find() reports basic failures.  We really need to check
>       that before looking at any other section fields, because
>       memory_region_find() doesn't initialize them on the failure path
>     * We're passing a length of '1' to memory_region_find(), but really the
>       guest is requesting that we put the entire page into the balloon,
>       so it makes more sense to call it with BALLOON_PAGE_SIZE
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 43af521884..eb357824d8 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          }
>  
>          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> -            ram_addr_t pa;
> -            ram_addr_t addr;
> +            hwaddr pa;
> +            hwaddr addr;
>              int p = virtio_ldl_p(vdev, &pfn);
>  
> -            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> +            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
>              offset += 4;
>  
> -            /* FIXME: remove get_system_memory(), but how? */
> -            section = memory_region_find(get_system_memory(), pa, 1);
> -            if (!int128_nz(section.size) ||
> -                !memory_region_is_ram(section.mr) ||
> +            section = memory_region_find(get_system_memory(), pa,
> +                                         BALLOON_PAGE_SIZE);
> +            if (!section.mr) {
> +                trace_virtio_balloon_bad_addr(pa);
> +                continue;
> +            }

memory_region_unref(section.mr) with section.mr == NULL is safe and
resolves to a nop. Not sure you need a separate if to handle this
case.

Apart from that,

Reviewed-by: Greg Kurz <groug@kaod.org>

> +            if (!memory_region_is_ram(section.mr) ||
>                  memory_region_is_rom(section.mr) ||
>                  memory_region_is_romd(section.mr)) {
>                  trace_virtio_balloon_bad_addr(pa);

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Peter Maydell]

On Fri, 22 Feb 2019 at 02:40, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
>
>   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 1f8c04f18d2ee2f6ec88217dfd547ab38d2be5c5:
>
>   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
>
> ----------------------------------------------------------------
> pci, pc, virtio: fixes, cleanups, tests
>
> Lots of work on tests: BiosTablesTest UEFI app,
> vhost-user testing for non-Linux hosts.
> Misc cleanups and fixes all over the place
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------

Compile failure on clang:

/home/petmay01/linaro/qemu-for-merges/hw/virtio/virtio-balloon.c:40:3:
error: redefinition of typedef 'PartiallyBalloonedPage' is a C11
feature [-Werror,-Wtypedef-redefinition]
} PartiallyBalloonedPage;
  ^
/home/petmay01/linaro/qemu-for-merges/include/hw/virtio/virtio-balloon.h:33:39:
note: previous definition is here
typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
                                      ^
1 error generated.
/home/petmay01/linaro/qemu-for-merges/rules.mak:69: recipe for target
'hw/virtio/virtio-balloon.o' failed

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Michael S. Tsirkin]

On Fri, Feb 22, 2019 at 03:47:36PM +0000, Peter Maydell wrote:
> On Fri, 22 Feb 2019 at 02:40, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
> >
> >   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 1f8c04f18d2ee2f6ec88217dfd547ab38d2be5c5:
> >
> >   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
> >
> > ----------------------------------------------------------------
> > pci, pc, virtio: fixes, cleanups, tests
> >
> > Lots of work on tests: BiosTablesTest UEFI app,
> > vhost-user testing for non-Linux hosts.
> > Misc cleanups and fixes all over the place
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> 
> Compile failure on clang:
> 
> /home/petmay01/linaro/qemu-for-merges/hw/virtio/virtio-balloon.c:40:3:
> error: redefinition of typedef 'PartiallyBalloonedPage' is a C11
> feature [-Werror,-Wtypedef-redefinition]
> } PartiallyBalloonedPage;
>   ^
> /home/petmay01/linaro/qemu-for-merges/include/hw/virtio/virtio-balloon.h:33:39:
> note: previous definition is here
> typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
>                                       ^
> 1 error generated.
> /home/petmay01/linaro/qemu-for-merges/rules.mak:69: recipe for target
> 'hw/virtio/virtio-balloon.o' failed
> 
> thanks
> -- PMM

Fixed up and re-pushed.
David, pls note above and don't add duplicate typedefs in the future.
There's always include/qemu/typedefs.h if you don't know where
to put a typedef.

-- 
MST

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Peter Maydell]

On Fri, 22 Feb 2019 at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Feb 22, 2019 at 03:47:36PM +0000, Peter Maydell wrote:
> > Compile failure on clang:
> >
> > /home/petmay01/linaro/qemu-for-merges/hw/virtio/virtio-balloon.c:40:3:
> > error: redefinition of typedef 'PartiallyBalloonedPage' is a C11
> > feature [-Werror,-Wtypedef-redefinition]
> > } PartiallyBalloonedPage;
> >   ^
> > /home/petmay01/linaro/qemu-for-merges/include/hw/virtio/virtio-balloon.h:33:39:
> > note: previous definition is here
> > typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
> >                                       ^
> > 1 error generated.
> > /home/petmay01/linaro/qemu-for-merges/rules.mak:69: recipe for target
> > 'hw/virtio/virtio-balloon.o' failed

> Fixed up and re-pushed.
> David, pls note above and don't add duplicate typedefs in the future.
> There's always include/qemu/typedefs.h if you don't know where
> to put a typedef.

It's an easy mistake to make, and it's only clang that complains -- I
did it myself the other week :-)

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Michael S. Tsirkin]

On Fri, Feb 22, 2019 at 10:53:54AM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 22, 2019 at 03:47:36PM +0000, Peter Maydell wrote:
> > On Fri, 22 Feb 2019 at 02:40, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
> > >
> > >   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > >
> > > for you to fetch changes up to 1f8c04f18d2ee2f6ec88217dfd547ab38d2be5c5:
> > >
> > >   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
> > >
> > > ----------------------------------------------------------------
> > > pci, pc, virtio: fixes, cleanups, tests
> > >
> > > Lots of work on tests: BiosTablesTest UEFI app,
> > > vhost-user testing for non-Linux hosts.
> > > Misc cleanups and fixes all over the place
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > ----------------------------------------------------------------
> > 
> > Compile failure on clang:
> > 
> > /home/petmay01/linaro/qemu-for-merges/hw/virtio/virtio-balloon.c:40:3:
> > error: redefinition of typedef 'PartiallyBalloonedPage' is a C11
> > feature [-Werror,-Wtypedef-redefinition]
> > } PartiallyBalloonedPage;
> >   ^
> > /home/petmay01/linaro/qemu-for-merges/include/hw/virtio/virtio-balloon.h:33:39:
> > note: previous definition is here
> > typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
> >                                       ^
> > 1 error generated.
> > /home/petmay01/linaro/qemu-for-merges/rules.mak:69: recipe for target
> > 'hw/virtio/virtio-balloon.o' failed
> > 
> > thanks
> > -- PMM
> 
> Fixed up and re-pushed.

Peter, can you merge for_upstream now pls? Don't want to spam
the list with a trivial change like that ...

> David, pls note above and don't add duplicate typedefs in the future.
> There's always include/qemu/typedefs.h if you don't know where
> to put a typedef.
> 
> -- 
> MST

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Peter Maydell]

On Sun, 24 Feb 2019 at 00:34, Michael S. Tsirkin <mst@redhat.com> wrote:
> Peter, can you merge for_upstream now pls? Don't want to spam
> the list with a trivial change like that ...

Yes, it's on my list, but so are seven other pullreqs;
seems like everybody likes to submit on a Friday, so
sending on a Friday guarantees maximum delay because
you'll be in a big queue with other people and I don't
generally handle pullreqs on the weekend either...

In general I prefer it if you just re-send the cover-letter
email as a v2 rather than informally asking for a retry: that
guarantees I'll see it and automatically makes it appear
in my list of things to process. You don't need to
resend all the individual patchmails if the change was
minor.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Michael S. Tsirkin]

On Sun, Feb 24, 2019 at 10:21:52AM +0000, Peter Maydell wrote:
> On Sun, 24 Feb 2019 at 00:34, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Peter, can you merge for_upstream now pls? Don't want to spam
> > the list with a trivial change like that ...
> 
> Yes, it's on my list, but so are seven other pullreqs;
> seems like everybody likes to submit on a Friday, so
> sending on a Friday guarantees maximum delay because
> you'll be in a big queue with other people and I don't
> generally handle pullreqs on the weekend either...

OK I'll try to switch over to middle of the week.

> In general I prefer it if you just re-send the cover-letter
> email as a v2 rather than informally asking for a retry: that
> guarantees I'll see it and automatically makes it appear
> in my list of things to process. You don't need to
> resend all the individual patchmails if the change was
> minor.
> 
> thanks
> -- PMM

Good to know.

-- 
MST

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

On Fri, Feb 22, 2019 at 10:53:54AM -0500, Michael S. Tsirkin wrote:
> On Fri, Feb 22, 2019 at 03:47:36PM +0000, Peter Maydell wrote:
> > On Fri, 22 Feb 2019 at 02:40, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
> > >
> > >   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > >
> > > for you to fetch changes up to 1f8c04f18d2ee2f6ec88217dfd547ab38d2be5c5:
> > >
> > >   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
> > >
> > > ----------------------------------------------------------------
> > > pci, pc, virtio: fixes, cleanups, tests
> > >
> > > Lots of work on tests: BiosTablesTest UEFI app,
> > > vhost-user testing for non-Linux hosts.
> > > Misc cleanups and fixes all over the place
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > ----------------------------------------------------------------
> > 
> > Compile failure on clang:
> > 
> > /home/petmay01/linaro/qemu-for-merges/hw/virtio/virtio-balloon.c:40:3:
> > error: redefinition of typedef 'PartiallyBalloonedPage' is a C11
> > feature [-Werror,-Wtypedef-redefinition]
> > } PartiallyBalloonedPage;
> >   ^
> > /home/petmay01/linaro/qemu-for-merges/include/hw/virtio/virtio-balloon.h:33:39:
> > note: previous definition is here
> > typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
> >                                       ^
> > 1 error generated.
> > /home/petmay01/linaro/qemu-for-merges/rules.mak:69: recipe for target
> > 'hw/virtio/virtio-balloon.o' failed
> > 
> > thanks
> > -- PMM
> 
> Fixed up and re-pushed.
> David, pls note above and don't add duplicate typedefs in the future.
> There's always include/qemu/typedefs.h if you don't know where
> to put a typedef.

Yeah, sorry.  I noticed the failure on Travis and was going to send a
fix, not realizing you'd already picked it up.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]

On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote:
> On Thu, 14 Feb 2019 15:39:13 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > The virtio-balloon device's verification of the address given to it by the
> > guest has a number of faults:
> >     * The addresses here are guest physical addresses, which should be
> >       'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
> >       pretty subtle and confusing)
> >     * We don't check for section.mr being NULL, which is the main way that
> >       memory_region_find() reports basic failures.  We really need to check
> >       that before looking at any other section fields, because
> >       memory_region_find() doesn't initialize them on the failure path
> >     * We're passing a length of '1' to memory_region_find(), but really the
> >       guest is requesting that we put the entire page into the balloon,
> >       so it makes more sense to call it with BALLOON_PAGE_SIZE
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 43af521884..eb357824d8 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >          }
> >  
> >          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> > -            ram_addr_t pa;
> > -            ram_addr_t addr;
> > +            hwaddr pa;
> > +            hwaddr addr;
> >              int p = virtio_ldl_p(vdev, &pfn);
> >  
> > -            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> > +            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
> >              offset += 4;
> >  
> > -            /* FIXME: remove get_system_memory(), but how? */
> > -            section = memory_region_find(get_system_memory(), pa, 1);
> > -            if (!int128_nz(section.size) ||
> > -                !memory_region_is_ram(section.mr) ||
> > +            section = memory_region_find(get_system_memory(), pa,
> > +                                         BALLOON_PAGE_SIZE);
> > +            if (!section.mr) {
> > +                trace_virtio_balloon_bad_addr(pa);
> > +                continue;
> > +            }
> 
> memory_region_unref(section.mr) with section.mr == NULL is safe and
> resolves to a nop. Not sure you need a separate if to handle this
> case.

memory_region_is_ram() and friends are not, however - they will
dereference their argument unconditionally.

> 
> Apart from that,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +            if (!memory_region_is_ram(section.mr) ||
> >                  memory_region_is_rom(section.mr) ||
> >                  memory_region_is_romd(section.mr)) {
> >                  trace_virtio_balloon_bad_addr(pa);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3808 bytes --]

On Mon, 25 Feb 2019 10:37:11 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote:
> > On Thu, 14 Feb 2019 15:39:13 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > The virtio-balloon device's verification of the address given to it by the
> > > guest has a number of faults:
> > >     * The addresses here are guest physical addresses, which should be
> > >       'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
> > >       pretty subtle and confusing)
> > >     * We don't check for section.mr being NULL, which is the main way that
> > >       memory_region_find() reports basic failures.  We really need to check
> > >       that before looking at any other section fields, because
> > >       memory_region_find() doesn't initialize them on the failure path
> > >     * We're passing a length of '1' to memory_region_find(), but really the
> > >       guest is requesting that we put the entire page into the balloon,
> > >       so it makes more sense to call it with BALLOON_PAGE_SIZE
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/virtio/virtio-balloon.c | 17 ++++++++++-------
> > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 43af521884..eb357824d8 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >          }
> > >  
> > >          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> > > -            ram_addr_t pa;
> > > -            ram_addr_t addr;
> > > +            hwaddr pa;
> > > +            hwaddr addr;
> > >              int p = virtio_ldl_p(vdev, &pfn);
> > >  
> > > -            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> > > +            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
> > >              offset += 4;
> > >  
> > > -            /* FIXME: remove get_system_memory(), but how? */
> > > -            section = memory_region_find(get_system_memory(), pa, 1);
> > > -            if (!int128_nz(section.size) ||
> > > -                !memory_region_is_ram(section.mr) ||
> > > +            section = memory_region_find(get_system_memory(), pa,
> > > +                                         BALLOON_PAGE_SIZE);
> > > +            if (!section.mr) {
> > > +                trace_virtio_balloon_bad_addr(pa);
> > > +                continue;
> > > +            }  
> > 
> > memory_region_unref(section.mr) with section.mr == NULL is safe and
> > resolves to a nop. Not sure you need a separate if to handle this
> > case.  
> 
> memory_region_is_ram() and friends are not, however - they will
> dereference their argument unconditionally.
> 

Indeed but the two ifs can be merged anyway:

            if (!section.mr ||
                !memory_region_is_ram(section.mr) ||
                memory_region_is_rom(section.mr) ||
                memory_region_is_romd(section.mr)) {
                trace_virtio_balloon_bad_addr(pa);
                memory_region_unref(section.mr);
                continue;
            }

> > 
> > Apart from that,
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >   
> > > +            if (!memory_region_is_ram(section.mr) ||
> > >                  memory_region_is_rom(section.mr) ||
> > >                  memory_region_is_romd(section.mr)) {
> > >                  trace_virtio_balloon_bad_addr(pa);  
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PULL v2 resend 00/26] pci, pc, virtio: fixes, cleanups, tests

[Michael S. Tsirkin]

The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:

  Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 88c869198aa630e0477d653d0abf3f42c7c44d1f

  pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)

----------------------------------------------------------------

Note: this is same as a fixup I sent earlier, this is just a resend to
make sure it's not missed.  This should also help me figure out whether
this is a good format to use.

----------------------------------------------------------------
pci, pc, virtio: fixes, cleanups, tests

Lots of work on tests: BiosTablesTest UEFI app,
vhost-user testing for non-Linux hosts.
Misc cleanups and fixes all over the place

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Alex Williamson (1):
      pci: Sanity test minimum downstream LNKSTA

Alexey Kardashevskiy (1):
      pci: Move NVIDIA vendor id to the rest of ids

Changpeng Liu (1):
      contrib/vhost-user-blk: fix the compilation issue

Daniel P. Berrangé (1):
      hw/smbios: fix offset of type 3 sku field

David Gibson (5):
      virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
      virtio-balloon: Corrections to address verification
      virtio-balloon: Rework ballon_page() interface
      virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
      virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

Laszlo Ersek (5):
      roms: add the edk2 project as a git submodule
      roms: build the EfiRom utility from the roms/edk2 submodule
      tests: introduce "uefi-test-tools" with the BiosTablesTest UEFI app
      tests/uefi-test-tools: add build scripts
      tests/data: introduce "uefi-boot-images" with the "bios-tables-test" ISOs

Paolo Bonzini (9):
      vhost-net: move stubs to a separate file
      vhost-net-user: add stubs for when no virtio-net device is present
      vhost: restrict Linux dependency to kernel vhost
      vhost-user: support cross-endian vnet headers
      vhost-net: compile it on all targets that have virtio-net.
      vhost-net: revamp configure logic
      vhost-user-test: create a main loop per TestServer
      vhost-user-test: small changes to init_hugepagefs
      vhost-user-test: create a temporary directory per TestServer

Peter Xu (1):
      i386/kvm: ignore masked irqs when update msi routes

Philippe Mathieu-Daudé (1):
      Revert "contrib/vhost-user-blk: fix the compilation issue"

Wei Yang (1):
      pc-dimm: use same mechanism for [get|set]_addr

 configure                                          | 102 ++++++++-----
 Makefile                                           |   6 +-
 default-configs/virtio.mak                         |   4 +-
 include/exec/poison.h                              |   1 -
 include/hw/firmware/smbios.h                       |   1 +
 include/hw/pci/pci_ids.h                           |   2 +
 include/hw/virtio/virtio-balloon.h                 |   3 +
 .../UefiTestToolsPkg/Include/Guid/BiosTablesTest.h |  67 +++++++++
 hw/mem/pc-dimm.c                                   |   4 +-
 hw/net/vhost_net-stub.c                            |  92 ++++++++++++
 hw/net/vhost_net.c                                 |  85 +----------
 hw/pci/pcie.c                                      |  13 +-
 hw/smbios/smbios.c                                 |   1 +
 hw/vfio/pci-quirks.c                               |   2 -
 hw/virtio/vhost-backend.c                          |  12 +-
 hw/virtio/vhost-user.c                             |  13 +-
 hw/virtio/vhost.c                                  |   2 +-
 hw/virtio/virtio-balloon.c                         | 102 ++++++++++---
 net/net.c                                          |   2 +-
 net/vhost-user-stub.c                              |  23 +++
 net/vhost-user.c                                   |  13 ++
 .../BiosTablesTest/BiosTablesTest.c                | 130 +++++++++++++++++
 tests/vhost-user-test.c                            | 160 +++++++++++----------
 .gitmodules                                        |   3 +
 backends/Makefile.objs                             |   5 +-
 hw/net/Makefile.objs                               |   4 +-
 hw/virtio/Makefile.objs                            |   8 +-
 net/Makefile.objs                                  |   4 +-
 roms/Makefile                                      |  13 +-
 roms/edk2                                          |   1 +
 tests/Makefile.include                             |   5 +-
 .../bios-tables-test.aarch64.iso.qcow2             | Bin 0 -> 11776 bytes
 .../bios-tables-test.arm.iso.qcow2                 | Bin 0 -> 11776 bytes
 .../bios-tables-test.i386.iso.qcow2                | Bin 0 -> 12800 bytes
 .../bios-tables-test.x86_64.iso.qcow2              | Bin 0 -> 13312 bytes
 tests/uefi-test-tools/.gitignore                   |   3 +
 tests/uefi-test-tools/LICENSE                      |  25 ++++
 tests/uefi-test-tools/Makefile                     | 106 ++++++++++++++
 .../BiosTablesTest/BiosTablesTest.inf              |  41 ++++++
 .../UefiTestToolsPkg/UefiTestToolsPkg.dec          |  27 ++++
 .../UefiTestToolsPkg/UefiTestToolsPkg.dsc          |  69 +++++++++
 tests/uefi-test-tools/build.sh                     | 145 +++++++++++++++++++
 42 files changed, 1051 insertions(+), 248 deletions(-)
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h
 create mode 100644 hw/net/vhost_net-stub.c
 create mode 100644 net/vhost-user-stub.c
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
 create mode 160000 roms/edk2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.arm.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.i386.iso.qcow2
 create mode 100644 tests/data/uefi-boot-images/bios-tables-test.x86_64.iso.qcow2
 create mode 100644 tests/uefi-test-tools/.gitignore
 create mode 100644 tests/uefi-test-tools/LICENSE
 create mode 100644 tests/uefi-test-tools/Makefile
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.inf
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dec
 create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dsc
 create mode 100755 tests/uefi-test-tools/build.sh

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Philippe Mathieu-Daudé]

On 2/24/19 5:41 PM, Michael S. Tsirkin wrote:
> On Sun, Feb 24, 2019 at 10:21:52AM +0000, Peter Maydell wrote:
>> In general I prefer it if you just re-send the cover-letter
>> email as a v2 rather than informally asking for a retry: that
>> guarantees I'll see it and automatically makes it appear
>> in my list of things to process. You don't need to
>> resend all the individual patchmails if the change was
>> minor.

I appreciate what some maintainers do (such Paolo/Richard):
sending the cover + the patches changed, because we can review the
updated patch on the list and tag/remove the patch from the mail queue,
rather than after it was merged via the git tree (if smth was wrong, it
is too late).

>>
>> thanks
>> -- PMM
> 
> Good to know.
> 

Re: [Qemu-devel] [PULL 00/26] pci, pc, virtio: fixes, cleanups, tests

[Peter Maydell]

On Mon, 25 Feb 2019 at 16:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 2/24/19 5:41 PM, Michael S. Tsirkin wrote:
> > On Sun, Feb 24, 2019 at 10:21:52AM +0000, Peter Maydell wrote:
> >> In general I prefer it if you just re-send the cover-letter
> >> email as a v2 rather than informally asking for a retry: that
> >> guarantees I'll see it and automatically makes it appear
> >> in my list of things to process. You don't need to
> >> resend all the individual patchmails if the change was
> >> minor.
>
> I appreciate what some maintainers do (such Paolo/Richard):
> sending the cover + the patches changed, because we can review the
> updated patch on the list and tag/remove the patch from the mail queue,
> rather than after it was merged via the git tree (if smth was wrong, it
> is too late).

Yes, I meant more "you don't need to resend everything";
sending the 1 changed patch is usually a good idea.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4383 bytes --]

On Mon, Feb 25, 2019 at 10:26:51AM +0100, Greg Kurz wrote:
> On Mon, 25 Feb 2019 10:37:11 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote:
> > > On Thu, 14 Feb 2019 15:39:13 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > The virtio-balloon device's verification of the address given to it by the
> > > > guest has a number of faults:
> > > >     * The addresses here are guest physical addresses, which should be
> > > >       'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
> > > >       pretty subtle and confusing)
> > > >     * We don't check for section.mr being NULL, which is the main way that
> > > >       memory_region_find() reports basic failures.  We really need to check
> > > >       that before looking at any other section fields, because
> > > >       memory_region_find() doesn't initialize them on the failure path
> > > >     * We're passing a length of '1' to memory_region_find(), but really the
> > > >       guest is requesting that we put the entire page into the balloon,
> > > >       so it makes more sense to call it with BALLOON_PAGE_SIZE
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio-balloon.c | 17 ++++++++++-------
> > > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > > index 43af521884..eb357824d8 100644
> > > > --- a/hw/virtio/virtio-balloon.c
> > > > +++ b/hw/virtio/virtio-balloon.c
> > > > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > > >          }
> > > >  
> > > >          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> > > > -            ram_addr_t pa;
> > > > -            ram_addr_t addr;
> > > > +            hwaddr pa;
> > > > +            hwaddr addr;
> > > >              int p = virtio_ldl_p(vdev, &pfn);
> > > >  
> > > > -            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> > > > +            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
> > > >              offset += 4;
> > > >  
> > > > -            /* FIXME: remove get_system_memory(), but how? */
> > > > -            section = memory_region_find(get_system_memory(), pa, 1);
> > > > -            if (!int128_nz(section.size) ||
> > > > -                !memory_region_is_ram(section.mr) ||
> > > > +            section = memory_region_find(get_system_memory(), pa,
> > > > +                                         BALLOON_PAGE_SIZE);
> > > > +            if (!section.mr) {
> > > > +                trace_virtio_balloon_bad_addr(pa);
> > > > +                continue;
> > > > +            }  
> > > 
> > > memory_region_unref(section.mr) with section.mr == NULL is safe and
> > > resolves to a nop. Not sure you need a separate if to handle this
> > > case.  
> > 
> > memory_region_is_ram() and friends are not, however - they will
> > dereference their argument unconditionally.
> > 
> 
> Indeed but the two ifs can be merged anyway:
> 
>             if (!section.mr ||
>                 !memory_region_is_ram(section.mr) ||
>                 memory_region_is_rom(section.mr) ||
>                 memory_region_is_romd(section.mr)) {
>                 trace_virtio_balloon_bad_addr(pa);
>                 memory_region_unref(section.mr);
>                 continue;
>             }

Oh, I see what you mean.  Hrm, I still kind of prefer visually
separating the validity check from tests which depend on that validity.

> 
> > > 
> > > Apart from that,
> > > 
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > >   
> > > > +            if (!memory_region_is_ram(section.mr) ||
> > > >                  memory_region_is_rom(section.mr) ||
> > > >                  memory_region_is_romd(section.mr)) {
> > > >                  trace_virtio_balloon_bad_addr(pa);  
> > >   
> > 
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] virtio-balloon: Corrections to address verification

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4535 bytes --]

On Wed, 27 Feb 2019 10:20:12 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Feb 25, 2019 at 10:26:51AM +0100, Greg Kurz wrote:
> > On Mon, 25 Feb 2019 10:37:11 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Feb 22, 2019 at 10:08:22AM +0100, Greg Kurz wrote:  
> > > > On Thu, 14 Feb 2019 15:39:13 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > The virtio-balloon device's verification of the address given to it by the
> > > > > guest has a number of faults:
> > > > >     * The addresses here are guest physical addresses, which should be
> > > > >       'hwaddr' rather than 'ram_addr_t' (the distinction is admittedly
> > > > >       pretty subtle and confusing)
> > > > >     * We don't check for section.mr being NULL, which is the main way that
> > > > >       memory_region_find() reports basic failures.  We really need to check
> > > > >       that before looking at any other section fields, because
> > > > >       memory_region_find() doesn't initialize them on the failure path
> > > > >     * We're passing a length of '1' to memory_region_find(), but really the
> > > > >       guest is requesting that we put the entire page into the balloon,
> > > > >       so it makes more sense to call it with BALLOON_PAGE_SIZE
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  hw/virtio/virtio-balloon.c | 17 ++++++++++-------
> > > > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > > > index 43af521884..eb357824d8 100644
> > > > > --- a/hw/virtio/virtio-balloon.c
> > > > > +++ b/hw/virtio/virtio-balloon.c
> > > > > @@ -221,17 +221,20 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > > > >          }
> > > > >  
> > > > >          while (iov_to_buf(elem->out_sg, elem->out_num, offset, &pfn, 4) == 4) {
> > > > > -            ram_addr_t pa;
> > > > > -            ram_addr_t addr;
> > > > > +            hwaddr pa;
> > > > > +            hwaddr addr;
> > > > >              int p = virtio_ldl_p(vdev, &pfn);
> > > > >  
> > > > > -            pa = (ram_addr_t) p << VIRTIO_BALLOON_PFN_SHIFT;
> > > > > +            pa = (hwaddr) p << VIRTIO_BALLOON_PFN_SHIFT;
> > > > >              offset += 4;
> > > > >  
> > > > > -            /* FIXME: remove get_system_memory(), but how? */
> > > > > -            section = memory_region_find(get_system_memory(), pa, 1);
> > > > > -            if (!int128_nz(section.size) ||
> > > > > -                !memory_region_is_ram(section.mr) ||
> > > > > +            section = memory_region_find(get_system_memory(), pa,
> > > > > +                                         BALLOON_PAGE_SIZE);
> > > > > +            if (!section.mr) {
> > > > > +                trace_virtio_balloon_bad_addr(pa);
> > > > > +                continue;
> > > > > +            }    
> > > > 
> > > > memory_region_unref(section.mr) with section.mr == NULL is safe and
> > > > resolves to a nop. Not sure you need a separate if to handle this
> > > > case.    
> > > 
> > > memory_region_is_ram() and friends are not, however - they will
> > > dereference their argument unconditionally.
> > >   
> > 
> > Indeed but the two ifs can be merged anyway:
> > 
> >             if (!section.mr ||
> >                 !memory_region_is_ram(section.mr) ||
> >                 memory_region_is_rom(section.mr) ||
> >                 memory_region_is_romd(section.mr)) {
> >                 trace_virtio_balloon_bad_addr(pa);
> >                 memory_region_unref(section.mr);
> >                 continue;
> >             }  
> 
> Oh, I see what you mean.  Hrm, I still kind of prefer visually
> separating the validity check from tests which depend on that validity.
> 

Makes sense. It's alright then :)

> >   
> > > > 
> > > > Apart from that,
> > > > 
> > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > >     
> > > > > +            if (!memory_region_is_ram(section.mr) ||
> > > > >                  memory_region_is_rom(section.mr) ||
> > > > >                  memory_region_is_romd(section.mr)) {
> > > > >                  trace_virtio_balloon_bad_addr(pa);    
> > > >     
> > >   
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[Michael S. Tsirkin]

On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> When the balloon is inflated, we discard memory place in it using madvise()
> with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> sounds like it makes sense but is actually unnecessary.
> 
> The misleadingly named MADV_DONTNEED just discards the memory in question,
> it doesn't set any persistent state on it in-kernel; all that's necessary
> to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> specifically says that the memory will be used soon and faults it in.
> 
> This patch simplify's the balloon operation by dropping the madvise()
> on deflate.  This might have an impact on performance - it will move a
> delay at deflate time until that memory is actually touched, which
> might be more latency sensitive.  However:
> 
>   * Memory that's being given back to the guest by deflating the
>     balloon *might* be used soon, but it equally could just sit around
>     in the guest's pools until needed (or even be faulted out again if
>     the host is under memory pressure).
> 
>   * Usually, the timescale over which you'll be adjusting the balloon
>     is long enough that a few extra faults after deflation aren't
>     going to make a difference.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I'm having second thoughts about this. It might affect performance but
probably won't but we have no idea.  Might cause latency jitter after
deflate where it previously didn't happen.  This kind of patch should
really be accompanied by benchmarking results, not philosophy.

> ---
>  hw/virtio/virtio-balloon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a12677d4d5..43af521884 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -35,9 +35,8 @@
>  
>  static void balloon_page(void *addr, int deflate)
>  {
> -    if (!qemu_balloon_is_inhibited()) {
> -        qemu_madvise(addr, BALLOON_PAGE_SIZE,
> -                deflate ? QEMU_MADV_WILLNEED : QEMU_MADV_DONTNEED);
> +    if (!qemu_balloon_is_inhibited() && !deflate) {
> +        qemu_madvise(addr, BALLOON_PAGE_SIZE, QEMU_MADV_DONTNEED);
>      }
>  }
>  
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[Michael S. Tsirkin]

On Thu, Feb 14, 2019 at 03:39:11PM +1100, David Gibson wrote:
> I posted some RFCs for this back in December, but didn't wrap it up in
> time for 3.1.  Posting again for inclusion in 4.0.
> 
> The virtio-balloon devices was never really thought out for cases
> other than 4kiB pagesize on both guest and host.  It works in some
> cases, but in others can be ineffectual or even cause guest memory
> corruption.
> 
> This series makes a handful of preliminary cleanups, then makes a
> change to safely, though not perfectly, handle cases with non 4kiB
> pagesizes.

I'd like to see a version of this that does not depend on patch 1 which
is not a cleanup nor a bugfix. Could you look into this please?
We can then debate merits of patch 1 separately.


> Changes since RFC:
>  * Further refinement of when to issue warnings in 5/5
> 
> David Gibson (5):
>   virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
>   virtio-balloon: Corrections to address verification
>   virtio-balloon: Rework ballon_page() interface
>   virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
>   virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size
> 
>  hw/virtio/virtio-balloon.c         | 102 ++++++++++++++++++++++++-----
>  include/hw/virtio/virtio-balloon.h |   3 +
>  2 files changed, 89 insertions(+), 16 deletions(-)
> 
> -- 
> 2.20.1

Re: [Qemu-devel] [PULL v2 resend 00/26] pci, pc, virtio: fixes, cleanups, tests

[Paolo Bonzini]

On 25/02/19 16:19, Michael S. Tsirkin wrote:
> The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
> 
>   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> 
> for you to fetch changes up to 88c869198aa630e0477d653d0abf3f42c7c44d1f
> 
>   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
> 
> ----------------------------------------------------------------
> 
> Note: this is same as a fixup I sent earlier, this is just a resend to
> make sure it's not missed.  This should also help me figure out whether
> this is a good format to use.

You should have sent it as a new toplevel message, too, otherwise Peter
doesn't see it.

Paolo

> ----------------------------------------------------------------
> pci, pc, virtio: fixes, cleanups, tests
> 
> Lots of work on tests: BiosTablesTest UEFI app,
> vhost-user testing for non-Linux hosts.
> Misc cleanups and fixes all over the place
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ----------------------------------------------------------------
> Alex Williamson (1):
>       pci: Sanity test minimum downstream LNKSTA
> 
> Alexey Kardashevskiy (1):
>       pci: Move NVIDIA vendor id to the rest of ids
> 
> Changpeng Liu (1):
>       contrib/vhost-user-blk: fix the compilation issue
> 
> Daniel P. Berrangé (1):
>       hw/smbios: fix offset of type 3 sku field
> 
> David Gibson (5):
>       virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
>       virtio-balloon: Corrections to address verification
>       virtio-balloon: Rework ballon_page() interface
>       virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
>       virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size
> 
> Laszlo Ersek (5):
>       roms: add the edk2 project as a git submodule
>       roms: build the EfiRom utility from the roms/edk2 submodule
>       tests: introduce "uefi-test-tools" with the BiosTablesTest UEFI app
>       tests/uefi-test-tools: add build scripts
>       tests/data: introduce "uefi-boot-images" with the "bios-tables-test" ISOs
> 
> Paolo Bonzini (9):
>       vhost-net: move stubs to a separate file
>       vhost-net-user: add stubs for when no virtio-net device is present
>       vhost: restrict Linux dependency to kernel vhost
>       vhost-user: support cross-endian vnet headers
>       vhost-net: compile it on all targets that have virtio-net.
>       vhost-net: revamp configure logic
>       vhost-user-test: create a main loop per TestServer
>       vhost-user-test: small changes to init_hugepagefs
>       vhost-user-test: create a temporary directory per TestServer
> 
> Peter Xu (1):
>       i386/kvm: ignore masked irqs when update msi routes
> 
> Philippe Mathieu-Daudé (1):
>       Revert "contrib/vhost-user-blk: fix the compilation issue"
> 
> Wei Yang (1):
>       pc-dimm: use same mechanism for [get|set]_addr
> 
>  configure                                          | 102 ++++++++-----
>  Makefile                                           |   6 +-
>  default-configs/virtio.mak                         |   4 +-
>  include/exec/poison.h                              |   1 -
>  include/hw/firmware/smbios.h                       |   1 +
>  include/hw/pci/pci_ids.h                           |   2 +
>  include/hw/virtio/virtio-balloon.h                 |   3 +
>  .../UefiTestToolsPkg/Include/Guid/BiosTablesTest.h |  67 +++++++++
>  hw/mem/pc-dimm.c                                   |   4 +-
>  hw/net/vhost_net-stub.c                            |  92 ++++++++++++
>  hw/net/vhost_net.c                                 |  85 +----------
>  hw/pci/pcie.c                                      |  13 +-
>  hw/smbios/smbios.c                                 |   1 +
>  hw/vfio/pci-quirks.c                               |   2 -
>  hw/virtio/vhost-backend.c                          |  12 +-
>  hw/virtio/vhost-user.c                             |  13 +-
>  hw/virtio/vhost.c                                  |   2 +-
>  hw/virtio/virtio-balloon.c                         | 102 ++++++++++---
>  net/net.c                                          |   2 +-
>  net/vhost-user-stub.c                              |  23 +++
>  net/vhost-user.c                                   |  13 ++
>  .../BiosTablesTest/BiosTablesTest.c                | 130 +++++++++++++++++
>  tests/vhost-user-test.c                            | 160 +++++++++++----------
>  .gitmodules                                        |   3 +
>  backends/Makefile.objs                             |   5 +-
>  hw/net/Makefile.objs                               |   4 +-
>  hw/virtio/Makefile.objs                            |   8 +-
>  net/Makefile.objs                                  |   4 +-
>  roms/Makefile                                      |  13 +-
>  roms/edk2                                          |   1 +
>  tests/Makefile.include                             |   5 +-
>  .../bios-tables-test.aarch64.iso.qcow2             | Bin 0 -> 11776 bytes
>  .../bios-tables-test.arm.iso.qcow2                 | Bin 0 -> 11776 bytes
>  .../bios-tables-test.i386.iso.qcow2                | Bin 0 -> 12800 bytes
>  .../bios-tables-test.x86_64.iso.qcow2              | Bin 0 -> 13312 bytes
>  tests/uefi-test-tools/.gitignore                   |   3 +
>  tests/uefi-test-tools/LICENSE                      |  25 ++++
>  tests/uefi-test-tools/Makefile                     | 106 ++++++++++++++
>  .../BiosTablesTest/BiosTablesTest.inf              |  41 ++++++
>  .../UefiTestToolsPkg/UefiTestToolsPkg.dec          |  27 ++++
>  .../UefiTestToolsPkg/UefiTestToolsPkg.dsc          |  69 +++++++++
>  tests/uefi-test-tools/build.sh                     | 145 +++++++++++++++++++
>  42 files changed, 1051 insertions(+), 248 deletions(-)
>  create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/Include/Guid/BiosTablesTest.h
>  create mode 100644 hw/net/vhost_net-stub.c
>  create mode 100644 net/vhost-user-stub.c
>  create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>  create mode 160000 roms/edk2
>  create mode 100644 tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
>  create mode 100644 tests/data/uefi-boot-images/bios-tables-test.arm.iso.qcow2
>  create mode 100644 tests/data/uefi-boot-images/bios-tables-test.i386.iso.qcow2
>  create mode 100644 tests/data/uefi-boot-images/bios-tables-test.x86_64.iso.qcow2
>  create mode 100644 tests/uefi-test-tools/.gitignore
>  create mode 100644 tests/uefi-test-tools/LICENSE
>  create mode 100644 tests/uefi-test-tools/Makefile
>  create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.inf
>  create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dec
>  create mode 100644 tests/uefi-test-tools/UefiTestToolsPkg/UefiTestToolsPkg.dsc
>  create mode 100755 tests/uefi-test-tools/build.sh
> 
> 

Re: [Qemu-devel] [PULL v2 resend 00/26] pci, pc, virtio: fixes, cleanups, tests

[Peter Maydell]

On Mon, 4 Mar 2019 at 10:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 25/02/19 16:19, Michael S. Tsirkin wrote:
> > The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:
> >
> >   Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into staging (2019-02-21 13:09:33 +0000)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 88c869198aa630e0477d653d0abf3f42c7c44d1f
> >
> >   pci: Sanity test minimum downstream LNKSTA (2019-02-21 12:28:41 -0500)
> >
> > ----------------------------------------------------------------
> >
> > Note: this is same as a fixup I sent earlier, this is just a resend to
> > make sure it's not missed.  This should also help me figure out whether
> > this is a good format to use.
>
> You should have sent it as a new toplevel message, too, otherwise Peter
> doesn't see it.

No, my filter doesn't care about top level messages or not (though
sending as a new top level message is a good idea). I'm not sure
why this slipped through the net.

In any case, now applied, thanks. Please update the changelog for
any user-visible changes.

-- PMM

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > When the balloon is inflated, we discard memory place in it using madvise()
> > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > sounds like it makes sense but is actually unnecessary.
> > 
> > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > it doesn't set any persistent state on it in-kernel; all that's necessary
> > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > specifically says that the memory will be used soon and faults it in.
> > 
> > This patch simplify's the balloon operation by dropping the madvise()
> > on deflate.  This might have an impact on performance - it will move a
> > delay at deflate time until that memory is actually touched, which
> > might be more latency sensitive.  However:
> > 
> >   * Memory that's being given back to the guest by deflating the
> >     balloon *might* be used soon, but it equally could just sit around
> >     in the guest's pools until needed (or even be faulted out again if
> >     the host is under memory pressure).
> > 
> >   * Usually, the timescale over which you'll be adjusting the balloon
> >     is long enough that a few extra faults after deflation aren't
> >     going to make a difference.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I'm having second thoughts about this. It might affect performance but
> probably won't but we have no idea.  Might cause latency jitter after
> deflate where it previously didn't happen.  This kind of patch should
> really be accompanied by benchmarking results, not philosophy.

I guess I see your point, much as it's annoying to spend time
benchmarking a device that's basically broken by design.

That said.. I don't really know how I'd go about benchmarking it.  Any
guesses at a suitable workload which would be most likely to show a
performance degradation here?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Thu, Feb 28, 2019 at 08:39:21AM -0500, Michael S. Tsirkin wrote:
> On Thu, Feb 14, 2019 at 03:39:11PM +1100, David Gibson wrote:
> > I posted some RFCs for this back in December, but didn't wrap it up in
> > time for 3.1.  Posting again for inclusion in 4.0.
> > 
> > The virtio-balloon devices was never really thought out for cases
> > other than 4kiB pagesize on both guest and host.  It works in some
> > cases, but in others can be ineffectual or even cause guest memory
> > corruption.
> > 
> > This series makes a handful of preliminary cleanups, then makes a
> > change to safely, though not perfectly, handle cases with non 4kiB
> > pagesizes.
> 
> I'd like to see a version of this that does not depend on patch 1 which
> is not a cleanup nor a bugfix. Could you look into this please?

Ok... the original series is already applied to master, so I'm not
exactly sure what you want me to do here.  Should I try to come up
with a "logical revert" of the first patch?  Or do you intend to
revert the whole series, and I rewrite the series without the first
patch?

> We can then debate merits of patch 1 separately.
> 
> 
> > Changes since RFC:
> >  * Further refinement of when to issue warnings in 5/5
> > 
> > David Gibson (5):
> >   virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
> >   virtio-balloon: Corrections to address verification
> >   virtio-balloon: Rework ballon_page() interface
> >   virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
> >   virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size
> > 
> >  hw/virtio/virtio-balloon.c         | 102 ++++++++++++++++++++++++-----
> >  include/hw/virtio/virtio-balloon.h |   3 +
> >  2 files changed, 89 insertions(+), 16 deletions(-)
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[Michael S. Tsirkin]

On Tue, Mar 05, 2019 at 11:53:32AM +1100, David Gibson wrote:
> On Thu, Feb 28, 2019 at 08:39:21AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Feb 14, 2019 at 03:39:11PM +1100, David Gibson wrote:
> > > I posted some RFCs for this back in December, but didn't wrap it up in
> > > time for 3.1.  Posting again for inclusion in 4.0.
> > > 
> > > The virtio-balloon devices was never really thought out for cases
> > > other than 4kiB pagesize on both guest and host.  It works in some
> > > cases, but in others can be ineffectual or even cause guest memory
> > > corruption.
> > > 
> > > This series makes a handful of preliminary cleanups, then makes a
> > > change to safely, though not perfectly, handle cases with non 4kiB
> > > pagesizes.
> > 
> > I'd like to see a version of this that does not depend on patch 1 which
> > is not a cleanup nor a bugfix. Could you look into this please?
> 
> Ok... the original series is already applied to master, so I'm not
> exactly sure what you want me to do here.  Should I try to come up
> with a "logical revert" of the first patch?  Or do you intend to
> revert the whole series, and I rewrite the series without the first
> patch?

Whatever you prefer. Maybe the best idea is to add a flag
that says whether to madvise or not. Default can be compatible.
Hmm?


> > We can then debate merits of patch 1 separately.
> > 
> > 
> > > Changes since RFC:
> > >  * Further refinement of when to issue warnings in 5/5
> > > 
> > > David Gibson (5):
> > >   virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate
> > >   virtio-balloon: Corrections to address verification
> > >   virtio-balloon: Rework ballon_page() interface
> > >   virtio-balloon: Use ram_block_discard_range() instead of raw madvise()
> > >   virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size
> > > 
> > >  hw/virtio/virtio-balloon.c         | 102 ++++++++++++++++++++++++-----
> > >  include/hw/virtio/virtio-balloon.h |   3 +
> > >  2 files changed, 89 insertions(+), 16 deletions(-)
> > > 
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[Michael S. Tsirkin]

On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > When the balloon is inflated, we discard memory place in it using madvise()
> > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > sounds like it makes sense but is actually unnecessary.
> > > 
> > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > specifically says that the memory will be used soon and faults it in.
> > > 
> > > This patch simplify's the balloon operation by dropping the madvise()
> > > on deflate.  This might have an impact on performance - it will move a
> > > delay at deflate time until that memory is actually touched, which
> > > might be more latency sensitive.  However:
> > > 
> > >   * Memory that's being given back to the guest by deflating the
> > >     balloon *might* be used soon, but it equally could just sit around
> > >     in the guest's pools until needed (or even be faulted out again if
> > >     the host is under memory pressure).
> > > 
> > >   * Usually, the timescale over which you'll be adjusting the balloon
> > >     is long enough that a few extra faults after deflation aren't
> > >     going to make a difference.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > I'm having second thoughts about this. It might affect performance but
> > probably won't but we have no idea.  Might cause latency jitter after
> > deflate where it previously didn't happen.  This kind of patch should
> > really be accompanied by benchmarking results, not philosophy.
> 
> I guess I see your point, much as it's annoying to spend time
> benchmarking a device that's basically broken by design.

Because of 4K page thing? It's an annoying bug for sure.  There were
patches to add a feature bit to just switch to plan s/g format, but they
were abandoned. You are welcome to revive them though.
Additionally or alternatively, we can easily add a field specifying page size.

> That said.. I don't really know how I'd go about benchmarking it.  Any
> guesses at a suitable workload which would be most likely to show a
> performance degradation here?
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



Here's one idea - I tried to come up with a worst case scenario here.
Basically based on idea by Alex Duyck. All credits are his, all bugs are
mine:


Setup:
Memory-15837 MB
Guest Memory Size-5 GB
Swap-Disabled
Test Program-Simple program which allocates 4GB memory via malloc, touches it via memset and exits.
Use case-Number of guests that can be launched completely including the successful execution of the test program.
Procedure:
Setup:
A first guest is launched and once its console is up,
test allocation program is executed with 4 GB memory request (Due to
this the guest occupies almost 4-5 GB of memory in the host)
Afterwards balloon is inflated by 4Gbyte in the guest.
We continue launching the guests until a guest gets
killed due to low memory condition in the host.


Now repeatedly, in each guest in turn, balloon is deflated and
test allocation program is executed with 4 GB memory request (Due to
this the guest occupies almost 4-5 GB of memory in the host)
After program finishes balloon is inflated by 4GB again.

Then we switch to another guest.

Time how many cycles of this we can do.


Hope this helps.



-- 
MST

Re: [Qemu-devel] [PATCH 0/5] Improve balloon handling of pagesizes other than 4kiB

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On Mon, Mar 04, 2019 at 09:13:03PM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 11:53:32AM +1100, David Gibson wrote:
> > On Thu, Feb 28, 2019 at 08:39:21AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Feb 14, 2019 at 03:39:11PM +1100, David Gibson wrote:
> > > > I posted some RFCs for this back in December, but didn't wrap it up in
> > > > time for 3.1.  Posting again for inclusion in 4.0.
> > > > 
> > > > The virtio-balloon devices was never really thought out for cases
> > > > other than 4kiB pagesize on both guest and host.  It works in some
> > > > cases, but in others can be ineffectual or even cause guest memory
> > > > corruption.
> > > > 
> > > > This series makes a handful of preliminary cleanups, then makes a
> > > > change to safely, though not perfectly, handle cases with non 4kiB
> > > > pagesizes.
> > > 
> > > I'd like to see a version of this that does not depend on patch 1 which
> > > is not a cleanup nor a bugfix. Could you look into this please?
> > 
> > Ok... the original series is already applied to master, so I'm not
> > exactly sure what you want me to do here.  Should I try to come up
> > with a "logical revert" of the first patch?  Or do you intend to
> > revert the whole series, and I rewrite the series without the first
> > patch?
> 
> Whatever you prefer. Maybe the best idea is to add a flag
> that says whether to madvise or not. Default can be compatible.
> Hmm?

Ok, will do.

As I was looking at this I noticed a bug in the current code.
Extremely unlikely to hit in practice, but could result in guest
memory corruption if it does.  So, I'll fix that as well.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > When the balloon is inflated, we discard memory place in it using madvise()
> > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > > sounds like it makes sense but is actually unnecessary.
> > > > 
> > > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > specifically says that the memory will be used soon and faults it in.
> > > > 
> > > > This patch simplify's the balloon operation by dropping the madvise()
> > > > on deflate.  This might have an impact on performance - it will move a
> > > > delay at deflate time until that memory is actually touched, which
> > > > might be more latency sensitive.  However:
> > > > 
> > > >   * Memory that's being given back to the guest by deflating the
> > > >     balloon *might* be used soon, but it equally could just sit around
> > > >     in the guest's pools until needed (or even be faulted out again if
> > > >     the host is under memory pressure).
> > > > 
> > > >   * Usually, the timescale over which you'll be adjusting the balloon
> > > >     is long enough that a few extra faults after deflation aren't
> > > >     going to make a difference.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > I'm having second thoughts about this. It might affect performance but
> > > probably won't but we have no idea.  Might cause latency jitter after
> > > deflate where it previously didn't happen.  This kind of patch should
> > > really be accompanied by benchmarking results, not philosophy.
> > 
> > I guess I see your point, much as it's annoying to spend time
> > benchmarking a device that's basically broken by design.
> 
> Because of 4K page thing?

For one thing.  I believe David H has bunch of other reasons.

> It's an annoying bug for sure.  There were
> patches to add a feature bit to just switch to plan s/g format, but they
> were abandoned. You are welcome to revive them though.
> Additionally or alternatively, we can easily add a field specifying
> page size.

We could, but I'm pretty disinclined to work on this when virtio-mem
is a better solution in nearly every way.

> > That said.. I don't really know how I'd go about benchmarking it.  Any
> > guesses at a suitable workload which would be most likely to show a
> > performance degradation here?
> 
> Here's one idea - I tried to come up with a worst case scenario here.
> Basically based on idea by Alex Duyck. All credits are his, all bugs are
> mine:

Ok.  I'll try to find time to implement this and test it.

> Setup:
> Memory-15837 MB
> Guest Memory Size-5 GB
> Swap-Disabled
> Test Program-Simple program which allocates 4GB memory via malloc, touches it via memset and exits.
> Use case-Number of guests that can be launched completely including the successful execution of the test program.
> Procedure:
> Setup:
> A first guest is launched and once its console is up,
> test allocation program is executed with 4 GB memory request (Due to
> this the guest occupies almost 4-5 GB of memory in the host)
> Afterwards balloon is inflated by 4Gbyte in the guest.
> We continue launching the guests until a guest gets
> killed due to low memory condition in the host.
> 
> 
> Now repeatedly, in each guest in turn, balloon is deflated and
> test allocation program is executed with 4 GB memory request (Due to
> this the guest occupies almost 4-5 GB of memory in the host)
> After program finishes balloon is inflated by 4GB again.
> 
> Then we switch to another guest.
> 
> Time how many cycles of this we can do.
> 
> 
> Hope this helps.
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[Michael S. Tsirkin]

On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > When the balloon is inflated, we discard memory place in it using madvise()
> > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > > > sounds like it makes sense but is actually unnecessary.
> > > > > 
> > > > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > > specifically says that the memory will be used soon and faults it in.
> > > > > 
> > > > > This patch simplify's the balloon operation by dropping the madvise()
> > > > > on deflate.  This might have an impact on performance - it will move a
> > > > > delay at deflate time until that memory is actually touched, which
> > > > > might be more latency sensitive.  However:
> > > > > 
> > > > >   * Memory that's being given back to the guest by deflating the
> > > > >     balloon *might* be used soon, but it equally could just sit around
> > > > >     in the guest's pools until needed (or even be faulted out again if
> > > > >     the host is under memory pressure).
> > > > > 
> > > > >   * Usually, the timescale over which you'll be adjusting the balloon
> > > > >     is long enough that a few extra faults after deflation aren't
> > > > >     going to make a difference.
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > I'm having second thoughts about this. It might affect performance but
> > > > probably won't but we have no idea.  Might cause latency jitter after
> > > > deflate where it previously didn't happen.  This kind of patch should
> > > > really be accompanied by benchmarking results, not philosophy.
> > > 
> > > I guess I see your point, much as it's annoying to spend time
> > > benchmarking a device that's basically broken by design.
> > 
> > Because of 4K page thing?
> 
> For one thing.  I believe David H has bunch of other reasons.
> 
> > It's an annoying bug for sure.  There were
> > patches to add a feature bit to just switch to plan s/g format, but they
> > were abandoned. You are welcome to revive them though.
> > Additionally or alternatively, we can easily add a field specifying
> > page size.
> 
> We could, but I'm pretty disinclined to work on this when virtio-mem
> is a better solution in nearly every way.

Then one way would be to just let balloon be. Make it behave same as
always and don't make changes to it :)

> > > That said.. I don't really know how I'd go about benchmarking it.  Any
> > > guesses at a suitable workload which would be most likely to show a
> > > performance degradation here?
> > 
> > Here's one idea - I tried to come up with a worst case scenario here.
> > Basically based on idea by Alex Duyck. All credits are his, all bugs are
> > mine:
> 
> Ok.  I'll try to find time to implement this and test it.
> 
> > Setup:
> > Memory-15837 MB
> > Guest Memory Size-5 GB
> > Swap-Disabled
> > Test Program-Simple program which allocates 4GB memory via malloc, touches it via memset and exits.
> > Use case-Number of guests that can be launched completely including the successful execution of the test program.
> > Procedure:
> > Setup:
> > A first guest is launched and once its console is up,
> > test allocation program is executed with 4 GB memory request (Due to
> > this the guest occupies almost 4-5 GB of memory in the host)
> > Afterwards balloon is inflated by 4Gbyte in the guest.
> > We continue launching the guests until a guest gets
> > killed due to low memory condition in the host.
> > 
> > 
> > Now repeatedly, in each guest in turn, balloon is deflated and
> > test allocation program is executed with 4 GB memory request (Due to
> > this the guest occupies almost 4-5 GB of memory in the host)
> > After program finishes balloon is inflated by 4GB again.
> > 
> > Then we switch to another guest.
> > 
> > Time how many cycles of this we can do.
> > 
> > 
> > Hope this helps.
> > 
> > 
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PULL 23/26] virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

[Peter Maydell]

On Fri, 22 Feb 2019 at 02:41, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: David Gibson <david@gibson.dropbear.id.au>
>
> The virtio-balloon always works in units of 4kiB (BALLOON_PAGE_SIZE), but
> we can only actually discard memory in units of the host page size.

Hi -- Coverity points out an issue in this patch (CID 1399146):

> +    /* Hard case
> +     *
> +     * We've put a piece of a larger host page into the balloon - we
> +     * need to keep track until we have a whole host page to
> +     * discard
> +     */
> +    warn_report_once(
> +"Balloon used with backing page size > 4kiB, this may not be reliable");
> +
> +    subpages = rb_page_size / BALLOON_PAGE_SIZE;
> +
> +    if (balloon->pbp
> +        && (rb != balloon->pbp->rb
> +            || host_page_base != balloon->pbp->base)) {
> +        /* We've partially ballooned part of a host page, but now
> +         * we're trying to balloon part of a different one.  Too hard,
> +         * give up on the old partial page */
> +        free(balloon->pbp);
> +        balloon->pbp = NULL;
>      }
>
> -    ram_block_discard_range(rb, ram_offset, rb_page_size);
> -    /* We ignore errors from ram_block_discard_range(), because it has
> -     * already reported them, and failing to discard a balloon page is
> -     * not fatal */
> +    if (!balloon->pbp) {
> +        /* Starting on a new host page */
> +        size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
> +        balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);


We allocate balloon->pbp with g_malloc0() here...

> +        balloon->pbp->rb = rb;
> +        balloon->pbp->base = host_page_base;
> +    }
> +
> +    bitmap_set(balloon->pbp->bitmap,
> +               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> +               subpages);
> +
> +    if (bitmap_full(balloon->pbp->bitmap, subpages)) {
> +        /* We've accumulated a full host page, we can actually discard
> +         * it now */
> +
> +        ram_block_discard_range(rb, balloon->pbp->base, rb_page_size);
> +        /* We ignore errors from ram_block_discard_range(), because it
> +         * has already reported them, and failing to discard a balloon
> +         * page is not fatal */
> +
> +        free(balloon->pbp);

...but we free it (here and elsewhere) with free(), not g_free().


thanks
-- PMM

Re: [Qemu-devel] [PULL 23/26] virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]

On Tue, Mar 05, 2019 at 04:06:54PM +0000, Peter Maydell wrote:
> On Fri, 22 Feb 2019 at 02:41, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: David Gibson <david@gibson.dropbear.id.au>
> >
> > The virtio-balloon always works in units of 4kiB (BALLOON_PAGE_SIZE), but
> > we can only actually discard memory in units of the host page size.
> 
> Hi -- Coverity points out an issue in this patch (CID 1399146):
> 
> > +    /* Hard case
> > +     *
> > +     * We've put a piece of a larger host page into the balloon - we
> > +     * need to keep track until we have a whole host page to
> > +     * discard
> > +     */
> > +    warn_report_once(
> > +"Balloon used with backing page size > 4kiB, this may not be reliable");
> > +
> > +    subpages = rb_page_size / BALLOON_PAGE_SIZE;
> > +
> > +    if (balloon->pbp
> > +        && (rb != balloon->pbp->rb
> > +            || host_page_base != balloon->pbp->base)) {
> > +        /* We've partially ballooned part of a host page, but now
> > +         * we're trying to balloon part of a different one.  Too hard,
> > +         * give up on the old partial page */
> > +        free(balloon->pbp);
> > +        balloon->pbp = NULL;
> >      }
> >
> > -    ram_block_discard_range(rb, ram_offset, rb_page_size);
> > -    /* We ignore errors from ram_block_discard_range(), because it has
> > -     * already reported them, and failing to discard a balloon page is
> > -     * not fatal */
> > +    if (!balloon->pbp) {
> > +        /* Starting on a new host page */
> > +        size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
> > +        balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);
> 
> 
> We allocate balloon->pbp with g_malloc0() here...
> 
> > +        balloon->pbp->rb = rb;
> > +        balloon->pbp->base = host_page_base;
> > +    }
> > +
> > +    bitmap_set(balloon->pbp->bitmap,
> > +               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> > +               subpages);
> > +
> > +    if (bitmap_full(balloon->pbp->bitmap, subpages)) {
> > +        /* We've accumulated a full host page, we can actually discard
> > +         * it now */
> > +
> > +        ram_block_discard_range(rb, balloon->pbp->base, rb_page_size);
> > +        /* We ignore errors from ram_block_discard_range(), because it
> > +         * has already reported them, and failing to discard a balloon
> > +         * page is not fatal */
> > +
> > +        free(balloon->pbp);
> 
> ...but we free it (here and elsewhere) with free(), not g_free().

Ah.  Whoops.

I'll put a fix for that in the series of followup balloon patches I'm
working on right now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5314 bytes --]

On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > When the balloon is inflated, we discard memory place in it using madvise()
> > > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > 
> > > > > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > > > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > > > specifically says that the memory will be used soon and faults it in.
> > > > > > 
> > > > > > This patch simplify's the balloon operation by dropping the madvise()
> > > > > > on deflate.  This might have an impact on performance - it will move a
> > > > > > delay at deflate time until that memory is actually touched, which
> > > > > > might be more latency sensitive.  However:
> > > > > > 
> > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > >     balloon *might* be used soon, but it equally could just sit around
> > > > > >     in the guest's pools until needed (or even be faulted out again if
> > > > > >     the host is under memory pressure).
> > > > > > 
> > > > > >   * Usually, the timescale over which you'll be adjusting the balloon
> > > > > >     is long enough that a few extra faults after deflation aren't
> > > > > >     going to make a difference.
> > > > > > 
> > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > 
> > > > > I'm having second thoughts about this. It might affect performance but
> > > > > probably won't but we have no idea.  Might cause latency jitter after
> > > > > deflate where it previously didn't happen.  This kind of patch should
> > > > > really be accompanied by benchmarking results, not philosophy.
> > > > 
> > > > I guess I see your point, much as it's annoying to spend time
> > > > benchmarking a device that's basically broken by design.
> > > 
> > > Because of 4K page thing?
> > 
> > For one thing.  I believe David H has bunch of other reasons.
> > 
> > > It's an annoying bug for sure.  There were
> > > patches to add a feature bit to just switch to plan s/g format, but they
> > > were abandoned. You are welcome to revive them though.
> > > Additionally or alternatively, we can easily add a field specifying
> > > page size.
> > 
> > We could, but I'm pretty disinclined to work on this when virtio-mem
> > is a better solution in nearly every way.
> 
> Then one way would be to just let balloon be. Make it behave same as
> always and don't make changes to it :)

I'd love to, but it is in real world use, so I think we do need to fix
serious bugs in it - at least if they can be fixed on one side,
without needing to roll out both qemu and guest changes (which adding
page size negotiation would require).

> 
> > > > That said.. I don't really know how I'd go about benchmarking it.  Any
> > > > guesses at a suitable workload which would be most likely to show a
> > > > performance degradation here?
> > > 
> > > Here's one idea - I tried to come up with a worst case scenario here.
> > > Basically based on idea by Alex Duyck. All credits are his, all bugs are
> > > mine:
> > 
> > Ok.  I'll try to find time to implement this and test it.
> > 
> > > Setup:
> > > Memory-15837 MB
> > > Guest Memory Size-5 GB
> > > Swap-Disabled
> > > Test Program-Simple program which allocates 4GB memory via malloc, touches it via memset and exits.
> > > Use case-Number of guests that can be launched completely including the successful execution of the test program.
> > > Procedure:
> > > Setup:
> > > A first guest is launched and once its console is up,
> > > test allocation program is executed with 4 GB memory request (Due to
> > > this the guest occupies almost 4-5 GB of memory in the host)
> > > Afterwards balloon is inflated by 4Gbyte in the guest.
> > > We continue launching the guests until a guest gets
> > > killed due to low memory condition in the host.
> > > 
> > > 
> > > Now repeatedly, in each guest in turn, balloon is deflated and
> > > test allocation program is executed with 4 GB memory request (Due to
> > > this the guest occupies almost 4-5 GB of memory in the host)
> > > After program finishes balloon is inflated by 4GB again.
> > > 
> > > Then we switch to another guest.
> > > 
> > > Time how many cycles of this we can do.
> > > 
> > > 
> > > Hope this helps.
> > > 
> > > 
> > > 
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[Michael S. Tsirkin]

On Wed, Mar 06, 2019 at 10:35:12AM +1100, David Gibson wrote:
> On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > > When the balloon is inflated, we discard memory place in it using madvise()
> > > > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > > 
> > > > > > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > > > > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > > > > specifically says that the memory will be used soon and faults it in.
> > > > > > > 
> > > > > > > This patch simplify's the balloon operation by dropping the madvise()
> > > > > > > on deflate.  This might have an impact on performance - it will move a
> > > > > > > delay at deflate time until that memory is actually touched, which
> > > > > > > might be more latency sensitive.  However:
> > > > > > > 
> > > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > > >     balloon *might* be used soon, but it equally could just sit around
> > > > > > >     in the guest's pools until needed (or even be faulted out again if
> > > > > > >     the host is under memory pressure).
> > > > > > > 
> > > > > > >   * Usually, the timescale over which you'll be adjusting the balloon
> > > > > > >     is long enough that a few extra faults after deflation aren't
> > > > > > >     going to make a difference.
> > > > > > > 
> > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > 
> > > > > > I'm having second thoughts about this. It might affect performance but
> > > > > > probably won't but we have no idea.  Might cause latency jitter after
> > > > > > deflate where it previously didn't happen.  This kind of patch should
> > > > > > really be accompanied by benchmarking results, not philosophy.
> > > > > 
> > > > > I guess I see your point, much as it's annoying to spend time
> > > > > benchmarking a device that's basically broken by design.
> > > > 
> > > > Because of 4K page thing?
> > > 
> > > For one thing.  I believe David H has bunch of other reasons.
> > > 
> > > > It's an annoying bug for sure.  There were
> > > > patches to add a feature bit to just switch to plan s/g format, but they
> > > > were abandoned. You are welcome to revive them though.
> > > > Additionally or alternatively, we can easily add a field specifying
> > > > page size.
> > > 
> > > We could, but I'm pretty disinclined to work on this when virtio-mem
> > > is a better solution in nearly every way.
> > 
> > Then one way would be to just let balloon be. Make it behave same as
> > always and don't make changes to it :)
> 
> I'd love to, but it is in real world use, so I think we do need to fix
> serious bugs in it - at least if they can be fixed on one side,
> without needing to roll out both qemu and guest changes (which adding
> page size negotiation would require).


Absolutely I'm just saying don't add optimizations in that case :)

> > 
> > > > > That said.. I don't really know how I'd go about benchmarking it.  Any
> > > > > guesses at a suitable workload which would be most likely to show a
> > > > > performance degradation here?
> > > > 
> > > > Here's one idea - I tried to come up with a worst case scenario here.
> > > > Basically based on idea by Alex Duyck. All credits are his, all bugs are
> > > > mine:
> > > 
> > > Ok.  I'll try to find time to implement this and test it.
> > > 
> > > > Setup:
> > > > Memory-15837 MB
> > > > Guest Memory Size-5 GB
> > > > Swap-Disabled
> > > > Test Program-Simple program which allocates 4GB memory via malloc, touches it via memset and exits.
> > > > Use case-Number of guests that can be launched completely including the successful execution of the test program.
> > > > Procedure:
> > > > Setup:
> > > > A first guest is launched and once its console is up,
> > > > test allocation program is executed with 4 GB memory request (Due to
> > > > this the guest occupies almost 4-5 GB of memory in the host)
> > > > Afterwards balloon is inflated by 4Gbyte in the guest.
> > > > We continue launching the guests until a guest gets
> > > > killed due to low memory condition in the host.
> > > > 
> > > > 
> > > > Now repeatedly, in each guest in turn, balloon is deflated and
> > > > test allocation program is executed with 4 GB memory request (Due to
> > > > this the guest occupies almost 4-5 GB of memory in the host)
> > > > After program finishes balloon is inflated by 4GB again.
> > > > 
> > > > Then we switch to another guest.
> > > > 
> > > > Time how many cycles of this we can do.
> > > > 
> > > > 
> > > > Hope this helps.
> > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 1/5] virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4131 bytes --]

On Tue, Mar 05, 2019 at 07:14:09PM -0500, Michael S. Tsirkin wrote:
> On Wed, Mar 06, 2019 at 10:35:12AM +1100, David Gibson wrote:
> > On Tue, Mar 05, 2019 at 09:41:34AM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Mar 05, 2019 at 04:03:00PM +1100, David Gibson wrote:
> > > > On Mon, Mar 04, 2019 at 09:29:24PM -0500, Michael S. Tsirkin wrote:
> > > > > On Tue, Mar 05, 2019 at 11:52:08AM +1100, David Gibson wrote:
> > > > > > On Thu, Feb 28, 2019 at 08:36:58AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Feb 14, 2019 at 03:39:12PM +1100, David Gibson wrote:
> > > > > > > > When the balloon is inflated, we discard memory place in it using madvise()
> > > > > > > > with MADV_DONTNEED.  And when we deflate it we use MADV_WILLNEED, which
> > > > > > > > sounds like it makes sense but is actually unnecessary.
> > > > > > > > 
> > > > > > > > The misleadingly named MADV_DONTNEED just discards the memory in question,
> > > > > > > > it doesn't set any persistent state on it in-kernel; all that's necessary
> > > > > > > > to bring the memory back is to touch it.  MADV_WILLNEED in contrast
> > > > > > > > specifically says that the memory will be used soon and faults it in.
> > > > > > > > 
> > > > > > > > This patch simplify's the balloon operation by dropping the madvise()
> > > > > > > > on deflate.  This might have an impact on performance - it will move a
> > > > > > > > delay at deflate time until that memory is actually touched, which
> > > > > > > > might be more latency sensitive.  However:
> > > > > > > > 
> > > > > > > >   * Memory that's being given back to the guest by deflating the
> > > > > > > >     balloon *might* be used soon, but it equally could just sit around
> > > > > > > >     in the guest's pools until needed (or even be faulted out again if
> > > > > > > >     the host is under memory pressure).
> > > > > > > > 
> > > > > > > >   * Usually, the timescale over which you'll be adjusting the balloon
> > > > > > > >     is long enough that a few extra faults after deflation aren't
> > > > > > > >     going to make a difference.
> > > > > > > > 
> > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > > > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > > > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > 
> > > > > > > I'm having second thoughts about this. It might affect performance but
> > > > > > > probably won't but we have no idea.  Might cause latency jitter after
> > > > > > > deflate where it previously didn't happen.  This kind of patch should
> > > > > > > really be accompanied by benchmarking results, not philosophy.
> > > > > > 
> > > > > > I guess I see your point, much as it's annoying to spend time
> > > > > > benchmarking a device that's basically broken by design.
> > > > > 
> > > > > Because of 4K page thing?
> > > > 
> > > > For one thing.  I believe David H has bunch of other reasons.
> > > > 
> > > > > It's an annoying bug for sure.  There were
> > > > > patches to add a feature bit to just switch to plan s/g format, but they
> > > > > were abandoned. You are welcome to revive them though.
> > > > > Additionally or alternatively, we can easily add a field specifying
> > > > > page size.
> > > > 
> > > > We could, but I'm pretty disinclined to work on this when virtio-mem
> > > > is a better solution in nearly every way.
> > > 
> > > Then one way would be to just let balloon be. Make it behave same as
> > > always and don't make changes to it :)
> > 
> > I'd love to, but it is in real world use, so I think we do need to fix
> > serious bugs in it - at least if they can be fixed on one side,
> > without needing to roll out both qemu and guest changes (which adding
> > page size negotiation would require).
> 
> 
> Absolutely I'm just saying don't add optimizations in that case :)

I don't plan to.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]