[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/6] kvm: support using KVM_MEM_READONLY flag
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/6] kvm: support using KVM_MEM_READONLY flag for readonly regions |
Date: |
Wed, 08 May 2013 02:56:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2013-05-08 01:37, Jordan Justen wrote:
> On Tue, May 7, 2013 at 3:25 PM, Paolo Bonzini <address@hidden> wrote:
>> Il 08/05/2013 00:01, Jordan Justen ha scritto:
>>> On Tue, May 7, 2013 at 1:35 PM, Paolo Bonzini <address@hidden> 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 <address@hidden>
>>>>> Reviewed-by: Xiao Guangrong <address@hidden>
>>>>> ---
>>>>> 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
- [Qemu-devel] [PATCH v4 2/6] kvm: add kvm_readonly_mem_enabled, (continued)
[Qemu-devel] [PATCH v4 1/6] isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS), Jordan Justen, 2013/05/07
[Qemu-devel] [PATCH v4 6/6] pc_sysfw: change rom_only default to 0, Jordan Justen, 2013/05/07
Re: [Qemu-devel] [PATCH v4 0/6] KVM flash memory support, Paolo Bonzini, 2013/05/07