Re: [PATCH v4 18/21] virtio-mem: Migration sanity checks

[David Hildenbrand <david@redhat.com>]

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!


-- 
Thanks,

David / dhildenb