| Message ID | 20200610115419.51688-19-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-mem: Paravirtualized memory hot(un)plug | expand |
* David Hildenbrand (david@redhat.com) wrote: > We want to make sure that certain properties don't change during > migration, especially to catch user errors in a nice way. Let's migrate > a temporary structure and validate that the properties didn't change. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> Yep OK, but some comment below Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/virtio/virtio-mem.c | 69 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c > index 2df33f9125..450b8dc49d 100644 > --- a/hw/virtio/virtio-mem.c > +++ b/hw/virtio/virtio-mem.c > @@ -519,12 +519,81 @@ static int virtio_mem_post_load(void *opaque, int version_id) > return virtio_mem_restore_unplugged(VIRTIO_MEM(opaque)); > } > > +typedef struct VirtIOMEMMigSanityChecks { > + VirtIOMEM *parent; > + uint64_t addr; > + uint64_t region_size; > + uint64_t block_size; > + uint32_t node; > +} VirtIOMEMMigSanityChecks; > + > +static int virtio_mem_mig_sanity_checks_pre_save(void *opaque) > +{ > + VirtIOMEMMigSanityChecks *tmp = opaque; > + VirtIOMEM *vmem = tmp->parent; > + > + tmp->addr = vmem->addr; > + tmp->region_size = memory_region_size(&vmem->memdev->mr); > + tmp->block_size = vmem->block_size; > + tmp->node = vmem->node; > + return 0; > +} > + > +static int virtio_mem_mig_sanity_checks_post_load(void *opaque, int version_id) > +{ > + VirtIOMEMMigSanityChecks *tmp = opaque; > + VirtIOMEM *vmem = tmp->parent; > + const uint64_t new_region_size = memory_region_size(&vmem->memdev->mr); > + > + if (tmp->addr != vmem->addr) { > + error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64, > + VIRTIO_MEM_ADDR_PROP, tmp->addr, vmem->addr); > + return -EINVAL; > + } It seems weird that you do 'Property ...' and then the string; although you only do it for 3 out of 4. I was going to ask you to include the device name here, but I'm guessing when it fails the outer migration code will print a 'Failed loading device.....' so at least you know what it is. I would want it to be obvious when I see a 'region size changed' that I knew it was my virtio-mem device that was screwy. Dave > + /* > + * Note: Preparation for resizeable memory regions. The maximum size > + * of the memory region must not change during migration. > + */ > + if (tmp->region_size != new_region_size) { > + error_report("region size changed from 0x%" PRIx64 " to 0x%" PRIx64, > + tmp->region_size, new_region_size); > + return -EINVAL; > + } > + if (tmp->block_size != vmem->block_size) { > + error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64, > + VIRTIO_MEM_BLOCK_SIZE_PROP, tmp->block_size, > + vmem->block_size); > + return -EINVAL; > + } > + if (tmp->node != vmem->node) { > + error_report("Property '%s' changed from %" PRIu32 " to %" PRIu32, > + VIRTIO_MEM_NODE_PROP, tmp->node, vmem->node); > + return -EINVAL; > + } > + return 0; > +} > + > +static const VMStateDescription vmstate_virtio_mem_sanity_checks = { > + .name = "virtio-mem-device/sanity-checks", > + .pre_save = virtio_mem_mig_sanity_checks_pre_save, > + .post_load = virtio_mem_mig_sanity_checks_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(addr, VirtIOMEMMigSanityChecks), > + VMSTATE_UINT64(region_size, VirtIOMEMMigSanityChecks), > + VMSTATE_UINT64(block_size, VirtIOMEMMigSanityChecks), > + VMSTATE_UINT32(node, VirtIOMEMMigSanityChecks), > + VMSTATE_END_OF_LIST(), > + }, > +}; > + > static const VMStateDescription vmstate_virtio_mem_device = { > .name = "virtio-mem-device", > .minimum_version_id = 1, > .version_id = 1, > .post_load = virtio_mem_post_load, > .fields = (VMStateField[]) { > + VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks, > + vmstate_virtio_mem_sanity_checks), > VMSTATE_UINT64(usable_region_size, VirtIOMEM), > VMSTATE_UINT64(size, VirtIOMEM), > VMSTATE_UINT64(requested_size, VirtIOMEM), > -- > 2.26.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 17.06.20 19:59, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> We want to make sure that certain properties don't change during >> migration, especially to catch user errors in a nice way. Let's migrate >> a temporary structure and validate that the properties didn't change. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Yep OK, but some comment below > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> --- >> hw/virtio/virtio-mem.c | 69 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 69 insertions(+) >> >> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c >> index 2df33f9125..450b8dc49d 100644 >> --- a/hw/virtio/virtio-mem.c >> +++ b/hw/virtio/virtio-mem.c >> @@ -519,12 +519,81 @@ static int virtio_mem_post_load(void *opaque, int version_id) >> return virtio_mem_restore_unplugged(VIRTIO_MEM(opaque)); >> } >> >> +typedef struct VirtIOMEMMigSanityChecks { >> + VirtIOMEM *parent; >> + uint64_t addr; >> + uint64_t region_size; >> + uint64_t block_size; >> + uint32_t node; >> +} VirtIOMEMMigSanityChecks; >> + >> +static int virtio_mem_mig_sanity_checks_pre_save(void *opaque) >> +{ >> + VirtIOMEMMigSanityChecks *tmp = opaque; >> + VirtIOMEM *vmem = tmp->parent; >> + >> + tmp->addr = vmem->addr; >> + tmp->region_size = memory_region_size(&vmem->memdev->mr); >> + tmp->block_size = vmem->block_size; >> + tmp->node = vmem->node; >> + return 0; >> +} >> + >> +static int virtio_mem_mig_sanity_checks_post_load(void *opaque, int version_id) >> +{ >> + VirtIOMEMMigSanityChecks *tmp = opaque; >> + VirtIOMEM *vmem = tmp->parent; >> + const uint64_t new_region_size = memory_region_size(&vmem->memdev->mr); >> + >> + if (tmp->addr != vmem->addr) { >> + error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64, >> + VIRTIO_MEM_ADDR_PROP, tmp->addr, vmem->addr); >> + return -EINVAL; >> + } > > It seems weird that you do 'Property ...' and then the string; although > you only do it for 3 out of 4. Yeah, because it is a derived property of the memdev. I can just make that clearer by referencing the memdev property. > I was going to ask you to include the device name here, but I'm guessing > when it fails the outer migration code will print a 'Failed loading > device.....' so at least you know what it is. > I would want it to be obvious when I see a 'region size changed' that I > knew it was my virtio-mem device that was screwy. IIRC that should be the case, yes. Thanks!
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index 2df33f9125..450b8dc49d 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -519,12 +519,81 @@ static int virtio_mem_post_load(void *opaque, int version_id) return virtio_mem_restore_unplugged(VIRTIO_MEM(opaque)); } +typedef struct VirtIOMEMMigSanityChecks { + VirtIOMEM *parent; + uint64_t addr; + uint64_t region_size; + uint64_t block_size; + uint32_t node; +} VirtIOMEMMigSanityChecks; + +static int virtio_mem_mig_sanity_checks_pre_save(void *opaque) +{ + VirtIOMEMMigSanityChecks *tmp = opaque; + VirtIOMEM *vmem = tmp->parent; + + tmp->addr = vmem->addr; + tmp->region_size = memory_region_size(&vmem->memdev->mr); + tmp->block_size = vmem->block_size; + tmp->node = vmem->node; + return 0; +} + +static int virtio_mem_mig_sanity_checks_post_load(void *opaque, int version_id) +{ + VirtIOMEMMigSanityChecks *tmp = opaque; + VirtIOMEM *vmem = tmp->parent; + const uint64_t new_region_size = memory_region_size(&vmem->memdev->mr); + + if (tmp->addr != vmem->addr) { + error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64, + VIRTIO_MEM_ADDR_PROP, tmp->addr, vmem->addr); + return -EINVAL; + } + /* + * Note: Preparation for resizeable memory regions. The maximum size + * of the memory region must not change during migration. + */ + if (tmp->region_size != new_region_size) { + error_report("region size changed from 0x%" PRIx64 " to 0x%" PRIx64, + tmp->region_size, new_region_size); + return -EINVAL; + } + if (tmp->block_size != vmem->block_size) { + error_report("Property '%s' changed from 0x%" PRIx64 " to 0x%" PRIx64, + VIRTIO_MEM_BLOCK_SIZE_PROP, tmp->block_size, + vmem->block_size); + return -EINVAL; + } + if (tmp->node != vmem->node) { + error_report("Property '%s' changed from %" PRIu32 " to %" PRIu32, + VIRTIO_MEM_NODE_PROP, tmp->node, vmem->node); + return -EINVAL; + } + return 0; +} + +static const VMStateDescription vmstate_virtio_mem_sanity_checks = { + .name = "virtio-mem-device/sanity-checks", + .pre_save = virtio_mem_mig_sanity_checks_pre_save, + .post_load = virtio_mem_mig_sanity_checks_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT64(addr, VirtIOMEMMigSanityChecks), + VMSTATE_UINT64(region_size, VirtIOMEMMigSanityChecks), + VMSTATE_UINT64(block_size, VirtIOMEMMigSanityChecks), + VMSTATE_UINT32(node, VirtIOMEMMigSanityChecks), + VMSTATE_END_OF_LIST(), + }, +}; + static const VMStateDescription vmstate_virtio_mem_device = { .name = "virtio-mem-device", .minimum_version_id = 1, .version_id = 1, .post_load = virtio_mem_post_load, .fields = (VMStateField[]) { + VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks, + vmstate_virtio_mem_sanity_checks), VMSTATE_UINT64(usable_region_size, VirtIOMEM), VMSTATE_UINT64(size, VirtIOMEM), VMSTATE_UINT64(requested_size, VirtIOMEM),
We want to make sure that certain properties don't change during migration, especially to catch user errors in a nice way. Let's migrate a temporary structure and validate that the properties didn't change. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/virtio/virtio-mem.c | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)