[Qemu-devel] [PATCH v4 0/6] KVM flash memory support

[Qemu-devel] [PATCH v4 0/6] KVM flash memory support

[Jordan Justen]

git://github.com/jljusten/qemu.git kvm-flash-v4

Utilize KVM_CAP_READONLY_MEM to support PC system flash emulation
with KVM.

v4:
 * With a machine type of isapc, don't mark the BIOS as read-only.
   isapc + seabios will not boot if the BIOS is read-only. This
   matches the current behavior of isapc with KVM, which is the
   only mode under which isapc currently works.

v3:
 * Squash patch 2 & 3 based on Xiao's feedback that what I
   was calling a 'workaround' in patch 3 was actually what
   is required by the KVM READONLY memory support.

v2:
 * Remove rom_only from PC_COMPAT_1_4
 * Only enable flash when a pflash drive is created.

Jordan Justen (6):
  isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)
  kvm: add kvm_readonly_mem_enabled
  kvm: support using KVM_MEM_READONLY flag for readonly regions
  pflash_cfi01: memory region should be set to enable readonly mode
  pc_sysfw: allow flash (-pflash) memory to be used with KVM
  pc_sysfw: change rom_only default to 0

 hw/block/pc_sysfw.c     |   64 ++++++++++++++++++++++++++++++-----------------
 hw/block/pflash_cfi01.c |    2 ++
 hw/i386/pc_piix.c       |    5 ++++
 include/hw/i386/pc.h    |    4 ---
 include/sysemu/kvm.h    |   10 ++++++++
 kvm-all.c               |   42 ++++++++++++++++++++++++-------
 kvm-stub.c              |    1 +
 7 files changed, 92 insertions(+), 36 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v4 1/6] isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)

[Jordan Justen]

The isapc machine with seabios currently requires the BIOS region
to be read/write memory rather than read-only memory.

KVM currently cannot support the BIOS as a ROM region, but qemu
in non-KVM mode can. Based on this, isapc machine currently only
works with KVM.

To work-around this isapc issue, this change avoids marking the
BIOS as readonly for isapc.

This change also will allow KVM to start supporting ROM mode
via KVM_CAP_READONLY_MEM.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/block/pc_sysfw.c |   14 ++++++++++----
 hw/i386/pc_piix.c   |    5 +++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index aad8614..90894af 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -39,6 +39,7 @@
 typedef struct PcSysFwDevice {
     SysBusDevice busdev;
     uint8_t rom_only;
+    uint8_t isapc_ram_fw;
 } PcSysFwDevice;
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
@@ -139,7 +140,7 @@ static void pc_system_flash_init(MemoryRegion *rom_memory,
     pc_isa_bios_init(rom_memory, flash_mem, size);
 }
 
-static void old_pc_system_rom_init(MemoryRegion *rom_memory)
+static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
     char *filename;
     MemoryRegion *bios, *isa_bios;
@@ -163,7 +164,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
     bios = g_malloc(sizeof(*bios));
     memory_region_init_ram(bios, "pc.bios", bios_size);
     vmstate_register_ram_global(bios);
-    memory_region_set_readonly(bios, true);
+    if (!isapc_ram_fw) {
+        memory_region_set_readonly(bios, true);
+    }
     ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
     if (ret != 0) {
     bios_error:
@@ -186,7 +189,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
                                         0x100000 - isa_bios_size,
                                         isa_bios,
                                         1);
-    memory_region_set_readonly(isa_bios, true);
+    if (!isapc_ram_fw) {
+        memory_region_set_readonly(isa_bios, true);
+    }
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,
@@ -216,7 +221,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
     qdev_init_nofail(DEVICE(sysfw_dev));
 
     if (sysfw_dev->rom_only) {
-        old_pc_system_rom_init(rom_memory);
+        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
         return;
     }
 
@@ -255,6 +260,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 }
 
 static Property pcsysfw_properties[] = {
+    DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
     DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fe52e5f..0e4d3b1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -715,6 +715,11 @@ static QEMUMachine isapc_machine = {
             .property = "rom_only",
             .value    = stringify(1),
         },
+        {
+            .driver   = "pc-sysfw",
+            .property = "isapc_ram_fw",
+            .value    = stringify(1),
+        },
         { /* end of list */ }
     },
     DEFAULT_MACHINE_OPTIONS,
-- 
1.7.10.4

[Qemu-devel] [PATCH v4 2/6] kvm: add kvm_readonly_mem_enabled

[Jordan Justen]

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 include/sysemu/kvm.h |   10 ++++++++++
 kvm-all.c            |    6 ++++++
 kvm-stub.c           |    1 +
 3 files changed, 17 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 9735c1d..13c4b2e 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -45,6 +45,7 @@ extern bool kvm_async_interrupts_allowed;
 extern bool kvm_irqfds_allowed;
 extern bool kvm_msi_via_irqfd_allowed;
 extern bool kvm_gsi_routing_allowed;
+extern bool kvm_readonly_mem_allowed;
 
 #if defined CONFIG_KVM || !defined NEED_CPU_H
 #define kvm_enabled()           (kvm_allowed)
@@ -97,6 +98,14 @@ extern bool kvm_gsi_routing_allowed;
  */
 #define kvm_gsi_routing_enabled() (kvm_gsi_routing_allowed)
 
+/**
+ * kvm_readonly_mem_enabled:
+ *
+ * Returns: true if KVM readonly memory is enabled (ie the kernel
+ * supports it and we're running in a configuration that permits it).
+ */
+#define kvm_readonly_mem_enabled() (kvm_readonly_mem_allowed)
+
 #else
 #define kvm_enabled()           (0)
 #define kvm_irqchip_in_kernel() (false)
@@ -104,6 +113,7 @@ extern bool kvm_gsi_routing_allowed;
 #define kvm_irqfds_enabled() (false)
 #define kvm_msi_via_irqfd_enabled() (false)
 #define kvm_gsi_routing_allowed() (false)
+#define kvm_readonly_mem_enabled() (false)
 #endif
 
 struct kvm_run;
diff --git a/kvm-all.c b/kvm-all.c
index 3a31602..1686adc 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -111,6 +111,7 @@ bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
 bool kvm_allowed;
+bool kvm_readonly_mem_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
@@ -1425,6 +1426,11 @@ int kvm_init(void)
         s->irq_set_ioctl = KVM_IRQ_LINE_STATUS;
     }
 
+#ifdef KVM_CAP_READONLY_MEM
+    kvm_readonly_mem_allowed =
+        (kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0);
+#endif
+
     ret = kvm_arch_init(s);
     if (ret < 0) {
         goto err;
diff --git a/kvm-stub.c b/kvm-stub.c
index b2c8f9b..22eaff0 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -26,6 +26,7 @@ bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
 bool kvm_allowed;
+bool kvm_readonly_mem_allowed;
 
 int kvm_init_vcpu(CPUState *cpu)
 {
-- 
1.7.10.4

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

[Jordan Justen]

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;
+        }
     }
 
     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) {
-- 
1.7.10.4

[Qemu-devel] [PATCH v4 4/6] pflash_cfi01: memory region should be set to enable readonly mode

[Jordan Justen]

This causes any writes to the memory region to trap to the
device handler.

This is also important for KVM, because this allows the memory
region to be set using KVM_MEM_READONLY, which allows the memory
region to be read & executed. (Without this, KVM will not support
executing from the memory region.)

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/block/pflash_cfi01.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 3ff20e0..b65225e 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -596,6 +596,8 @@ static int pflash_cfi01_init(SysBusDevice *dev)
         }
     }
 
+    memory_region_set_readonly(&pfl->mem, true);
+
     if (pfl->bs) {
         pfl->ro = bdrv_is_read_only(pfl->bs);
     } else {
-- 
1.7.10.4

[Qemu-devel] [PATCH v4 5/6] pc_sysfw: allow flash (-pflash) memory to be used with KVM

[Jordan Justen]

When pc-sysfw.rom_only != 0, flash memory will be
usable with kvm. In order to enable flash memory mode,
a pflash device must be created. (For example, by
using the -pflash command line parameter.)

Usage of a flash memory device with kvm requires the
KVM READONLY memory capability, and kvm will abort if
a flash device is used with an older kvm which does
not support this capability.

If a flash device is not used, then qemu/kvm will
operate in the original rom-mode.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/block/pc_sysfw.c |   50 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index 90894af..d8a499d 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -220,28 +220,40 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
     qdev_init_nofail(DEVICE(sysfw_dev));
 
-    if (sysfw_dev->rom_only) {
-        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
-        return;
-    }
-
     pflash_drv = drive_get(IF_PFLASH, 0, 0);
 
-    /* Currently KVM cannot execute from device memory.
-       Use old rom based firmware initialization for KVM. */
-    /*
-     * This is a Bad Idea, because it makes enabling/disabling KVM
-     * guest-visible.  Do it only in bug-compatibility mode.
-     */
-    if (pc_sysfw_flash_vs_rom_bug_compatible && kvm_enabled()) {
-        if (pflash_drv != NULL) {
-            fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
-            exit(1);
-        } else {
-            sysfw_dev->rom_only = 1;
-            old_pc_system_rom_init(rom_memory);
-            return;
+    if (pc_sysfw_flash_vs_rom_bug_compatible) {
+        /*
+         * This is a Bad Idea, because it makes enabling/disabling KVM
+         * guest-visible.  Do it only in bug-compatibility mode.
+         */
+        if (kvm_enabled()) {
+            if (pflash_drv != NULL) {
+                fprintf(stderr, "qemu: pflash cannot be used with kvm enabled\n");
+                exit(1);
+            } else {
+                /* In old pc_sysfw_flash_vs_rom_bug_compatible mode, we assume
+                 * that KVM cannot execute from device memory. In this case, we
+                 * use old rom based firmware initialization for KVM. But, since
+                 * this is different from non-kvm mode, this behavior is
+                 * undesirable */
+                sysfw_dev->rom_only = 1;
+            }
         }
+    } else if (pflash_drv == NULL) {
+        /* When a pflash drive is not found, use rom-mode */
+        sysfw_dev->rom_only = 1;
+    } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
+        /* Older KVM cannot execute from device memory. So, flash memory
+         * cannot be used unless the readonly memory kvm capability is present. */
+        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
+        exit(1);
+    }
+
+    /* If rom-mode is active, use the old pc system rom initialization. */
+    if (sysfw_dev->rom_only) {
+        old_pc_system_rom_init(rom_memory, sysfw_dev->isapc_ram_fw);
+        return;
     }
 
     /* If a pflash drive is not found, then create one using
-- 
1.7.10.4

[Qemu-devel] [PATCH v4 6/6] pc_sysfw: change rom_only default to 0

[Jordan Justen]

Now KVM can support a flash memory. This feature depends on
KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.

Flash memory will only be enabled if a pflash device is
created. (For example, by using the -pflash command line
parameter.)

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/block/pc_sysfw.c  |    2 +-
 include/hw/i386/pc.h |    4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index d8a499d..259fe5d 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -273,7 +273,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 
 static Property pcsysfw_properties[] = {
     DEFINE_PROP_UINT8("isapc_ram_fw", PcSysFwDevice, isapc_ram_fw, 0),
-    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
+    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 41869e5..10c9347 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -238,10 +238,6 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .driver   = "virtio-net-pci",\
             .property = "romfile",\
             .value    = "pxe-virtio.rom",\
-        },{\
-            .driver   = "pc-sysfw",\
-            .property = "rom_only",\
-            .value    = stringify(0),\
         }
 
 #endif
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v4 0/6] KVM flash memory support

[Paolo Bonzini]

Il 07/05/2013 19:15, Jordan Justen ha scritto:
> git://github.com/jljusten/qemu.git kvm-flash-v4
> 
> Utilize KVM_CAP_READONLY_MEM to support PC system flash emulation
> with KVM.
> 
> v4:
>  * With a machine type of isapc, don't mark the BIOS as read-only.
>    isapc + seabios will not boot if the BIOS is read-only. This
>    matches the current behavior of isapc with KVM, which is the
>    only mode under which isapc currently works.

Thanks for looking at it!

Patch 1 should definitely be 1.5 material.  At this point it is late for
1.5 and I found a small problem in patch 3, so let's defer it.

However, I'm thinking of reverting commit 9953f8822 in 1.5.  Yes, it's a
bugfix but the change is only visible in TCG and having three modes in
three versions is uselessly complicated.

Paolo

> v3:
>  * Squash patch 2 & 3 based on Xiao's feedback that what I
>    was calling a 'workaround' in patch 3 was actually what
>    is required by the KVM READONLY memory support.
> 
> v2:
>  * Remove rom_only from PC_COMPAT_1_4
>  * Only enable flash when a pflash drive is created.
> 
> Jordan Justen (6):
>   isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)
>   kvm: add kvm_readonly_mem_enabled
>   kvm: support using KVM_MEM_READONLY flag for readonly regions
>   pflash_cfi01: memory region should be set to enable readonly mode
>   pc_sysfw: allow flash (-pflash) memory to be used with KVM
>   pc_sysfw: change rom_only default to 0
> 
>  hw/block/pc_sysfw.c     |   64 ++++++++++++++++++++++++++++++-----------------
>  hw/block/pflash_cfi01.c |    2 ++
>  hw/i386/pc_piix.c       |    5 ++++
>  include/hw/i386/pc.h    |    4 ---
>  include/sysemu/kvm.h    |   10 ++++++++
>  kvm-all.c               |   42 ++++++++++++++++++++++++-------
>  kvm-stub.c              |    1 +
>  7 files changed, 92 insertions(+), 36 deletions(-)
> 

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

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v4 4/6] pflash_cfi01: memory region should be set to enable readonly mode

[Paolo Bonzini]

Il 07/05/2013 19:15, Jordan Justen ha scritto:
> This causes any writes to the memory region to trap to the
> device handler.
> 
> This is also important for KVM, because this allows the memory
> region to be set using KVM_MEM_READONLY, which allows the memory
> region to be read & executed. (Without this, KVM will not support
> executing from the memory region.)
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  hw/block/pflash_cfi01.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 3ff20e0..b65225e 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -596,6 +596,8 @@ static int pflash_cfi01_init(SysBusDevice *dev)
>          }
>      }
>  
> +    memory_region_set_readonly(&pfl->mem, true);
> +
>      if (pfl->bs) {
>          pfl->ro = bdrv_is_read_only(pfl->bs);
>      } else {
> 

NACK, romd mode already implies this.  See my review of patch 3.

Paolo

Re: [Qemu-devel] [PATCH v4 0/6] KVM flash memory support

[Jordan Justen]

On Tue, May 7, 2013 at 1:28 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/05/2013 19:15, Jordan Justen ha scritto:
>> git://github.com/jljusten/qemu.git kvm-flash-v4
>>
>> Utilize KVM_CAP_READONLY_MEM to support PC system flash emulation
>> with KVM.
>>
>> v4:
>>  * With a machine type of isapc, don't mark the BIOS as read-only.
>>    isapc + seabios will not boot if the BIOS is read-only. This
>>    matches the current behavior of isapc with KVM, which is the
>>    only mode under which isapc currently works.
>
> Thanks for looking at it!
>
> Patch 1 should definitely be 1.5 material.  At this point it is late for
> 1.5 and I found a small problem in patch 3, so let's defer it.
>
> However, I'm thinking of reverting commit 9953f8822 in 1.5.  Yes, it's a
> bugfix but the change is only visible in TCG and having three modes in
> three versions is uselessly complicated.

This would be nice. I was definitely not excited about 1.5 having yet
another compatibility mode with regards to how PC system firmware
behaves. (And, having to try to document this...)

-Jordan

> Paolo
>
>> v3:
>>  * Squash patch 2 & 3 based on Xiao's feedback that what I
>>    was calling a 'workaround' in patch 3 was actually what
>>    is required by the KVM READONLY memory support.
>>
>> v2:
>>  * Remove rom_only from PC_COMPAT_1_4
>>  * Only enable flash when a pflash drive is created.
>>
>> Jordan Justen (6):
>>   isapc: Fix non-KVM qemu boot (read/write memory for isapc BIOS)
>>   kvm: add kvm_readonly_mem_enabled
>>   kvm: support using KVM_MEM_READONLY flag for readonly regions
>>   pflash_cfi01: memory region should be set to enable readonly mode
>>   pc_sysfw: allow flash (-pflash) memory to be used with KVM
>>   pc_sysfw: change rom_only default to 0
>>
>>  hw/block/pc_sysfw.c     |   64 ++++++++++++++++++++++++++++++-----------------
>>  hw/block/pflash_cfi01.c |    2 ++
>>  hw/i386/pc_piix.c       |    5 ++++
>>  include/hw/i386/pc.h    |    4 ---
>>  include/sysemu/kvm.h    |   10 ++++++++
>>  kvm-all.c               |   42 ++++++++++++++++++++++++-------
>>  kvm-stub.c              |    1 +
>>  7 files changed, 92 insertions(+), 36 deletions(-)
>>
>
>

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

[Jordan Justen]

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

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

[Peter Maydell]

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

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

[Paolo Bonzini]

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

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

[Jordan Justen]

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

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

[Jan Kiszka]

[-- 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 --]