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

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

[Jordan Justen]

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

Utilize KVM_CAP_READONLY_MEM to support PC system flash emulation
with KVM.

Jordan Justen (6):
  kvm: add kvm_readonly_mem_enabled
  kvm: support using KVM_MEM_READONLY flag for readonly regions
  kvm: workaround a possible KVM bug when using KVM_MEM_READONLY
  pflash_cfi01: memory region should be set to enable readonly mode
  pc_sysfw: allow flash memory to be used with KVM
  pc_sysfw: change rom_only default to 0

 hw/block/pc_sysfw.c     |    5 ++++-
 hw/block/pflash_cfi01.c |    2 ++
 include/sysemu/kvm.h    |   10 ++++++++++
 kvm-all.c               |   41 +++++++++++++++++++++++++++++++++--------
 kvm-stub.c              |    1 +
 5 files changed, 50 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/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 75bd7d9..c83f51c 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 2d92721..f634c41 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -109,6 +109,7 @@ bool kvm_async_interrupts_allowed;
 bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
+bool kvm_readonly_mem_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
@@ -1423,6 +1424,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 5f52186..40dc368 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -22,6 +22,7 @@ bool kvm_async_interrupts_allowed;
 bool kvm_irqfds_allowed;
 bool kvm_msi_via_irqfd_allowed;
 bool kvm_gsi_routing_allowed;
+bool kvm_readonly_mem_allowed;
 
 int kvm_init_vcpu(CPUState *cpu)
 {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/6] kvm: add kvm_readonly_mem_enabled

[Jan Kiszka]

On 2013-04-28 10:32, Jordan Justen wrote:
> 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 75bd7d9..c83f51c 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 2d92721..f634c41 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -109,6 +109,7 @@ bool kvm_async_interrupts_allowed;
>  bool kvm_irqfds_allowed;
>  bool kvm_msi_via_irqfd_allowed;
>  bool kvm_gsi_routing_allowed;
> +bool kvm_readonly_mem_allowed;
>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -1423,6 +1424,11 @@ int kvm_init(void)
>          s->irq_set_ioctl = KVM_IRQ_LINE_STATUS;
>      }
>  
> +#ifdef KVM_CAP_READONLY_MEM

Grr, someone needlessly defined the cap value conditionally in the
kernel header. Could you fix that, pull in updated headers, and avoid
the #ifdef here? Can be a follow-up to this series.

Thanks,
Jan

> +    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 5f52186..40dc368 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -22,6 +22,7 @@ bool kvm_async_interrupts_allowed;
>  bool kvm_irqfds_allowed;
>  bool kvm_msi_via_irqfd_allowed;
>  bool kvm_gsi_routing_allowed;
> +bool kvm_readonly_mem_allowed;
>  
>  int kvm_init_vcpu(CPUState *cpu)
>  {
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 2/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>
Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 kvm-all.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index f634c41..95e6bf2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -266,9 +266,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)
@@ -279,7 +284,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 */
@@ -636,7 +641,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;
@@ -685,7 +697,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) {
@@ -706,7 +718,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) {
@@ -730,7 +742,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) {
@@ -752,7 +764,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

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

[Xiao Guangrong]

On 04/28/2013 04:32 PM, Jordan Justen wrote:
> 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.)
> 

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 3/6] kvm: workaround a possible KVM bug when using KVM_MEM_READONLY

[Jordan Justen]

On a Linux 3.8.0 based kernel, I occasionally saw a situation
where the memory region would continue to trap on memory
read even though KVM_MEM_READONLY was set.

I found that if I set the slot to a size of 0, and before
setting the slot, it would then behave as expected.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 kvm-all.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kvm-all.c b/kvm-all.c
index 95e6bf2..e2ddbcb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -205,6 +205,13 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     if (s->migration_log) {
         mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
     }
+    if (mem.flags & KVM_MEM_READONLY && mem.memory_size != 0) {
+        /* Workaround an issue with setting a READONLY slot. Set the
+         * slot size to 0 before setting the slot to the desired value. */
+        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);
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/6] kvm: workaround a possible KVM bug when using KVM_MEM_READONLY

[Jan Kiszka]

On 2013-04-28 10:32, Jordan Justen wrote:
> On a Linux 3.8.0 based kernel, I occasionally saw a situation
> where the memory region would continue to trap on memory
> read even though KVM_MEM_READONLY was set.

Only 3.8.0? Did you bisect the issue down to the causing commit? Is it
fixed in later versions?

Jan

> 
> I found that if I set the slot to a size of 0, and before
> setting the slot, it would then behave as expected.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  kvm-all.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 95e6bf2..e2ddbcb 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -205,6 +205,13 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>      if (s->migration_log) {
>          mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
>      }
> +    if (mem.flags & KVM_MEM_READONLY && mem.memory_size != 0) {
> +        /* Workaround an issue with setting a READONLY slot. Set the
> +         * slot size to 0 before setting the slot to the desired value. */
> +        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);
>  }
>  
> 
-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/6] kvm: workaround a possible KVM bug when using KVM_MEM_READONLY

[Jordan Justen]

On Mon, Apr 29, 2013 at 3:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-04-28 10:32, Jordan Justen wrote:
>> On a Linux 3.8.0 based kernel, I occasionally saw a situation
>> where the memory region would continue to trap on memory
>> read even though KVM_MEM_READONLY was set.
>
> Only 3.8.0? Did you bisect the issue down to the causing commit? Is it
> fixed in later versions?

I'm sorry, I have not tried to bisect, nor have I tried a newer kernel version.

Speculating a bit, it seems that a trap to the region might cause the
issue. This is what happens in the failing case:
* Disable mem region
* Trap on access to region
* Enable readonly region
* Next read access will trap when it shouldn't

Here is the what happen with the work-around:
* Disable mem region
* Trap on access to region
* (Re-)disable mem region (work-around adds this)
* Enable readonly region
* Next read access will not trap (proper behavior)

-Jordan

Re: [Qemu-devel] [PATCH 3/6] kvm: workaround a possible KVM bug when using KVM_MEM_READONLY

[Xiao Guangrong]

On 04/28/2013 04:32 PM, Jordan Justen wrote:
> On a Linux 3.8.0 based kernel, I occasionally saw a situation
> where the memory region would continue to trap on memory
> read even though KVM_MEM_READONLY was set.
> 
> I found that if I set the slot to a size of 0, and before
> setting the slot, it would then behave as expected.

Yes, the KVM_MEM_READONLY flag can not be directly changed, see
the commit 75d61fbcf563373696578570e914f555e12c8d97 on kvm tree.

Do you think the way which deletes the memslot before changing
the KVM_MEM_READONLY hurts the performance? If yes, i will
make it be directly changed.

Thanks for you implementing the READONLY memory in Qemu.

Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 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 5/6] pc_sysfw: allow flash memory to be used with KVM

[Jordan Justen]

This requires the KVM READONLY memory capability.

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

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index aad8614..e88d67e 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -237,6 +237,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
             old_pc_system_rom_init(rom_memory);
             return;
         }
+    } else if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
+        fprintf(stderr, "qemu: pflash with kvm requires KVM readonly memory support\n");
+        exit(1);
     }
 
     /* If a pflash drive is not found, then create one using
-- 
1.7.10.4

[Qemu-devel] [PATCH 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.

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

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index e88d67e..c5067c1 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -258,7 +258,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
 }
 
 static Property pcsysfw_properties[] = {
-    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
+    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.7.10.4

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

[Paolo Bonzini]

Il 28/04/2013 10:32, Jordan Justen ha scritto:
> Now KVM can support a flash memory. This feature depends on
> KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.

I don't think we can require such a new kernel to run KVM.  IIUC, an
older kernel would just fail to start, right?

Paolo

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

[Jordan Justen]

On Mon, Apr 29, 2013 at 1:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/04/2013 10:32, Jordan Justen ha scritto:
>> Now KVM can support a flash memory. This feature depends on
>> KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.
>
> I don't think we can require such a new kernel to run KVM.  IIUC, an
> older kernel would just fail to start, right?

Would it also be unacceptable to alter the behavior based on the kvm
capabilities? So, we can't use rom-mode for old kvm, but flash mode
for new kvm? (In other words, behave similarly to qemu 1.2-1.4 in kvm
vs. non-kvm mode.)

-Jordan

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

[Paolo Bonzini]

Il 29/04/2013 20:23, Jordan Justen ha scritto:
> On Mon, Apr 29, 2013 at 1:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 28/04/2013 10:32, Jordan Justen ha scritto:
>>> Now KVM can support a flash memory. This feature depends on
>>> KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.
>>
>> I don't think we can require such a new kernel to run KVM.  IIUC, an
>> older kernel would just fail to start, right?
> 
> Would it also be unacceptable to alter the behavior based on the kvm
> capabilities? So, we can't use rom-mode for old kvm, but flash mode
> for new kvm? (In other words, behave similarly to qemu 1.2-1.4 in kvm
> vs. non-kvm mode.)

No, unfortunately the host versions must not affect the hardware.

Paolo

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

[Jordan Justen]

On Mon, Apr 29, 2013 at 2:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/04/2013 20:23, Jordan Justen ha scritto:
>> On Mon, Apr 29, 2013 at 1:20 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 28/04/2013 10:32, Jordan Justen ha scritto:
>>>> Now KVM can support a flash memory. This feature depends on
>>>> KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.
>>>
>>> I don't think we can require such a new kernel to run KVM.  IIUC, an
>>> older kernel would just fail to start, right?
>>
>> Would it also be unacceptable to alter the behavior based on the kvm
>> capabilities? So, we can't use rom-mode for old kvm, but flash mode
>> for new kvm? (In other words, behave similarly to qemu 1.2-1.4 in kvm
>> vs. non-kvm mode.)
>
> No, unfortunately the host versions must not affect the hardware.

I don't think this is as black & white as you guys are claiming.

The rom in qemu is read-only. In kvm, it has been read-write. That is
different behavior.

I assume with kvm readonly support, at some point the rom may be
read-only or read/write under kvm depending on the capabilities.

Anyway, would this work:
if (-pflash is not used) {
  qemu and kvm always use a 'rom' mode
} else if (non-kvm or kvm read-only capable) {
  PC flash used
} else {
  die "kvm+pflash requires read-only capability"
}

Someone could create a flash capable VM on qemu or newer kvm, and then
have trouble with it on an older kvm, but the chance of potential
issues seems greatly reduced.

For people using rom firmware, this should have no impact.

-Jordan

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

[Paolo Bonzini]

Il 30/04/2013 05:51, Jordan Justen ha scritto:
> The rom in qemu is read-only. In kvm, it has been read-write. That is
> different behavior.

And it was a problem, hence it was reverted.

> I assume with kvm readonly support, at some point the rom may be
> read-only or read/write under kvm depending on the capabilities.

No, this would be bad.

> Anyway, would this work:
> if (-pflash is not used) {
>   qemu and kvm always use a 'rom' mode
> } else if (non-kvm or kvm read-only capable) {
>   PC flash used
> } else {
>   die "kvm+pflash requires read-only capability"
> }
> 
> Someone could create a flash capable VM on qemu or newer kvm, and then
> have trouble with it on an older kvm, but the chance of potential
> issues seems greatly reduced.
> 
> For people using rom firmware, this should have no impact.

Yes, _this_ would work because it requires a special command line, and
it would have never worked before.

Paolo

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

[Markus Armbruster]

Jordan Justen <jordan.l.justen@intel.com> writes:

> Now KVM can support a flash memory. This feature depends on
> KVM_CAP_READONLY_MEM, which was introduced in Linux 3.7.
>
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  hw/block/pc_sysfw.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
> index e88d67e..c5067c1 100644
> --- a/hw/block/pc_sysfw.c
> +++ b/hw/block/pc_sysfw.c
> @@ -258,7 +258,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory)
>  }
>  
>  static Property pcsysfw_properties[] = {
> -    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 1),
> +    DEFINE_PROP_UINT8("rom_only", PcSysFwDevice, rom_only, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Do we still need the pc-sysfw.rom_only entry in include/hw/i386/pc.h
PC_COMPAT_1_4?