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