qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH v4 3/6] kvm: support using KVM_MEM_READONLY flag for readonly regions
Date: Wed, 08 May 2013 00:25:29 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4

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.

Paolo

> 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) {
>>>
>>
>>
> 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]