On 17.06.20 19:59, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@xxxxxxxxxx) 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@xxxxxxxxxx>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@xxxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>
> Yep OK, but some comment below
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@xxxxxxxxxx>
>
>> ---
>> 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!
--
Thanks,
David / dhildenb
