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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5964 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]