[Qemu-devel] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Qemu-devel] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Peter Maydell]

The API of cpu_physical_memory_write_rom() is odd, because it
takes an AddressSpace, unlike all the other cpu_physical_memory_*
access functions. We note this oddity as a TODO in the
docs/devel/loads-stores.rst documentation.

Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
and give it an API that matches address_space_write().
We also adjest the cpu_physical_memory_write_rom_internal()
function which is local to exec.c similarly.

thanks
-- PMM
    
Peter Maydell (2):
  exec.c: Rename cpu_physical_memory_write_rom_internal()
  Rename cpu_physical_memory_write_rom() to address_space_write_rom()

 include/exec/cpu-common.h   |  2 --
 include/exec/memory.h       | 26 ++++++++++++++++++++++++++
 exec.c                      | 30 +++++++++++++++++++-----------
 hw/core/loader.c            |  4 ++--
 hw/intc/apic.c              |  7 ++++---
 hw/misc/tz-mpc.c            |  2 +-
 hw/sparc/sun4m.c            |  5 +++--
 docs/devel/loads-stores.rst | 35 ++++++++++++++++-------------------
 8 files changed, 71 insertions(+), 40 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-4.0 1/2] exec.c: Rename cpu_physical_memory_write_rom_internal()

[Peter Maydell]

Rename cpu_physical_memory_write_rom_internal() to
address_space_write_rom_internal(), and make it take
MemTxAttrs and return a MemTxResult. This brings its
API into line with address_space_write().

This is an internal function to exec.c; fixing its API
will allow us to change the global function
cpu_physical_memory_write_rom().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index bb6170dbffe..92679508ba3 100644
--- a/exec.c
+++ b/exec.c
@@ -3388,8 +3388,12 @@ enum write_rom_type {
     FLUSH_CACHE,
 };
 
-static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
-    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
+static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
+                                                           hwaddr addr,
+                                                           MemTxAttrs attrs,
+                                                           const uint8_t *buf,
+                                                           int len,
+                                                           enum write_rom_type type)
 {
     hwaddr l;
     uint8_t *ptr;
@@ -3399,8 +3403,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
     rcu_read_lock();
     while (len > 0) {
         l = len;
-        mr = address_space_translate(as, addr, &addr1, &l, true,
-                                     MEMTXATTRS_UNSPECIFIED);
+        mr = address_space_translate(as, addr, &addr1, &l, true, attrs);
 
         if (!(memory_region_is_ram(mr) ||
               memory_region_is_romd(mr))) {
@@ -3423,13 +3426,15 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
         addr += l;
     }
     rcu_read_unlock();
+    return MEMTX_OK;
 }
 
 /* used for ROM loading : can write in RAM and ROM */
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
                                    const uint8_t *buf, int len)
 {
-    cpu_physical_memory_write_rom_internal(as, addr, buf, len, WRITE_DATA);
+    address_space_write_rom_internal(as, addr, MEMTXATTRS_UNSPECIFIED,
+                                     buf, len, WRITE_DATA);
 }
 
 void cpu_flush_icache_range(hwaddr start, int len)
@@ -3444,8 +3449,9 @@ void cpu_flush_icache_range(hwaddr start, int len)
         return;
     }
 
-    cpu_physical_memory_write_rom_internal(&address_space_memory,
-                                           start, NULL, len, FLUSH_CACHE);
+    address_space_write_rom_internal(&address_space_memory,
+                                     start, MEMTXATTRS_UNSPECIFIED,
+                                     NULL, len, FLUSH_CACHE);
 }
 
 typedef struct {
-- 
2.19.1

[Qemu-devel] [PATCH for-4.0 2/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Peter Maydell]

The API of cpu_physical_memory_write_rom() is odd, because it
takes an AddressSpace, unlike all the other cpu_physical_memory_*
access functions. Rename it to address_space_write_rom(), and
bring its API into line with address_space_write().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu-common.h   |  2 --
 include/exec/memory.h       | 26 ++++++++++++++++++++++++++
 exec.c                      | 14 ++++++++------
 hw/core/loader.c            |  4 ++--
 hw/intc/apic.c              |  7 ++++---
 hw/misc/tz-mpc.c            |  2 +-
 hw/sparc/sun4m.c            |  5 +++--
 docs/devel/loads-stores.rst | 35 ++++++++++++++++-------------------
 8 files changed, 60 insertions(+), 35 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 18b40d6145c..2ad2d6d86bb 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -111,8 +111,6 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr);
  */
 void qemu_flush_coalesced_mmio_buffer(void);
 
-void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len);
 void cpu_flush_icache_range(hwaddr start, int len);
 
 extern struct MemoryRegion io_mem_rom;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8e61450de32..ffd23ed8d8d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1792,6 +1792,32 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
                                 const uint8_t *buf, int len);
 
+/**
+ * address_space_write_rom: write to address space, including ROM.
+ *
+ * This function writes to the specified address space, but will
+ * write data to both ROM and RAM. This is used for non-guest
+ * writes like writes from the gdb debug stub or initial loading
+ * of ROM contents.
+ *
+ * Note that portions of the write which attempt to write data to
+ * a device will be silently ignored -- only real RAM and ROM will
+ * be written to.
+ *
+ * Return a MemTxResult indicating whether the operation succeeded
+ * or failed (eg unassigned memory, device rejected the transaction,
+ * IOMMU fault).
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @attrs: memory transaction attributes
+ * @buf: buffer with the data transferred
+ * @len: the number of bytes to write
+ */
+MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
+                                    MemTxAttrs attrs,
+                                    const uint8_t *buf, int len);
+
 /* address_space_ld*: load from an address space
  * address_space_st*: store to an address space
  *
diff --git a/exec.c b/exec.c
index 92679508ba3..6e875f0640a 100644
--- a/exec.c
+++ b/exec.c
@@ -3430,11 +3430,12 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
 }
 
 /* used for ROM loading : can write in RAM and ROM */
-void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len)
+MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
+                                    MemTxAttrs attrs,
+                                    const uint8_t *buf, int len)
 {
-    address_space_write_rom_internal(as, addr, MEMTXATTRS_UNSPECIFIED,
-                                     buf, len, WRITE_DATA);
+    return address_space_write_rom_internal(as, addr, attrs,
+                                            buf, len, WRITE_DATA);
 }
 
 void cpu_flush_icache_range(hwaddr start, int len)
@@ -3879,8 +3880,9 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
         if (is_write) {
-            cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as,
-                                          phys_addr, buf, l);
+            address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
+                                    MEMTXATTRS_UNSPECIFIED,
+                                    buf, l);
         } else {
             address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
                              MEMTXATTRS_UNSPECIFIED,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index aa0b3fc8679..66a616608af 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1103,8 +1103,8 @@ static void rom_reset(void *unused)
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
-            cpu_physical_memory_write_rom(rom->as, rom->addr, rom->data,
-                                          rom->datasize);
+            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
+                                    rom->data, rom->datasize);
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 97ffdd820f2..c9dd65b3a03 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -122,9 +122,10 @@ static void apic_sync_vapic(APICCommonState *s, int sync_type)
         }
         vapic_state.irr = vector & 0xff;
 
-        cpu_physical_memory_write_rom(&address_space_memory,
-                                      s->vapic_paddr + start,
-                                      ((void *)&vapic_state) + start, length);
+        address_space_write_rom(&address_space_memory,
+                                s->vapic_paddr + start,
+                                MEMTXATTRS_UNSPECIFIED,
+                                ((void *)&vapic_state) + start, length);
     }
 }
 
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index e0c58ba37ec..fb48a1540b9 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -448,7 +448,7 @@ static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
 {
     /* We treat unspecified attributes like secure. Transactions with
      * unspecified attributes come from places like
-     * cpu_physical_memory_write_rom() for initial image load, and we want
+     * rom_reset() for initial image load, and we want
      * those to pass through the from-reset "everything is secure" config.
      * All the real during-emulation transactions from the CPU will
      * specify attributes.
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 3c29b68e67f..639906cca30 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -559,8 +559,9 @@ static void idreg_init(hwaddr addr)
     s = SYS_BUS_DEVICE(dev);
 
     sysbus_mmio_map(s, 0, addr);
-    cpu_physical_memory_write_rom(&address_space_memory,
-                                  addr, idreg_data, sizeof(idreg_data));
+    address_space_write_rom(&address_space_memory, addr,
+                            MEMTXATTRS_UNSPECIFIED,
+                            idreg_data, sizeof(idreg_data));
 }
 
 #define MACIO_ID_REGISTER(obj) \
diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
index 57d8c524bfe..c74cd090e64 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -253,6 +253,22 @@ Regexes for git grep
  - ``\<address_space_ldu\?[bwql]\(_[lb]e\)\?\>``
  - ``\<address_space_st[bwql]\(_[lb]e\)\?\>``
 
+``address_space_write_rom``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This function performs a write by physical address like
+``address_space_write``, except that if the write is to a ROM then
+the ROM contents will be modified, even though a write by the guest
+CPU to the ROM would be ignored. This is used for non-guest writes
+like writes from the gdb debug stub or initial loading of ROM contents.
+
+Note that portions of the write which attempt to write data to a
+device will be silently ignored -- only real RAM and ROM will
+be written to.
+
+Regexes for git grep
+ - ``address_space_write_rom``
+
 ``{ld,st}*_phys``
 ~~~~~~~~~~~~~~~~~
 
@@ -315,25 +331,6 @@ For new code they are better avoided:
 Regexes for git grep
  - ``\<cpu_physical_memory_\(read\|write\|rw\)\>``
 
-``cpu_physical_memory_write_rom``
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-This function performs a write by physical address like
-``address_space_write``, except that if the write is to a ROM then
-the ROM contents will be modified, even though a write by the guest
-CPU to the ROM would be ignored.
-
-Note that unlike ``cpu_physical_memory_write()`` this function takes
-an AddressSpace argument, but unlike ``address_space_write()`` this
-function does not take a ``MemTxAttrs`` or return a ``MemTxResult``.
-
-**TODO**: we should probably clean up this inconsistency and
-turn the function into ``address_space_write_rom`` with an API
-matching ``address_space_write``.
-
-``cpu_physical_memory_write_rom``
-
-
 ``cpu_memory_rw_debug``
 ~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Philippe Mathieu-Daudé]

Le jeu. 22 nov. 2018 14:36, Peter Maydell <peter.maydell@linaro.org> a
écrit :

> The API of cpu_physical_memory_write_rom() is odd, because it
> takes an AddressSpace, unlike all the other cpu_physical_memory_*
> access functions. We note this oddity as a TODO in the
> docs/devel/loads-stores.rst documentation.
>
> Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
> and give it an API that matches address_space_write().
> We also adjest the cpu_physical_memory_write_rom_internal()
> function which is local to exec.c similarly.
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   exec.c: Rename cpu_physical_memory_write_rom_internal()
>   Rename cpu_physical_memory_write_rom() to address_space_write_rom()
>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Michael S. Tsirkin]

On Thu, Nov 22, 2018 at 01:35:05PM +0000, Peter Maydell wrote:
> The API of cpu_physical_memory_write_rom() is odd, because it
> takes an AddressSpace, unlike all the other cpu_physical_memory_*
> access functions. We note this oddity as a TODO in the
> docs/devel/loads-stores.rst documentation.
> 
> Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
> and give it an API that matches address_space_write().
> We also adjest the cpu_physical_memory_write_rom_internal()
> function which is local to exec.c similarly.
> 
> thanks
> -- PMM


Acked-by: Michael S. Tsirkin <mst@redhat.com>

> Peter Maydell (2):
>   exec.c: Rename cpu_physical_memory_write_rom_internal()
>   Rename cpu_physical_memory_write_rom() to address_space_write_rom()
> 
>  include/exec/cpu-common.h   |  2 --
>  include/exec/memory.h       | 26 ++++++++++++++++++++++++++
>  exec.c                      | 30 +++++++++++++++++++-----------
>  hw/core/loader.c            |  4 ++--
>  hw/intc/apic.c              |  7 ++++---
>  hw/misc/tz-mpc.c            |  2 +-
>  hw/sparc/sun4m.c            |  5 +++--
>  docs/devel/loads-stores.rst | 35 ++++++++++++++++-------------------
>  8 files changed, 71 insertions(+), 40 deletions(-)
> 
> -- 
> 2.19.1

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Peter Maydell]

On Thu, 22 Nov 2018 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The API of cpu_physical_memory_write_rom() is odd, because it
> takes an AddressSpace, unlike all the other cpu_physical_memory_*
> access functions. We note this oddity as a TODO in the
> docs/devel/loads-stores.rst documentation.
>
> Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
> and give it an API that matches address_space_write().
> We also adjest the cpu_physical_memory_write_rom_internal()
> function which is local to exec.c similarly.

Unless somebody would prefer a different route, I'm going
to add this series to a 'misc' pullreq I'm putting together.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH for-4.0 0/2] Rename cpu_physical_memory_write_rom() to address_space_write_rom()

[Paolo Bonzini]

On 14/12/18 12:32, Peter Maydell wrote:
> On Thu, 22 Nov 2018 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> The API of cpu_physical_memory_write_rom() is odd, because it
>> takes an AddressSpace, unlike all the other cpu_physical_memory_*
>> access functions. We note this oddity as a TODO in the
>> docs/devel/loads-stores.rst documentation.
>>
>> Rename cpu_physical_memory_write_rom() to address_space_write_rom(),
>> and give it an API that matches address_space_write().
>> We also adjest the cpu_physical_memory_write_rom_internal()
>> function which is local to exec.c similarly.
> 
> Unless somebody would prefer a different route, I'm going
> to add this series to a 'misc' pullreq I'm putting together.

Go ahead, please.

Paolo