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 3:25 PM, Paolo Bonzini <address@hidden> wrote:
> Il 08/05/2013 00:01, Jordan Justen ha scritto:
>> 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
>
> mtree_print_mr tells us how it really goes:
>
>          mr->readable ? 'R' : '-',
>         !mr->readonly && !(mr->rom_device && mr->readable) ? 'W'
>                                                            : '-',
>
> So perhaps
>
>         bool readable = mr->readable;
>         bool writeable = !mr->readonly && !memory_region_is_romd(mr);
>         assert(!(writeable && !readable));
>         if (writeable || !kvm_readonly_mem_allowed) {
>             return;
>         }
>         if (!readable) {
>             /* If the memory range is not readable, then we actually want
>              * to remove the kvm memory slot so all accesses will trap. */
>             add = false;
>         }
>
> This should still make patch 4 unnecessary.

Patch 4 sets readonly for the pflash device. Basically saying, it
always should trap on writes.

memory_region_is_romd seems to have more to do with the readability of
the range.

Patch 4 would seem to make more sense if written as
memory_region_set_write_trapping(mr, true).

-Jordan