* Wolfgang Bumiller (w.bumil...@proxmox.com) wrote:
> On Thu, Jun 27, 2019 at 03:12:52PM +0200, Wolfgang Bumiller wrote:
> > While testing with 4.0 we've run into issues with live migration from
> > 3.0.1 to 4.0 when a balloon device was involved.
> >
> > We'd see the following error on the destination:
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read:
> > a1 device: 1 cmask: ff wmask: c0 w1cmask:0
> > qemu-system-x86_64: Failed to load PCIDevice:config
> > qemu-system-x86_64: Failed to load virtio-balloon:virtio
> > qemu-system-x86_64: error while loading state for instance 0x0 of device
> > '0000:00:03.0/virtio-balloon'
> > qemu-system-x86_64: load of migration failed: Invalid argument
>
> So if I'm not mistaken this seems to cover the BAR containing the mapped
> address, which now has a stricter alignment requirement (since the
> config space crossed the 32 byte boundary and now is now 64 bytes and
> therefor requires a 64 byte alignment).
> (This in turn means that it only affects guests where the balloon's
> mapping isn't already 64 byte aligned.)
Right, I agree; thanks for figuring this out.
> I tested with the changes below meant to use the old configuration size
> for machines depending on their machine version... This seems to fix the
> problem for me here.
OK, can you post this as a separate mail please; cc'ing all who I've
added on this list.
> With this changing compatibility options I'm not sure if this is a
> desired upstream change since 4.0 is already released?
There's no great answer here - one of them is going to stay broken;
we can either fix migration earlier 4.0 and break migration to 4.0
or leave migration from earlier broken.
Let's add the property at least - downstream I know I'll need
it.
As for your actual patch; it's way bigger than I expected - can't you
just use DEFINE_PROP - like for exmaple the 'disable-modern' in
virtio/virtio-pci.c ?
Stefan/mst: What do you reckon - should we:
a) Fix it so migration with older qemu's works but break 4.0
b) Or leave 4.0 working and keep older broken.
Dave
> ---8<---
> From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> From: Wolfgang Bumiller <w.bumil...@proxmox.com>
> Date: Tue, 2 Jul 2019 09:23:43 +0200
> Subject: [PATCH] virtio-balloon: use smaller config on older guests
>
> The increased config size changes its alignment requirements
> preventing guests from migrating from old qemu versions if
> their balloon mapping isn't stricter aligned.
> So base the default config size on the machine version.
>
> Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com>
> ---
> hw/i386/pc.c | 2 ++
> hw/virtio/virtio-balloon.c | 52 +++++++++++++++++++++++++++---
> include/hw/virtio/virtio-balloon.h | 1 +
> 3 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 652eb72b2b..3676187a19 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -77,6 +77,7 @@
> #include "hw/i386/intel_iommu.h"
> #include "hw/net/ne2000-isa.h"
> #include "standard-headers/asm-x86/bootparam.h"
> +#include "hw/virtio/virtio-balloon.h"
>
> /* debug PC/ISA interrupts */
> //#define DEBUG_IRQ
> @@ -137,6 +138,7 @@ GlobalProperty pc_compat_3_1[] = {
> { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> + { TYPE_VIRTIO_BALLOON, "x-use-large-balloon-config", "off" },
> };
> const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index d96e4aa96f..cd0f124fbb 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -328,6 +328,28 @@ static void balloon_stats_set_poll_interval(Object *obj,
> Visitor *v,
> balloon_stats_change_timer(s, 0);
> }
>
> +static void balloon_get_large_config(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOBalloon *s = opaque;
> + visit_type_bool(v, name, &s->use_large_config, errp);
> +}
> +
> +static void balloon_set_large_config(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOBalloon *s = opaque;
> + DeviceState *dev = DEVICE(s);
> +
> + if (dev->realized) {
> + error_setg(errp, "balloon config size cannot be changed at runtime");
> + } else {
> + visit_type_bool(v, name, &s->use_large_config, errp);
> + }
> +}
> +
> static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> {
> VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> @@ -526,6 +548,17 @@ static bool virtio_balloon_free_page_support(void
> *opaque)
> return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> }
>
> +static size_t virtio_balloon_config_size(void *opaque)
> +{
> + VirtIOBalloon *s = opaque;
> +
> + if (s->use_large_config) {
> + return sizeof(struct virtio_balloon_config);
> + } else {
> + return offsetof(struct virtio_balloon_config,
> free_page_report_cmd_id);
> + }
> +}
> +
> static void virtio_balloon_free_page_start(VirtIOBalloon *s)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -635,7 +668,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev,
> uint8_t *config_data)
> }
>
> trace_virtio_balloon_get_config(config.num_pages, config.actual);
> - memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> + memcpy(config_data, &config, virtio_balloon_config_size(dev));
> }
>
> static int build_dimm_list(Object *obj, void *opaque)
> @@ -679,7 +712,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> uint32_t oldactual = dev->actual;
> ram_addr_t vm_ram_size = get_current_ram_size();
>
> - memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
> + memcpy(&config, config_data, virtio_balloon_config_size(dev));
> dev->actual = le32_to_cpu(config.actual);
> if (dev->actual != oldactual) {
> qapi_event_send_balloon_change(vm_ram_size -
> @@ -795,7 +828,7 @@ static void virtio_balloon_device_realize(DeviceState
> *dev, Error **errp)
> int ret;
>
> virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> - sizeof(struct virtio_balloon_config));
> + virtio_balloon_config_size(s));
>
> ret = qemu_add_balloon_handler(virtio_balloon_to_target,
> virtio_balloon_stat, s);
> @@ -820,7 +853,7 @@ static void virtio_balloon_device_realize(DeviceState
> *dev, Error **errp)
> s->free_page_report_notify.notify =
>
> virtio_balloon_free_page_report_notify;
> precopy_add_notifier(&s->free_page_report_notify);
> - if (s->iothread) {
> + if (s->iothread && s->use_large_config) {
> object_ref(OBJECT(s->iothread));
> s->free_page_bh =
> aio_bh_new(iothread_get_aio_context(s->iothread),
> virtio_ballloon_get_free_page_hints,
> s);
> @@ -833,6 +866,11 @@ static void virtio_balloon_device_realize(DeviceState
> *dev, Error **errp)
> virtio_error(vdev, "iothread is missing");
> }
> }
> +
> + if (!s->use_large_config) {
> + s->host_features &= ~(1 << VIRTIO_BALLOON_F_PAGE_POISON);
> + }
> +
> reset_stats(s);
> }
>
> @@ -909,6 +947,12 @@ static void virtio_balloon_instance_init(Object *obj)
> balloon_stats_get_poll_interval,
> balloon_stats_set_poll_interval,
> NULL, s, NULL);
> +
> + s->use_large_config = true;
> + object_property_add(obj, "x-use-large-balloon-config", "bool",
> + balloon_get_large_config,
> + balloon_set_large_config,
> + NULL, s, NULL);
> }
>
> static const VMStateDescription vmstate_virtio_balloon = {
> diff --git a/include/hw/virtio/virtio-balloon.h
> b/include/hw/virtio/virtio-balloon.h
> index 1afafb12f6..f212c08bd7 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -45,6 +45,7 @@ enum virtio_balloon_free_page_report_status {
> typedef struct VirtIOBalloon {
> VirtIODevice parent_obj;
> VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> + bool use_large_config;
> uint32_t free_page_report_status;
> uint32_t num_pages;
> uint32_t actual;
> --
> 2.20.1
>
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
