Re: [PATCH V1 09/32] savevm: prevent cprsave if memory is volatile

[Steven Sistare <steven.sistare@oracle.com>]

On 9/11/2020 1:35 PM, Dr. David Alan Gilbert wrote:
> * Steve Sistare (steven.sistare@oracle.com) wrote:
>> cprsave and cprload require that guest ram be backed by an externally
>> visible shared file.  Check that in cprsave.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  exec.c                | 32 ++++++++++++++++++++++++++++++++
>>  include/exec/memory.h |  2 ++
>>  migration/savevm.c    |  4 ++++
>>  3 files changed, 38 insertions(+)
>>
>> diff --git a/exec.c b/exec.c
>> index 6f381f9..02160e0 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2726,6 +2726,38 @@ ram_addr_t qemu_ram_addr_from_host(void *ptr)
>>      return block->offset + offset;
>>  }
>>  
>> +/*
>> + * Return true if any memory regions are writable and not backed by shared
>> + * memory.  Exclude x86 option rom shadow "pc.rom" by name, even though it is
>> + * writable.
> 
> Tell me about 'pc.rom' - this is a very odd hack.
> Again note the trick done by the existing migration capability
> x-ignore-shared ; it doesn't special case, it just doesn't migrate
> the 'shared' blocks.

pc.rom is indeed a rom.  Its contents do not change, and it can be recreated
from scratch after a restart.  However, the x86 arch code does not mark it
readonly, so there is no proper way to tell it is not volatile, and its
presence blocks cprsave reboot.  Checking for the name "pc.rom" was the only
way to recognize this anomaly, short of modifying the x86 code.

However, I initially developed the above using an old version of qemu, and
a more recent version has fixed it:

pc_memory_init()
    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
                           &error_fatal);
    if (pcmc->pci_enabled) {
        memory_region_set_readonly(option_rom_mr, true);
    }

Now memory_region_is_rom() will correctly classify this segment, and I will
happily delete the hack.

- Steve

>> + */
>> +bool qemu_ram_volatile(Error **errp)
>> +{
>> +    RAMBlock *block;
>> +    MemoryRegion *mr;
>> +    bool ret = false;
>> +
>> +    rcu_read_lock();
>> +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
>> +        mr = block->mr;
>> +        if (mr &&
>> +            memory_region_is_ram(mr) &&
>> +            !memory_region_is_ram_device(mr) &&
>> +            !memory_region_is_rom(mr) &&
>> +            (!mr->name || strcmp(mr->name, "pc.rom")) &&
>> +            (block->fd == -1 || !qemu_ram_is_shared(block))) {
>> +
>> +            error_setg(errp, "Memory region %s is volatile",
>> +                       memory_region_name(mr));
>> +            ret = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    rcu_read_unlock();
>> +    return ret;
>> +}
>> +
>>  /* Generate a debug exception if a watchpoint has been hit.  */
>>  void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
>>                            MemTxAttrs attrs, int flags, uintptr_t ra)
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 307e527..6aafbb0 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -2519,6 +2519,8 @@ bool ram_block_discard_is_disabled(void);
>>   */
>>  bool ram_block_discard_is_required(void);
>>  
>> +bool qemu_ram_volatile(Error **errp);
>> +
>>  #endif
>>  
>>  #endif
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 1509173..f101039 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2713,6 +2713,10 @@ void save_cpr_snapshot(const char *file, const char *mode, Error **errp)
>>          return;
>>      }
>>  
>> +    if (op == VMS_REBOOT && qemu_ram_volatile(errp)) {
>> +        return;
>> +    }
>> +
>>      f = qf_file_open(file, O_CREAT | O_WRONLY | O_TRUNC, 0600, errp);
>>      if (!f) {
>>          return;
>> -- 
>> 1.8.3.1
>>