[Qemu-devel] [PATCH for-2.7 0/3] Add memory_region_init_rom() and use to fix imx board crashes

[Qemu-devel] [PATCH for-2.7 0/3] Add memory_region_init_rom() and use to fix imx board crashes

[Peter Maydell]

    
The imx boards were all incorrectly creating ROMs using
memory_region_init_rom_device() with a NULL ops pointer.  This will
cause QEMU to abort if the guest tries to write to the ROM.  The way
we currently provide for "create a pure ROM" is to use
memory_region_init_ram() followed by memory_region_set_readonly(),
but this is a bit obscure. So provide a new memory_region_init_rom()
which does the equivalent of those two calls, and use it in
the imx boards.

We can then add an assert() in memory_region_init_rom_device()
to prevent further misuse. (Passing NULL was documented as
"I/O space is not supposed to be handled by QEMU itself", but this
doesn't make much sense (who would be handling the writes?) and
isn't used by any of the callers.)

This is for-2.7 because it fixes a crash in the imx boards
if the guest misbehaves:
https://bugs.launchpad.net/qemu/+bug/1596160

thanks
-- PMM

Peter Maydell (3):
  memory: Provide memory_region_init_rom()
  imx: Use memory_region_init_rom() for ROMs
  memory: Assert that memory_region_init_rom_device() ops aren't NULL

 docs/memory.txt       |  9 +++++++--
 hw/arm/fsl-imx25.c    |  8 ++++----
 hw/arm/fsl-imx31.c    |  9 ++++-----
 hw/arm/fsl-imx6.c     |  8 ++++----
 include/exec/memory.h | 24 ++++++++++++++++++++----
 memory.c              | 16 ++++++++++++++++
 6 files changed, 55 insertions(+), 19 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH for-2.7 1/3] memory: Provide memory_region_init_rom()

[Peter Maydell]

Provide a new helper function memory_region_init_rom() for memory
regions which are read-only (and unlike those created by
memory_region_init_rom_device() don't have special behaviour
for writes). This has the same behaviour as calling
memory_region_init_ram() and then memory_region_set_readonly()
(which is what we do today in boards with pure ROMs) but is a
more easily discoverable API for the purpose.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/memory.txt       |  9 +++++++--
 include/exec/memory.h | 19 +++++++++++++++++++
 memory.c              | 15 +++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/docs/memory.txt b/docs/memory.txt
index 431d9ca..811b1bd 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -41,8 +41,13 @@ MemoryRegion):
   MemoryRegionOps structure describing the callbacks.
 
 - ROM: a ROM memory region works like RAM for reads (directly accessing
-  a region of host memory), but like MMIO for writes (invoking a callback).
-  You initialize these with memory_region_init_rom_device().
+  a region of host memory), and forbids writes. You initialize these with
+  memory_region_init_rom().
+
+- ROM device: a ROM device memory region works like RAM for reads
+  (directly accessing a region of host memory), but like MMIO for
+  writes (invoking a callback).  You initialize these with
+  memory_region_init_rom_device().
 
 - IOMMU region: an IOMMU region translates addresses of accesses made to it
   and forwards them to some other target memory region.  As the name suggests,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e3829f7..742c52f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -441,6 +441,25 @@ void memory_region_init_alias(MemoryRegion *mr,
                               uint64_t size);
 
 /**
+ * memory_region_init_rom: Initialize a ROM memory region.
+ *
+ * This has the same effect as calling memory_region_init_ram()
+ * and then marking the resulting region read-only with
+ * memory_region_set_readonly().
+ *
+ * @mr: the #MemoryRegion to be initialized.
+ * @owner: the object that tracks the region's reference count
+ * @name: the name of the region.
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ */
+void memory_region_init_rom(MemoryRegion *mr,
+                            struct Object *owner,
+                            const char *name,
+                            uint64_t size,
+                            Error **errp);
+
+/**
  * memory_region_init_rom_device:  Initialize a ROM memory region.  Writes are
  *                                 handled via callbacks.
  *
diff --git a/memory.c b/memory.c
index 8549c79..c18ccc8 100644
--- a/memory.c
+++ b/memory.c
@@ -1376,6 +1376,21 @@ void memory_region_init_alias(MemoryRegion *mr,
     mr->alias_offset = offset;
 }
 
+void memory_region_init_rom(MemoryRegion *mr,
+                            struct Object *owner,
+                            const char *name,
+                            uint64_t size,
+                            Error **errp)
+{
+    memory_region_init(mr, owner, name, size);
+    mr->ram = true;
+    mr->readonly = true;
+    mr->terminates = true;
+    mr->destructor = memory_region_destructor_ram;
+    mr->ram_block = qemu_ram_alloc(size, mr, errp);
+    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
+}
+
 void memory_region_init_rom_device(MemoryRegion *mr,
                                    Object *owner,
                                    const MemoryRegionOps *ops,
-- 
1.9.1

[Qemu-devel] [PATCH for-2.7 2/3] imx: Use memory_region_init_rom() for ROMs

[Peter Maydell]

The imx boards were all incorrectly creating ROMs using
memory_region_init_rom_device() with a NULL ops pointer. This
will cause QEMU to abort if the guest tries to write to the
ROM. Switch to the new memory_region_init_rom() instead.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/fsl-imx25.c | 8 ++++----
 hw/arm/fsl-imx31.c | 9 ++++-----
 hw/arm/fsl-imx6.c  | 8 ++++----
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 1cd749a..1a53e51 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -249,16 +249,16 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
     }
 
     /* initialize 2 x 16 KB ROM */
-    memory_region_init_rom_device(&s->rom[0], NULL, NULL, NULL,
-                                  "imx25.rom0", FSL_IMX25_ROM0_SIZE, &err);
+    memory_region_init_rom(&s->rom[0], NULL,
+                           "imx25.rom0", FSL_IMX25_ROM0_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX25_ROM0_ADDR,
                                 &s->rom[0]);
-    memory_region_init_rom_device(&s->rom[1], NULL, NULL, NULL,
-                                  "imx25.rom1", FSL_IMX25_ROM1_SIZE, &err);
+    memory_region_init_rom(&s->rom[1], NULL,
+                           "imx25.rom1", FSL_IMX25_ROM1_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 31a3a87..b283b71 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -219,9 +219,8 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
     }
 
     /* On a real system, the first 16k is a `secure boot rom' */
-    memory_region_init_rom_device(&s->secure_rom, NULL, NULL, NULL,
-                                  "imx31.secure_rom",
-                                  FSL_IMX31_SECURE_ROM_SIZE, &err);
+    memory_region_init_rom(&s->secure_rom, NULL, "imx31.secure_rom",
+                           FSL_IMX31_SECURE_ROM_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
@@ -230,8 +229,8 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
                                 &s->secure_rom);
 
     /* There is also a 16k ROM */
-    memory_region_init_rom_device(&s->rom, NULL, NULL, NULL, "imx31.rom",
-                                  FSL_IMX31_ROM_SIZE, &err);
+    memory_region_init_rom(&s->rom, NULL, "imx31.rom",
+                           FSL_IMX31_ROM_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 0c00e7a..ed392a9 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -399,8 +399,8 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                         FSL_IMX6_ENET_MAC_1588_IRQ));
 
     /* ROM memory */
-    memory_region_init_rom_device(&s->rom, NULL, NULL, NULL, "imx6.rom",
-                                  FSL_IMX6_ROM_SIZE, &err);
+    memory_region_init_rom(&s->rom, NULL, "imx6.rom",
+                           FSL_IMX6_ROM_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
@@ -409,8 +409,8 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                 &s->rom);
 
     /* CAAM memory */
-    memory_region_init_rom_device(&s->caam, NULL, NULL, NULL, "imx6.caam",
-                                  FSL_IMX6_CAAM_MEM_SIZE, &err);
+    memory_region_init_rom(&s->caam, NULL, "imx6.caam",
+                           FSL_IMX6_CAAM_MEM_SIZE, &err);
     if (err) {
         error_propagate(errp, err);
         return;
-- 
1.9.1

[Qemu-devel] [PATCH for-2.7 3/3] memory: Assert that memory_region_init_rom_device() ops aren't NULL

[Peter Maydell]

It doesn't make sense to pass a NULL ops argument to
memory_region_init_rom_device(), because the effect will
be that if the guest tries to write to the memory region
then QEMU will segfault. Catch the bug earlier by sanity
checking the arguments to this function, and remove the
misleading documentation that suggests that passing NULL
might be sensible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h | 5 +----
 memory.c              | 1 +
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 742c52f..0435e79 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -463,12 +463,9 @@ void memory_region_init_rom(MemoryRegion *mr,
  * memory_region_init_rom_device:  Initialize a ROM memory region.  Writes are
  *                                 handled via callbacks.
  *
- * If NULL callbacks pointer is given, then I/O space is not supposed to be
- * handled by QEMU itself. Any access via the memory API will cause an abort().
- *
  * @mr: the #MemoryRegion to be initialized.
  * @owner: the object that tracks the region's reference count
- * @ops: callbacks for write access handling.
+ * @ops: callbacks for write access handling (must not be NULL).
  * @name: the name of the region.
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
diff --git a/memory.c b/memory.c
index c18ccc8..de0d4b5 100644
--- a/memory.c
+++ b/memory.c
@@ -1399,6 +1399,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    uint64_t size,
                                    Error **errp)
 {
+    assert(ops);
     memory_region_init(mr, owner, name, size);
     mr->ops = ops;
     mr->opaque = opaque;
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.7 0/3] Add memory_region_init_rom() and use to fix imx board crashes

[Paolo Bonzini]

> We can then add an assert() in memory_region_init_rom_device()
> to prevent further misuse. (Passing NULL was documented as
> "I/O space is not supposed to be handled by QEMU itself", but this
> doesn't make much sense (who would be handling the writes?) and
> isn't used by any of the callers.)
> 
> This is for-2.7 because it fixes a crash in the imx boards
> if the guest misbehaves:
> https://bugs.launchpad.net/qemu/+bug/1596160

Go ahead and include this in target-arm (and since we're at it,
it would be great if you handled Cedric's m25p80 series when he
sends out the rebase).

Paolo

Re: [Qemu-devel] [PATCH for-2.7 0/3] Add memory_region_init_rom() and use to fix imx board crashes

[Cédric Le Goater]

On 06/28/2016 07:53 PM, Paolo Bonzini wrote:
> 
>> We can then add an assert() in memory_region_init_rom_device()
>> to prevent further misuse. (Passing NULL was documented as
>> "I/O space is not supposed to be handled by QEMU itself", but this
>> doesn't make much sense (who would be handling the writes?) and
>> isn't used by any of the callers.)
>>
>> This is for-2.7 because it fixes a crash in the imx boards
>> if the guest misbehaves:
>> https://bugs.launchpad.net/qemu/+bug/1596160
> 
> Go ahead and include this in target-arm (and since we're at it,
> it would be great if you handled Cedric's m25p80 series when he
> sends out the rebase).

Quick question, who is handling this one :

	https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg04339.html

Shall I include it also in the patchset ? 

Thanks,

C.