On 2013-05-08 01:37, Jordan Justen wrote:
> 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.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
signature.asc
Description: OpenPGP digital signature
