On Tue, May 7, 2013 at 3:25 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > Il 08/05/2013 00:01, Jordan Justen ha scritto: >> 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 > > 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