[RESEND,v5,3/3] Add a Hyper-V Dynamic Memory Protocol driver (hv-balloon)

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

This driver is like virtio-balloon on steroids: it allows both changing the
guest memory allocation via ballooning and inserting pieces of extra RAM
into it on demand from a provided memory backend.

One of advantages of these over ACPI-based PC DIMM hotplug is that such
memory can be hotplugged in much smaller granularity because the ACPI DIMM
slot limit does not apply.

In order to enable hot-adding additional memory a new memory backend needs
to be created and provided to the driver via the "memdev" parameter.
This can be achieved by, for example, adding
"-object memory-backend-ram,id=mem1,size=32G" to the QEMU command line and
then instantiating the driver with "memdev=mem1" parameter.

In contrast with ACPI DIMM hotplug where one can only request to unplug a
whole DIMM stick this driver allows removing memory from guest in single
page (4k) units via ballooning.

The actual resizing is done via ballooning interface (for example, via
the "balloon" HMP command)
This includes resizing the guest past its boot size - that is, hot-adding
additional memory in granularity limited only by the guest alignment
requirements.

After a VM reboot the guest is back to its original (boot) size.

In the future, the guest boot memory size might be changed on reboot
instead, taking into account the effective size that VM had before that
reboot (much like Hyper-V does).

For performance reasons, the guest-released memory is tracked in a few
range trees, as a series of (start, count) ranges.
Each time a new page range is inserted into such tree its neighbors are
checked as candidates for possible merging with it.

Besides performance reasons, the Dynamic Memory protocol itself uses page
ranges as the data structure in its messages, so relevant pages need to be
merged into such ranges anyway.

One has to be careful when tracking the guest-released pages, since the
guest can maliciously report returning pages outside its current address
space, which later clash with the address range of newly added memory.
Similarly, the guest can report freeing the same page twice.

The above design results in much better ballooning performance than when
using virtio-balloon with the same guest: 230 GB / minute with this driver
versus 70 GB / minute with virtio-balloon.

During a ballooning operation most of time is spent waiting for the guest
to come up with newly freed page ranges, processing the received ranges on
the host side (in QEMU and KVM) is nearly instantaneous.

The unballoon operation is also pretty much instantaneous:
thanks to the merging of the ballooned out page ranges 200 GB of memory can
be returned to the guest in about 1 second.
With virtio-balloon this operation takes about 2.5 minutes.

These tests were done against a Windows Server 2019 guest running on a
Xeon E5-2699, after dirtying the whole memory inside guest before each
balloon operation.

Using a range tree instead of a bitmap to track the removed memory also
means that the solution scales well with the guest size: even a 1 TB range
takes just a few bytes of such metadata.

Since the required GTree operations aren't present in every Glib version
a check for them was added to the meson build script, together with new
"--enable-hv-balloon" and "--disable-hv-balloon" configure arguments.
If these GTree operations are missing in the system's Glib version this
driver will be skipped during QEMU build.

An optional "status-report=on" device parameter requests memory status
events from the guest (typically sent every second), which allow the host
to learn both the guest memory available and the guest memory in use
counts.
They are emitted externally as "HV_BALLOON_STATUS_REPORT" QMP events.

The driver is named hv-balloon since the Linux kernel client driver for
the Dynamic Memory Protocol is named as such and to follow the naming
pattern established by the virtio-balloon driver.
The whole protocol runs over Hyper-V VMBus.

The driver was tested against Windows Server 2012 R2, Windows Server 2016
and Windows Server 2019 guests and obeys the guest alignment requirements
reported to the host via DM_CAPABILITIES_REPORT message.

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 Kconfig.host                  |    3 +
 hw/hyperv/Kconfig             |    5 +
 hw/hyperv/hv-balloon.c        | 2040 +++++++++++++++++++++++++++++++++
 hw/hyperv/meson.build         |    1 +
 hw/hyperv/trace-events        |   16 +
 meson.build                   |   28 +-
 meson_options.txt             |    2 +
 qapi/machine.json             |   25 +
 scripts/meson-buildoptions.sh |    3 +
 9 files changed, 2122 insertions(+), 1 deletion(-)
 create mode 100644 hw/hyperv/hv-balloon.c

On 12.06.23 16:00, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> This driver is like virtio-balloon on steroids: it allows both changing the
> guest memory allocation via ballooning and inserting pieces of extra RAM
> into it on demand from a provided memory backend.
> 
> One of advantages of these over ACPI-based PC DIMM hotplug is that such
> memory can be hotplugged in much smaller granularity because the ACPI DIMM
> slot limit does not apply.
> 
> In order to enable hot-adding additional memory a new memory backend needs
> to be created and provided to the driver via the "memdev" parameter.
> This can be achieved by, for example, adding
> "-object memory-backend-ram,id=mem1,size=32G" to the QEMU command line and
> then instantiating the driver with "memdev=mem1" parameter.
> 
> In contrast with ACPI DIMM hotplug where one can only request to unplug a
> whole DIMM stick this driver allows removing memory from guest in single
> page (4k) units via ballooning.
> 
> The actual resizing is done via ballooning interface (for example, via
> the "balloon" HMP command)
> This includes resizing the guest past its boot size - that is, hot-adding
> additional memory in granularity limited only by the guest alignment
> requirements.
> 
> After a VM reboot the guest is back to its original (boot) size.
> 
> In the future, the guest boot memory size might be changed on reboot
> instead, taking into account the effective size that VM had before that
> reboot (much like Hyper-V does).
> 
> For performance reasons, the guest-released memory is tracked in a few
> range trees, as a series of (start, count) ranges.
> Each time a new page range is inserted into such tree its neighbors are
> checked as candidates for possible merging with it.
> 
> Besides performance reasons, the Dynamic Memory protocol itself uses page
> ranges as the data structure in its messages, so relevant pages need to be
> merged into such ranges anyway.
> 
> One has to be careful when tracking the guest-released pages, since the
> guest can maliciously report returning pages outside its current address
> space, which later clash with the address range of newly added memory.
> Similarly, the guest can report freeing the same page twice.
> 
> The above design results in much better ballooning performance than when
> using virtio-balloon with the same guest: 230 GB / minute with this driver
> versus 70 GB / minute with virtio-balloon.
> 
> During a ballooning operation most of time is spent waiting for the guest
> to come up with newly freed page ranges, processing the received ranges on
> the host side (in QEMU and KVM) is nearly instantaneous.
> 
> The unballoon operation is also pretty much instantaneous:
> thanks to the merging of the ballooned out page ranges 200 GB of memory can
> be returned to the guest in about 1 second.
> With virtio-balloon this operation takes about 2.5 minutes.
> 
> These tests were done against a Windows Server 2019 guest running on a
> Xeon E5-2699, after dirtying the whole memory inside guest before each
> balloon operation.
> 
> Using a range tree instead of a bitmap to track the removed memory also
> means that the solution scales well with the guest size: even a 1 TB range
> takes just a few bytes of such metadata.
> 
> Since the required GTree operations aren't present in every Glib version
> a check for them was added to the meson build script, together with new
> "--enable-hv-balloon" and "--disable-hv-balloon" configure arguments.
> If these GTree operations are missing in the system's Glib version this
> driver will be skipped during QEMU build.
> 
> An optional "status-report=on" device parameter requests memory status
> events from the guest (typically sent every second), which allow the host
> to learn both the guest memory available and the guest memory in use
> counts.
> They are emitted externally as "HV_BALLOON_STATUS_REPORT" QMP events.
> 
> The driver is named hv-balloon since the Linux kernel client driver for
> the Dynamic Memory Protocol is named as such and to follow the naming
> pattern established by the virtio-balloon driver.
> The whole protocol runs over Hyper-V VMBus.
> 
> The driver was tested against Windows Server 2012 R2, Windows Server 2016
> and Windows Server 2019 guests and obeys the guest alignment requirements
> reported to the host via DM_CAPABILITIES_REPORT message.
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>   Kconfig.host                  |    3 +
>   hw/hyperv/Kconfig             |    5 +
>   hw/hyperv/hv-balloon.c        | 2040 +++++++++++++++++++++++++++++++++
>   hw/hyperv/meson.build         |    1 +
>   hw/hyperv/trace-events        |   16 +
>   meson.build                   |   28 +-
>   meson_options.txt             |    2 +
>   qapi/machine.json             |   25 +
>   scripts/meson-buildoptions.sh |    3 +
>   9 files changed, 2122 insertions(+), 1 deletion(-)
>   create mode 100644 hw/hyperv/hv-balloon.c
> 
> diff --git a/Kconfig.host b/Kconfig.host
> index d763d892693c..2ee71578f38f 100644
> --- a/Kconfig.host
> +++ b/Kconfig.host
> @@ -46,3 +46,6 @@ config FUZZ
>   config VFIO_USER_SERVER_ALLOWED
>       bool
>       imply VFIO_USER_SERVER
> +
> +config HV_BALLOON_POSSIBLE
> +    bool
> diff --git a/hw/hyperv/Kconfig b/hw/hyperv/Kconfig
> index fcf65903bd05..3db31f4f4b40 100644
> --- a/hw/hyperv/Kconfig
> +++ b/hw/hyperv/Kconfig
> @@ -16,3 +16,8 @@ config SYNDBG
>       bool
>       default y
>       depends on VMBUS
> +
> +config HV_BALLOON
> +    bool
> +    default y
> +    depends on HV_BALLOON_POSSIBLE && VMBUS
> diff --git a/hw/hyperv/hv-balloon.c b/hw/hyperv/hv-balloon.c
> new file mode 100644
> index 000000000000..076ac9a8bd90
> --- /dev/null
> +++ b/hw/hyperv/hv-balloon.c
> @@ -0,0 +1,2040 @@
> +/*
> + * QEMU Hyper-V Dynamic Memory Protocol driver
> + *
> + * Copyright (C) 2020-2023 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "exec/address-spaces.h"
> +#include "exec/cpu-common.h"
> +#include "exec/memory.h"
> +#include "exec/ramblock.h"
> +#include "hw/boards.h"
> +#include "hw/hyperv/dynmem-proto.h"
> +#include "hw/hyperv/vmbus.h"
> +#include "hw/mem/memory-device.h"
> +#include "hw/mem/pc-dimm.h"
> +#include "hw/qdev-core.h"
> +#include "hw/qdev-properties.h"
> +#include "monitor/qdev.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-events-machine.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/visitor.h"
> +#include "qemu/error-report.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "qemu/timer.h"
> +#include "sysemu/balloon.h"
> +#include "sysemu/hostmem.h"
> +#include "sysemu/reset.h"
> +#include "trace.h"
> +
> +/*
> + * temporarily avoid warnings about enhanced GTree API usage requiring a
> + * too recent Glib version until GLIB_VERSION_MAX_ALLOWED finally reaches
> + * the Glib version with this API
> + */
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> +
> +#define TYPE_HV_BALLOON "hv-balloon"
> +#define HV_BALLOON_ADDR_PROP "addr"
> +#define HV_BALLOON_MEMDEV_PROP "memdev"
> +#define HV_BALLOON_GUID "525074DC-8985-46e2-8057-A307DC18A502"
> +#define HV_BALLOON_PFN_SHIFT 12
> +#define HV_BALLOON_PAGE_SIZE (1 << HV_BALLOON_PFN_SHIFT)
> +
> +/*
> + * Some Windows versions (at least Server 2019) will crash with various
> + * error codes when receiving DM protocol requests (at least
> + * DM_MEM_HOT_ADD_REQUEST) immediately after boot.
> + *
> + * It looks like Hyper-V from Server 2016 uses a 50-second after-boot
> + * delay, probably to workaround this issue, so we'll use this value, too.
> + */
> +#define HV_BALLOON_POST_INIT_WAIT (50 * 1000)
> +
> +#define HV_BALLOON_HA_CHUNK_SIZE (2 * GiB)
> +#define HV_BALLOON_HA_CHUNK_PAGES (HV_BALLOON_HA_CHUNK_SIZE / HV_BALLOON_PAGE_SIZE)
> +
> +#define HV_BALLOON_HR_CHUNK_PAGES 585728
> +/*
> + *                                ^ that's the maximum number of pages
> + * that Windows returns in one hot remove response
> + *
> + * If the number requested is too high Windows will no longer honor
> + * these requests
> + */
> +
> +typedef enum State {
> +    /* not a real state */
> +    S_NO_CHANGE = 0,
> +
> +    S_WAIT_RESET,
> +    S_POST_RESET_CLOSED,
> +
> +    /* init flow */
> +    S_VERSION,
> +    S_CAPS,
> +    S_POST_INIT_WAIT,
> +
> +    S_IDLE,
> +
> +    /* balloon op flow */
> +    S_BALLOON_POSTING,
> +    S_BALLOON_RB_WAIT,
> +    S_BALLOON_REPLY_WAIT,
> +
> +    /* unballoon + hot add ops flow */
> +    S_UNBALLOON_POSTING,
> +    S_UNBALLOON_RB_WAIT,
> +    S_UNBALLOON_REPLY_WAIT,
> +    S_HOT_ADD_SETUP,
> +    S_HOT_ADD_RB_WAIT,
> +    S_HOT_ADD_POSTING,
> +    S_HOT_ADD_REPLY_WAIT,
> +} State;
> +
> +typedef struct StateDesc {
> +    State state;
> +    const char *desc;
> +} StateDesc;
> +
> +typedef struct PageRange {
> +    uint64_t start;
> +    uint64_t count;
> +} PageRange;
> +
> +/* type safety */
> +typedef struct PageRangeTree {
> +    GTree *t;
> +} PageRangeTree;
> +
> +/* "our range" means the memory range owned by this driver (for hot-adding) */
> +typedef struct OurRange {
> +    PageRange range;
> +
> +    /* How many pages were hot-added to the guest */
> +    uint64_t added;
> +
> +    /* Pages at the end not currently usable */
> +    uint64_t unusable_tail;
> +
> +    /* Memory removed from the guest */
> +    PageRangeTree removed_guest, removed_both;
> +} OurRange;
> +
> +typedef struct OurRangePlugged {
> +    OurRange range;
> +    MemoryDeviceState *md;
> +    HostMemoryBackend *hostmem;
> +} OurRangePlugged;
> +
> +typedef struct HvBalloon {
> +    VMBusDevice parent;
> +    State state;
> +    bool status_reports;
> +
> +    union dm_version version;
> +    union dm_caps caps;
> +
> +    QEMUTimer post_init_timer;
> +
> +    unsigned int trans_id;
> +
> +    /* Guest target size */
> +    uint64_t target;
> +    bool target_changed;
> +
> +    /* Current (un)balloon / hot-add operation parameters */
> +    union {
> +        uint64_t balloon_diff;
> +
> +        struct {
> +            uint64_t unballoon_diff;
> +            uint64_t hot_add_diff;
> +        };
> +
> +        struct {
> +            PageRange hot_add_range;
> +            uint64_t ha_current_count;
> +        };
> +    };
> +
> +    OurRangePlugged *our_range;
> +
> +    /* Non-ours removed memory */
> +    PageRangeTree removed_guest, removed_both;
> +
> +    /* Grand totals of removed memory (both ours and non-ours) */
> +    uint64_t removed_guest_ctr, removed_both_ctr;
> +
> +    /* MEMORY_DEVICE props */
> +    uint64_t addr;
> +    HostMemoryBackend *hostmem;
> +} HvBalloon;
> +
> +#define HV_BALLOON(obj) OBJECT_CHECK(HvBalloon, (obj), TYPE_HV_BALLOON)
> +
> +#define HV_BALLOON_SET_STATE(hvb, news)             \
> +    do {                                            \
> +        assert(news != S_NO_CHANGE);                \
> +        hv_balloon_state_set(hvb, news, # news);    \
> +    } while (0)
> +
> +#define HV_BALLOON_STATE_DESC_SET(stdesc, news)         \
> +    _hv_balloon_state_desc_set(stdesc, news, # news)
> +
> +#define HV_BALLOON_STATE_DESC_INIT \
> +    {                              \
> +        .state = S_NO_CHANGE,      \
> +    }
> +
> +#define SUM_OVERFLOW_U64(in1, in2) ((in1) > UINT64_MAX - (in2))
> +#define SUM_SATURATE_U64(in1, in2)              \
> +    ({                                          \
> +        uint64_t _in1 = (in1), _in2 = (in2);    \
> +        uint64_t _result;                       \
> +                                                \
> +        if (!SUM_OVERFLOW_U64(_in1, _in2)) {    \
> +            _result = _in1 + _in2;              \
> +        } else {                                \
> +            _result = UINT64_MAX;               \
> +        }                                       \
> +                                                \
> +        _result;                                \
> +    })
> +
> +typedef struct HvBalloonReq {
> +    VMBusChanReq vmreq;
> +} HvBalloonReq;
> +
> +/* PageRange */
> +static void page_range_intersect(const PageRange *range,
> +                                 uint64_t start, uint64_t count,
> +                                 PageRange *out)
> +{
> +    uint64_t end1 = range->start + range->count;
> +    uint64_t end2 = start + count;
> +    uint64_t end = MIN(end1, end2);
> +
> +    out->start = MAX(range->start, start);
> +    out->count = out->start < end ? end - out->start : 0;
> +}
> +
> +static uint64_t page_range_intersection_size(const PageRange *range,
> +                                             uint64_t start, uint64_t count)
> +{
> +    PageRange trange;
> +
> +    page_range_intersect(range, start, count, &trange);
> +    return trange.count;
> +}
> +
> +/* return just the part of range before (start) */
> +static void page_range_part_before(const PageRange *range,
> +                                   uint64_t start, PageRange *out)
> +{
> +    uint64_t endr = range->start + range->count;
> +    uint64_t end = MIN(endr, start);
> +
> +    out->start = range->start;
> +    if (end > out->start) {
> +        out->count = end - out->start;
> +    } else {
> +        out->count = 0;
> +    }
> +}
> +
> +/* return just the part of range after (start, count) */
> +static void page_range_part_after(const PageRange *range,
> +                                  uint64_t start, uint64_t count,
> +                                  PageRange *out)
> +{
> +    uint64_t end = range->start + range->count;
> +    uint64_t ends = start + count;
> +
> +    out->start = MAX(range->start, ends);
> +    if (end > out->start) {
> +        out->count = end - out->start;
> +    } else {
> +        out->count = 0;
> +    }
> +}
> +
> +static bool page_range_joinable_left(const PageRange *range,
> +                                     uint64_t start, uint64_t count)
> +{
> +    return start + count == range->start;
> +}
> +
> +static bool page_range_joinable_right(const PageRange *range,
> +                                      uint64_t start, uint64_t count)
> +{
> +    return range->start + range->count == start;
> +}
> +
> +static bool page_range_joinable(const PageRange *range,
> +                                uint64_t start, uint64_t count)
> +{
> +    return page_range_joinable_left(range, start, count) ||
> +        page_range_joinable_right(range, start, count);
> +}
> +
> +/* PageRangeTree */
> +static gint page_range_tree_key_compare(gconstpointer leftp,
> +                                        gconstpointer rightp,
> +                                        gpointer user_data)
> +{
> +    const uint64_t *left = leftp, *right = rightp;
> +
> +    if (*left < *right) {
> +        return -1;
> +    } else if (*left > *right) {
> +        return 1;
> +    } else { /* *left == *right */
> +        return 0;
> +    }
> +}
> +
> +static GTreeNode *page_range_tree_insert_new(PageRangeTree tree,
> +                                             uint64_t start, uint64_t count)
> +{
> +    uint64_t *key = g_malloc(sizeof(*key));
> +    PageRange *range = g_malloc(sizeof(*range));
> +
> +    assert(count > 0);
> +
> +    *key = range->start = start;
> +    range->count = count;
> +
> +    return g_tree_insert_node(tree.t, key, range);
> +}
> +
> +static void page_range_tree_insert(PageRangeTree tree,
> +                                   uint64_t start, uint64_t count,
> +                                   uint64_t *dupcount)
> +{
> +    GTreeNode *node;
> +    bool joinable;
> +    uint64_t intersection;
> +    PageRange *range;
> +
> +    assert(!SUM_OVERFLOW_U64(start, count));
> +    if (count == 0) {
> +        return;
> +    }
> +
> +    node = g_tree_upper_bound(tree.t, &start);
> +    if (node) {
> +        node = g_tree_node_previous(node);
> +    } else {
> +        node = g_tree_node_last(tree.t);
> +    }
> +
> +    if (node) {
> +        range = g_tree_node_value(node);
> +        assert(range);
> +        intersection = page_range_intersection_size(range, start, count);
> +        joinable = page_range_joinable_right(range, start, count);
> +    }
> +
> +    if (!node ||
> +        (!intersection && !joinable)) {
> +        /*
> +         * !node case: the tree is empty or the very first node in the tree
> +         * already has a higher key (the start of its range).
> +         * the other case: there is a gap in the tree between the new range
> +         * and the previous one.
> +         * anyway, let's just insert the new range into the tree.
> +         */
> +        node = page_range_tree_insert_new(tree, start, count);
> +        assert(node);
> +        range = g_tree_node_value(node);
> +        assert(range);
> +    } else {
> +        /*
> +         * the previous range in the tree either partially covers the new
> +         * range or ends just at its beginning - extend it
> +         */
> +        if (dupcount) {
> +            *dupcount += intersection;
> +        }
> +
> +        count += start - range->start;
> +        range->count = MAX(range->count, count);
> +    }
> +
> +    /* check next nodes for possible merging */
> +    for (node = g_tree_node_next(node); node; ) {
> +        PageRange *rangecur;
> +
> +        rangecur = g_tree_node_value(node);
> +        assert(rangecur);
> +
> +        intersection = page_range_intersection_size(rangecur,
> +                                                    range->start, range->count);
> +        joinable = page_range_joinable_left(rangecur,
> +                                            range->start, range->count);
> +        if (!intersection && !joinable) {
> +            /* the current node is disjoint */
> +            break;
> +        }
> +
> +        if (dupcount) {
> +            *dupcount += intersection;
> +        }
> +
> +        count = rangecur->count + (rangecur->start - range->start);
> +        range->count = MAX(range->count, count);
> +
> +        /* the current node was merged in, remove it */
> +        start = rangecur->start;
> +        node = g_tree_node_next(node);
> +        /* no hinted removal in GTree... */
> +        g_tree_remove(tree.t, &start);
> +    }
> +}
> +
> +static bool page_range_tree_pop(PageRangeTree tree, PageRange *out,
> +                                uint64_t maxcount)
> +{
> +    GTreeNode *node;
> +    PageRange *range;
> +
> +    node = g_tree_node_last(tree.t);
> +    if (!node) {
> +        return false;
> +    }
> +
> +    range = g_tree_node_value(node);
> +    assert(range);
> +
> +    out->start = range->start;
> +
> +    /* can't modify range->start as it is the node key */
> +    if (range->count > maxcount) {
> +        out->start += range->count - maxcount;
> +        out->count = maxcount;
> +        range->count -= maxcount;
> +    } else {
> +        out->count = range->count;
> +        /* no hinted removal in GTree... */
> +        g_tree_remove(tree.t, &out->start);
> +    }
> +
> +    return true;
> +}
> +
> +static bool page_range_tree_intree_any(PageRangeTree tree,
> +                                       uint64_t start, uint64_t count)
> +{
> +    GTreeNode *node;
> +
> +    if (count == 0) {
> +        return false;
> +    }
> +
> +    /* find the first node that can possibly intersect our range */
> +    node = g_tree_upper_bound(tree.t, &start);
> +    if (node) {
> +        /*
> +         * a NULL node below means that the very first node in the tree
> +         * already has a higher key (the start of its range).
> +         */
> +        node = g_tree_node_previous(node);
> +    } else {
> +        /* a NULL node below means that the tree is empty */
> +        node = g_tree_node_last(tree.t);
> +    }
> +    /* node range start <= range start */
> +
> +    if (!node) {
> +        /* node range start > range start */
> +        node = g_tree_node_first(tree.t);
> +    }
> +
> +    for ( ; node; node = g_tree_node_next(node)) {
> +        PageRange *range = g_tree_node_value(node);
> +
> +        assert(range);
> +        /*
> +         * if this node starts beyond or at the end of our range so does
> +         * every next one
> +         */
> +        if (range->start >= start + count) {
> +            break;
> +        }
> +
> +        if (page_range_intersection_size(range, start, count) > 0) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static void page_range_tree_init(PageRangeTree *tree)
> +{
> +    tree->t = g_tree_new_full(page_range_tree_key_compare, NULL,
> +                              g_free, g_free);
> +}
> +
> +static void page_range_tree_destroy(PageRangeTree *tree)
> +{
> +    /* g_tree_destroy() is not NULL-safe */
> +    if (!tree->t) {
> +        return;
> +    }
> +
> +    g_tree_destroy(tree->t);
> +    tree->t = NULL;
> +}
> +
> +/* OurRange */
> +#define OUR_RANGE(ptr) ((OurRange *)(ptr))
> +
> +static void our_range_init(OurRange *our_range, uint64_t start, uint64_t count)
> +{
> +    our_range->range.start = start;
> +    our_range->range.count = count;
> +
> +    page_range_tree_init(&our_range->removed_guest);
> +    page_range_tree_init(&our_range->removed_both);
> +
> +    /* mark the whole range as unused but for potential use */
> +    our_range->added = 0;
> +    our_range->unusable_tail = 0;
> +}
> +
> +static void our_range_destroy(OurRange *our_range)
> +{
> +    page_range_tree_destroy(&our_range->removed_guest);
> +    page_range_tree_destroy(&our_range->removed_both);
> +}
> +
> +/* OurRangePlugged */
> +static OurRangePlugged *our_range_plugged_new(MemoryDeviceState *md,
> +                                              HostMemoryBackend *hostmem,
> +                                              uint64_t align,
> +                                              Error **errp)
> +{
> +    ERRP_GUARD();
> +    OurRangePlugged *our_range;
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
> +    uint64_t addr, count;
> +
> +    if (!align) {
> +        align = HV_BALLOON_PAGE_SIZE;
> +    }
> +
> +    if (host_memory_backend_is_mapped(hostmem)) {
> +        error_setg(errp, "memory backend already mapped");
> +        return NULL;
> +    }
> +
> +    count = memory_device_get_region_size(md, &error_abort) / HV_BALLOON_PAGE_SIZE;
> +    if (!count) {
> +        error_setg(errp, "memory backend too small to be usable");
> +        return NULL;
> +    }
> +
> +    memory_device_pre_plug(md, ms, &align, errp);
> +    if (*errp) {
> +        return NULL;
> +    }
> +

This usage is still problematic I suspect (well, and a layer violation 
regarding the machine). The machine hotplug handler is supposed to call 
the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug 
notifications from the core. See how we handle 
virtio-mem/virtio-pmem/nvdimms as an example.

We assume that when memory_device_pre_plug() gets called, that the 
device is not realized yet, but once it gets plugged, that it already is 
realized, and that the device will actually vanish (get unrealized) when 
unplugging the device. Otherwise memory device logic like in 
get_plugged_memory_size() stops working.

As an example, see device_set_realized() on the pre_plug+realize+plug 
interaction.

IIRC, you're reusing the already-realized hv-balloon device here, correct?

Why can't you call the pre_plug/plug/unplug functions from the machine 
pre_plug/plug/unplug hooks -- exactly once for the memory device when 
plugging the hv-balloon device?

Is it to support the !memdev case or why is this this 
plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() 
required?


Supporting the !memdev case is interesting: you essentially want to plug 
a memory device without a device region (or with an empty stub). I guess 
we should get that figured out somehow.

On 12.06.2023 19:42, David Hildenbrand wrote:
> On 12.06.23 16:00, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> This driver is like virtio-balloon on steroids: it allows both changing the
>> guest memory allocation via ballooning and inserting pieces of extra RAM
>> into it on demand from a provided memory backend.
>>
>> One of advantages of these over ACPI-based PC DIMM hotplug is that such
>> memory can be hotplugged in much smaller granularity because the ACPI DIMM
>> slot limit does not apply.
>>
>> In order to enable hot-adding additional memory a new memory backend needs
>> to be created and provided to the driver via the "memdev" parameter.
>> This can be achieved by, for example, adding
>> "-object memory-backend-ram,id=mem1,size=32G" to the QEMU command line and
>> then instantiating the driver with "memdev=mem1" parameter.
>>
>> In contrast with ACPI DIMM hotplug where one can only request to unplug a
>> whole DIMM stick this driver allows removing memory from guest in single
>> page (4k) units via ballooning.
>>
>> The actual resizing is done via ballooning interface (for example, via
>> the "balloon" HMP command)
>> This includes resizing the guest past its boot size - that is, hot-adding
>> additional memory in granularity limited only by the guest alignment
>> requirements.
>>
>> After a VM reboot the guest is back to its original (boot) size.
>>
>> In the future, the guest boot memory size might be changed on reboot
>> instead, taking into account the effective size that VM had before that
>> reboot (much like Hyper-V does).
>>
>> For performance reasons, the guest-released memory is tracked in a few
>> range trees, as a series of (start, count) ranges.
>> Each time a new page range is inserted into such tree its neighbors are
>> checked as candidates for possible merging with it.
>>
>> Besides performance reasons, the Dynamic Memory protocol itself uses page
>> ranges as the data structure in its messages, so relevant pages need to be
>> merged into such ranges anyway.
>>
>> One has to be careful when tracking the guest-released pages, since the
>> guest can maliciously report returning pages outside its current address
>> space, which later clash with the address range of newly added memory.
>> Similarly, the guest can report freeing the same page twice.
>>
>> The above design results in much better ballooning performance than when
>> using virtio-balloon with the same guest: 230 GB / minute with this driver
>> versus 70 GB / minute with virtio-balloon.
>>
>> During a ballooning operation most of time is spent waiting for the guest
>> to come up with newly freed page ranges, processing the received ranges on
>> the host side (in QEMU and KVM) is nearly instantaneous.
>>
>> The unballoon operation is also pretty much instantaneous:
>> thanks to the merging of the ballooned out page ranges 200 GB of memory can
>> be returned to the guest in about 1 second.
>> With virtio-balloon this operation takes about 2.5 minutes.
>>
>> These tests were done against a Windows Server 2019 guest running on a
>> Xeon E5-2699, after dirtying the whole memory inside guest before each
>> balloon operation.
>>
>> Using a range tree instead of a bitmap to track the removed memory also
>> means that the solution scales well with the guest size: even a 1 TB range
>> takes just a few bytes of such metadata.
>>
>> Since the required GTree operations aren't present in every Glib version
>> a check for them was added to the meson build script, together with new
>> "--enable-hv-balloon" and "--disable-hv-balloon" configure arguments.
>> If these GTree operations are missing in the system's Glib version this
>> driver will be skipped during QEMU build.
>>
>> An optional "status-report=on" device parameter requests memory status
>> events from the guest (typically sent every second), which allow the host
>> to learn both the guest memory available and the guest memory in use
>> counts.
>> They are emitted externally as "HV_BALLOON_STATUS_REPORT" QMP events.
>>
>> The driver is named hv-balloon since the Linux kernel client driver for
>> the Dynamic Memory Protocol is named as such and to follow the naming
>> pattern established by the virtio-balloon driver.
>> The whole protocol runs over Hyper-V VMBus.
>>
>> The driver was tested against Windows Server 2012 R2, Windows Server 2016
>> and Windows Server 2019 guests and obeys the guest alignment requirements
>> reported to the host via DM_CAPABILITIES_REPORT message.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> ---
(...)
>> +/* OurRangePlugged */
>> +static OurRangePlugged *our_range_plugged_new(MemoryDeviceState *md,
>> +                                              HostMemoryBackend *hostmem,
>> +                                              uint64_t align,
>> +                                              Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +    OurRangePlugged *our_range;
>> +    MachineState *ms = MACHINE(qdev_get_machine());
>> +    MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
>> +    uint64_t addr, count;
>> +
>> +    if (!align) {
>> +        align = HV_BALLOON_PAGE_SIZE;
>> +    }
>> +
>> +    if (host_memory_backend_is_mapped(hostmem)) {
>> +        error_setg(errp, "memory backend already mapped");
>> +        return NULL;
>> +    }
>> +
>> +    count = memory_device_get_region_size(md, &error_abort) / HV_BALLOON_PAGE_SIZE;
>> +    if (!count) {
>> +        error_setg(errp, "memory backend too small to be usable");
>> +        return NULL;
>> +    }
>> +
>> +    memory_device_pre_plug(md, ms, &align, errp);
>> +    if (*errp) {
>> +        return NULL;
>> +    }
>> +
> 
> This usage is still problematic I suspect (well, and a layer violation regarding the machine). The machine hotplug handler is supposed to call the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug notifications from the core. See how we handle virtio-mem/virtio-pmem/nvdimms as an example.
> 
> We assume that when memory_device_pre_plug() gets called, that the device is not realized yet, but once it gets plugged, that it already is realized, and that the device will actually vanish (get unrealized) when unplugging the device.
> Otherwise memory device logic like in get_plugged_memory_size() stops working.

get_plugged_memory_size() just calls get_plugged_size() method on every
realized TYPE_MEMORY_DEVICE.

While this now always returns the whole backing memory size (once the
backend gets plugged) I don't see a reason why this method could not be
overridden in hv-balloon to return just the currently hot-added size.

By the way, this function seems to be used just for reporting stats via QMP.

> 
> As an example, see device_set_realized() on the pre_plug+realize+plug interaction.
> 
> IIRC, you're reusing the already-realized hv-balloon device here, correct?

Yes - in this version of the driver.

The previous version used separate virtual DIMM devices instead but you have
recommended against that approach.

> 
> Why can't you call the pre_plug/plug/unplug functions from the machine pre_plug/plug/unplug hooks -- exactly once for the memory device when plugging the hv-balloon device?
>
> Is it to support the !memdev case or why is this this plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() required?

At least for three (four) reasons:
1a) At the hv-balloon plug time the device doesn't yet know the guest
alignement requirements - or whether the guest supports memory hot add at
all - that's what the device will learn only once the guest connects
to the protocol.

1b) For the same reason the memory region has to be unplugged at the VM
reset time - the new guest might have stricter alignement requirements.

By the way, the memory region *can't* be unplugged yet at VMBus device
reset time - Windows keeps on using it until the system is restarted,
even after disconnecting from the VMBus.

2) The !memdev case, when the driver is just used for Windows-native
ballooning and stats reporting.

3) This will hopefully allow sharing the backing memory device between
virtio-mem and hv-balloon in the future - Linux guests will connect to
the former interface while Windows guests will connect to the later.

> 
> Supporting the !memdev case is interesting: you essentially want to plug a memory device without a device region (or with an empty stub). I guess we should get that figured out somehow.
> 

That's why the previous version of this driver used a parent VMBus
device (non-TYPE_MEMORY_DEVICE) which then had virtual DIMMs as its
children carrying memory for hot add.

I guess both approaches have its own pros and cons.

Thanks,
Maciej

[...]

Sorry for the late reply!

Still trying to make up my mind what the right way forward with this is.

>>
>> This usage is still problematic I suspect (well, and a layer violation regarding the machine). The machine hotplug handler is supposed to call the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug notifications from the core. See how we handle virtio-mem/virtio-pmem/nvdimms as an example.
>>
>> We assume that when memory_device_pre_plug() gets called, that the device is not realized yet, but once it gets plugged, that it already is realized, and that the device will actually vanish (get unrealized) when unplugging the device.
>> Otherwise memory device logic like in get_plugged_memory_size() stops working.
> 
> get_plugged_memory_size() just calls get_plugged_size() method on every
> realized TYPE_MEMORY_DEVICE.
> 
> While this now always returns the whole backing memory size (once the
> backend gets plugged) I don't see a reason why this method could not be
> overridden in hv-balloon to return just the currently hot-added size.
> 
> By the way, this function seems to be used just for reporting stats via QMP.

memory_device_build_list() is another example, used for 
memory_device_get_free_addr(). You'd be blocking memory address ranges 
with an unplugged-but-realized memory device.

Memory device code expects that realized memory devices are plugged and 
vice versa.

> 
>>
>> As an example, see device_set_realized() on the pre_plug+realize+plug interaction.
>>
>> IIRC, you're reusing the already-realized hv-balloon device here, correct?
> 
> Yes - in this version of the driver.
> 
> The previous version used separate virtual DIMM devices instead but you have
> recommended against that approach.
> 

Yes. My recommendation was to make the hv-balloon device a memory device 
and use a single memory region, which you did (and I think it's much 
better).

It's now all about when we (un)plug the memory device itself -- and how.

>>
>> Why can't you call the pre_plug/plug/unplug functions from the machine pre_plug/plug/unplug hooks -- exactly once for the memory device when plugging the hv-balloon device?
>>
>> Is it to support the !memdev case or why is this this plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() required?
> 
> At least for three (four) reasons:
> 1a) At the hv-balloon plug time the device doesn't yet know the guest
> alignement requirements - or whether the guest supports memory hot add at
> all - that's what the device will learn only once the guest connects
> to the protocol.

Understood, so you want to at least expose the memory dynamically to the 
VM (map the MR on demand).

That could be done using a memory region container like virtio-mem is 
planning [1] on using fairly easily.

[1] https://lkml.kernel.org/r/20230616092654.175518-14-david@redhat.com

> 1b) For the same reason the memory region has to be unplugged at the VM
> reset time - the new guest might have stricter alignement requirements

Alignment is certainly interesting, but is it a real problem?

As default (not other memory devices) you get an address that's aligned 
to 1 GiB. And, in fact, you can simply always request a 1 GiB alignment 
for the device, independent of the guest requirement.

Would the guest requirement be even stricter than that (e.g., 2 GiB)?

In theory, when using a memory region container (again [1]) into which 
you dynamically map the RAM region, you can do this alignment internally.

So it might be an option to use a memory region container and 
dynamically map into that one as you please (it just has to have a fixed 
size).

> 
> By the way, the memory region *can't* be unplugged yet at VMBus device
> reset time - Windows keeps on using it until the system is restarted,
> even after disconnecting from the VMBus.

Yes, similar to virtio-mem -- we can only e.g. do it at system reset time.

> 
> 2) The !memdev case, when the driver is just used for Windows-native
> ballooning and stats reporting.

So we'd want support for a memory device that doesn't expose any memory 
-- in the current configuration. Should be doable (NULL returned as 
device memory region -> skip pre_plug/plug/unplug and teach the other 
code to just ignore this device). It would be easier if we could decide 
at runtime that this device is not a memory device ...

But let's first figure out if that's the right approach.


> 
> 3) This will hopefully allow sharing the backing memory device between
> virtio-mem and hv-balloon in the future - Linux guests will connect to
> the former interface while Windows guests will connect to the later.
> 

I've been told that the virtio-mem driver for Windows will show up 
polished in the near future ... we'll see :)

Anyhow, I consider that a secondary requirement. (virtio-mem is not 
compatible with shared memdevs)


>>
>> Supporting the !memdev case is interesting: you essentially want to plug a memory device without a device region (or with an empty stub). I guess we should get that figured out somehow.
>>
> 
> That's why the previous version of this driver used a parent VMBus
> device (non-TYPE_MEMORY_DEVICE) which then had virtual DIMMs as its
> children carrying memory for hot add.

So we have the following two options I think

(1) Make the hv-balloon device a memory device, plug/unplug it from the 
machine code when plug/unplug'ing the hv-balloon device.

We'd use a memory region container as device memory region (like [1]) 
and would have to handle the !memdev case (I can help with that). Into 
that, you can map the RAM memory region on demand (and eventually even 
using multiple slots like [1]).

(2) Use a single virtual DIMM and (un)plug that on demand. Let the 
machine code handle (un)plugging of the device.


(1) feels cleanest to me, although it will require a bit more work.

On 19.06.2023 17:58, David Hildenbrand wrote:
> [...]
> 
> Sorry for the late reply!
> 
> Still trying to make up my mind what the right way forward with this is.
> 
>>>
>>> This usage is still problematic I suspect (well, and a layer violation regarding the machine). The machine hotplug handler is supposed to call the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug notifications from the core. See how we handle virtio-mem/virtio-pmem/nvdimms as an example.
>>>
>>> We assume that when memory_device_pre_plug() gets called, that the device is not realized yet, but once it gets plugged, that it already is realized, and that the device will actually vanish (get unrealized) when unplugging the device.
>>> Otherwise memory device logic like in get_plugged_memory_size() stops working.
>>
>> get_plugged_memory_size() just calls get_plugged_size() method on every
>> realized TYPE_MEMORY_DEVICE.
>>
>> While this now always returns the whole backing memory size (once the
>> backend gets plugged) I don't see a reason why this method could not be
>> overridden in hv-balloon to return just the currently hot-added size.
>>
>> By the way, this function seems to be used just for reporting stats via QMP.
> 
> memory_device_build_list() is another example, used for memory_device_get_free_addr().

I don't see it calling get_plugged_size() method, I can see it only using
(indirectly) get_addr() method.

> You'd be blocking memory address ranges with an unplugged-but-realized memory device.> 
> Memory device code expects that realized memory devices are plugged and vice versa.

Which QEMU code you mean specifically? Maybe it just needs a trivial
change.

Before the driver hot-adds the first chunk of memory it does not use any
part of the address space.

After that, it has to reserve address space for the whole backing memory
device, so no other devices will claim parts of it and because a
TYPE_MEMORY_DEVICE (currently) can have just a single range.

This address space is released when the VM is restarted.

>>
>>>
>>> As an example, see device_set_realized() on the pre_plug+realize+plug interaction.
>>>
>>> IIRC, you're reusing the already-realized hv-balloon device here, correct?
>>
>> Yes - in this version of the driver.
>>
>> The previous version used separate virtual DIMM devices instead but you have
>> recommended against that approach.
>>
> 
> Yes. My recommendation was to make the hv-balloon device a memory device and use a single memory region, which you did (and I think it's much better).
> 
> It's now all about when we (un)plug the memory device itself -- and how.
> 
>>>
>>> Why can't you call the pre_plug/plug/unplug functions from the machine pre_plug/plug/unplug hooks -- exactly once for the memory device when plugging the hv-balloon device?
>>>
>>> Is it to support the !memdev case or why is this this plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() required?
>>
>> At least for three (four) reasons:
>> 1a) At the hv-balloon plug time the device doesn't yet know the guest
>> alignement requirements - or whether the guest supports memory hot add at
>> all - that's what the device will learn only once the guest connects
>> to the protocol.
> 
> Understood, so you want to at least expose the memory dynamically to the VM (map the MR on demand).
> 
> That could be done using a memory region container like virtio-mem is planning [1] on using fairly easily.
> 
> [1] https://lkml.kernel.org/r/20230616092654.175518-14-david@redhat.com

Thanks for the pointer to your series - I've looked at it and it seems
to me that while it allows multiple memory subregions, each backed by
a separate memslot it still needs a single big main region for
the particular TYPE_MEMORY_DEVICE, am I right?

>> 1b) For the same reason the memory region has to be unplugged at the VM
>> reset time - the new guest might have stricter alignement requirements
> 
> Alignment is certainly interesting, but is it a real problem?
> 
> As default (not other memory devices) you get an address that's aligned to 1 GiB. And, in fact, you can simply always request a 1 GiB alignment for the device, independent of the guest requirement.
> 
> Would the guest requirement be even stricter than that (e.g., 2 GiB)?

The protocol allows up to 32 GiB alignement so we cannot simply
hardcode the alignement to 1 GiB, especially since this is Windows
we're talking about (so this parameter is subject to unpredictable
changes).

> In theory, when using a memory region container (again [1]) into which you dynamically map the RAM region, you can do this alignment internally.
> 
> So it might be an option to use a memory region container and dynamically map into that one as you please (it just has to have a fixed size).

Still, demand-allocating just the right memory region (with the right
alignement) seems to me like a cleaner solution than allocating a huge
worst-case memory region upfront and then trying to carve the right
part of it.

>>
>> By the way, the memory region *can't* be unplugged yet at VMBus device
>> reset time - Windows keeps on using it until the system is restarted,
>> even after disconnecting from the VMBus.
> 
> Yes, similar to virtio-mem -- we can only e.g. do it at system reset time.
> 
>>
>> 2) The !memdev case, when the driver is just used for Windows-native
>> ballooning and stats reporting.
> 
> So we'd want support for a memory device that doesn't expose any memory -- in the current configuration. Should be doable (NULL returned as device memory region -> skip pre_plug/plug/unplug and teach the other code to just ignore this device). It would be easier if we could decide at runtime that this device is not a memory device ...
> 
> But let's first figure out if that's the right approach.
> 
> 
>>
>> 3) This will hopefully allow sharing the backing memory device between
>> virtio-mem and hv-balloon in the future - Linux guests will connect to
>> the former interface while Windows guests will connect to the later.
>>
> 
> I've been told that the virtio-mem driver for Windows will show up polished in the near future ... we'll see :)

virtio-mem driver for Windows is still at version 1 at [1] so I guess
it's going to take a while for it to be production-ready - due to, in part,
difficulty in acquiring stability guarantees for the required Windows
kernel MM APIs.

> Anyhow, I consider that a secondary requirement. (virtio-mem is not compatible with shared memdevs)
> 
> 
>>>
>>> Supporting the !memdev case is interesting: you essentially want to plug a memory device without a device region (or with an empty stub). I guess we should get that figured out somehow.
>>>
>>
>> That's why the previous version of this driver used a parent VMBus
>> device (non-TYPE_MEMORY_DEVICE) which then had virtual DIMMs as its
>> children carrying memory for hot add.
> 
> So we have the following two options I think
> 
> (1) Make the hv-balloon device a memory device, 

It's already a TYPE_MEMORY_DEVICE, so no change required here.

> plug/unplug it from the machine code when plug/unplug'ing the hv-balloon device.

Don't quite follow that - the device is not designed to be hot-pluggable
(I'm not sure that Windows would even allow hot-plugging a DM protocol
device).

It is rather designed to be instantiated at the QEMU command line for the
lifetime of the VM.

> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
> 
> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
> 
> 
> (1) feels cleanest to me, although it will require a bit more work.
> 

I also think approach (1) makes more sense as it avoids memslot metadata
overhead for not-yet-hot-added parts of the memory backing device.

Not sure what you mean that the !memdev case would be problematic in this
case - it is working in the current driver shape so why would adding
potential memory subregions (used in the memdev case) change that?

Thanks,
Maciej

[1]: https://github.com/virtio-win/kvm-guest-drivers-windows/pull/794

On 20.06.23 22:13, Maciej S. Szmigiero wrote:
> On 19.06.2023 17:58, David Hildenbrand wrote:
>> [...]
>>
>> Sorry for the late reply!
>>
>> Still trying to make up my mind what the right way forward with this is.
>>
>>>>
>>>> This usage is still problematic I suspect (well, and a layer violation regarding the machine). The machine hotplug handler is supposed to call the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug notifications from the core. See how we handle virtio-mem/virtio-pmem/nvdimms as an example.
>>>>
>>>> We assume that when memory_device_pre_plug() gets called, that the device is not realized yet, but once it gets plugged, that it already is realized, and that the device will actually vanish (get unrealized) when unplugging the device.
>>>> Otherwise memory device logic like in get_plugged_memory_size() stops working.
>>>
>>> get_plugged_memory_size() just calls get_plugged_size() method on every
>>> realized TYPE_MEMORY_DEVICE.
>>>
>>> While this now always returns the whole backing memory size (once the
>>> backend gets plugged) I don't see a reason why this method could not be
>>> overridden in hv-balloon to return just the currently hot-added size.
>>>
>>> By the way, this function seems to be used just for reporting stats via QMP.
>>
>> memory_device_build_list() is another example, used for memory_device_get_free_addr().
> 
> I don't see it calling get_plugged_size() method, I can see it only using
> (indirectly) get_addr() method.

It similarly contains the TYPE_MEMORY_DEVICE -> dev->realized logic.

> 
>> You'd be blocking memory address ranges with an unplugged-but-realized memory device.>
>> Memory device code expects that realized memory devices are plugged and vice versa.
> 
> Which QEMU code you mean specifically? Maybe it just needs a trivial
> change.
> 
> Before the driver hot-adds the first chunk of memory it does not use any
> part of the address space.
> 
> After that, it has to reserve address space for the whole backing memory
> device, so no other devices will claim parts of it and because a
> TYPE_MEMORY_DEVICE (currently) can have just a single range.
> 
> This address space is released when the VM is restarted.
> 

As I said, memory device code currently expects that you don't have realized
TYPE_MEMORY_DEVICE that are not plugged, and currently that holds true for all
memory devices.

We could modify memory device code to change that, but IMHO it's the wrong way
around: the machine (hotplug) is responsible for (un)plugging memory devices
as they get realized.

Doing a qdev_get_machine()/current_machine from device code and then
modifying the state of the machine (here: calling plug/unplug handlers)
is usually a warning sign that there is a layer violation.

That's why I'm thinking about a cleaner way to handle that.

[...]

>>>> Is it to support the !memdev case or why is this this plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() required?
>>>
>>> At least for three (four) reasons:
>>> 1a) At the hv-balloon plug time the device doesn't yet know the guest
>>> alignement requirements - or whether the guest supports memory hot add at
>>> all - that's what the device will learn only once the guest connects
>>> to the protocol.
>>
>> Understood, so you want to at least expose the memory dynamically to the VM (map the MR on demand).
>>
>> That could be done using a memory region container like virtio-mem is planning [1] on using fairly easily.
>>
>> [1] https://lkml.kernel.org/r/20230616092654.175518-14-david@redhat.com
> 
> Thanks for the pointer to your series - I've looked at it and it seems
> to me that while it allows multiple memory subregions, each backed by
> a separate memslot it still needs a single big main region for
> the particular TYPE_MEMORY_DEVICE, am I right?

Yes.

> 
>>> 1b) For the same reason the memory region has to be unplugged at the VM
>>> reset time - the new guest might have stricter alignement requirements
>>
>> Alignment is certainly interesting, but is it a real problem?
>>
>> As default (not other memory devices) you get an address that's aligned to 1 GiB. And, in fact, you can simply always request a 1 GiB alignment for the device, independent of the guest requirement.
>>
>> Would the guest requirement be even stricter than that (e.g., 2 GiB)?
> 
> The protocol allows up to 32 GiB alignement so we cannot simply
> hardcode the alignement to 1 GiB, especially since this is Windows
> we're talking about (so this parameter is subject to unpredictable
> changes).

Note that anything bigger than 1 GiB is not really guaranteed to work in
QEMU on x86-64. See the warning in memory_device_get_free_addr():

     /* start of address space indicates the maximum alignment we expect */
     if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
         warn_report("the alignment (0x%" PRIx64 ") exceeds the expected"
                     " maximum alignment, memory will get fragmented and not"
                     " all 'maxmem' might be usable for memory devices.",
                     align);
     }


So assume you do a "-m 4G,maxmem=36G"

You cannot add a 32 GiB device with an alignment of 32 GiB -- thee is not
sufficient room in the device region size. To achieve that, you'd have to
increase maxmem to something like:

"-m 4G,maxmem=68G"

to eventually make that work.

So "it's complicated" already to guarantee anything > 1 GiB, even when doing it
from within QEMU.


Thinking about how to handle it with a device memory region container:
With the 32 GiB max alignment, you could similarly always size the device managed
region container effectively sizeof(memdev)+32 GiB, and document that. Adding a
32 GiB device to the "-m 4G,maxmem=36G" configuration would fail right from the
start, instead of later at runtime when you fail to find an aligned address range
in the configuration.

Within that region container, you can then align the address as you want, without
getting warnings about alignment from the core or failing hotplug if the alignment
in the configuration cannot be satisfied.

> 
>> In theory, when using a memory region container (again [1]) into which you dynamically map the RAM region, you can do this alignment internally.
>>
>> So it might be an option to use a memory region container and dynamically map into that one as you please (it just has to have a fixed size).
> 
> Still, demand-allocating just the right memory region (with the right
> alignement) seems to me like a cleaner solution than allocating a huge
> worst-case memory region upfront and then trying to carve the right
> part of it.

See above, this does not work in the general case as you would need it.

[...]

>>> 3) This will hopefully allow sharing the backing memory device between
>>> virtio-mem and hv-balloon in the future - Linux guests will connect to
>>> the former interface while Windows guests will connect to the later.
>>>
>>
>> I've been told that the virtio-mem driver for Windows will show up polished in the near future ... we'll see :)
> 
> virtio-mem driver for Windows is still at version 1 at [1] so I guess
> it's going to take a while for it to be production-ready - due to, in part,
> difficulty in acquiring stability guarantees for the required Windows
> kernel MM APIs.

I heard that there is progress on both, and that it should be upstream soonish. But I'm not
involved in the actual development (and Microsoft discussions), so I can only echo what
people told me.

> 
>> Anyhow, I consider that a secondary requirement. (virtio-mem is not compatible with shared memdevs)
>>
>>
>>>>
>>>> Supporting the !memdev case is interesting: you essentially want to plug a memory device without a device region (or with an empty stub). I guess we should get that figured out somehow.
>>>>
>>>
>>> That's why the previous version of this driver used a parent VMBus
>>> device (non-TYPE_MEMORY_DEVICE) which then had virtual DIMMs as its
>>> children carrying memory for hot add.
>>
>> So we have the following two options I think
>>
>> (1) Make the hv-balloon device a memory device,
> 
> It's already a TYPE_MEMORY_DEVICE, so no change required here.
> 
>> plug/unplug it from the machine code when plug/unplug'ing the hv-balloon device.
> 
> Don't quite follow that - the device is not designed to be hot-pluggable
> (I'm not sure that Windows would even allow hot-plugging a DM protocol
> device).

There is coldplug and hotplug. Coldplug is just specifying a device on the QEMU
cmdline and letting the (hot)plug handler wire it up correctly.

> 
> It is rather designed to be instantiated at the QEMU command line for the
> lifetime of the VM.
> 

Right, and I assume it cannot get unplugged.

>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>
>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>
>>
>> (1) feels cleanest to me, although it will require a bit more work.
>>
> 
> I also think approach (1) makes more sense as it avoids memslot metadata
> overhead for not-yet-hot-added parts of the memory backing device.
> 
> Not sure what you mean that the !memdev case would be problematic in this
> case - it is working in the current driver shape so why would adding
> potential memory subregions (used in the memdev case) change that?

I'm thinking about the case where you have a hv-balloon device without a memdev.

Without -m X,maxmem=y we don't currently expect to have memory devices around
(and especially them getting (un)plugged. But why should we "force" to set the
"maxmem" option


I hope I'll find some time soonish to prototype what I have in mind, to see
if it could be made working.

On 21.06.2023 12:32, David Hildenbrand wrote:
> On 20.06.23 22:13, Maciej S. Szmigiero wrote:
>> On 19.06.2023 17:58, David Hildenbrand wrote:
>>> [...]
>>>
>>> Sorry for the late reply!
>>>
>>> Still trying to make up my mind what the right way forward with this is.
>>>
>>>>>
>>>>> This usage is still problematic I suspect (well, and a layer violation regarding the machine). The machine hotplug handler is supposed to call the pre_plug/plug/unplug hooks as response to pre_plug/plug/unplug notifications from the core. See how we handle virtio-mem/virtio-pmem/nvdimms as an example.
>>>>>
>>>>> We assume that when memory_device_pre_plug() gets called, that the device is not realized yet, but once it gets plugged, that it already is realized, and that the device will actually vanish (get unrealized) when unplugging the device.
>>>>> Otherwise memory device logic like in get_plugged_memory_size() stops working.
>>>>
>>>> get_plugged_memory_size() just calls get_plugged_size() method on every
>>>> realized TYPE_MEMORY_DEVICE.
>>>>
>>>> While this now always returns the whole backing memory size (once the
>>>> backend gets plugged) I don't see a reason why this method could not be
>>>> overridden in hv-balloon to return just the currently hot-added size.
>>>>
>>>> By the way, this function seems to be used just for reporting stats via QMP.
>>>
>>> memory_device_build_list() is another example, used for memory_device_get_free_addr().
>>
>> I don't see it calling get_plugged_size() method, I can see it only using
>> (indirectly) get_addr() method.
> 
> It similarly contains the TYPE_MEMORY_DEVICE -> dev->realized logic.

All right, I thought at first you meant just that the get_plugged_memory_size()
function reports misleading values.

> 
>>
>>> You'd be blocking memory address ranges with an unplugged-but-realized memory device.>
>>> Memory device code expects that realized memory devices are plugged and vice versa.
>>
>> Which QEMU code you mean specifically? Maybe it just needs a trivial
>> change.
>>
>> Before the driver hot-adds the first chunk of memory it does not use any
>> part of the address space.
>>
>> After that, it has to reserve address space for the whole backing memory
>> device, so no other devices will claim parts of it and because a
>> TYPE_MEMORY_DEVICE (currently) can have just a single range.
>>
>> This address space is released when the VM is restarted.
>>
> 
> As I said, memory device code currently expects that you don't have realized
> TYPE_MEMORY_DEVICE that are not plugged, and currently that holds true for all
> memory devices.
> 
> We could modify memory device code to change that, but IMHO it's the wrong way
> around: the machine (hotplug) is responsible for (un)plugging memory devices
> as they get realized.
> 
> Doing a qdev_get_machine()/current_machine from device code and then
> modifying the state of the machine (here: calling plug/unplug handlers)
> is usually a warning sign that there is a layer violation.
> 
> That's why I'm thinking about a cleaner way to handle that.

Okay, now I think I understand what you think is questionable:
calling memory_device_pre_plug(), memory_device_plug() and friends from
the driver when hot-adding the first memory chunk, even thought no actual
device is getting plugged in at that time.

I'm open to other approaches here (besides the virtual DIMMs one that we
already tried in the past).

> [...]
> 
>>>>> Is it to support the !memdev case or why is this this plugging/unplugging in our_range_plugged_new()/our_range_plugged_free() required?
>>>>
>>>> At least for three (four) reasons:
>>>> 1a) At the hv-balloon plug time the device doesn't yet know the guest
>>>> alignement requirements - or whether the guest supports memory hot add at
>>>> all - that's what the device will learn only once the guest connects
>>>> to the protocol.
>>>
>>> Understood, so you want to at least expose the memory dynamically to the VM (map the MR on demand).
>>>
>>> That could be done using a memory region container like virtio-mem is planning [1] on using fairly easily.
>>>
>>> [1] https://lkml.kernel.org/r/20230616092654.175518-14-david@redhat.com
>>
>> Thanks for the pointer to your series - I've looked at it and it seems
>> to me that while it allows multiple memory subregions, each backed by
>> a separate memslot it still needs a single big main region for
>> the particular TYPE_MEMORY_DEVICE, am I right?
> 
> Yes.
> 
>>
>>>> 1b) For the same reason the memory region has to be unplugged at the VM
>>>> reset time - the new guest might have stricter alignement requirements
>>>
>>> Alignment is certainly interesting, but is it a real problem?
>>>
>>> As default (not other memory devices) you get an address that's aligned to 1 GiB. And, in fact, you can simply always request a 1 GiB alignment for the device, independent of the guest requirement.
>>>
>>> Would the guest requirement be even stricter than that (e.g., 2 GiB)?
>>
>> The protocol allows up to 32 GiB alignement so we cannot simply
>> hardcode the alignement to 1 GiB, especially since this is Windows
>> we're talking about (so this parameter is subject to unpredictable
>> changes).
> 
> Note that anything bigger than 1 GiB is not really guaranteed to work in
> QEMU on x86-64. See the warning in memory_device_get_free_addr():
> 
>      /* start of address space indicates the maximum alignment we expect */
>      if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
>          warn_report("the alignment (0x%" PRIx64 ") exceeds the expected"
>                      " maximum alignment, memory will get fragmented and not"
>                      " all 'maxmem' might be usable for memory devices.",
>                      align);
>      }
> 
> 
> So assume you do a "-m 4G,maxmem=36G"
> 
> You cannot add a 32 GiB device with an alignment of 32 GiB -- thee is not
> sufficient room in the device region size. To achieve that, you'd have to
> increase maxmem to something like:
> 
> "-m 4G,maxmem=68G"
> 
> to eventually make that work.
> 
> So "it's complicated" already to guarantee anything > 1 GiB, even when doing it
> from within QEMU.
> 
> 
> Thinking about how to handle it with a device memory region container:
> With the 32 GiB max alignment, you could similarly always size the device managed
> region container effectively sizeof(memdev)+32 GiB, and document that. Adding a
> 32 GiB device to the "-m 4G,maxmem=36G" configuration would fail right from the
> start, instead of later at runtime when you fail to find an aligned address range
> in the configuration.
> 
> Within that region container, you can then align the address as you want, without
> getting warnings about alignment from the core or failing hotplug if the alignment
> in the configuration cannot be satisfied.

Won't using a bigger container also require up-sizing the backing memory
device?

In other words, is it possible to attach the backing memory device at
non-zero offset in the container only at runtime, when the required
alignement becomes known?
If such offset attachment is indeed possible, then I don't see a problem
with using such container.

In the current driver form, with the effective requirement of
maxmem=32G+sizeof(memdev), we're just wasting address space, not parts of
the actual backing memory device.

>>
>>> In theory, when using a memory region container (again [1]) into which you dynamically map the RAM region, you can do this alignment internally.
>>>
>>> So it might be an option to use a memory region container and dynamically map into that one as you please (it just has to have a fixed size).
>>
>> Still, demand-allocating just the right memory region (with the right
>> alignement) seems to me like a cleaner solution than allocating a huge
>> worst-case memory region upfront and then trying to carve the right
>> part of it.
> 
> See above, this does not work in the general case as you would need it.
> 
> [...]
> 
>>>> 3) This will hopefully allow sharing the backing memory device between
>>>> virtio-mem and hv-balloon in the future - Linux guests will connect to
>>>> the former interface while Windows guests will connect to the later.
>>>>
>>>
>>> I've been told that the virtio-mem driver for Windows will show up polished in the near future ... we'll see :)
>>
>> virtio-mem driver for Windows is still at version 1 at [1] so I guess
>> it's going to take a while for it to be production-ready - due to, in part,
>> difficulty in acquiring stability guarantees for the required Windows
>> kernel MM APIs.
> 
> I heard that there is progress on both, and that it should be upstream soonish. But I'm not
> involved in the actual development (and Microsoft discussions), so I can only echo what
> people told me.

As you said, we'll see - competition is generally a good thing (if kept
within reason).

>>
>>> Anyhow, I consider that a secondary requirement. (virtio-mem is not compatible with shared memdevs)
>>>
>>>
>>>>>
>>>>> Supporting the !memdev case is interesting: you essentially want to plug a memory device without a device region (or with an empty stub). I guess we should get that figured out somehow.
>>>>>
>>>>
>>>> That's why the previous version of this driver used a parent VMBus
>>>> device (non-TYPE_MEMORY_DEVICE) which then had virtual DIMMs as its
>>>> children carrying memory for hot add.
>>>
>>> So we have the following two options I think
>>>
>>> (1) Make the hv-balloon device a memory device,
>>
>> It's already a TYPE_MEMORY_DEVICE, so no change required here.
>>
>>> plug/unplug it from the machine code when plug/unplug'ing the hv-balloon device.
>>
>> Don't quite follow that - the device is not designed to be hot-pluggable
>> (I'm not sure that Windows would even allow hot-plugging a DM protocol
>> device).
> 
> There is coldplug and hotplug. Coldplug is just specifying a device on the QEMU
> cmdline and letting the (hot)plug handler wire it up correctly.

So we'll only have to support the coldplug case for the hv-balloon driver
itself.

On the other hand, previous versions of the driver used to hot-plug
virtual DIMM devices.

>>
>> It is rather designed to be instantiated at the QEMU command line for the
>> lifetime of the VM.
>>
> 
> Right, and I assume it cannot get unplugged.

That's right.

>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>
>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>
>>>
>>> (1) feels cleanest to me, although it will require a bit more work.
>>>
>>
>> I also think approach (1) makes more sense as it avoids memslot metadata
>> overhead for not-yet-hot-added parts of the memory backing device.
>>
>> Not sure what you mean that the !memdev case would be problematic in this
>> case - it is working in the current driver shape so why would adding
>> potential memory subregions (used in the memdev case) change that?
> 
> I'm thinking about the case where you have a hv-balloon device without a memdev.
> 
> Without -m X,maxmem=y we don't currently expect to have memory devices around
> (and especially them getting (un)plugged. But why should we "force" to set the
> "maxmem" option

I guess it's only a small change to QEMU to allow having hv-balloon
device (without a memdev) even in the case where there's no "maxmem"
option given on the QEMU command line.

> 
> I hope I'll find some time soonish to prototype what I have in mind, to see
> if it could be made working.
> 

Okay, so I'll wait for your prototype before commencing further work on
the next version of this driver.

Thanks,
Maciej

[...]

>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>
>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>
>>>>
>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>
>>>
>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>
>>> Not sure what you mean that the !memdev case would be problematic in this
>>> case - it is working in the current driver shape so why would adding
>>> potential memory subregions (used in the memdev case) change that?
>>
>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>
>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>> (and especially them getting (un)plugged. But why should we "force" to set the
>> "maxmem" option
> 
> I guess it's only a small change to QEMU to allow having hv-balloon
> device (without a memdev) even in the case where there's no "maxmem"
> option given on the QEMU command line.
> 
>>
>> I hope I'll find some time soonish to prototype what I have in mind, to see
>> if it could be made working.
>>
> 
> Okay, so I'll wait for your prototype before commencing further work on
> the next version of this driver.

About to have something simplistic running -- I think. Want to test with 
a Linux VM, but I don't seem to get it working (also without my changes).


#!/bin/bash

build/qemu-system-x86_64 \
     --enable-kvm \
     -m 4G,maxmem=36G \
     -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
     -smp 16 \
     -nographic \
     -nodefaults \
     -net nic -net user \
     -chardev stdio,nosignal,id=serial \
     -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
     -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
     -device isa-serial,chardev=serial \
     -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
     -mon chardev=monitor,mode=readline \
     -device vmbus-bridge \
     -object memory-backend-ram,size=2G,id=mem0 \
     -device hv-balloon,id=hv1,memdev=mem0



[root@vm-0 ~]# uname -r
6.3.5-100.fc37.x86_64
[root@vm-0 ~]# modprobe hv_balloon
modprobe: ERROR: could not insert 'hv_balloon': No such device


Any magic flag I am missing? Or is there something preventing this to 
work with Linux VMs?

On 22.06.2023 13:01, David Hildenbrand wrote:
> [...]
> 
>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>
>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>
>>>>>
>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>
>>>>
>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>
>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>> case - it is working in the current driver shape so why would adding
>>>> potential memory subregions (used in the memdev case) change that?
>>>
>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>
>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>> "maxmem" option
>>
>> I guess it's only a small change to QEMU to allow having hv-balloon
>> device (without a memdev) even in the case where there's no "maxmem"
>> option given on the QEMU command line.
>>
>>>
>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>> if it could be made working.
>>>
>>
>> Okay, so I'll wait for your prototype before commencing further work on
>> the next version of this driver.
> 
> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
> 
> 
> #!/bin/bash
> 
> build/qemu-system-x86_64 \
>      --enable-kvm \
>      -m 4G,maxmem=36G \
>      -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>      -smp 16 \
>      -nographic \
>      -nodefaults \
>      -net nic -net user \
>      -chardev stdio,nosignal,id=serial \
>      -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>      -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>      -device isa-serial,chardev=serial \
>      -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>      -mon chardev=monitor,mode=readline \
>      -device vmbus-bridge \
>      -object memory-backend-ram,size=2G,id=mem0 \
>      -device hv-balloon,id=hv1,memdev=mem0
> 
> 
> 
> [root@vm-0 ~]# uname -r
> 6.3.5-100.fc37.x86_64
> [root@vm-0 ~]# modprobe hv_balloon
> modprobe: ERROR: could not insert 'hv_balloon': No such device
> 
> 
> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
> 

Haven't tested the driver with Linux guests in a long time (as it is
targeting Windows), but I think you need to disable KVM PV interface for
the Hyper-V one to be detected by Linux.

Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
the detected hypervisor is now Hyper-V.

Also, you need to disable S4 in the guest for hot-add capability to work
(I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).

Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
to QEMU command line to see what's happening.

Thanks,
Maciej

On 22.06.23 13:12, Maciej S. Szmigiero wrote:
> On 22.06.2023 13:01, David Hildenbrand wrote:
>> [...]
>>
>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>
>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>
>>>>>>
>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>
>>>>>
>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>
>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>> case - it is working in the current driver shape so why would adding
>>>>> potential memory subregions (used in the memdev case) change that?
>>>>
>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>
>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>> "maxmem" option
>>>
>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>> device (without a memdev) even in the case where there's no "maxmem"
>>> option given on the QEMU command line.
>>>
>>>>
>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>> if it could be made working.
>>>>
>>>
>>> Okay, so I'll wait for your prototype before commencing further work on
>>> the next version of this driver.
>>
>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>
>>
>> #!/bin/bash
>>
>> build/qemu-system-x86_64 \
>>       --enable-kvm \
>>       -m 4G,maxmem=36G \
>>       -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>       -smp 16 \
>>       -nographic \
>>       -nodefaults \
>>       -net nic -net user \
>>       -chardev stdio,nosignal,id=serial \
>>       -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>       -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>       -device isa-serial,chardev=serial \
>>       -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>       -mon chardev=monitor,mode=readline \
>>       -device vmbus-bridge \
>>       -object memory-backend-ram,size=2G,id=mem0 \
>>       -device hv-balloon,id=hv1,memdev=mem0
>>
>>
>>
>> [root@vm-0 ~]# uname -r
>> 6.3.5-100.fc37.x86_64
>> [root@vm-0 ~]# modprobe hv_balloon
>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>
>>
>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>
> 
> Haven't tested the driver with Linux guests in a long time (as it is
> targeting Windows), but I think you need to disable KVM PV interface for
> the Hyper-V one to be detected by Linux.
> 
> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
> the detected hypervisor is now Hyper-V.
> 
> Also, you need to disable S4 in the guest for hot-add capability to work
> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
> 
> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
> to QEMU command line to see what's happening.

VM is not happy:

[    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
[    1.908837] #PF: supervisor read access in kernel mode
[    1.908837] #PF: error_code(0x0000) - not-present page
[    1.908837] PGD 0 P4D 0
[    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
[    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
[    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
[    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
[    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
[    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
[    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
[    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
[    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
[    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
[    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
[    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
[    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
[    1.908837] PKRU: 55555554
[    1.908837] Call Trace:
[    1.908837]  <TASK>
[    1.908837]  ? __die+0x23/0x70
[    1.908837]  ? page_fault_oops+0x171/0x4e0
[    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
[    1.908837]  ? exc_page_fault+0x74/0x170
[    1.908837]  ? asm_exc_page_fault+0x26/0x30
[    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
[    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
[    1.908837]  ? down_timeout+0x3e/0x60
[    1.908837]  ? acpi_ns_get_node+0x3e/0x60
[    1.908837]  acpi_ns_get_node+0x3e/0x60
[    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
[    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
[    1.908837]  acpi_rs_get_method_data+0x37/0x80
[    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
[    1.908837]  acpi_walk_resources+0x91/0xe0
[    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
[    1.908837]  acpi_device_probe+0x47/0x160
[    1.908837]  really_probe+0x19f/0x400
[    1.908837]  ? __pfx___driver_attach+0x10/0x10
[    1.908837]  __driver_probe_device+0x78/0x160
[    1.908837]  driver_probe_device+0x1f/0x90
[    1.908837]  __driver_attach+0xd2/0x1c0
[    1.908837]  bus_for_each_dev+0x85/0xd0
[    1.908837]  bus_add_driver+0x116/0x220
[    1.908837]  driver_register+0x59/0x100
[    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
[    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
[    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
[    1.908837]  do_one_initcall+0x5a/0x240
[    1.908837]  do_init_module+0x4a/0x210
[    1.908837]  __do_sys_init_module+0x17f/0x1b0
[    1.908837]  do_syscall_64+0x5c/0x90
[    1.908837]  ? handle_mm_fault+0x11e/0x310
[    1.908837]  ? do_user_addr_fault+0x1e0/0x720
[    1.908837]  ? exc_page_fault+0x74/0x170
[    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc


Maybe I'll have to install a Windows guest :/

On 22.06.2023 13:15, David Hildenbrand wrote:
> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>> [...]
>>>
>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>
>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>
>>>>>>>
>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>
>>>>>>
>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>
>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>> case - it is working in the current driver shape so why would adding
>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>
>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>
>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>> "maxmem" option
>>>>
>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>> option given on the QEMU command line.
>>>>
>>>>>
>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>> if it could be made working.
>>>>>
>>>>
>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>> the next version of this driver.
>>>
>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>
>>>
>>> #!/bin/bash
>>>
>>> build/qemu-system-x86_64 \
>>>       --enable-kvm \
>>>       -m 4G,maxmem=36G \
>>>       -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>       -smp 16 \
>>>       -nographic \
>>>       -nodefaults \
>>>       -net nic -net user \
>>>       -chardev stdio,nosignal,id=serial \
>>>       -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>       -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>       -device isa-serial,chardev=serial \
>>>       -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>       -mon chardev=monitor,mode=readline \
>>>       -device vmbus-bridge \
>>>       -object memory-backend-ram,size=2G,id=mem0 \
>>>       -device hv-balloon,id=hv1,memdev=mem0
>>>
>>>
>>>
>>> [root@vm-0 ~]# uname -r
>>> 6.3.5-100.fc37.x86_64
>>> [root@vm-0 ~]# modprobe hv_balloon
>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>
>>>
>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>
>>
>> Haven't tested the driver with Linux guests in a long time (as it is
>> targeting Windows), but I think you need to disable KVM PV interface for
>> the Hyper-V one to be detected by Linux.
>>
>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>> the detected hypervisor is now Hyper-V.
>>
>> Also, you need to disable S4 in the guest for hot-add capability to work
>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>
>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>> to QEMU command line to see what's happening.
> 
> VM is not happy:
> 
> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
> [    1.908837] #PF: supervisor read access in kernel mode
> [    1.908837] #PF: error_code(0x0000) - not-present page
> [    1.908837] PGD 0 P4D 0
> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
> [    1.908837] PKRU: 55555554
> [    1.908837] Call Trace:
> [    1.908837]  <TASK>
> [    1.908837]  ? __die+0x23/0x70
> [    1.908837]  ? page_fault_oops+0x171/0x4e0
> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
> [    1.908837]  ? exc_page_fault+0x74/0x170
> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
> [    1.908837]  ? down_timeout+0x3e/0x60
> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
> [    1.908837]  acpi_ns_get_node+0x3e/0x60
> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
> [    1.908837]  acpi_walk_resources+0x91/0xe0
> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
> [    1.908837]  acpi_device_probe+0x47/0x160
> [    1.908837]  really_probe+0x19f/0x400
> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
> [    1.908837]  __driver_probe_device+0x78/0x160
> [    1.908837]  driver_probe_device+0x1f/0x90
> [    1.908837]  __driver_attach+0xd2/0x1c0
> [    1.908837]  bus_for_each_dev+0x85/0xd0
> [    1.908837]  bus_add_driver+0x116/0x220
> [    1.908837]  driver_register+0x59/0x100
> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
> [    1.908837]  do_one_initcall+0x5a/0x240
> [    1.908837]  do_init_module+0x4a/0x210
> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
> [    1.908837]  do_syscall_64+0x5c/0x90
> [    1.908837]  ? handle_mm_fault+0x11e/0x310
> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
> [    1.908837]  ? exc_page_fault+0x74/0x170
> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 

I guess *few* people run Linux with QEMU Hyper-V interfaces
implementation..
  
> Maybe I'll have to install a Windows guest :/
> 
I think that makes more sense, since we're targeting Windows anyway.

Thanks,
Maciej

On 22.06.23 13:17, Maciej S. Szmigiero wrote:
> On 22.06.2023 13:15, David Hildenbrand wrote:
>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>> [...]
>>>>
>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>
>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>
>>>>>>>>
>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>
>>>>>>>
>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>
>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>
>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>
>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>> "maxmem" option
>>>>>
>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>> option given on the QEMU command line.
>>>>>
>>>>>>
>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>> if it could be made working.
>>>>>>
>>>>>
>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>> the next version of this driver.
>>>>
>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>
>>>>
>>>> #!/bin/bash
>>>>
>>>> build/qemu-system-x86_64 \
>>>>        --enable-kvm \
>>>>        -m 4G,maxmem=36G \
>>>>        -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>        -smp 16 \
>>>>        -nographic \
>>>>        -nodefaults \
>>>>        -net nic -net user \
>>>>        -chardev stdio,nosignal,id=serial \
>>>>        -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>        -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>        -device isa-serial,chardev=serial \
>>>>        -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>        -mon chardev=monitor,mode=readline \
>>>>        -device vmbus-bridge \
>>>>        -object memory-backend-ram,size=2G,id=mem0 \
>>>>        -device hv-balloon,id=hv1,memdev=mem0
>>>>
>>>>
>>>>
>>>> [root@vm-0 ~]# uname -r
>>>> 6.3.5-100.fc37.x86_64
>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>
>>>>
>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>
>>>
>>> Haven't tested the driver with Linux guests in a long time (as it is
>>> targeting Windows), but I think you need to disable KVM PV interface for
>>> the Hyper-V one to be detected by Linux.
>>>
>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>> the detected hypervisor is now Hyper-V.
>>>
>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>
>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>> to QEMU command line to see what's happening.
>>
>> VM is not happy:
>>
>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>> [    1.908837] #PF: supervisor read access in kernel mode
>> [    1.908837] #PF: error_code(0x0000) - not-present page
>> [    1.908837] PGD 0 P4D 0
>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>> [    1.908837] PKRU: 55555554
>> [    1.908837] Call Trace:
>> [    1.908837]  <TASK>
>> [    1.908837]  ? __die+0x23/0x70
>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>> [    1.908837]  ? exc_page_fault+0x74/0x170
>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>> [    1.908837]  ? down_timeout+0x3e/0x60
>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>> [    1.908837]  acpi_device_probe+0x47/0x160
>> [    1.908837]  really_probe+0x19f/0x400
>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>> [    1.908837]  __driver_probe_device+0x78/0x160
>> [    1.908837]  driver_probe_device+0x1f/0x90
>> [    1.908837]  __driver_attach+0xd2/0x1c0
>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>> [    1.908837]  bus_add_driver+0x116/0x220
>> [    1.908837]  driver_register+0x59/0x100
>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>> [    1.908837]  do_one_initcall+0x5a/0x240
>> [    1.908837]  do_init_module+0x4a/0x210
>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>> [    1.908837]  do_syscall_64+0x5c/0x90
>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>> [    1.908837]  ? exc_page_fault+0x74/0x170
>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>
> 
> I guess *few* people run Linux with QEMU Hyper-V interfaces
> implementation..
>    
>> Maybe I'll have to install a Windows guest :/
>>
> I think that makes more sense, since we're targeting Windows anyway.
> 

Having installed fairly recent Win10 and running with master+your 
patches, I still can't get it to work.

Windows is stuck booting (before the little circle starts turning).

Removing the hv-balloon device makes it work again (well, at least the 
circle spins again my windows installation now seems to be broken and I 
have to reinstall ... windows).

Do you have a working cmdline for Windows I can try?

On 22.06.2023 14:06, David Hildenbrand wrote:
> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>> [...]
>>>>>
>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>
>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>
>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>
>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>
>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>> "maxmem" option
>>>>>>
>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>> option given on the QEMU command line.
>>>>>>
>>>>>>>
>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>> if it could be made working.
>>>>>>>
>>>>>>
>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>> the next version of this driver.
>>>>>
>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>
>>>>>
>>>>> #!/bin/bash
>>>>>
>>>>> build/qemu-system-x86_64 \
>>>>>        --enable-kvm \
>>>>>        -m 4G,maxmem=36G \
>>>>>        -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>        -smp 16 \
>>>>>        -nographic \
>>>>>        -nodefaults \
>>>>>        -net nic -net user \
>>>>>        -chardev stdio,nosignal,id=serial \
>>>>>        -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>        -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>        -device isa-serial,chardev=serial \
>>>>>        -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>        -mon chardev=monitor,mode=readline \
>>>>>        -device vmbus-bridge \
>>>>>        -object memory-backend-ram,size=2G,id=mem0 \
>>>>>        -device hv-balloon,id=hv1,memdev=mem0
>>>>>
>>>>>
>>>>>
>>>>> [root@vm-0 ~]# uname -r
>>>>> 6.3.5-100.fc37.x86_64
>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>
>>>>>
>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>
>>>>
>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>> the Hyper-V one to be detected by Linux.
>>>>
>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>> the detected hypervisor is now Hyper-V.
>>>>
>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>
>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>> to QEMU command line to see what's happening.
>>>
>>> VM is not happy:
>>>
>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>> [    1.908837] #PF: supervisor read access in kernel mode
>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>> [    1.908837] PGD 0 P4D 0
>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>> [    1.908837] PKRU: 55555554
>>> [    1.908837] Call Trace:
>>> [    1.908837]  <TASK>
>>> [    1.908837]  ? __die+0x23/0x70
>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>> [    1.908837]  really_probe+0x19f/0x400
>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>> [    1.908837]  bus_add_driver+0x116/0x220
>>> [    1.908837]  driver_register+0x59/0x100
>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>> [    1.908837]  do_init_module+0x4a/0x210
>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>
>>
>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>> implementation..
>>> Maybe I'll have to install a Windows guest :/
>>>
>> I think that makes more sense, since we're targeting Windows anyway.
>>
> 
> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
> 
> Windows is stuck booting (before the little circle starts turning).
> 
> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
> 
> Do you have a working cmdline for Windows I can try?
> 

Do you get any tracing output from the driver when it is loaded or does its mere presence
make your Windows installation fail to boot?

I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
			-cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-vpindex,hv-runtime,hv-synic,hv-stimer
			-smp sockets=1,cores=4,threads=2 -m 4G,slots=2,maxmem=1T -nodefaults -monitor stdio -d guest_errors \
			-rtc base=localtime,driftfix=slew -global kvm-pit.lost_tick_policy=discard \
	  		-device VGA,id=video0 -vnc :0 -usb -device usb-tablet,id=usb-tablet \
			-drive format="raw",file="${SYSIMG}",cache=writeback,if=none,id=disk-sys \
			-device virtio-blk-pci,drive=disk-sys,id=disk-sys-pci,bootindex=1 \
			-netdev user,id=user-net,restrict=on \
			-device virtio-net-pci,id=nic,netdev=user-net \
			-global ICH9-LPC.disable_s4=1 -qmp tcp:localhost:4444,server,nowait" \
			-snapshot -device vmbus-bridge,id=vmbus-bridge -object memory-backend-ram,id=mem1,size=32G \
			-device hv-balloon,id=hvb,memdev=mem1,status-report=on --trace 'hv_balloon_*' --trace 'memory_device_*'

Some of these might not be actually required since I was using that VM for other tests too.

Thanks,
Maciej

On 22.06.2023 14:14, Maciej S. Szmigiero wrote:
> On 22.06.2023 14:06, David Hildenbrand wrote:
>> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>>> [...]
>>>>>>
>>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>>
>>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>>
>>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>>
>>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>>
>>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>>> "maxmem" option
>>>>>>>
>>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>>> option given on the QEMU command line.
>>>>>>>
>>>>>>>>
>>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>>> if it could be made working.
>>>>>>>>
>>>>>>>
>>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>>> the next version of this driver.
>>>>>>
>>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>>
>>>>>>
>>>>>> #!/bin/bash
>>>>>>
>>>>>> build/qemu-system-x86_64 \
>>>>>>        --enable-kvm \
>>>>>>        -m 4G,maxmem=36G \
>>>>>>        -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>>        -smp 16 \
>>>>>>        -nographic \
>>>>>>        -nodefaults \
>>>>>>        -net nic -net user \
>>>>>>        -chardev stdio,nosignal,id=serial \
>>>>>>        -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>>        -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>>        -device isa-serial,chardev=serial \
>>>>>>        -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>>        -mon chardev=monitor,mode=readline \
>>>>>>        -device vmbus-bridge \
>>>>>>        -object memory-backend-ram,size=2G,id=mem0 \
>>>>>>        -device hv-balloon,id=hv1,memdev=mem0
>>>>>>
>>>>>>
>>>>>>
>>>>>> [root@vm-0 ~]# uname -r
>>>>>> 6.3.5-100.fc37.x86_64
>>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>>
>>>>>>
>>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>>
>>>>>
>>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>>> the Hyper-V one to be detected by Linux.
>>>>>
>>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>>> the detected hypervisor is now Hyper-V.
>>>>>
>>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>>
>>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>>> to QEMU command line to see what's happening.
>>>>
>>>> VM is not happy:
>>>>
>>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>>> [    1.908837] #PF: supervisor read access in kernel mode
>>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>>> [    1.908837] PGD 0 P4D 0
>>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>>> [    1.908837] PKRU: 55555554
>>>> [    1.908837] Call Trace:
>>>> [    1.908837]  <TASK>
>>>> [    1.908837]  ? __die+0x23/0x70
>>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>>> [    1.908837]  really_probe+0x19f/0x400
>>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>>> [    1.908837]  bus_add_driver+0x116/0x220
>>>> [    1.908837]  driver_register+0x59/0x100
>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>>> [    1.908837]  do_init_module+0x4a/0x210
>>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>>
>>>
>>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>>> implementation..
>>>> Maybe I'll have to install a Windows guest :/
>>>>
>>> I think that makes more sense, since we're targeting Windows anyway.
>>>
>>
>> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
>>
>> Windows is stuck booting (before the little circle starts turning).
>>
>> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
>>
>> Do you have a working cmdline for Windows I can try?
>>
> 
> Do you get any tracing output from the driver when it is loaded or does its mere presence
> make your Windows installation fail to boot?
> 
> I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
>              -cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-vpindex,hv-runtime,hv-synic,hv-stimer
>              -smp sockets=1,cores=4,threads=2 -m 4G,slots=2,maxmem=1T -nodefaults -monitor stdio -d guest_errors \
>              -rtc base=localtime,driftfix=slew -global kvm-pit.lost_tick_policy=discard \
>                -device VGA,id=video0 -vnc :0 -usb -device usb-tablet,id=usb-tablet \
>              -drive format="raw",file="${SYSIMG}",cache=writeback,if=none,id=disk-sys \
>              -device virtio-blk-pci,drive=disk-sys,id=disk-sys-pci,bootindex=1 \
>              -netdev user,id=user-net,restrict=on \
>              -device virtio-net-pci,id=nic,netdev=user-net \
>              -global ICH9-LPC.disable_s4=1 -qmp tcp:localhost:4444,server,nowait" \
>              -snapshot -device vmbus-bridge,id=vmbus-bridge -object memory-backend-ram,id=mem1,size=32G \
>              -device hv-balloon,id=hvb,memdev=mem1,status-report=on --trace 'hv_balloon_*' --trace 'memory_device_*'
> 
> Some of these might not be actually required since I was using that VM for other tests too.
> 

Also - the first time Windows boots with VMBus enabled it has to install the necessary drivers,
so if your VM doesn't boot now even without hv-balloon maybe something bad happened during
that installation.

Thanks,
Maciej

On 22.06.23 14:14, Maciej S. Szmigiero wrote:
> On 22.06.2023 14:06, David Hildenbrand wrote:
>> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>>> [...]
>>>>>>
>>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>>
>>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>>
>>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>>
>>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>>
>>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>>> "maxmem" option
>>>>>>>
>>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>>> option given on the QEMU command line.
>>>>>>>
>>>>>>>>
>>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>>> if it could be made working.
>>>>>>>>
>>>>>>>
>>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>>> the next version of this driver.
>>>>>>
>>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>>
>>>>>>
>>>>>> #!/bin/bash
>>>>>>
>>>>>> build/qemu-system-x86_64 \
>>>>>>         --enable-kvm \
>>>>>>         -m 4G,maxmem=36G \
>>>>>>         -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>>         -smp 16 \
>>>>>>         -nographic \
>>>>>>         -nodefaults \
>>>>>>         -net nic -net user \
>>>>>>         -chardev stdio,nosignal,id=serial \
>>>>>>         -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>>         -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>>         -device isa-serial,chardev=serial \
>>>>>>         -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>>         -mon chardev=monitor,mode=readline \
>>>>>>         -device vmbus-bridge \
>>>>>>         -object memory-backend-ram,size=2G,id=mem0 \
>>>>>>         -device hv-balloon,id=hv1,memdev=mem0
>>>>>>
>>>>>>
>>>>>>
>>>>>> [root@vm-0 ~]# uname -r
>>>>>> 6.3.5-100.fc37.x86_64
>>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>>
>>>>>>
>>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>>
>>>>>
>>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>>> the Hyper-V one to be detected by Linux.
>>>>>
>>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>>> the detected hypervisor is now Hyper-V.
>>>>>
>>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>>
>>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>>> to QEMU command line to see what's happening.
>>>>
>>>> VM is not happy:
>>>>
>>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>>> [    1.908837] #PF: supervisor read access in kernel mode
>>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>>> [    1.908837] PGD 0 P4D 0
>>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>>> [    1.908837] PKRU: 55555554
>>>> [    1.908837] Call Trace:
>>>> [    1.908837]  <TASK>
>>>> [    1.908837]  ? __die+0x23/0x70
>>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>>> [    1.908837]  really_probe+0x19f/0x400
>>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>>> [    1.908837]  bus_add_driver+0x116/0x220
>>>> [    1.908837]  driver_register+0x59/0x100
>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>>> [    1.908837]  do_init_module+0x4a/0x210
>>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>>
>>>
>>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>>> implementation..
>>>> Maybe I'll have to install a Windows guest :/
>>>>
>>> I think that makes more sense, since we're targeting Windows anyway.
>>>
>>
>> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
>>
>> Windows is stuck booting (before the little circle starts turning).
>>
>> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
>>
>> Do you have a working cmdline for Windows I can try?
>>
> 
> Do you get any tracing output from the driver when it is loaded or does its mere presence
> make your Windows installation fail to boot?
> 
> I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
> 			-cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-

^ That magic collection of flags seems to make Win10 happy.

Master+your patches

(qemu) balloon 3000
(qemu) info balloon
balloon: actual=3000
(qemu) balloon 8000
(qemu) info balloon
balloon: actual=6144


As the VM has 4G and the memdev 2G, that's expected (although I'd have
expected an error when trying to set above what's possible).


On my prototype:

(qemu) balloon 3000
(qemu) info balloon
balloon: actual=3000
(qemu) balloon 8000
(qemu) info balloon
balloon: actual=6144


Further (some adjustments from me):
(qemu) info memory-devices
Memory device [hv-balloon]: "hv1"
   memaddr: 0x800000000
   max-size: 2147483648
   memdev: /objects/mem0
(qemu) info mtree
...
address-space: device-memory
   0000000180000000-000000097fffffff (prio 0, i/o): device-memory
     0000000800000000-000000087fffffff (prio 0, i/o): hv-balloon
       0000000800000000-000000087fffffff (prio 0, ram): mem0
...
(qemu) info memory_size_summary
base memory: 4294967296
plugged memory: 2147483648


Without a memdev:

(qemu) info memory-devices
Memory device [hv-balloon]: "hv1"
   max-size: 0



I noticed that sometimes balloon requests get essentially ignored.
Even retrying to set a target doesn't change anything. Only after
some time it eventually works.

(qemu) balloon 3000
(qemu) info balloon
balloon: actual=4096
...
(qemu) info balloon
balloon: actual=4096
(qemu) balloon 4000
(qemu) info balloon
balloon: actual=4096
...
(qemu) balloon 3000
(qemu) info balloon
balloon: actual=4096
(qemu) balloon 5000
(qemu) balloon 2000
(qemu) info balloon
balloon: actual=3152
(qemu) info balloon
balloon: actual=2000


Maybe the VM needs some time after start-up to actually
process requests.


Anyhow, the prototype lives at:
	https://github.com/davidhildenbrand/qemu/tree/hv-balloon
Right now it's on top of:
	https://github.com/davidhildenbrand/qemu/tree/virtio-mem-memslots

Patches/commits:

1) memory-device: Support empty memory devices
2) memory-device: Drop size alignment check
-> Two added patches
3) error: define g_autoptr() cleanup function for the Error type
4) Add Hyper-V Dynamic Memory Protocol definitions
5) Add a Hyper-V Dynamic Memory Protocol driver (hv-balloon)
-> Your unmodified patches
6) ea0e4775ca tmp
-> Me going wild


The alignment thingy is indeed ugly, but unless we want to fail hotadd for devices
with crazy alignment requirements, it's probably best the way it is on that branch.


You should definitely split up patch #3 into self-contained logical chunks
(e.g., split off the whole hotadd part, common code changes, ...). Also,
I'm not sure if the whole dynamic OurRangePlugged allocation thingy is
not really required, looks like that can be optimized.

I have some other cleanups in mind, but that is best kept to later, once the
patches are in an easier-to-review-sized shape.

On 22.06.2023 14:52, David Hildenbrand wrote:
> On 22.06.23 14:14, Maciej S. Szmigiero wrote:
>> On 22.06.2023 14:06, David Hildenbrand wrote:
>>> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>>>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>>>> [...]
>>>>>>>
>>>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>>>
>>>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>>>
>>>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>>>
>>>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>>>
>>>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>>>> "maxmem" option
>>>>>>>>
>>>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>>>> option given on the QEMU command line.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>>>> if it could be made working.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>>>> the next version of this driver.
>>>>>>>
>>>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>>>
>>>>>>>
>>>>>>> #!/bin/bash
>>>>>>>
>>>>>>> build/qemu-system-x86_64 \
>>>>>>>         --enable-kvm \
>>>>>>>         -m 4G,maxmem=36G \
>>>>>>>         -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>>>         -smp 16 \
>>>>>>>         -nographic \
>>>>>>>         -nodefaults \
>>>>>>>         -net nic -net user \
>>>>>>>         -chardev stdio,nosignal,id=serial \
>>>>>>>         -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>>>         -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>>>         -device isa-serial,chardev=serial \
>>>>>>>         -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>>>         -mon chardev=monitor,mode=readline \
>>>>>>>         -device vmbus-bridge \
>>>>>>>         -object memory-backend-ram,size=2G,id=mem0 \
>>>>>>>         -device hv-balloon,id=hv1,memdev=mem0
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [root@vm-0 ~]# uname -r
>>>>>>> 6.3.5-100.fc37.x86_64
>>>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>>>
>>>>>>>
>>>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>>>
>>>>>>
>>>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>>>> the Hyper-V one to be detected by Linux.
>>>>>>
>>>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>>>> the detected hypervisor is now Hyper-V.
>>>>>>
>>>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>>>
>>>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>>>> to QEMU command line to see what's happening.
>>>>>
>>>>> VM is not happy:
>>>>>
>>>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>>>> [    1.908837] #PF: supervisor read access in kernel mode
>>>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>>>> [    1.908837] PGD 0 P4D 0
>>>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>>>> [    1.908837] PKRU: 55555554
>>>>> [    1.908837] Call Trace:
>>>>> [    1.908837]  <TASK>
>>>>> [    1.908837]  ? __die+0x23/0x70
>>>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>>>> [    1.908837]  really_probe+0x19f/0x400
>>>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>>>> [    1.908837]  bus_add_driver+0x116/0x220
>>>>> [    1.908837]  driver_register+0x59/0x100
>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>>>> [    1.908837]  do_init_module+0x4a/0x210
>>>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>>>
>>>>
>>>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>>>> implementation..
>>>>> Maybe I'll have to install a Windows guest :/
>>>>>
>>>> I think that makes more sense, since we're targeting Windows anyway.
>>>>
>>>
>>> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
>>>
>>> Windows is stuck booting (before the little circle starts turning).
>>>
>>> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
>>>
>>> Do you have a working cmdline for Windows I can try?
>>>
>>
>> Do you get any tracing output from the driver when it is loaded or does its mere presence
>> make your Windows installation fail to boot?
>>
>> I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
>>             -cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-
> 
> ^ That magic collection of flags seems to make Win10 happy.

I will try to determine the minimum set of Hyper-V flags required
for Windows guests since having to manually specify the "right"
set seems like a poor UX (and also try to determine why Linux
guest crashes in ACPI when booted with above flags).

But I think both of these problems are related more to VMBus support
itself rather than hv-balloon.

> Master+your patches
> 
> (qemu) balloon 3000
> (qemu) info balloon
> balloon: actual=3000
> (qemu) balloon 8000
> (qemu) info balloon
> balloon: actual=6144
> 
> 
> As the VM has 4G and the memdev 2G, that's expected (although I'd have
> expected an error when trying to set above what's possible).
> 
> 
> On my prototype:
> 
> (qemu) balloon 3000
> (qemu) info balloon
> balloon: actual=3000
> (qemu) balloon 8000
> (qemu) info balloon
> balloon: actual=6144
> 
> 
> Further (some adjustments from me):
> (qemu) info memory-devices
> Memory device [hv-balloon]: "hv1"
>    memaddr: 0x800000000
>    max-size: 2147483648
>    memdev: /objects/mem0
> (qemu) info mtree
> ...
> address-space: device-memory
>    0000000180000000-000000097fffffff (prio 0, i/o): device-memory
>      0000000800000000-000000087fffffff (prio 0, i/o): hv-balloon
>        0000000800000000-000000087fffffff (prio 0, ram): mem0
> ...
> (qemu) info memory_size_summary
> base memory: 4294967296
> plugged memory: 2147483648
> 
> 
> Without a memdev:
> 
> (qemu) info memory-devices
> Memory device [hv-balloon]: "hv1"
>    max-size: 0
> 
> 
> 
> I noticed that sometimes balloon requests get essentially ignored.
> Even retrying to set a target doesn't change anything. Only after
> some time it eventually works.
> 
> (qemu) balloon 3000
> (qemu) info balloon
> balloon: actual=4096
> ...
> (qemu) info balloon
> balloon: actual=4096
> (qemu) balloon 4000
> (qemu) info balloon
> balloon: actual=4096
> ...
> (qemu) balloon 3000
> (qemu) info balloon
> balloon: actual=4096
> (qemu) balloon 5000
> (qemu) balloon 2000
> (qemu) info balloon
> balloon: actual=3152
> (qemu) info balloon
> balloon: actual=2000
> 
> 
> Maybe the VM needs some time after start-up to actually
> process requests.

Yes, there's a waiting period immediately after the guest
boot before executing any protocol operations in the guest:
> /*
>  * Some Windows versions (at least Server 2019) will crash with various
>  * error codes when receiving DM protocol requests (at least
>  * DM_MEM_HOT_ADD_REQUEST) immediately after boot.
>  *
>  * It looks like Hyper-V from Server 2016 uses a 50-second after-boot
>  * delay, probably to workaround this issue, so we'll use this value, too.
>  */
> #define HV_BALLOON_POST_INIT_WAIT (50 * 1000)


That's why I suggested running with tracing on, since then
you can observe the device waiting in the "POST_INIT_WAIT" state
(and also how things are proceeding in general).

> 
> Anyhow, the prototype lives at:
>      https://github.com/davidhildenbrand/qemu/tree/hv-balloon
> Right now it's on top of:
>      https://github.com/davidhildenbrand/qemu/tree/virtio-mem-memslots
> 
> Patches/commits:
> 
> 1) memory-device: Support empty memory devices
> 2) memory-device: Drop size alignment check
> -> Two added patches
> 3) error: define g_autoptr() cleanup function for the Error type
> 4) Add Hyper-V Dynamic Memory Protocol definitions
> 5) Add a Hyper-V Dynamic Memory Protocol driver (hv-balloon)
> -> Your unmodified patches
> 6) ea0e4775ca tmp
> -> Me going wild
> 
> 
> The alignment thingy is indeed ugly, but unless we want to fail hotadd for devices
> with crazy alignment requirements, it's probably best the way it is on that branch.
> 
> 
> You should definitely split up patch #3 into self-contained logical chunks
> (e.g., split off the whole hotadd part, common code changes, ...). Also,
> I'm not sure if the whole dynamic OurRangePlugged allocation thingy is
> not really required, looks like that can be optimized.
> 
> I have some other cleanups in mind, but that is best kept to later, once the
> patches are in an easier-to-review-sized shape.
> 

Thanks for the quick prototype, I will try to review it tomorrow and
come back with my comments regarding it.

Thanks,
Maciej

On 22.06.23 20:45, Maciej S. Szmigiero wrote:
> On 22.06.2023 14:52, David Hildenbrand wrote:
>> On 22.06.23 14:14, Maciej S. Szmigiero wrote:
>>> On 22.06.2023 14:06, David Hildenbrand wrote:
>>>> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>>>>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>>>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>>>>
>>>>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>>>>
>>>>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>>>>
>>>>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>>>>
>>>>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>>>>> "maxmem" option
>>>>>>>>>
>>>>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>>>>> option given on the QEMU command line.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>>>>> if it could be made working.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>>>>> the next version of this driver.
>>>>>>>>
>>>>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>>>>
>>>>>>>>
>>>>>>>> #!/bin/bash
>>>>>>>>
>>>>>>>> build/qemu-system-x86_64 \
>>>>>>>>          --enable-kvm \
>>>>>>>>          -m 4G,maxmem=36G \
>>>>>>>>          -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>>>>          -smp 16 \
>>>>>>>>          -nographic \
>>>>>>>>          -nodefaults \
>>>>>>>>          -net nic -net user \
>>>>>>>>          -chardev stdio,nosignal,id=serial \
>>>>>>>>          -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>>>>          -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>>>>          -device isa-serial,chardev=serial \
>>>>>>>>          -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>>>>          -mon chardev=monitor,mode=readline \
>>>>>>>>          -device vmbus-bridge \
>>>>>>>>          -object memory-backend-ram,size=2G,id=mem0 \
>>>>>>>>          -device hv-balloon,id=hv1,memdev=mem0
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [root@vm-0 ~]# uname -r
>>>>>>>> 6.3.5-100.fc37.x86_64
>>>>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>>>>
>>>>>>>>
>>>>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>>>>
>>>>>>>
>>>>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>>>>> the Hyper-V one to be detected by Linux.
>>>>>>>
>>>>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>>>>> the detected hypervisor is now Hyper-V.
>>>>>>>
>>>>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>>>>
>>>>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>>>>> to QEMU command line to see what's happening.
>>>>>>
>>>>>> VM is not happy:
>>>>>>
>>>>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>>>>> [    1.908837] #PF: supervisor read access in kernel mode
>>>>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>>>>> [    1.908837] PGD 0 P4D 0
>>>>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>>>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>>>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>>>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>>>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>>>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>>>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>>>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>>>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>>>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>>>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>>>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>>>>> [    1.908837] PKRU: 55555554
>>>>>> [    1.908837] Call Trace:
>>>>>> [    1.908837]  <TASK>
>>>>>> [    1.908837]  ? __die+0x23/0x70
>>>>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>>>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>>>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>>>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>>>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>>>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>>>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>>>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>>>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>>>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>>>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>>>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>>>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>>>>> [    1.908837]  really_probe+0x19f/0x400
>>>>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>>>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>>>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>>>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>>>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>>>>> [    1.908837]  bus_add_driver+0x116/0x220
>>>>>> [    1.908837]  driver_register+0x59/0x100
>>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>>>>> [    1.908837]  do_init_module+0x4a/0x210
>>>>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>>>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>>>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>>>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>>>>
>>>>>
>>>>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>>>>> implementation..
>>>>>> Maybe I'll have to install a Windows guest :/
>>>>>>
>>>>> I think that makes more sense, since we're targeting Windows anyway.
>>>>>
>>>>
>>>> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
>>>>
>>>> Windows is stuck booting (before the little circle starts turning).
>>>>
>>>> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
>>>>
>>>> Do you have a working cmdline for Windows I can try?
>>>>
>>>
>>> Do you get any tracing output from the driver when it is loaded or does its mere presence
>>> make your Windows installation fail to boot?
>>>
>>> I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
>>>              -cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-
>>
>> ^ That magic collection of flags seems to make Win10 happy.
> 
> I will try to determine the minimum set of Hyper-V flags required
> for Windows guests since having to manually specify the "right"
> set seems like a poor UX (and also try to determine why Linux
> guest crashes in ACPI when booted with above flags).
> 
> But I think both of these problems are related more to VMBus support
> itself rather than hv-balloon.

That's also my feeling. It would be great to extend

https://www.qemu.org/docs/master/system/i386/hyperv.html

and include some of the dirty details in you next submission, so it's 
easier to get running.

[...]

>> Maybe the VM needs some time after start-up to actually
>> process requests.
> 
> Yes, there's a waiting period immediately after the guest
> boot before executing any protocol operations in the guest:
>> /*
>>   * Some Windows versions (at least Server 2019) will crash with various
>>   * error codes when receiving DM protocol requests (at least
>>   * DM_MEM_HOT_ADD_REQUEST) immediately after boot.
>>   *
>>   * It looks like Hyper-V from Server 2016 uses a 50-second after-boot
>>   * delay, probably to workaround this issue, so we'll use this value, too.
>>   */
>> #define HV_BALLOON_POST_INIT_WAIT (50 * 1000)
> 
> 
> That's why I suggested running with tracing on, since then
> you can observe the device waiting in the "POST_INIT_WAIT" state
> (and also how things are proceeding in general).

Ah, makes sense.

On 22.06.2023 20:45, Maciej S. Szmigiero wrote:
> On 22.06.2023 14:52, David Hildenbrand wrote:
>> On 22.06.23 14:14, Maciej S. Szmigiero wrote:
>>> On 22.06.2023 14:06, David Hildenbrand wrote:
>>>> On 22.06.23 13:17, Maciej S. Szmigiero wrote:
>>>>> On 22.06.2023 13:15, David Hildenbrand wrote:
>>>>>> On 22.06.23 13:12, Maciej S. Szmigiero wrote:
>>>>>>> On 22.06.2023 13:01, David Hildenbrand wrote:
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>>>> We'd use a memory region container as device memory region (like [1]) and would have to handle the !memdev case (I can help with that). > Into that, you can map the RAM memory region on demand (and eventually even using multiple slots like [1]).
>>>>>>>>>>>>
>>>>>>>>>>>> (2) Use a single virtual DIMM and (un)plug that on demand. Let the machine code handle (un)plugging of the device.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> (1) feels cleanest to me, although it will require a bit more work.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I also think approach (1) makes more sense as it avoids memslot metadata
>>>>>>>>>>> overhead for not-yet-hot-added parts of the memory backing device.
>>>>>>>>>>>
>>>>>>>>>>> Not sure what you mean that the !memdev case would be problematic in this
>>>>>>>>>>> case - it is working in the current driver shape so why would adding
>>>>>>>>>>> potential memory subregions (used in the memdev case) change that?
>>>>>>>>>>
>>>>>>>>>> I'm thinking about the case where you have a hv-balloon device without a memdev.
>>>>>>>>>>
>>>>>>>>>> Without -m X,maxmem=y we don't currently expect to have memory devices around
>>>>>>>>>> (and especially them getting (un)plugged. But why should we "force" to set the
>>>>>>>>>> "maxmem" option
>>>>>>>>>
>>>>>>>>> I guess it's only a small change to QEMU to allow having hv-balloon
>>>>>>>>> device (without a memdev) even in the case where there's no "maxmem"
>>>>>>>>> option given on the QEMU command line.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I hope I'll find some time soonish to prototype what I have in mind, to see
>>>>>>>>>> if it could be made working.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Okay, so I'll wait for your prototype before commencing further work on
>>>>>>>>> the next version of this driver.
>>>>>>>>
>>>>>>>> About to have something simplistic running -- I think. Want to test with a Linux VM, but I don't seem to get it working (also without my changes).
>>>>>>>>
>>>>>>>>
>>>>>>>> #!/bin/bash
>>>>>>>>
>>>>>>>> build/qemu-system-x86_64 \
>>>>>>>>         --enable-kvm \
>>>>>>>>         -m 4G,maxmem=36G \
>>>>>>>>         -cpu host,hv-syndbg=on,hv-synic,hv-relaxed,hv-vpindex \
>>>>>>>>         -smp 16 \
>>>>>>>>         -nographic \
>>>>>>>>         -nodefaults \
>>>>>>>>         -net nic -net user \
>>>>>>>>         -chardev stdio,nosignal,id=serial \
>>>>>>>>         -hda Fedora-Cloud-Base-37-1.7.x86_64.qcow2 \
>>>>>>>>         -cdrom /home/dhildenb/git/cloud-init/cloud-init.iso \
>>>>>>>>         -device isa-serial,chardev=serial \
>>>>>>>>         -chardev socket,id=monitor,path=/var/tmp/mon_src,server,nowait \
>>>>>>>>         -mon chardev=monitor,mode=readline \
>>>>>>>>         -device vmbus-bridge \
>>>>>>>>         -object memory-backend-ram,size=2G,id=mem0 \
>>>>>>>>         -device hv-balloon,id=hv1,memdev=mem0
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [root@vm-0 ~]# uname -r
>>>>>>>> 6.3.5-100.fc37.x86_64
>>>>>>>> [root@vm-0 ~]# modprobe hv_balloon
>>>>>>>> modprobe: ERROR: could not insert 'hv_balloon': No such device
>>>>>>>>
>>>>>>>>
>>>>>>>> Any magic flag I am missing? Or is there something preventing this to work with Linux VMs?
>>>>>>>>
>>>>>>>
>>>>>>> Haven't tested the driver with Linux guests in a long time (as it is
>>>>>>> targeting Windows), but I think you need to disable KVM PV interface for
>>>>>>> the Hyper-V one to be detected by Linux.
>>>>>>>
>>>>>>> Something like adding "kvm=off" to "-cpu" and seeing in the dmesg whether
>>>>>>> the detected hypervisor is now Hyper-V.
>>>>>>>
>>>>>>> Also, you need to disable S4 in the guest for hot-add capability to work
>>>>>>> (I'm adding "-global ICH9-LPC.disable_s4=1" with q35 machine for this).
>>>>>>>
>>>>>>> Would also suggest adding "--trace 'hv_balloon_*' --trace 'memory_device_*'"
>>>>>>> to QEMU command line to see what's happening.
>>>>>>
>>>>>> VM is not happy:
>>>>>>
>>>>>> [    1.908595] BUG: kernel NULL pointer dereference, address: 0000000000000007
>>>>>> [    1.908837] #PF: supervisor read access in kernel mode
>>>>>> [    1.908837] #PF: error_code(0x0000) - not-present page
>>>>>> [    1.908837] PGD 0 P4D 0
>>>>>> [    1.908837] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>>>> [    1.908837] CPU: 13 PID: 492 Comm: (udev-worker) Not tainted 6.3.5-100.fc37.x86_64 #1
>>>>>> [    1.908837] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-p4
>>>>>> [    1.908837] RIP: 0010:acpi_ns_lookup+0x8f/0x4c0
>>>>>> [    1.908837] Code: 8b 3d f5 eb 1c 03 83 05 52 ec 1c 03 01 48 85 ff 0f 84 51 03 00 00 44 89 c3 4c 89 cb
>>>>>> [    1.908837] RSP: 0018:ffff95b680ad7950 EFLAGS: 00010286
>>>>>> [    1.908837] RAX: ffff95b680ad79e0 RBX: 0000000000000002 RCX: 0000000000000003
>>>>>> [    1.908837] RDX: 0000000000000000 RSI: ffff8a0283a3c558 RDI: ffffffffa4b376e0
>>>>>> [    1.908837] RBP: 0000000000000000 R08: 0000000000000002 R09: 0000000000000000
>>>>>> [    1.908837] R10: ffff8a02811034ec R11: 0000000000000000 R12: ffffffffffffffff
>>>>>> [    1.908837] R13: ffff8a02811034e8 R14: ffff8a02811034e8 R15: 0000000000000000
>>>>>> [    1.908837] FS:  00007f3bb2e7d0c0(0000) GS:ffff8a02bbd40000(0000) knlGS:0000000000000000
>>>>>> [    1.908837] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> [    1.908837] CR2: 0000000000000007 CR3: 0000000100a58002 CR4: 0000000000770ee0
>>>>>> [    1.908837] PKRU: 55555554
>>>>>> [    1.908837] Call Trace:
>>>>>> [    1.908837]  <TASK>
>>>>>> [    1.908837]  ? __die+0x23/0x70
>>>>>> [    1.908837]  ? page_fault_oops+0x171/0x4e0
>>>>>> [    1.908837]  ? prepare_alloc_pages.constprop.0+0xf6/0x1a0
>>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>>> [    1.908837]  ? asm_exc_page_fault+0x26/0x30
>>>>>> [    1.908837]  ? acpi_ns_lookup+0x8f/0x4c0
>>>>>> [    1.908837]  acpi_ns_get_node_unlocked+0xdd/0x110
>>>>>> [    1.908837]  ? down_timeout+0x3e/0x60
>>>>>> [    1.908837]  ? acpi_ns_get_node+0x3e/0x60
>>>>>> [    1.908837]  acpi_ns_get_node+0x3e/0x60
>>>>>> [    1.908837]  acpi_ns_evaluate+0x1cb/0x2d0
>>>>>> [    1.908837]  acpi_ut_evaluate_object+0x68/0x1c0
>>>>>> [    1.908837]  acpi_rs_get_method_data+0x37/0x80
>>>>>> [    1.908837]  ? __pfx_vmbus_walk_resources+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  acpi_walk_resources+0x91/0xe0
>>>>>> [    1.908837]  vmbus_acpi_add+0x87/0x170 [hv_vmbus]
>>>>>> [    1.908837]  acpi_device_probe+0x47/0x160
>>>>>> [    1.908837]  really_probe+0x19f/0x400
>>>>>> [    1.908837]  ? __pfx___driver_attach+0x10/0x10
>>>>>> [    1.908837]  __driver_probe_device+0x78/0x160
>>>>>> [    1.908837]  driver_probe_device+0x1f/0x90
>>>>>> [    1.908837]  __driver_attach+0xd2/0x1c0
>>>>>> [    1.908837]  bus_for_each_dev+0x85/0xd0
>>>>>> [    1.908837]  bus_add_driver+0x116/0x220
>>>>>> [    1.908837]  driver_register+0x59/0x100
>>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  hv_acpi_init+0x39/0xff0 [hv_vmbus]
>>>>>> [    1.908837]  ? __pfx_init_module+0x10/0x10 [hv_vmbus]
>>>>>> [    1.908837]  do_one_initcall+0x5a/0x240
>>>>>> [    1.908837]  do_init_module+0x4a/0x210
>>>>>> [    1.908837]  __do_sys_init_module+0x17f/0x1b0
>>>>>> [    1.908837]  do_syscall_64+0x5c/0x90
>>>>>> [    1.908837]  ? handle_mm_fault+0x11e/0x310
>>>>>> [    1.908837]  ? do_user_addr_fault+0x1e0/0x720
>>>>>> [    1.908837]  ? exc_page_fault+0x74/0x170
>>>>>> [    1.908837]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
>>>>>>
>>>>>
>>>>> I guess *few* people run Linux with QEMU Hyper-V interfaces
>>>>> implementation..
>>>>>> Maybe I'll have to install a Windows guest :/
>>>>>>
>>>>> I think that makes more sense, since we're targeting Windows anyway.
>>>>>
>>>>
>>>> Having installed fairly recent Win10 and running with master+your patches, I still can't get it to work.
>>>>
>>>> Windows is stuck booting (before the little circle starts turning).
>>>>
>>>> Removing the hv-balloon device makes it work again (well, at least the circle spins again my windows installation now seems to be broken and I have to reinstall ... windows).
>>>>
>>>> Do you have a working cmdline for Windows I can try?
>>>>
>>>
>>> Do you get any tracing output from the driver when it is loaded or does its mere presence
>>> make your Windows installation fail to boot?
>>>
>>> I'm was testing using "qemu-system-x86_64 -machine q35,accel=kvm \
>>>             -cpu host,vmx,-mpx,-pmu,check,hv-crash,hv-relaxed,hv-time,hv-vapic,hv-spinlocks=0x1fff,hv-
>>
>> ^ That magic collection of flags seems to make Win10 happy.
> 
> I will try to determine the minimum set of Hyper-V flags required
> for Windows guests since having to manually specify the "right"
> set seems like a poor UX (and also try to determine why Linux
> guest crashes in ACPI when booted with above flags).
> 
> But I think both of these problems are related more to VMBus support
> itself rather than hv-balloon.
> 
>> Master+your patches
>>
>> (qemu) balloon 3000
>> (qemu) info balloon
>> balloon: actual=3000
>> (qemu) balloon 8000
>> (qemu) info balloon
>> balloon: actual=6144
>>
>>
>> As the VM has 4G and the memdev 2G, that's expected (although I'd have
>> expected an error when trying to set above what's possible).
>>
>>
>> On my prototype:
>>
>> (qemu) balloon 3000
>> (qemu) info balloon
>> balloon: actual=3000
>> (qemu) balloon 8000
>> (qemu) info balloon
>> balloon: actual=6144
>>
>>
>> Further (some adjustments from me):
>> (qemu) info memory-devices
>> Memory device [hv-balloon]: "hv1"
>>    memaddr: 0x800000000
>>    max-size: 2147483648
>>    memdev: /objects/mem0
>> (qemu) info mtree
>> ...
>> address-space: device-memory
>>    0000000180000000-000000097fffffff (prio 0, i/o): device-memory
>>      0000000800000000-000000087fffffff (prio 0, i/o): hv-balloon
>>        0000000800000000-000000087fffffff (prio 0, ram): mem0
>> ...
>> (qemu) info memory_size_summary
>> base memory: 4294967296
>> plugged memory: 2147483648
>>
>>
>> Without a memdev:
>>
>> (qemu) info memory-devices
>> Memory device [hv-balloon]: "hv1"
>>    max-size: 0
>>
>>
>>
>> I noticed that sometimes balloon requests get essentially ignored.
>> Even retrying to set a target doesn't change anything. Only after
>> some time it eventually works.
>>
>> (qemu) balloon 3000
>> (qemu) info balloon
>> balloon: actual=4096
>> ...
>> (qemu) info balloon
>> balloon: actual=4096
>> (qemu) balloon 4000
>> (qemu) info balloon
>> balloon: actual=4096
>> ...
>> (qemu) balloon 3000
>> (qemu) info balloon
>> balloon: actual=4096
>> (qemu) balloon 5000
>> (qemu) balloon 2000
>> (qemu) info balloon
>> balloon: actual=3152
>> (qemu) info balloon
>> balloon: actual=2000
>>
>>
>> Maybe the VM needs some time after start-up to actually
>> process requests.
> 
> Yes, there's a waiting period immediately after the guest
> boot before executing any protocol operations in the guest:
>> /*
>>  * Some Windows versions (at least Server 2019) will crash with various
>>  * error codes when receiving DM protocol requests (at least
>>  * DM_MEM_HOT_ADD_REQUEST) immediately after boot.
>>  *
>>  * It looks like Hyper-V from Server 2016 uses a 50-second after-boot
>>  * delay, probably to workaround this issue, so we'll use this value, too.
>>  */
>> #define HV_BALLOON_POST_INIT_WAIT (50 * 1000)
> 
> 
> That's why I suggested running with tracing on, since then
> you can observe the device waiting in the "POST_INIT_WAIT" state
> (and also how things are proceeding in general).
> 
>>
>> Anyhow, the prototype lives at:
>>      https://github.com/davidhildenbrand/qemu/tree/hv-balloon
>> Right now it's on top of:
>>      https://github.com/davidhildenbrand/qemu/tree/virtio-mem-memslots
>>
>> Patches/commits:
>>
>> 1) memory-device: Support empty memory devices
>> 2) memory-device: Drop size alignment check
>> -> Two added patches
>> 3) error: define g_autoptr() cleanup function for the Error type
>> 4) Add Hyper-V Dynamic Memory Protocol definitions
>> 5) Add a Hyper-V Dynamic Memory Protocol driver (hv-balloon)
>> -> Your unmodified patches
>> 6) ea0e4775ca tmp
>> -> Me going wild
>>
>>
>> The alignment thingy is indeed ugly, but unless we want to fail hotadd for devices
>> with crazy alignment requirements, it's probably best the way it is on that branch.
>>
>>
>> You should definitely split up patch #3 into self-contained logical chunks
>> (e.g., split off the whole hotadd part, common code changes, ...). Also,
>> I'm not sure if the whole dynamic OurRangePlugged allocation thingy is
>> not really required, looks like that can be optimized.
>>
>> I have some other cleanups in mind, but that is best kept to later, once the
>> patches are in an easier-to-review-sized shape.
>>
> 
> Thanks for the quick prototype, I will try to review it tomorrow and
> come back with my comments regarding it.

Looked at the code, did some testing and it seems to work well.

Will prepare a new version of the driver with per-hot-add-request subregions
support since I think it makes sense for the driver to benefit from them
upfront if it is going to be based on your virtio-mem patch set anyway.

Thanks,
Maciej