Message ID | 1367946947-17109-4-git-send-email-jordan.l.justen@intel.com |
---|---|
State | New |
Headers | show |
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 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. 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) { >
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 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) { >> > >
On 7 May 2013 23:01, Jordan Justen <jljusten@gmail.com> wrote:
> I think qemu would be better served by mr->readtrap and mr->writetrap booleans.
I'm not convinced, because from QEMU's point of view
"trap" ought to mean "deliver a fault to the guest",
which isn't what we want to do for writes here.
-- PMM
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. 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) { >>> >> >> > >
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. 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
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
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; + } } 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) {