| Message ID | 20200422182120.12258.67417.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-balloon: add support for free page reporting | expand |
On 22.04.20 20:21, Alexander Duyck wrote: > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > We need to make certain to advertise support for page poison tracking if > we want to actually get data on if the guest will be poisoning pages. > > Add a value for tracking the poison value being used if page poisoning is > enabled. With this we can determine if we will need to skip page reporting > when it is enabled in the future. Maybe add something about the semantics "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page hinting or ordinary balloon inflation/deflation." I do wonder if we should just unconditionally enable VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine (via a property that is default-enabled, and disabled from compat machines). Because, as Michael said, knowing that the guest is using page poisoning might be interesting even if free page reporting is not around. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > --- > hw/virtio/virtio-balloon.c | 7 +++++++ > include/hw/virtio/virtio-balloon.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index a1d6fb52c876..5effc8b4653b 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) > > config.num_pages = cpu_to_le32(dev->num_pages); > config.actual = cpu_to_le32(dev->actual); > + config.poison_val = cpu_to_le32(dev->poison_val); > > if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) { > config.free_page_hint_cmd_id = > @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, > qapi_event_send_balloon_change(vm_ram_size - > ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT)); > } > + dev->poison_val = 0; > + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { > + dev->poison_val = le32_to_cpu(config.poison_val); > + } > trace_virtio_balloon_set_config(dev->actual, oldactual); > } > > @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev) > g_free(s->stats_vq_elem); > s->stats_vq_elem = NULL; > } > + > + s->poison_val = 0; > } > > static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > index 108cff97e71a..3ca2a78e1aca 100644 > --- a/include/hw/virtio/virtio-balloon.h > +++ b/include/hw/virtio/virtio-balloon.h > @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon { > uint32_t host_features; > > bool qemu_4_0_config_size; > + uint32_t poison_val; > } VirtIOBalloon; > > #endif > You still have to migrate poison_val if I am not wrong, otherwise you would lose it during migration if I am not mistaking.
On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote: > > On 22.04.20 20:21, Alexander Duyck wrote: > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > > > We need to make certain to advertise support for page poison tracking if > > we want to actually get data on if the guest will be poisoning pages. > > > > Add a value for tracking the poison value being used if page poisoning is > > enabled. With this we can determine if we will need to skip page reporting > > when it is enabled in the future. > > Maybe add something about the semantics > > "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page > hinting or ordinary balloon inflation/deflation." > > I do wonder if we should just unconditionally enable > VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine > (via a property that is default-enabled, and disabled from compat machines). > > Because, as Michael said, knowing that the guest is using page poisoning > might be interesting even if free page reporting is not around. I could do that. So if that is the case though would I disable page reporting if it isn't enabled, or would I be enabling page poison if page reporting is enabled? > > > > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > --- > > hw/virtio/virtio-balloon.c | 7 +++++++ > > include/hw/virtio/virtio-balloon.h | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > > index a1d6fb52c876..5effc8b4653b 100644 > > --- a/hw/virtio/virtio-balloon.c > > +++ b/hw/virtio/virtio-balloon.c > > @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) > > > > config.num_pages = cpu_to_le32(dev->num_pages); > > config.actual = cpu_to_le32(dev->actual); > > + config.poison_val = cpu_to_le32(dev->poison_val); > > > > if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) { > > config.free_page_hint_cmd_id = > > @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, > > qapi_event_send_balloon_change(vm_ram_size - > > ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT)); > > } > > + dev->poison_val = 0; > > + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { > > + dev->poison_val = le32_to_cpu(config.poison_val); > > + } > > trace_virtio_balloon_set_config(dev->actual, oldactual); > > } > > > > @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev) > > g_free(s->stats_vq_elem); > > s->stats_vq_elem = NULL; > > } > > + > > + s->poison_val = 0; > > } > > > > static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) > > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > > index 108cff97e71a..3ca2a78e1aca 100644 > > --- a/include/hw/virtio/virtio-balloon.h > > +++ b/include/hw/virtio/virtio-balloon.h > > @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon { > > uint32_t host_features; > > > > bool qemu_4_0_config_size; > > + uint32_t poison_val; > > } VirtIOBalloon; > > > > #endif > > > > You still have to migrate poison_val if I am not wrong, otherwise you > would lose it during migration if I am not mistaking. So what are the requirements to migrate a value? Would I need to add a property so it can be retrieved/set?
On 23.04.20 16:46, Alexander Duyck wrote: > On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 22.04.20 20:21, Alexander Duyck wrote: >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> >>> We need to make certain to advertise support for page poison tracking if >>> we want to actually get data on if the guest will be poisoning pages. >>> >>> Add a value for tracking the poison value being used if page poisoning is >>> enabled. With this we can determine if we will need to skip page reporting >>> when it is enabled in the future. >> >> Maybe add something about the semantics >> >> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page >> hinting or ordinary balloon inflation/deflation." >> >> I do wonder if we should just unconditionally enable >> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine >> (via a property that is default-enabled, and disabled from compat machines). >> >> Because, as Michael said, knowing that the guest is using page poisoning >> might be interesting even if free page reporting is not around. > > I could do that. So if that is the case though would I disable page > reporting if it isn't enabled, or would I be enabling page poison if > page reporting is enabled? So, I would suggest this the following as a diff to this patch (the TODO part as a separate patch - we will have these compat properties briefly after the 5.0 release) diff --git a/hw/core/machine.c b/hw/core/machine.c index c1a444cb75..2e96cba4ff 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -28,6 +28,12 @@ #include "hw/mem/nvdimm.h" #include "migration/vmstate.h" +// TODO: After 5.0 release +GlobalProperty hw_compat_5_0[] = { + { "virtio-balloon-device", "page-hint", "false"}, +}; +const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0); + GlobalProperty hw_compat_4_2[] = { { "virtio-blk-device", "queue-size", "128"}, { "virtio-scsi-device", "virtqueue_size", "128"}, diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index a4729f7fc9..5ff8a669cf 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = { qemu_4_0_config_size, false), DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD, IOThread *), + DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features, + VIRTIO_BALLOON_F_PAGE_POISON, true), DEFINE_PROP_END_OF_LIST(), }; What to do with page reporting depends: I would not implicitly enable features. I think we are talking about -device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning. b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'"). With new QEMU machines, this should not happen with -device virtio-balloon-pci,...,free-page-reporting=true as page-poison=true is the new default. What's your opinion? > >>> >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> >>> --- >>> hw/virtio/virtio-balloon.c | 7 +++++++ >>> include/hw/virtio/virtio-balloon.h | 1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c >>> index a1d6fb52c876..5effc8b4653b 100644 >>> --- a/hw/virtio/virtio-balloon.c >>> +++ b/hw/virtio/virtio-balloon.c >>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) >>> >>> config.num_pages = cpu_to_le32(dev->num_pages); >>> config.actual = cpu_to_le32(dev->actual); >>> + config.poison_val = cpu_to_le32(dev->poison_val); >>> >>> if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) { >>> config.free_page_hint_cmd_id = >>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, >>> qapi_event_send_balloon_change(vm_ram_size - >>> ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT)); >>> } >>> + dev->poison_val = 0; >>> + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { >>> + dev->poison_val = le32_to_cpu(config.poison_val); >>> + } >>> trace_virtio_balloon_set_config(dev->actual, oldactual); >>> } >>> >>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev) >>> g_free(s->stats_vq_elem); >>> s->stats_vq_elem = NULL; >>> } >>> + >>> + s->poison_val = 0; >>> } >>> >>> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) >>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h >>> index 108cff97e71a..3ca2a78e1aca 100644 >>> --- a/include/hw/virtio/virtio-balloon.h >>> +++ b/include/hw/virtio/virtio-balloon.h >>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon { >>> uint32_t host_features; >>> >>> bool qemu_4_0_config_size; >>> + uint32_t poison_val; >>> } VirtIOBalloon; >>> >>> #endif >>> >> >> You still have to migrate poison_val if I am not wrong, otherwise you >> would lose it during migration if I am not mistaking. > > So what are the requirements to migrate a value? Would I need to add a > property so it can be retrieved/set? > Something like this would do the trick: diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index a4729f7fc9..ea0afec5f6 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque) return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT); } +static bool virtio_balloon_page_poison_support(void *opaque) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(opaque); + + return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON); +} + static void virtio_balloon_free_page_start(VirtIOBalloon *s) { VirtIODevice *vdev = VIRTIO_DEVICE(s); @@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = { } }; +static const VMStateDescription vmstate_virtio_balloon_page_poison = { + .name = "vitio-balloon-device/page-poison", + .version_id = 1, + .minimum_version_id = 1, + .needed = virtio_balloon_page_poison_support, + .fields = (VMStateField[]) { + VMSTATE_UINT32(poison_val, VirtIOBalloon), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_virtio_balloon_device = { .name = "virtio-balloon-device", .version_id = 1, @@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = { }, .subsections = (const VMStateDescription * []) { &vmstate_virtio_balloon_free_page_report, + &vmstate_virtio_balloon_page_poison, NULL } }; But I *think* the following should work as well IIRC (could be that migrating new QEMU -> old QEMU would be an issue, I don't recall if both directions are safe): diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index a4729f7fc9..01bccf26fb 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -757,12 +757,13 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = { static const VMStateDescription vmstate_virtio_balloon_device = { .name = "virtio-balloon-device", - .version_id = 1, + .version_id = 2, .minimum_version_id = 1, .post_load = virtio_balloon_post_load_device, .fields = (VMStateField[]) { VMSTATE_UINT32(num_pages, VirtIOBalloon), VMSTATE_UINT32(actual, VirtIOBalloon), + VMSTATE_UINT32_V(poison_val, VirtIOBalloon, 2), VMSTATE_END_OF_LIST() }, .subsections = (const VMStateDescription * []) {
On Thu, Apr 23, 2020 at 9:02 AM David Hildenbrand <david@redhat.com> wrote: > > On 23.04.20 16:46, Alexander Duyck wrote: > > On Thu, Apr 23, 2020 at 1:11 AM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 22.04.20 20:21, Alexander Duyck wrote: > >>> From: Alexander Duyck <alexander.h.duyck@linux.intel.com> > >>> > >>> We need to make certain to advertise support for page poison tracking if > >>> we want to actually get data on if the guest will be poisoning pages. > >>> > >>> Add a value for tracking the poison value being used if page poisoning is > >>> enabled. With this we can determine if we will need to skip page reporting > >>> when it is enabled in the future. > >> > >> Maybe add something about the semantics > >> > >> "VIRTIO_BALLOON_F_PAGE_POISON will not change the behavior of free page > >> hinting or ordinary balloon inflation/deflation." > >> > >> I do wonder if we should just unconditionally enable > >> VIRTIO_BALLOON_F_PAGE_POISON here, gluing it to the QEMU compat machine > >> (via a property that is default-enabled, and disabled from compat machines). > >> > >> Because, as Michael said, knowing that the guest is using page poisoning > >> might be interesting even if free page reporting is not around. > > > > I could do that. So if that is the case though would I disable page > > reporting if it isn't enabled, or would I be enabling page poison if > > page reporting is enabled? > > > So, I would suggest this the following as a diff to this patch (the TODO part as a > separate patch - we will have these compat properties briefly after the 5.0 > release) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index c1a444cb75..2e96cba4ff 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -28,6 +28,12 @@ > #include "hw/mem/nvdimm.h" > #include "migration/vmstate.h" > > +// TODO: After 5.0 release > +GlobalProperty hw_compat_5_0[] = { > + { "virtio-balloon-device", "page-hint", "false"}, > +}; > +const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0); > + > GlobalProperty hw_compat_4_2[] = { > { "virtio-blk-device", "queue-size", "128"}, > { "virtio-scsi-device", "virtqueue_size", "128"}, Okay, so the bit above is for after 5_0 is released then? Is there a way to queue up a reminder or something so we get to it when the time comes, or I just need to watch for 5.0 to come out and submit a patch then? > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index a4729f7fc9..5ff8a669cf 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -924,6 +924,8 @@ static Property virtio_balloon_properties[] = { > qemu_4_0_config_size, false), > DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD, > IOThread *), > + DEFINE_PROP_BIT("page-poison", VirtIOBalloon, host_features, > + VIRTIO_BALLOON_F_PAGE_POISON, true), > DEFINE_PROP_END_OF_LIST(), > }; > > > What to do with page reporting depends: I would not implicitly enable features. > I think we are talking about > > -device virtio-balloon-pci,...,page-poison=false,free-page-reporting=true > > a) Valid scenario. Fix Linux guests as we discussed to not use reporting if they rely on page poisoning. Okay I will probably go this route and resubmit the patch I had submitted before that only allows us to do page reporting if poisoning is disabled on the guest kernel, or the page-poison is enabled on the hypervisor. > b) Invalid scenario. E.g., bail out when trying to realize that device ("'free-page-reporting' requires 'page-poison'"). > > With new QEMU machines, this should not happen with > > -device virtio-balloon-pci,...,free-page-reporting=true > > as page-poison=true is the new default. > > What's your opinion? I will just clean up and resubmit my earlier kernel patch, this time without it messing with free page hinting. > > > >>> > >>> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > >>> --- > >>> hw/virtio/virtio-balloon.c | 7 +++++++ > >>> include/hw/virtio/virtio-balloon.h | 1 + > >>> 2 files changed, 8 insertions(+) > >>> > >>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > >>> index a1d6fb52c876..5effc8b4653b 100644 > >>> --- a/hw/virtio/virtio-balloon.c > >>> +++ b/hw/virtio/virtio-balloon.c > >>> @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) > >>> > >>> config.num_pages = cpu_to_le32(dev->num_pages); > >>> config.actual = cpu_to_le32(dev->actual); > >>> + config.poison_val = cpu_to_le32(dev->poison_val); > >>> > >>> if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) { > >>> config.free_page_hint_cmd_id = > >>> @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, > >>> qapi_event_send_balloon_change(vm_ram_size - > >>> ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT)); > >>> } > >>> + dev->poison_val = 0; > >>> + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { > >>> + dev->poison_val = le32_to_cpu(config.poison_val); > >>> + } > >>> trace_virtio_balloon_set_config(dev->actual, oldactual); > >>> } > >>> > >>> @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev) > >>> g_free(s->stats_vq_elem); > >>> s->stats_vq_elem = NULL; > >>> } > >>> + > >>> + s->poison_val = 0; > >>> } > >>> > >>> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) > >>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h > >>> index 108cff97e71a..3ca2a78e1aca 100644 > >>> --- a/include/hw/virtio/virtio-balloon.h > >>> +++ b/include/hw/virtio/virtio-balloon.h > >>> @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon { > >>> uint32_t host_features; > >>> > >>> bool qemu_4_0_config_size; > >>> + uint32_t poison_val; > >>> } VirtIOBalloon; > >>> > >>> #endif > >>> > >> > >> You still have to migrate poison_val if I am not wrong, otherwise you > >> would lose it during migration if I am not mistaking. > > > > So what are the requirements to migrate a value? Would I need to add a > > property so it can be retrieved/set? > > > > Something like this would do the trick: > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index a4729f7fc9..ea0afec5f6 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -522,6 +522,13 @@ static bool virtio_balloon_free_page_support(void *opaque) > return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT); > } > > +static bool virtio_balloon_page_poison_support(void *opaque) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(opaque); > + > + return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON); > +} > + > static void virtio_balloon_free_page_start(VirtIOBalloon *s) > { > VirtIODevice *vdev = VIRTIO_DEVICE(s); > @@ -755,6 +762,17 @@ static const VMStateDescription vmstate_virtio_balloon_free_page_report = { > } > }; > > +static const VMStateDescription vmstate_virtio_balloon_page_poison = { > + .name = "vitio-balloon-device/page-poison", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = virtio_balloon_page_poison_support, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(poison_val, VirtIOBalloon), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_virtio_balloon_device = { > .name = "virtio-balloon-device", > .version_id = 1, > @@ -767,6 +785,7 @@ static const VMStateDescription vmstate_virtio_balloon_device = { > }, > .subsections = (const VMStateDescription * []) { > &vmstate_virtio_balloon_free_page_report, > + &vmstate_virtio_balloon_page_poison, > NULL > } > }; So I will probably go this route. It looks like that is the way we went for free page reporting so it is easy enough to just do some cut/paste/replace and have something ready to go later today without having to second guess things.
>> GlobalProperty hw_compat_4_2[] = { >> { "virtio-blk-device", "queue-size", "128"}, >> { "virtio-scsi-device", "virtqueue_size", "128"}, > > Okay, so the bit above is for after 5_0 is released then? Is there a Yes. > way to queue up a reminder or something so we get to it when the time > comes, or I just need to watch for 5.0 to come out and submit a patch > then? I think what happened usually happens is that someone introduces all the compat machines, sometimes directly with empty hw_compat. E.g., see commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4 Author: Cornelia Huck <cohuck@redhat.com> Date: Tue Nov 12 11:48:11 2019 +0100 hw: add compat machines for 5.0 Add 5.0 machine types for arm/i440fx/q35/s390x/spapr. and commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be Author: Cornelia Huck <cohuck@redhat.com> Date: Wed Jul 24 12:35:24 2019 +0200 hw: add compat machines for 4.2 Add 4.2 machine types for arm/i440fx/q35/s390x/spapr. The latter already introduced hw_compat_4_1, for examnple. @Conny, do you already have a patch for 5.1 compat patch lying around somewhere? [...] > So I will probably go this route. It looks like that is the way we > went for free page reporting so it is easy enough to just do some > cut/paste/replace and have something ready to go later today without > having to second guess things. I remember that a v1->v2 vmstate migration works (e.g., old QEMU to new QEMU). But I can't tell from the top of my head what would happen when we migrate v2->v1 (e.g., new QEMU to old QEMU). My guess is that the latter won't work, but I might be wrong. Looking at migration/vmstate.c:vmstate_load_state() "incoming version_id ... is too new for local version_id" One would have to tell the new QEMU to send via vmstate v1 when running under the compat machine. I don't recall if and how that would be possible. So the other approach is a save bet.
On Fri, 24 Apr 2020 09:07:25 +0200 David Hildenbrand <david@redhat.com> wrote: > >> GlobalProperty hw_compat_4_2[] = { > >> { "virtio-blk-device", "queue-size", "128"}, > >> { "virtio-scsi-device", "virtqueue_size", "128"}, > > > > Okay, so the bit above is for after 5_0 is released then? Is there a > > Yes. > > > way to queue up a reminder or something so we get to it when the time > > comes, or I just need to watch for 5.0 to come out and submit a patch > > then? > > I think what happened usually happens is that someone introduces all the > compat machines, sometimes directly with empty hw_compat. > > E.g., see > > commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4 > Author: Cornelia Huck <cohuck@redhat.com> > Date: Tue Nov 12 11:48:11 2019 +0100 > > hw: add compat machines for 5.0 > > Add 5.0 machine types for arm/i440fx/q35/s390x/spapr. > > and > > commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be > Author: Cornelia Huck <cohuck@redhat.com> > Date: Wed Jul 24 12:35:24 2019 +0200 > > hw: add compat machines for 4.2 > > Add 4.2 machine types for arm/i440fx/q35/s390x/spapr. > > > The latter already introduced hw_compat_4_1, for examnple. > > @Conny, do you already have a patch for 5.1 compat patch lying around > somewhere? Not yet, will do so now. Thanks for the reminder, nearly forgot about that :)
On 24.04.20 09:53, Cornelia Huck wrote: > On Fri, 24 Apr 2020 09:07:25 +0200 > David Hildenbrand <david@redhat.com> wrote: > >>>> GlobalProperty hw_compat_4_2[] = { >>>> { "virtio-blk-device", "queue-size", "128"}, >>>> { "virtio-scsi-device", "virtqueue_size", "128"}, >>> >>> Okay, so the bit above is for after 5_0 is released then? Is there a >> >> Yes. >> >>> way to queue up a reminder or something so we get to it when the time >>> comes, or I just need to watch for 5.0 to come out and submit a patch >>> then? >> >> I think what happened usually happens is that someone introduces all the >> compat machines, sometimes directly with empty hw_compat. >> >> E.g., see >> >> commit 3eb74d2087d3bd6cb51c06a49ba94222248d2de4 >> Author: Cornelia Huck <cohuck@redhat.com> >> Date: Tue Nov 12 11:48:11 2019 +0100 >> >> hw: add compat machines for 5.0 >> >> Add 5.0 machine types for arm/i440fx/q35/s390x/spapr. >> >> and >> >> commit 9aec2e52ce9d9632a86be2d1d0dd493722d2e7be >> Author: Cornelia Huck <cohuck@redhat.com> >> Date: Wed Jul 24 12:35:24 2019 +0200 >> >> hw: add compat machines for 4.2 >> >> Add 4.2 machine types for arm/i440fx/q35/s390x/spapr. >> >> >> The latter already introduced hw_compat_4_1, for examnple. >> >> @Conny, do you already have a patch for 5.1 compat patch lying around >> somewhere? > > Not yet, will do so now. Thanks for the reminder, nearly forgot about > that :) > Awesome! I just re-read my sentences and decided to consume more coffee before starting to write emails in the morning. :)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index a1d6fb52c876..5effc8b4653b 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -634,6 +634,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) config.num_pages = cpu_to_le32(dev->num_pages); config.actual = cpu_to_le32(dev->actual); + config.poison_val = cpu_to_le32(dev->poison_val); if (dev->free_page_hint_status == FREE_PAGE_HINT_S_REQUESTED) { config.free_page_hint_cmd_id = @@ -697,6 +698,10 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, qapi_event_send_balloon_change(vm_ram_size - ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT)); } + dev->poison_val = 0; + if (virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) { + dev->poison_val = le32_to_cpu(config.poison_val); + } trace_virtio_balloon_set_config(dev->actual, oldactual); } @@ -854,6 +859,8 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev) g_free(s->stats_vq_elem); s->stats_vq_elem = NULL; } + + s->poison_val = 0; } static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status) diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h index 108cff97e71a..3ca2a78e1aca 100644 --- a/include/hw/virtio/virtio-balloon.h +++ b/include/hw/virtio/virtio-balloon.h @@ -70,6 +70,7 @@ typedef struct VirtIOBalloon { uint32_t host_features; bool qemu_4_0_config_size; + uint32_t poison_val; } VirtIOBalloon; #endif