On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <pbonz...@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.jus...@intel.com>
>> Reviewed-by: Xiao Guangrong <xiaoguangr...@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) {
>>
>
>
