Re: [Qemu-devel] [PATCH v4 3/6] kvm: support using KVM_MEM_READONLY flag for readonly regions

[Jordan Justen]

On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <address@hidden> wrote:
> Il 07/05/2013 19:15, Jordan Justen ha scritto:
>> A slot that uses KVM_MEM_READONLY can be read from and code
>> can execute from the region, but writes will trap.
>>
>> For regions that are readonly and also not writeable, we
>> force the slot to be removed so reads or writes to the region
>> will trap. (A memory region in this state is not executable
>> within kvm.)
>>
>> Signed-off-by: Jordan Justen <address@hidden>
>> Reviewed-by: Xiao Guangrong <address@hidden>
>> ---
>>  kvm-all.c |   36 +++++++++++++++++++++++++++---------
>>  1 file changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 1686adc..fffd2f4 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -201,12 +201,18 @@ static int kvm_set_user_memory_region(KVMState *s, 
>> KVMSlot *slot)
>>
>>      mem.slot = slot->slot;
>>      mem.guest_phys_addr = slot->start_addr;
>> -    mem.memory_size = slot->memory_size;
>>      mem.userspace_addr = (unsigned long)slot->ram;
>>      mem.flags = slot->flags;
>>      if (s->migration_log) {
>>          mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
>>      }
>> +    if (mem.flags & KVM_MEM_READONLY && mem.memory_size != 0) {
>> +        /* Set the slot size to 0 before setting the slot to the desired
>> +         * value. This is needed based on KVM commit 75d61fbc. */
>> +        mem.memory_size = 0;
>> +        kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>> +    }
>> +    mem.memory_size = slot->memory_size;
>>      return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
>>  }
>>
>> @@ -268,9 +274,14 @@ err:
>>   * dirty pages logging control
>>   */
>>
>> -static int kvm_mem_flags(KVMState *s, bool log_dirty)
>> +static int kvm_mem_flags(KVMState *s, bool log_dirty, bool readonly)
>>  {
>> -    return log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
>> +    int flags = 0;
>> +    flags = log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
>> +    if (readonly && kvm_readonly_mem_allowed) {
>> +        flags |= KVM_MEM_READONLY;
>> +    }
>> +    return flags;
>>  }
>>
>>  static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
>> @@ -281,7 +292,7 @@ static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, 
>> bool log_dirty)
>>
>>      old_flags = mem->flags;
>>
>> -    flags = (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty);
>> +    flags = (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty, false);
>>      mem->flags = flags;
>>
>>      /* If nothing changed effectively, no need to issue ioctl */
>> @@ -638,7 +649,14 @@ static void kvm_set_phys_mem(MemoryRegionSection 
>> *section, bool add)
>>      }
>>
>>      if (!memory_region_is_ram(mr)) {
>> -        return;
>> +        if (!mr->readonly || !kvm_readonly_mem_allowed) {
>> +            return;
>> +        } else if (!mr->readable && add) {
>> +            /* If the memory range is not readable, then we actually want
>> +             * to remove the kvm memory slot so all accesses will trap. */
>> +            assert(mr->readonly && kvm_readonly_mem_allowed);
>> +            add = false;
>> +        }
>
> mr->readable really means "read from ROM, write to device", hence it

"read from ROM, write to device" confuses me.

Here is how I currently to interpret things:
!mr->readable: trap on read
mr->readonly: trap on write

KVM:
* trap on reads and writes: no mem slot
* trap on writes, but not on reads: readonly slot
* trap on reads, but not on writes: not supported
* never trap: normal ram slot

In kvm, I think this is the best we can do:
if (!mr->readable)
    no mem slot so traps always happen
else if (mr->readonly)
    add slot with kvm readonly support
else
    add slot as ram so traps never occur

(Clearly things aren't so simple in the kvm code.)

I think qemu would be better served by mr->readtrap and mr->writetrap booleans.

> should always be read-only from KVM's point of view.
>
> I think this should be
>
>      if (!memory_region_is_ram(mr)) {
>          if (!mr->readable) {
>              return;
>          }
>          assert(kvm_readonly_mem_allowed);
>      }
>
> with occurrences below of mr->readonly, like
>
>          mem->flags = kvm_mem_flags(s, log_dirty, mr->readonly);
>
> changed to mr->readonly || mr->readable.

I did try these changes, and kvm became very angry. :)

-Jordan

> This should eliminate the need for patch 4, too.
>
> Should have pointed out this before.  I'm just learning this stuff as
> well...
>
> Paolo
>
>
>>      }
>>
>>      ram = memory_region_get_ram_ptr(mr) + section->offset_within_region + 
>> delta;
>> @@ -687,7 +705,7 @@ static void kvm_set_phys_mem(MemoryRegionSection 
>> *section, bool add)
>>              mem->memory_size = old.memory_size;
>>              mem->start_addr = old.start_addr;
>>              mem->ram = old.ram;
>> -            mem->flags = kvm_mem_flags(s, log_dirty);
>> +            mem->flags = kvm_mem_flags(s, log_dirty, mr->readonly);
>>
>>              err = kvm_set_user_memory_region(s, mem);
>>              if (err) {
>> @@ -708,7 +726,7 @@ static void kvm_set_phys_mem(MemoryRegionSection 
>> *section, bool add)
>>              mem->memory_size = start_addr - old.start_addr;
>>              mem->start_addr = old.start_addr;
>>              mem->ram = old.ram;
>> -            mem->flags =  kvm_mem_flags(s, log_dirty);
>> +            mem->flags =  kvm_mem_flags(s, log_dirty, mr->readonly);
>>
>>              err = kvm_set_user_memory_region(s, mem);
>>              if (err) {
>> @@ -732,7 +750,7 @@ static void kvm_set_phys_mem(MemoryRegionSection 
>> *section, bool add)
>>              size_delta = mem->start_addr - old.start_addr;
>>              mem->memory_size = old.memory_size - size_delta;
>>              mem->ram = old.ram + size_delta;
>> -            mem->flags = kvm_mem_flags(s, log_dirty);
>> +            mem->flags = kvm_mem_flags(s, log_dirty, mr->readonly);
>>
>>              err = kvm_set_user_memory_region(s, mem);
>>              if (err) {
>> @@ -754,7 +772,7 @@ static void kvm_set_phys_mem(MemoryRegionSection 
>> *section, bool add)
>>      mem->memory_size = size;
>>      mem->start_addr = start_addr;
>>      mem->ram = ram;
>> -    mem->flags = kvm_mem_flags(s, log_dirty);
>> +    mem->flags = kvm_mem_flags(s, log_dirty, mr->readonly);
>>
>>      err = kvm_set_user_memory_region(s, mem);
>>      if (err) {
>>
>
>