diff mbox

[v4,3/6] kvm: support using KVM_MEM_READONLY flag for readonly regions

Message ID 1367946947-17109-4-git-send-email-jordan.l.justen@intel.com
State New
Headers show

Commit Message

jordan.l.justen@intel.com May 7, 2013, 5:15 p.m. UTC 
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 <jordan.l.justen@intel.com>
Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 kvm-all.c |   36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

Comments

Paolo Bonzini May 7, 2013, 8:35 p.m. UTC | #1 
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 <jordan.l.justen@intel.com>
> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  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
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.

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) {
>
Jordan Justen May 7, 2013, 10:01 p.m. UTC | #2 
On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <jordan.l.justen@intel.com>
>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  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) {
>>
>
>
Peter Maydell May 7, 2013, 10:12 p.m. UTC | #3 
On 7 May 2013 23:01, Jordan Justen <jljusten@gmail.com> wrote:
> I think qemu would be better served by mr->readtrap and mr->writetrap booleans.

I'm not convinced, because from QEMU's point of view
"trap" ought to mean "deliver a fault to the guest",
which isn't what we want to do for writes here.

-- PMM
Paolo Bonzini May 7, 2013, 10:25 p.m. UTC | #4 
Il 08/05/2013 00:01, Jordan Justen ha scritto:
> On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <jordan.l.justen@intel.com>
>>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>> ---
>>>  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) {
>>>
>>
>>
> 
>
Jordan Justen May 7, 2013, 11:37 p.m. UTC | #5 
On Tue, May 7, 2013 at 3:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/05/2013 00:01, Jordan Justen ha scritto:
>> On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <jordan.l.justen@intel.com>
>>>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>>> ---
>>>>  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
Jan Kiszka May 8, 2013, 12:56 a.m. UTC | #6 
On 2013-05-08 01:37, Jordan Justen wrote:
> On Tue, May 7, 2013 at 3:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 08/05/2013 00:01, Jordan Justen ha scritto:
>>> On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <pbonzini@redhat.com> 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 <jordan.l.justen@intel.com>
>>>>> Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>>>> ---
>>>>>  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.

This is wrong. The semantic of readonly is that writes shall be ignored
by the emulation. But a flash device is _never_ readonly - otherwise you
couldn't send the magic write-enable commands to them.

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

Also wrong. A ROMD device is always readable (hence my patch to rename
"readable" to "romd_mode" - it is misleading). What is changed via
"readable" is who handles the read access, RAM or a device model.

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

Patch 4 is not correct. And "trapping" is, as Peter mentioned, a
misleading term. You are talking about trapping /wrt KVM. But that's an
internal thing, nothing a memory region user has to care about.

What you need for proper KVM mapping:
 - region is readonly -> KVM read-only memslot on backing RAM, writes
   shall be ignored (in user space)
 - memory_region_is_romd() -> KVM read-only memslot, writes go to the
   MMIO handler of the region

IOW, a read-only KVM slot is required if readonly || is_romd. That
should be all.

Jan
diff mbox

Patch

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;
+        }
     }
 
     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) {