Re: [Qemu-devel] balloon config change seems to break live migration from 3.0.1 to 4.0

[Dr. David Alan Gilbert]

* Wolfgang Bumiller (address@hidden) 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 <address@hidden>
> 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 <address@hidden>
> ---
>  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 / address@hidden / Manchester, UK