[PATCH-for-9.0 00/25] memory: Propagate Error* when possible

[PATCH-for-9.0 00/25] memory: Propagate Error* when possible

[Philippe Mathieu-Daudé]

Hi,

This series is remotely connected with the "dynamic machine"
project. We need QOM objects created either from command line,
QMP or loaded by modules to NOT fail exiting the whole QEMU
process, but cleanly propagate any error before failing cleanly.

In preparation for that big goal, we start reworking a bit the
Memory API to be able to propagate all errors.

This targets the 9.0 release, but posting earlier to discuss with
Markus meanwhile.

Regards,

Phil.

Philippe Mathieu-Daudé (25):
  memory: Have memory_region_init_ram_flags_nomigrate() return a boolean
  memory: Have memory_region_init_ram_nomigrate() handler return a
    boolean
  memory: Have memory_region_init_rom_nomigrate() handler return a
    boolean
  memory: Simplify memory_region_init_rom_nomigrate() calls
  memory: Simplify memory_region_init_ram_from_fd() calls
  memory: Have memory_region_init_ram() handler return a boolean
  memory: Have memory_region_init_rom() handler return a boolean
  memory: Have memory_region_init_rom_device_nomigrate() return a
    boolean
  memory: Simplify memory_region_init_rom_device_nomigrate() calls
  memory: Have memory_region_init_rom_device() handler return a boolean
  memory: Have memory_region_init_resizeable_ram() return a boolean
  memory: Have memory_region_init_ram_from_file() handler return a
    boolean
  memory: Have memory_region_init_ram_from_fd() handler return a boolean
  backends: Use g_autofree in HostMemoryBackendClass::alloc() handlers
  backends: Simplify host_memory_backend_memory_complete()
  backends: Have HostMemoryBackendClass::alloc() handler return a
    boolean
  backends: Reduce variable scope in host_memory_backend_memory_complete
  util/oslib: Have qemu_prealloc_mem() handler return a boolean
  misc: Simplify qemu_prealloc_mem() calls
  hw: Simplify memory_region_init_ram() calls
  hw/arm: Simplify memory_region_init_rom() calls
  hw/sparc: Simplify memory_region_init_ram_nomigrate() calls
  hw/misc: Simplify memory_region_init_ram_from_fd() calls
  hw/nvram: Simplify memory_region_init_rom_device() calls
  hw/pci-host/raven: Propagate error in raven_realize()

 include/exec/memory.h    |  40 ++++++++---
 include/qemu/osdep.h     |   4 +-
 include/sysemu/hostmem.h |  10 ++-
 backends/hostmem-epc.c   |  14 ++--
 backends/hostmem-file.c  |  22 +++---
 backends/hostmem-memfd.c |  13 ++--
 backends/hostmem-ram.c   |  12 ++--
 backends/hostmem.c       | 144 ++++++++++++++++++---------------------
 hw/arm/aspeed_ast2400.c  |   6 +-
 hw/arm/aspeed_ast2600.c  |   6 +-
 hw/arm/fsl-imx25.c       |  19 ++----
 hw/arm/fsl-imx31.c       |  19 ++----
 hw/arm/fsl-imx6.c        |  19 ++----
 hw/arm/integratorcp.c    |   7 +-
 hw/arm/nrf51_soc.c       |   7 +-
 hw/misc/ivshmem.c        |   8 +--
 hw/nvram/nrf51_nvm.c     |   7 +-
 hw/pci-host/raven.c      |   6 +-
 hw/ppc/rs6000_mc.c       |   7 +-
 hw/sparc/sun4m.c         |  20 ++----
 hw/sparc64/sun4u.c       |   7 +-
 hw/virtio/virtio-mem.c   |   6 +-
 system/memory.c          |  68 ++++++++++--------
 util/oslib-posix.c       |   7 +-
 util/oslib-win32.c       |   4 +-
 25 files changed, 234 insertions(+), 248 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 01/25] memory: Have memory_region_init_ram_flags_nomigrate() return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 831f7c996d..f038a7e5cf 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1310,8 +1310,10 @@ void memory_region_init_ram_nomigrate(MemoryRegion *mr,
  *
  * Note that this function does not do anything to cause the data in the
  * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
                                             Object *owner,
                                             const char *name,
                                             uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 4d9cb0a7ff..313dbb2544 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1583,7 +1583,7 @@ void memory_region_init_ram_nomigrate(MemoryRegion *mr,
     memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
 }
 
-void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
                                             Object *owner,
                                             const char *name,
                                             uint64_t size,
@@ -1600,7 +1600,9 @@ void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
         error_propagate(errp, err);
+        return false;
     }
+    return true;
 }
 
 void memory_region_init_resizeable_ram(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 01/25] memory: Have memory_region_init_ram_flags_nomigrate() return a boolean

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/exec/memory.h | 4 +++-
> system/memory.c       | 4 +++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/include/exec/memory.h b/include/exec/memory.h
>index 831f7c996d..f038a7e5cf 100644
>--- a/include/exec/memory.h
>+++ b/include/exec/memory.h
>@@ -1310,8 +1310,10 @@ void memory_region_init_ram_nomigrate(MemoryRegion *mr,
>  *
>  * Note that this function does not do anything to cause the data in the
>  * RAM memory region to be migrated; that is the responsibility of the caller.
>+ *
>+ * Return: true on success, else false setting @errp with error.
>  */
>-void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>                                             Object *owner,
>                                             const char *name,
>                                             uint64_t size,
>diff --git a/system/memory.c b/system/memory.c
>index 4d9cb0a7ff..313dbb2544 100644
>--- a/system/memory.c
>+++ b/system/memory.c
>@@ -1583,7 +1583,7 @@ void memory_region_init_ram_nomigrate(MemoryRegion *mr,
>     memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
> }
> 
>-void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>                                             Object *owner,
>                                             const char *name,
>                                             uint64_t size,
>@@ -1600,7 +1600,9 @@ void memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>         mr->size = int128_zero();
>         object_unparent(OBJECT(mr));
>         error_propagate(errp, err);
>+        return false;
>     }
>+    return true;
> }
> 
> void memory_region_init_resizeable_ram(MemoryRegion *mr,
>-- 
>2.41.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 01/25] memory: Have memory_region_init_ram_flags_nomigrate() return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:35PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()

s/cpu_exec_realizefn/memory_region_init_ram_nomigrate/

Similar issue for initial 3 patches (or just not call out the function
name).

> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Other than that:

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 01/25] memory: Have memory_region_init_ram_flags_nomigrate() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 

With Peter Xu's comments addressed, to replace cpu_exec_realizefn()
with memory_region_init_ram_flags_nomigrate() in the commit log.

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 02/25] memory: Have memory_region_init_ram_nomigrate() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index f038a7e5cf..4140eb0c95 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1288,8 +1288,10 @@ void memory_region_init_io(MemoryRegion *mr,
  *
  * Note that this function does not do anything to cause the data in the
  * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_ram_nomigrate(MemoryRegion *mr,
+bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 313dbb2544..337b12a674 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1574,13 +1574,14 @@ void memory_region_init_io(MemoryRegion *mr,
     mr->terminates = true;
 }
 
-void memory_region_init_ram_nomigrate(MemoryRegion *mr,
+bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
                                       Error **errp)
 {
-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
+    return memory_region_init_ram_flags_nomigrate(mr, owner, name,
+                                                  size, 0, errp);
 }
 
 bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 02/25] memory: Have memory_region_init_ram_nomigrate() handler return a boolean

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/exec/memory.h | 4 +++-
> system/memory.c       | 5 +++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/include/exec/memory.h b/include/exec/memory.h
>index f038a7e5cf..4140eb0c95 100644
>--- a/include/exec/memory.h
>+++ b/include/exec/memory.h
>@@ -1288,8 +1288,10 @@ void memory_region_init_io(MemoryRegion *mr,
>  *
>  * Note that this function does not do anything to cause the data in the
>  * RAM memory region to be migrated; that is the responsibility of the caller.
>+ *
>+ * Return: true on success, else false setting @errp with error.
>  */
>-void memory_region_init_ram_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>diff --git a/system/memory.c b/system/memory.c
>index 313dbb2544..337b12a674 100644
>--- a/system/memory.c
>+++ b/system/memory.c
>@@ -1574,13 +1574,14 @@ void memory_region_init_io(MemoryRegion *mr,
>     mr->terminates = true;
> }
> 
>-void memory_region_init_ram_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>                                       Error **errp)
> {
>-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
>+    return memory_region_init_ram_flags_nomigrate(mr, owner, name,
>+                                                  size, 0, errp);
> }
> 
> bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>-- 
>2.41.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 02/25] memory: Have memory_region_init_ram_nomigrate() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 5 +++--
>   2 files changed, 6 insertions(+), 3 deletions(-)
>

s/cpu_exec_realizefn()/memory_region_init_ram_nomigrate() for the commit
message, as mentioned by Peter Xu.

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4140eb0c95..8e6fb55f59 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1498,8 +1498,10 @@ void memory_region_init_alias(MemoryRegion *mr,
  *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 337b12a674..bfe0b62d59 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
     mr->alias_offset = offset;
 }
 
-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
                                       Error **errp)
 {
-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
+    bool rv;
+
+    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
     mr->readonly = true;
+
+    return rv;
 }
 
 void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/exec/memory.h | 4 +++-
> system/memory.c       | 8 ++++++--
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/include/exec/memory.h b/include/exec/memory.h
>index 4140eb0c95..8e6fb55f59 100644
>--- a/include/exec/memory.h
>+++ b/include/exec/memory.h
>@@ -1498,8 +1498,10 @@ void memory_region_init_alias(MemoryRegion *mr,
>  *        must be unique within any device
>  * @size: size of the region.
>  * @errp: pointer to Error*, to store an error if it happens.
>+ *
>+ * Return: true on success, else false setting @errp with error.
>  */
>-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>diff --git a/system/memory.c b/system/memory.c
>index 337b12a674..bfe0b62d59 100644
>--- a/system/memory.c
>+++ b/system/memory.c
>@@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>     mr->alias_offset = offset;
> }
> 
>-void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>+bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>                                       Object *owner,
>                                       const char *name,
>                                       uint64_t size,
>                                       Error **errp)
> {
>-    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
>+    bool rv;
>+
>+    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, errp);
>     mr->readonly = true;
>+

By the way, do we want to set mr->readonly on failure? Should there be 
modifications if an error is propagated upwards?

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Philippe Mathieu-Daudé]

On 21/11/23 13:10, Manos Pitsidianakis wrote:
> On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> Following the example documented since commit e3fe3988d7 ("error:
>> Document Error API usage rules"), have cpu_exec_realizefn()
>> return a boolean indicating whether an error is set or not.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> include/exec/memory.h | 4 +++-
>> system/memory.c       | 8 ++++++--
>> 2 files changed, 9 insertions(+), 3 deletions(-)


>> diff --git a/system/memory.c b/system/memory.c
>> index 337b12a674..bfe0b62d59 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>>     mr->alias_offset = offset;
>> }
>>
>> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>                                       Object *owner,
>>                                       const char *name,
>>                                       uint64_t size,
>>                                       Error **errp)
>> {
>> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0, 
>> errp);
>> +    bool rv;
>> +
>> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, 
>> size, 0, errp);
>>     mr->readonly = true;
>> +
> 
> By the way, do we want to set mr->readonly on failure? Should there be 
> modifications if an error is propagated upwards?

Good point, I'm squashing:

-- >8 --
diff --git a/system/memory.c b/system/memory.c
index a748de3694..72c6441e20 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1707,12 +1707,13 @@ bool 
memory_region_init_rom_nomigrate(MemoryRegion *mr,
                                        uint64_t size,
                                        Error **errp)
  {
-    bool rv;
-
-    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 
0, errp);
+    if (!memory_region_init_ram_flags_nomigrate(mr, owner, name,
+                                                size, 0, errp)) {
+         return false;
+    }
      mr->readonly = true;

-    return rv;
+    return true;
  }
---

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Peter Maydell]

On Fri, 5 Jan 2024 at 14:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 21/11/23 13:10, Manos Pitsidianakis wrote:
> > On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org>
> > wrote:
> >> Following the example documented since commit e3fe3988d7 ("error:
> >> Document Error API usage rules"), have cpu_exec_realizefn()
> >> return a boolean indicating whether an error is set or not.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >> include/exec/memory.h | 4 +++-
> >> system/memory.c       | 8 ++++++--
> >> 2 files changed, 9 insertions(+), 3 deletions(-)
>
>
> >> diff --git a/system/memory.c b/system/memory.c
> >> index 337b12a674..bfe0b62d59 100644
> >> --- a/system/memory.c
> >> +++ b/system/memory.c
> >> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
> >>     mr->alias_offset = offset;
> >> }
> >>
> >> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
> >> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
> >>                                       Object *owner,
> >>                                       const char *name,
> >>                                       uint64_t size,
> >>                                       Error **errp)
> >> {
> >> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0,
> >> errp);
> >> +    bool rv;
> >> +
> >> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name,
> >> size, 0, errp);
> >>     mr->readonly = true;
> >> +
> >
> > By the way, do we want to set mr->readonly on failure? Should there be
> > modifications if an error is propagated upwards?
>
> Good point

I don't think it matters much. If the init function fails,
then the MemoryRegion is not initialized, and there's
nothing you can do with the struct except free it (if it
was in allocated memory). Whether the readonly field is
true or false doesn't matter, because conceptually it's
all undefined-values. And memory_region_init_ram_flags_nomigrate()
has already written to some fields, so avoiding changing
mr->readonly specifically doesn't seem worthwhile.

-- PMM

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Philippe Mathieu-Daudé]

Hi Peter,

On 5/1/24 15:57, Peter Maydell wrote:
> On Fri, 5 Jan 2024 at 14:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 21/11/23 13:10, Manos Pitsidianakis wrote:
>>> On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org>
>>> wrote:
>>>> Following the example documented since commit e3fe3988d7 ("error:
>>>> Document Error API usage rules"), have cpu_exec_realizefn()
>>>> return a boolean indicating whether an error is set or not.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> include/exec/memory.h | 4 +++-
>>>> system/memory.c       | 8 ++++++--
>>>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>
>>
>>>> diff --git a/system/memory.c b/system/memory.c
>>>> index 337b12a674..bfe0b62d59 100644
>>>> --- a/system/memory.c
>>>> +++ b/system/memory.c
>>>> @@ -1729,14 +1729,18 @@ void memory_region_init_alias(MemoryRegion *mr,
>>>>      mr->alias_offset = offset;
>>>> }
>>>>
>>>> -void memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>>> +bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>>>>                                        Object *owner,
>>>>                                        const char *name,
>>>>                                        uint64_t size,
>>>>                                        Error **errp)
>>>> {
>>>> -    memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0,
>>>> errp);
>>>> +    bool rv;
>>>> +
>>>> +    rv = memory_region_init_ram_flags_nomigrate(mr, owner, name,
>>>> size, 0, errp);
>>>>      mr->readonly = true;
>>>> +
>>>
>>> By the way, do we want to set mr->readonly on failure? Should there be
>>> modifications if an error is propagated upwards?
>>
>> Good point
> 
> I don't think it matters much. If the init function fails,
> then the MemoryRegion is not initialized, and there's
> nothing you can do with the struct except free it (if it
> was in allocated memory). Whether the readonly field is
> true or false doesn't matter, because conceptually it's
> all undefined-values. And memory_region_init_ram_flags_nomigrate()
> has already written to some fields, so avoiding changing
> mr->readonly specifically doesn't seem worthwhile.

I concur with your analysis. QEMU Error* type is helpful to propagate
errors, but the cleanup path is rarely well implemented. See for
example the many returns in DeviceRealize handlers without releasing
previously allocated mem.

In this particular patch case, I find Manos suggestion useful, the
code now uses a simpler pattern and avoid having to look at the
callee implementation.

The updated patch is already in a PR I posted before reading your
comment. The changes seem innocuous to me, so not worthwhile to
restore the previous patch content. But if you object, I don't mind
reposting the PR.

Regards,

Phil.

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 8 ++++++--
>   2 files changed, 9 insertions(+), 3 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_rom_nomigrate() in the
commit message, mentioned by Peter Xu. With this:

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp);
    if (
-       errp
+       !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/memory.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index bfe0b62d59..2fe4c3861b 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3628,11 +3628,8 @@ void memory_region_init_rom(MemoryRegion *mr,
                             Error **errp)
 {
     DeviceState *owner_dev;
-    Error *err = NULL;
 
-    memory_region_init_rom_nomigrate(mr, owner, name, size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom_nomigrate(mr, owner, name, size, errp)) {
         return;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
-- 
2.41.0

Re: [PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:38PM +0100, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp);
>     if (
> -       errp
> +       !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp)
>     ) {
>         ...
>         return;
>     }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Richard Henderson]

On 11/20/23 15:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp)

This coccinelle script doesn't quite match...

> @@ -3628,11 +3628,8 @@ void memory_region_init_rom(MemoryRegion *mr,
>                               Error **errp)
>   {
>       DeviceState *owner_dev;
> -    Error *err = NULL;
>   
> -    memory_region_init_rom_nomigrate(mr, owner, name, size, &err);

... this?

That said, the actual code change is good.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Richard Henderson]

On 11/21/23 12:50, Richard Henderson wrote:
> On 11/20/23 15:32, Philippe Mathieu-Daudé wrote:
>> Mechanical change using the following coccinelle script:
>>
>> @@
>> expression mr, owner, arg3, arg4, errp;
>> @@
>> -   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp);
>>      if (
>> -       errp
>> +       !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp)
> 
> This coccinelle script doesn't quite match...
> 
>> @@ -3628,11 +3628,8 @@ void memory_region_init_rom(MemoryRegion *mr,
>>                               Error **errp)
>>   {
>>       DeviceState *owner_dev;
>> -    Error *err = NULL;
>> -    memory_region_init_rom_nomigrate(mr, owner, name, size, &err);
> 
> ... this?

I'm sorry, it does.  "expression errp" can match "err".
It's the manual local variable removal that threw me off.


r~

Re: [PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   system/memory.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 05/25] memory: Simplify memory_region_init_ram_from_fd() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
@@
-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
    if (
-       errp
+       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/memory.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 2fe4c3861b..ca05c4defa 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3604,11 +3604,8 @@ void memory_region_init_ram(MemoryRegion *mr,
                             Error **errp)
 {
     DeviceState *owner_dev;
-    Error *err = NULL;
 
-    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
         return;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
-- 
2.41.0

Re: [PATCH-for-9.0 05/25] memory: Simplify memory_region_init_ram_from_fd() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Mechanical change using the following coccinelle script:
>
>@@
>expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
>@@
>-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
>    if (
>-       errp
>+       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
>    ) {
>        ...
>        return;
>    }
>
>and removing the local Error variable.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> system/memory.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
>diff --git a/system/memory.c b/system/memory.c
>index 2fe4c3861b..ca05c4defa 100644
>--- a/system/memory.c
>+++ b/system/memory.c
>@@ -3604,11 +3604,8 @@ void memory_region_init_ram(MemoryRegion *mr,
>                             Error **errp)
> {
>     DeviceState *owner_dev;
>-    Error *err = NULL;
> 
>-    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
>-    if (err) {
>-        error_propagate(errp, err);
>+    if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
>         return;
>     }
>     /* This will assert if owner is neither NULL nor a DeviceState.
>-- 
>2.41.0
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 05/25] memory: Simplify memory_region_init_ram_from_fd() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
> @@
> -   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
>      if (
> -       errp
> +       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   system/memory.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 06/25] memory: Have memory_region_init_ram() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8e6fb55f59..788872e4a4 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1582,8 +1582,10 @@ void memory_region_init_iommu(void *_iommu_mr,
  * give the RAM block a unique name for migration purposes.
  * We should lift this restriction and allow arbitrary Objects.
  * If you pass a non-NULL non-device @owner then we will assert.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_ram(MemoryRegion *mr,
+bool memory_region_init_ram(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
                             uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index ca05c4defa..4142eac498 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3597,7 +3597,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
     }
 }
 
-void memory_region_init_ram(MemoryRegion *mr,
+bool memory_region_init_ram(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
                             uint64_t size,
@@ -3606,7 +3606,7 @@ void memory_region_init_ram(MemoryRegion *mr,
     DeviceState *owner_dev;
 
     if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
-        return;
+        return false;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
      * We only want the owner here for the purposes of defining a
@@ -3616,6 +3616,8 @@ void memory_region_init_ram(MemoryRegion *mr,
      */
     owner_dev = DEVICE(owner);
     vmstate_register_ram(mr, owner_dev);
+
+    return true;
 }
 
 void memory_region_init_rom(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 06/25] memory: Have memory_region_init_ram() handler return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:40PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()

(same)

> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 06/25] memory: Have memory_region_init_ram() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 6 ++++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 

With Peter Xu's comments addressed:

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 07/25] memory: Have memory_region_init_rom() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 788872e4a4..9d9798a527 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1611,8 +1611,10 @@ bool memory_region_init_ram(MemoryRegion *mr,
  *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_rom(MemoryRegion *mr,
+bool memory_region_init_rom(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
                             uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 4142eac498..2c764947fa 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3620,7 +3620,7 @@ bool memory_region_init_ram(MemoryRegion *mr,
     return true;
 }
 
-void memory_region_init_rom(MemoryRegion *mr,
+bool memory_region_init_rom(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
                             uint64_t size,
@@ -3629,7 +3629,7 @@ void memory_region_init_rom(MemoryRegion *mr,
     DeviceState *owner_dev;
 
     if (!memory_region_init_rom_nomigrate(mr, owner, name, size, errp)) {
-        return;
+        return false;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
      * We only want the owner here for the purposes of defining a
@@ -3639,6 +3639,8 @@ void memory_region_init_rom(MemoryRegion *mr,
      */
     owner_dev = DEVICE(owner);
     vmstate_register_ram(mr, owner_dev);
+
+    return true;
 }
 
 void memory_region_init_rom_device(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 07/25] memory: Have memory_region_init_rom() handler return a boolean

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/exec/memory.h | 4 +++-

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 07/25] memory: Have memory_region_init_rom() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 6 ++++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
>

s/cpu_exec_realizefn()/memory_region_init_rom()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 08/25] memory: Have memory_region_init_rom_device_nomigrate() return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9d9798a527..e2cf3e58de 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1523,8 +1523,10 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
  *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
                                              Object *owner,
                                              const MemoryRegionOps *ops,
                                              void *opaque,
diff --git a/system/memory.c b/system/memory.c
index 2c764947fa..1cccc4b755 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1743,7 +1743,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
     return rv;
 }
 
-void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
+bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
                                              Object *owner,
                                              const MemoryRegionOps *ops,
                                              void *opaque,
@@ -1764,7 +1764,9 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
         error_propagate(errp, err);
+        return false;
     }
+    return true;
 }
 
 void memory_region_init_iommu(void *_iommu_mr,
-- 
2.41.0

Re: [PATCH-for-9.0 08/25] memory: Have memory_region_init_rom_device_nomigrate() return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:42PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()

same

> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 08/25] memory: Have memory_region_init_rom_device_nomigrate() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_rom_device_nomigrate()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 09/25] memory: Simplify memory_region_init_rom_device_nomigrate() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, errp;
@@
-   memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp);
    if (
-       errp
+       !memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/memory.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 1cccc4b755..6d1d315d0e 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3654,12 +3654,9 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    Error **errp)
 {
     DeviceState *owner_dev;
-    Error *err = NULL;
 
-    memory_region_init_rom_device_nomigrate(mr, owner, ops, opaque,
-                                            name, size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom_device_nomigrate(mr, owner, ops, opaque,
+                                                 name, size, errp)) {
         return;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
-- 
2.41.0

Re: [PATCH-for-9.0 09/25] memory: Simplify memory_region_init_rom_device_nomigrate() calls

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:43PM +0100, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, arg5, arg6, errp;
> @@
> -   memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp);
>     if (
> -       errp
> +       !memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp)
>     ) {
>         ...
>         return;
>     }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 09/25] memory: Simplify memory_region_init_rom_device_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, arg5, arg6, errp;
> @@
> -   memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp);
>      if (
> -       errp
> +       !memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   system/memory.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 10/25] memory: Have memory_region_init_rom_device() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e2cf3e58de..92b0c0aa46 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1646,8 +1646,10 @@ bool memory_region_init_rom(MemoryRegion *mr,
  *        must be unique within any device
  * @size: size of the region.
  * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_rom_device(MemoryRegion *mr,
+bool memory_region_init_rom_device(MemoryRegion *mr,
                                    Object *owner,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
diff --git a/system/memory.c b/system/memory.c
index 6d1d315d0e..1b10e177f5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3645,7 +3645,7 @@ bool memory_region_init_rom(MemoryRegion *mr,
     return true;
 }
 
-void memory_region_init_rom_device(MemoryRegion *mr,
+bool memory_region_init_rom_device(MemoryRegion *mr,
                                    Object *owner,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
@@ -3657,7 +3657,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
 
     if (!memory_region_init_rom_device_nomigrate(mr, owner, ops, opaque,
                                                  name, size, errp)) {
-        return;
+        return false;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
      * We only want the owner here for the purposes of defining a
@@ -3667,6 +3667,8 @@ void memory_region_init_rom_device(MemoryRegion *mr,
      */
     owner_dev = DEVICE(owner);
     vmstate_register_ram(mr, owner_dev);
+
+    return true;
 }
 
 /*
-- 
2.41.0

Re: [PATCH-for-9.0 10/25] memory: Have memory_region_init_rom_device() handler return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:44PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()

same

> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 10/25] memory: Have memory_region_init_rom_device() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 6 ++++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 

With Peter Xu's comments addressed, to s/cpu_exec_realizefn()/memory_region_init_rom_device()
in the commit messages.

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 11/25] memory: Have memory_region_init_resizeable_ram() return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 92b0c0aa46..218b35a849 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1342,8 +1342,10 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
  *
  * Note that this function does not do anything to cause the data in the
  * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_resizeable_ram(MemoryRegion *mr,
+bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                        Object *owner,
                                        const char *name,
                                        uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 1b10e177f5..f424282526 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1606,7 +1606,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
     return true;
 }
 
-void memory_region_init_resizeable_ram(MemoryRegion *mr,
+bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                        Object *owner,
                                        const char *name,
                                        uint64_t size,
@@ -1627,7 +1627,9 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
         error_propagate(errp, err);
+        return false;
     }
+    return true;
 }
 
 #ifdef CONFIG_POSIX
-- 
2.41.0

Re: [PATCH-for-9.0 11/25] memory: Have memory_region_init_resizeable_ram() return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:45PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()

same

> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 11/25] memory: Have memory_region_init_resizeable_ram() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_resizeable_ram()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 12/25] memory: Have memory_region_init_ram_from_file() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 218b35a849..2034a48544 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1376,8 +1376,10 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
  *
  * Note that this function does not do anything to cause the data in the
  * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_ram_from_file(MemoryRegion *mr,
+bool memory_region_init_ram_from_file(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index f424282526..4a36779ba1 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1633,7 +1633,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
 }
 
 #ifdef CONFIG_POSIX
-void memory_region_init_ram_from_file(MemoryRegion *mr,
+bool memory_region_init_ram_from_file(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
                                       uint64_t size,
@@ -1656,7 +1656,9 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
         error_propagate(errp, err);
+        return false;
     }
+    return true;
 }
 
 void memory_region_init_ram_from_fd(MemoryRegion *mr,
-- 
2.41.0

Re: [PATCH-for-9.0 12/25] memory: Have memory_region_init_ram_from_file() handler return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:46PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 12/25] memory: Have memory_region_init_ram_from_file() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_ram_from_file()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 13/25] memory: Have memory_region_init_ram_from_fd() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/memory.h | 4 +++-
 system/memory.c       | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2034a48544..f81b48499a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1406,8 +1406,10 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
  *
  * Note that this function does not do anything to cause the data in the
  * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void memory_region_init_ram_from_fd(MemoryRegion *mr,
+bool memory_region_init_ram_from_fd(MemoryRegion *mr,
                                     Object *owner,
                                     const char *name,
                                     uint64_t size,
diff --git a/system/memory.c b/system/memory.c
index 4a36779ba1..e55fe3dfdf 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1661,7 +1661,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
     return true;
 }
 
-void memory_region_init_ram_from_fd(MemoryRegion *mr,
+bool memory_region_init_ram_from_fd(MemoryRegion *mr,
                                     Object *owner,
                                     const char *name,
                                     uint64_t size,
@@ -1682,7 +1682,9 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
         error_propagate(errp, err);
+        return false;
     }
+    return true;
 }
 #endif
 
-- 
2.41.0

Re: [PATCH-for-9.0 13/25] memory: Have memory_region_init_ram_from_fd() handler return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:47PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 13/25] memory: Have memory_region_init_ram_from_fd() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/exec/memory.h | 4 +++-
>   system/memory.c       | 4 +++-
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 

s/cpu_exec_realizefn()/memory_region_init_ram_from_fd()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 14/25] backends: Use g_autofree in HostMemoryBackendClass::alloc() handlers

[Philippe Mathieu-Daudé]

In preparation of having HostMemoryBackendClass::alloc() handlers
return a boolean, have them use g_autofree.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/hostmem-epc.c   | 3 +--
 backends/hostmem-file.c  | 3 +--
 backends/hostmem-memfd.c | 3 +--
 backends/hostmem-ram.c   | 3 +--
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/backends/hostmem-epc.c b/backends/hostmem-epc.c
index 4e162d6789..3ceb079f9e 100644
--- a/backends/hostmem-epc.c
+++ b/backends/hostmem-epc.c
@@ -20,8 +20,8 @@
 static void
 sgx_epc_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
+    g_autofree char *name = NULL;
     uint32_t ram_flags;
-    char *name;
     int fd;
 
     if (!backend->size) {
@@ -41,7 +41,6 @@ sgx_epc_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
                                    name, backend->size, ram_flags,
                                    fd, 0, errp);
-    g_free(name);
 }
 
 static void sgx_epc_backend_instance_init(Object *obj)
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 361d4a8103..fe8c481f8f 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -44,8 +44,8 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                object_get_typename(OBJECT(backend)));
 #else
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
+    g_autofree gchar *name = NULL;
     uint32_t ram_flags;
-    gchar *name;
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
@@ -89,7 +89,6 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
                                      backend->size, fb->align, ram_flags,
                                      fb->mem_path, fb->offset, errp);
-    g_free(name);
 #endif
 }
 
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index 3fc85c3db8..db28ab5a56 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -35,8 +35,8 @@ static void
 memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(backend);
+    g_autofree char *name = NULL;
     uint32_t ram_flags;
-    char *name;
     int fd;
 
     if (!backend->size) {
@@ -57,7 +57,6 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
     memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
                                    backend->size, ram_flags, fd, 0, errp);
-    g_free(name);
 }
 
 static bool
diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index b8e55cdbd0..0a670fc22a 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -19,8 +19,8 @@
 static void
 ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
+    g_autofree char *name = NULL;
     uint32_t ram_flags;
-    char *name;
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
@@ -32,7 +32,6 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
     memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend), name,
                                            backend->size, ram_flags, errp);
-    g_free(name);
 }
 
 static void
-- 
2.41.0

Re: [PATCH-for-9.0 14/25] backends: Use g_autofree in HostMemoryBackendClass::alloc() handlers

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>In preparation of having HostMemoryBackendClass::alloc() handlers
>return a boolean, have them use g_autofree.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---


Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 14/25] backends: Use g_autofree in HostMemoryBackendClass::alloc() handlers

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> In preparation of having HostMemoryBackendClass::alloc() handlers
> return a boolean, have them use g_autofree.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/hostmem-epc.c   | 3 +--
>   backends/hostmem-file.c  | 3 +--
>   backends/hostmem-memfd.c | 3 +--
>   backends/hostmem-ram.c   | 3 +--
>   4 files changed, 4 insertions(+), 8 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 15/25] backends: Simplify host_memory_backend_memory_complete()

[Philippe Mathieu-Daudé]

Return early if bc->alloc is NULL. De-indent the if() ladder.

Note, this avoids a pointless call to error_propagate() with
errp=NULL at the 'out:' label.

Change trivial when reviewed with 'git-diff --ignore-all-space'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/hostmem.c | 133 +++++++++++++++++++++++----------------------
 1 file changed, 67 insertions(+), 66 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 747e7838c0..1723c19165 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -328,83 +328,84 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
     void *ptr;
     uint64_t sz;
 
-    if (bc->alloc) {
-        bc->alloc(backend, &local_err);
-        if (local_err) {
-            goto out;
-        }
+    if (!bc->alloc) {
+        return;
+    }
+    bc->alloc(backend, &local_err);
+    if (local_err) {
+        goto out;
+    }
 
-        ptr = memory_region_get_ram_ptr(&backend->mr);
-        sz = memory_region_size(&backend->mr);
+    ptr = memory_region_get_ram_ptr(&backend->mr);
+    sz = memory_region_size(&backend->mr);
 
-        if (backend->merge) {
-            qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE);
-        }
-        if (!backend->dump) {
-            qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
-        }
+    if (backend->merge) {
+        qemu_madvise(ptr, sz, QEMU_MADV_MERGEABLE);
+    }
+    if (!backend->dump) {
+        qemu_madvise(ptr, sz, QEMU_MADV_DONTDUMP);
+    }
 #ifdef CONFIG_NUMA
-        unsigned long lastbit = find_last_bit(backend->host_nodes, MAX_NODES);
-        /* lastbit == MAX_NODES means maxnode = 0 */
-        unsigned long maxnode = (lastbit + 1) % (MAX_NODES + 1);
-        /* ensure policy won't be ignored in case memory is preallocated
-         * before mbind(). note: MPOL_MF_STRICT is ignored on hugepages so
-         * this doesn't catch hugepage case. */
-        unsigned flags = MPOL_MF_STRICT | MPOL_MF_MOVE;
-        int mode = backend->policy;
+    unsigned long lastbit = find_last_bit(backend->host_nodes, MAX_NODES);
+    /* lastbit == MAX_NODES means maxnode = 0 */
+    unsigned long maxnode = (lastbit + 1) % (MAX_NODES + 1);
+    /* ensure policy won't be ignored in case memory is preallocated
+     * before mbind(). note: MPOL_MF_STRICT is ignored on hugepages so
+     * this doesn't catch hugepage case. */
+    unsigned flags = MPOL_MF_STRICT | MPOL_MF_MOVE;
+    int mode = backend->policy;
 
-        /* check for invalid host-nodes and policies and give more verbose
-         * error messages than mbind(). */
-        if (maxnode && backend->policy == MPOL_DEFAULT) {
-            error_setg(errp, "host-nodes must be empty for policy default,"
-                       " or you should explicitly specify a policy other"
-                       " than default");
-            return;
-        } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
-            error_setg(errp, "host-nodes must be set for policy %s",
-                       HostMemPolicy_str(backend->policy));
-            return;
-        }
+    /* check for invalid host-nodes and policies and give more verbose
+     * error messages than mbind(). */
+    if (maxnode && backend->policy == MPOL_DEFAULT) {
+        error_setg(errp, "host-nodes must be empty for policy default,"
+                   " or you should explicitly specify a policy other"
+                   " than default");
+        return;
+    } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
+        error_setg(errp, "host-nodes must be set for policy %s",
+                   HostMemPolicy_str(backend->policy));
+        return;
+    }
 
-        /* We can have up to MAX_NODES nodes, but we need to pass maxnode+1
-         * as argument to mbind() due to an old Linux bug (feature?) which
-         * cuts off the last specified node. This means backend->host_nodes
-         * must have MAX_NODES+1 bits available.
-         */
-        assert(sizeof(backend->host_nodes) >=
-               BITS_TO_LONGS(MAX_NODES + 1) * sizeof(unsigned long));
-        assert(maxnode <= MAX_NODES);
+    /* We can have up to MAX_NODES nodes, but we need to pass maxnode+1
+     * as argument to mbind() due to an old Linux bug (feature?) which
+     * cuts off the last specified node. This means backend->host_nodes
+     * must have MAX_NODES+1 bits available.
+     */
+    assert(sizeof(backend->host_nodes) >=
+           BITS_TO_LONGS(MAX_NODES + 1) * sizeof(unsigned long));
+    assert(maxnode <= MAX_NODES);
 
 #ifdef HAVE_NUMA_HAS_PREFERRED_MANY
-        if (mode == MPOL_PREFERRED && numa_has_preferred_many() > 0) {
-            /*
-             * Replace with MPOL_PREFERRED_MANY otherwise the mbind() below
-             * silently picks the first node.
-             */
-            mode = MPOL_PREFERRED_MANY;
-        }
+    if (mode == MPOL_PREFERRED && numa_has_preferred_many() > 0) {
+        /*
+         * Replace with MPOL_PREFERRED_MANY otherwise the mbind() below
+         * silently picks the first node.
+         */
+        mode = MPOL_PREFERRED_MANY;
+    }
 #endif
 
-        if (maxnode &&
-            mbind(ptr, sz, mode, backend->host_nodes, maxnode + 1, flags)) {
-            if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
-                error_setg_errno(errp, errno,
-                                 "cannot bind memory to host NUMA nodes");
-                return;
-            }
+    if (maxnode &&
+        mbind(ptr, sz, mode, backend->host_nodes, maxnode + 1, flags)) {
+        if (backend->policy != MPOL_DEFAULT || errno != ENOSYS) {
+            error_setg_errno(errp, errno,
+                             "cannot bind memory to host NUMA nodes");
+            return;
         }
+    }
 #endif
-        /* Preallocate memory after the NUMA policy has been instantiated.
-         * This is necessary to guarantee memory is allocated with
-         * specified NUMA policy in place.
-         */
-        if (backend->prealloc) {
-            qemu_prealloc_mem(memory_region_get_fd(&backend->mr), ptr, sz,
-                              backend->prealloc_threads,
-                              backend->prealloc_context, &local_err);
-            if (local_err) {
-                goto out;
-            }
+    /* Preallocate memory after the NUMA policy has been instantiated.
+     * This is necessary to guarantee memory is allocated with
+     * specified NUMA policy in place.
+     */
+    if (backend->prealloc) {
+        qemu_prealloc_mem(memory_region_get_fd(&backend->mr), ptr, sz,
+                          backend->prealloc_threads,
+                          backend->prealloc_context, &local_err);
+        if (local_err) {
+            goto out;
         }
     }
 out:
-- 
2.41.0

Re: [PATCH-for-9.0 15/25] backends: Simplify host_memory_backend_memory_complete()

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Return early if bc->alloc is NULL. De-indent the if() ladder.
>
>Note, this avoids a pointless call to error_propagate() with
>errp=NULL at the 'out:' label.
>
>Change trivial when reviewed with 'git-diff --ignore-all-space'.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 15/25] backends: Simplify host_memory_backend_memory_complete()

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Return early if bc->alloc is NULL. De-indent the if() ladder.
> 
> Note, this avoids a pointless call to error_propagate() with
> errp=NULL at the 'out:' label.
> 
> Change trivial when reviewed with 'git-diff --ignore-all-space'.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/hostmem.c | 133 +++++++++++++++++++++++----------------------
>   1 file changed, 67 insertions(+), 66 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 16/25] backends: Have HostMemoryBackendClass::alloc() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/hostmem.h | 10 +++++++++-
 backends/hostmem-epc.c   | 11 +++++------
 backends/hostmem-file.c  | 19 ++++++++++---------
 backends/hostmem-memfd.c | 10 +++++-----
 backends/hostmem-ram.c   |  9 +++++----
 backends/hostmem.c       |  5 ++---
 6 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 39326f1d4f..0e411aaa29 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -47,7 +47,15 @@ OBJECT_DECLARE_TYPE(HostMemoryBackend, HostMemoryBackendClass,
 struct HostMemoryBackendClass {
     ObjectClass parent_class;
 
-    void (*alloc)(HostMemoryBackend *backend, Error **errp);
+    /**
+     * alloc: Allocate memory from backend.
+     *
+     * @backend: the #HostMemoryBackend.
+     * @errp: pointer to Error*, to store an error if it happens.
+     *
+     * Return: true on success, else false setting @errp with error.
+     */
+    bool (*alloc)(HostMemoryBackend *backend, Error **errp);
 };
 
 /**
diff --git a/backends/hostmem-epc.c b/backends/hostmem-epc.c
index 3ceb079f9e..735e2e1cf8 100644
--- a/backends/hostmem-epc.c
+++ b/backends/hostmem-epc.c
@@ -17,7 +17,7 @@
 #include "sysemu/hostmem.h"
 #include "hw/i386/hostmem-epc.h"
 
-static void
+static bool
 sgx_epc_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     g_autofree char *name = NULL;
@@ -26,21 +26,20 @@ sgx_epc_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
-        return;
+        return false;
     }
 
     fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
     if (fd < 0) {
         error_setg_errno(errp, errno,
                          "failed to open /dev/sgx_vepc to alloc SGX EPC");
-        return;
+        return false;
     }
 
     name = object_get_canonical_path(OBJECT(backend));
     ram_flags = (backend->share ? RAM_SHARED : 0) | RAM_PROTECTED;
-    memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
-                                   name, backend->size, ram_flags,
-                                   fd, 0, errp);
+    return memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
+                                          backend->size, ram_flags, fd, 0, errp);
 }
 
 static void sgx_epc_backend_instance_init(Object *obj)
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index fe8c481f8f..ac3e433cbd 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -36,12 +36,13 @@ struct HostMemoryBackendFile {
     OnOffAuto rom;
 };
 
-static void
+static bool
 file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
 #ifndef CONFIG_POSIX
     error_setg(errp, "backend '%s' not supported on this host",
                object_get_typename(OBJECT(backend)));
+    return false;
 #else
     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
     g_autofree gchar *name = NULL;
@@ -49,11 +50,11 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
-        return;
+        return false;
     }
     if (!fb->mem_path) {
         error_setg(errp, "mem-path property not set");
-        return;
+        return false;
     }
 
     switch (fb->rom) {
@@ -65,18 +66,18 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         if (!fb->readonly) {
             error_setg(errp, "property 'rom' = 'on' is not supported with"
                        " 'readonly' = 'off'");
-            return;
+            return false;
         }
         break;
     case ON_OFF_AUTO_OFF:
         if (fb->readonly && backend->share) {
             error_setg(errp, "property 'rom' = 'off' is incompatible with"
                        " 'readonly' = 'on' and 'share' = 'on'");
-            return;
+            return false;
         }
         break;
     default:
-        assert(false);
+        g_assert_not_reached();
     }
 
     name = host_memory_backend_get_name(backend);
@@ -86,9 +87,9 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
     ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
     ram_flags |= RAM_NAMED_FILE;
-    memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
-                                     backend->size, fb->align, ram_flags,
-                                     fb->mem_path, fb->offset, errp);
+    return memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
+                                            backend->size, fb->align, ram_flags,
+                                            fb->mem_path, fb->offset, errp);
 #endif
 }
 
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index db28ab5a56..3923ea9364 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -31,7 +31,7 @@ struct HostMemoryBackendMemfd {
     bool seal;
 };
 
-static void
+static bool
 memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     HostMemoryBackendMemfd *m = MEMORY_BACKEND_MEMFD(backend);
@@ -41,7 +41,7 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
-        return;
+        return false;
     }
 
     fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size,
@@ -49,14 +49,14 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL : 0,
                            errp);
     if (fd == -1) {
-        return;
+        return false;
     }
 
     name = host_memory_backend_get_name(backend);
     ram_flags = backend->share ? RAM_SHARED : 0;
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
-    memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
-                                   backend->size, ram_flags, fd, 0, errp);
+    return memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend), name,
+                                          backend->size, ram_flags, fd, 0, errp);
 }
 
 static bool
diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index 0a670fc22a..d121249f0f 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -16,7 +16,7 @@
 #include "qemu/module.h"
 #include "qom/object_interfaces.h"
 
-static void
+static bool
 ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 {
     g_autofree char *name = NULL;
@@ -24,14 +24,15 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 
     if (!backend->size) {
         error_setg(errp, "can't create backend with size 0");
-        return;
+        return false;
     }
 
     name = host_memory_backend_get_name(backend);
     ram_flags = backend->share ? RAM_SHARED : 0;
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
-    memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend), name,
-                                           backend->size, ram_flags, errp);
+    return memory_region_init_ram_flags_nomigrate(&backend->mr, OBJECT(backend),
+                                                  name, backend->size,
+                                                  ram_flags, errp);
 }
 
 static void
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 1723c19165..3f8eb936d7 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -331,9 +331,8 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
     if (!bc->alloc) {
         return;
     }
-    bc->alloc(backend, &local_err);
-    if (local_err) {
-        goto out;
+    if (!bc->alloc(backend, errp)) {
+        return;
     }
 
     ptr = memory_region_get_ram_ptr(&backend->mr);
-- 
2.41.0

Re: [PATCH-for-9.0 16/25] backends: Have HostMemoryBackendClass::alloc() handler return a boolean

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Following the example documented since commit e3fe3988d7 ("error:
>Document Error API usage rules"), have cpu_exec_realizefn()
>return a boolean indicating whether an error is set or not.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 16/25] backends: Have HostMemoryBackendClass::alloc() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/sysemu/hostmem.h | 10 +++++++++-
>   backends/hostmem-epc.c   | 11 +++++------
>   backends/hostmem-file.c  | 19 ++++++++++---------
>   backends/hostmem-memfd.c | 10 +++++-----
>   backends/hostmem-ram.c   |  9 +++++----
>   backends/hostmem.c       |  5 ++---
>   6 files changed, 36 insertions(+), 28 deletions(-)
> 

s/cpu_exec_realizefn()/HostMemoryBackendClass::alloc()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 17/25] backends: Reduce variable scope in host_memory_backend_memory_complete

[Philippe Mathieu-Daudé]

Reduce the &local_err variable use and remove the 'out:' label.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/hostmem.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 3f8eb936d7..1b0043a0d9 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -324,7 +324,6 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(uc);
     HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
-    Error *local_err = NULL;
     void *ptr;
     uint64_t sz;
 
@@ -400,15 +399,16 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
      * specified NUMA policy in place.
      */
     if (backend->prealloc) {
+        Error *local_err = NULL;
+
         qemu_prealloc_mem(memory_region_get_fd(&backend->mr), ptr, sz,
                           backend->prealloc_threads,
                           backend->prealloc_context, &local_err);
         if (local_err) {
-            goto out;
+            error_propagate(errp, local_err);
+            return;
         }
     }
-out:
-    error_propagate(errp, local_err);
 }
 
 static bool
-- 
2.41.0

Re: [PATCH-for-9.0 17/25] backends: Reduce variable scope in host_memory_backend_memory_complete

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Reduce the &local_err variable use and remove the 'out:' label.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> backends/hostmem.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/backends/hostmem.c b/backends/hostmem.c
>index 3f8eb936d7..1b0043a0d9 100644
>--- a/backends/hostmem.c
>+++ b/backends/hostmem.c
>@@ -324,7 +324,6 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
> {
>     HostMemoryBackend *backend = MEMORY_BACKEND(uc);
>     HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
>-    Error *local_err = NULL;
>     void *ptr;
>     uint64_t sz;
> 
>@@ -400,15 +399,16 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>      * specified NUMA policy in place.
>      */
>     if (backend->prealloc) {
>+        Error *local_err = NULL;
>+
>         qemu_prealloc_mem(memory_region_get_fd(&backend->mr), ptr, sz,
>                           backend->prealloc_threads,
>                           backend->prealloc_context, &local_err);
>         if (local_err) {
>-            goto out;
>+            error_propagate(errp, local_err);
>+            return;
>         }
>     }
>-out:
>-    error_propagate(errp, local_err);
> }
> 
> static bool
>-- 

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 17/25] backends: Reduce variable scope in host_memory_backend_memory_complete

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Reduce the &local_err variable use and remove the 'out:' label.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/hostmem.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 18/25] util/oslib: Have qemu_prealloc_mem() handler return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu/osdep.h | 4 +++-
 util/oslib-posix.c   | 7 +++++--
 util/oslib-win32.c   | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 475a1c62ff..9a405bed89 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -678,8 +678,10 @@ typedef struct ThreadContext ThreadContext;
  * memory area starting at @area with the size of @sz. After a successful call,
  * each page in the area was faulted in writable at least once, for example,
  * after allocating file blocks for mapped files.
+ *
+ * Return: true on success, else false setting @errp with error.
  */
-void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
+bool qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
                        ThreadContext *tc, Error **errp);
 
 /**
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index e86fd64e09..7c297003b9 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -497,7 +497,7 @@ static bool madv_populate_write_possible(char *area, size_t pagesize)
            errno != EINVAL;
 }
 
-void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
+bool qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
                        ThreadContext *tc, Error **errp)
 {
     static gsize initialized;
@@ -506,6 +506,7 @@ void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
     size_t numpages = DIV_ROUND_UP(sz, hpagesize);
     bool use_madv_populate_write;
     struct sigaction act;
+    bool rv = true;
 
     /*
      * Sense on every invocation, as MADV_POPULATE_WRITE cannot be used for
@@ -534,7 +535,7 @@ void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
             qemu_mutex_unlock(&sigbus_mutex);
             error_setg_errno(errp, errno,
                 "qemu_prealloc_mem: failed to install signal handler");
-            return;
+            return false;
         }
     }
 
@@ -544,6 +545,7 @@ void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
     if (ret) {
         error_setg_errno(errp, -ret,
                          "qemu_prealloc_mem: preallocating memory failed");
+        rv = false;
     }
 
     if (!use_madv_populate_write) {
@@ -555,6 +557,7 @@ void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
         }
         qemu_mutex_unlock(&sigbus_mutex);
     }
+    return rv;
 }
 
 char *qemu_get_pid_name(pid_t pid)
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 55b0189dc3..c4a5f05a49 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -264,7 +264,7 @@ int getpagesize(void)
     return system_info.dwPageSize;
 }
 
-void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
+bool qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
                        ThreadContext *tc, Error **errp)
 {
     int i;
@@ -274,6 +274,8 @@ void qemu_prealloc_mem(int fd, char *area, size_t sz, int max_threads,
     for (i = 0; i < sz / pagesize; i++) {
         memset(area + pagesize * i, 0, 1);
     }
+
+    return true;
 }
 
 char *qemu_get_pid_name(pid_t pid)
-- 
2.41.0

Re: [PATCH-for-9.0 18/25] util/oslib: Have qemu_prealloc_mem() handler return a boolean

[Peter Xu]

On Mon, Nov 20, 2023 at 10:32:52PM +0100, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH-for-9.0 18/25] util/oslib: Have qemu_prealloc_mem() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/qemu/osdep.h | 4 +++-
>   util/oslib-posix.c   | 7 +++++--
>   util/oslib-win32.c   | 4 +++-
>   3 files changed, 11 insertions(+), 4 deletions(-)
>

s/cpu_exec_realizefn()/qemu_prealloc_mem()

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 19/25] misc: Simplify qemu_prealloc_mem() calls

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/hostmem.c     | 22 +++++++---------------
 hw/virtio/virtio-mem.c |  6 ++----
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 1b0043a0d9..30f69b2cb5 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -219,7 +219,6 @@ static bool host_memory_backend_get_prealloc(Object *obj, Error **errp)
 static void host_memory_backend_set_prealloc(Object *obj, bool value,
                                              Error **errp)
 {
-    Error *local_err = NULL;
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
 
     if (!backend->reserve && value) {
@@ -237,10 +236,8 @@ static void host_memory_backend_set_prealloc(Object *obj, bool value,
         void *ptr = memory_region_get_ram_ptr(&backend->mr);
         uint64_t sz = memory_region_size(&backend->mr);
 
-        qemu_prealloc_mem(fd, ptr, sz, backend->prealloc_threads,
-                          backend->prealloc_context, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!qemu_prealloc_mem(fd, ptr, sz, backend->prealloc_threads,
+                               backend->prealloc_context, errp)) {
             return;
         }
         backend->prealloc = true;
@@ -398,16 +395,11 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
      * This is necessary to guarantee memory is allocated with
      * specified NUMA policy in place.
      */
-    if (backend->prealloc) {
-        Error *local_err = NULL;
-
-        qemu_prealloc_mem(memory_region_get_fd(&backend->mr), ptr, sz,
-                          backend->prealloc_threads,
-                          backend->prealloc_context, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
+    if (backend->prealloc && !qemu_prealloc_mem(memory_region_get_fd(&backend->mr),
+                                                ptr, sz,
+                                                backend->prealloc_threads,
+                                                backend->prealloc_context, errp)) {
+        return;
     }
 }
 
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 75ee38aa46..12dc39e0b1 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -605,8 +605,7 @@ static int virtio_mem_set_block_state(VirtIOMEM *vmem, uint64_t start_gpa,
         int fd = memory_region_get_fd(&vmem->memdev->mr);
         Error *local_err = NULL;
 
-        qemu_prealloc_mem(fd, area, size, 1, NULL, &local_err);
-        if (local_err) {
+        if (!qemu_prealloc_mem(fd, area, size, 1, NULL, &local_err)) {
             static bool warned;
 
             /*
@@ -1249,8 +1248,7 @@ static int virtio_mem_prealloc_range_cb(VirtIOMEM *vmem, void *arg,
     int fd = memory_region_get_fd(&vmem->memdev->mr);
     Error *local_err = NULL;
 
-    qemu_prealloc_mem(fd, area, size, 1, NULL, &local_err);
-    if (local_err) {
+    if (!qemu_prealloc_mem(fd, area, size, 1, NULL, &local_err)) {
         error_report_err(local_err);
         return -ENOMEM;
     }
-- 
2.41.0

Re: [PATCH-for-9.0 19/25] misc: Simplify qemu_prealloc_mem() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---

Commit message missing but indeed there's not much to say

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 19/25] misc: Simplify qemu_prealloc_mem() calls

[Philippe Mathieu-Daudé]

On 22/11/23 08:38, Manos Pitsidianakis wrote:
> On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
> 
> Commit message missing but indeed there's not much to say

I amended:

   Since qemu_prealloc_mem() returns whether or not an error
   occured, we don't need to check the @errp pointer. Remove
   local_err uses when we can return directly.

> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Thanks!

Re: [PATCH-for-9.0 19/25] misc: Simplify qemu_prealloc_mem() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/hostmem.c     | 22 +++++++---------------
>   hw/virtio/virtio-mem.c |  6 ++----
>   2 files changed, 9 insertions(+), 19 deletions(-)
>

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_ram(mr, owner, arg3, arg4, &errp);
    if (
-       errp
+       !memory_region_init_ram(mr, owner, arg3, arg4, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/aspeed_ast2400.c | 6 ++----
 hw/arm/aspeed_ast2600.c | 6 ++----
 hw/arm/fsl-imx25.c      | 6 ++----
 hw/arm/fsl-imx31.c      | 6 ++----
 hw/arm/fsl-imx6.c       | 6 ++----
 hw/arm/integratorcp.c   | 7 ++-----
 hw/arm/nrf51_soc.c      | 7 ++-----
 hw/ppc/rs6000_mc.c      | 7 ++-----
 8 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
index a4334c81b8..0baa2ff96e 100644
--- a/hw/arm/aspeed_ast2400.c
+++ b/hw/arm/aspeed_ast2400.c
@@ -247,7 +247,6 @@ static void aspeed_ast2400_soc_realize(DeviceState *dev, Error **errp)
     Aspeed2400SoCState *a = ASPEED2400_SOC(dev);
     AspeedSoCState *s = ASPEED_SOC(dev);
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-    Error *err = NULL;
     g_autofree char *sram_name = NULL;
 
     /* Default boot region (SPI memory or ROMs) */
@@ -276,9 +275,8 @@ static void aspeed_ast2400_soc_realize(DeviceState *dev, Error **errp)
 
     /* SRAM */
     sram_name = g_strdup_printf("aspeed.sram.%d", CPU(&a->cpu[0])->cpu_index);
-    memory_region_init_ram(&s->sram, OBJECT(s), sram_name, sc->sram_size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->sram, OBJECT(s), sram_name, sc->sram_size,
+                                errp)) {
         return;
     }
     memory_region_add_subregion(s->memory,
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index b965fbab5e..3a9a303ab8 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -282,7 +282,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     Aspeed2600SoCState *a = ASPEED2600_SOC(dev);
     AspeedSoCState *s = ASPEED_SOC(dev);
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-    Error *err = NULL;
     qemu_irq irq;
     g_autofree char *sram_name = NULL;
 
@@ -355,9 +354,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
 
     /* SRAM */
     sram_name = g_strdup_printf("aspeed.sram.%d", CPU(&a->cpu[0])->cpu_index);
-    memory_region_init_ram(&s->sram, OBJECT(s), sram_name, sc->sram_size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->sram, OBJECT(s), sram_name, sc->sram_size,
+                                errp)) {
         return;
     }
     memory_region_add_subregion(s->memory,
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 9aabbf7f58..b15435ccaf 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -299,10 +299,8 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
                                 &s->rom[1]);
 
     /* initialize internal RAM (128 KB) */
-    memory_region_init_ram(&s->iram, NULL, "imx25.iram", FSL_IMX25_IRAM_SIZE,
-                           &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->iram, NULL, "imx25.iram",
+                                FSL_IMX25_IRAM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX25_IRAM_ADDR,
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index def27bb913..1d5dcd51e8 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -208,10 +208,8 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
                                 &s->rom);
 
     /* initialize internal RAM (16 KB) */
-    memory_region_init_ram(&s->iram, NULL, "imx31.iram", FSL_IMX31_IRAM_SIZE,
-                           &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->iram, NULL, "imx31.iram",
+                                FSL_IMX31_IRAM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX31_IRAM_ADDR,
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 7dc42cbfe6..58f37e7c11 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -443,10 +443,8 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
                                 &s->caam);
 
     /* OCRAM memory */
-    memory_region_init_ram(&s->ocram, NULL, "imx6.ocram", FSL_IMX6_OCRAM_SIZE,
-                           &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->ocram, NULL, "imx6.ocram",
+                                FSL_IMX6_OCRAM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_OCRAM_ADDR,
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index d176e9af7e..bb722fd46f 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -291,12 +291,9 @@ static void integratorcm_realize(DeviceState *d, Error **errp)
 {
     IntegratorCMState *s = INTEGRATOR_CM(d);
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
-    Error *local_err = NULL;
 
-    memory_region_init_ram(&s->flash, OBJECT(d), "integrator.flash", 0x100000,
-                           &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!memory_region_init_ram(&s->flash, OBJECT(d), "integrator.flash",
+                                0x100000, errp)) {
         return;
     }
 
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 34da0d62f0..ac53441630 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -58,7 +58,6 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
     MemoryRegion *mr;
-    Error *err = NULL;
     uint8_t i = 0;
     hwaddr base_addr = 0;
 
@@ -92,10 +91,8 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
 
-    memory_region_init_ram(&s->sram, OBJECT(s), "nrf51.sram", s->sram_size,
-                           &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_ram(&s->sram, OBJECT(s), "nrf51.sram", s->sram_size,
+                                errp)) {
         return;
     }
     memory_region_add_subregion(&s->container, NRF51_SRAM_BASE, &s->sram);
diff --git a/hw/ppc/rs6000_mc.c b/hw/ppc/rs6000_mc.c
index c0bc212e92..0bf9cb9c47 100644
--- a/hw/ppc/rs6000_mc.c
+++ b/hw/ppc/rs6000_mc.c
@@ -143,7 +143,6 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
     RS6000MCState *s = RS6000MC(dev);
     int socket = 0;
     unsigned int ram_size = s->ram_size / MiB;
-    Error *local_err = NULL;
 
     while (socket < 6) {
         if (ram_size >= 64) {
@@ -165,10 +164,8 @@ static void rs6000mc_realize(DeviceState *dev, Error **errp)
         if (s->simm_size[socket]) {
             char name[] = "simm.?";
             name[5] = socket + '0';
-            memory_region_init_ram(&s->simm[socket], OBJECT(dev), name,
-                                   s->simm_size[socket] * MiB, &local_err);
-            if (local_err) {
-                error_propagate(errp, local_err);
+            if (!memory_region_init_ram(&s->simm[socket], OBJECT(dev), name,
+                                        s->simm_size[socket] * MiB, errp)) {
                 return;
             }
             memory_region_add_subregion_overlap(get_system_memory(), 0,
-- 
2.41.0

Re: [PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Mechanical change using the following coccinelle script:
>
>@@
>expression mr, owner, arg3, arg4, errp;
>@@
>-   memory_region_init_ram(mr, owner, arg3, arg4, &errp);
>    if (
>-       errp
>+       !memory_region_init_ram(mr, owner, arg3, arg4, &errp)
>    ) {
>        ...
>        return;
>    }
>
>and removing the local Error variable.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls

[Andrew Jeffery]

On Mon, 2023-11-20 at 22:32 +0100, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_ram(mr, owner, arg3, arg4, &errp);
>     if (
> -       errp
> +       !memory_region_init_ram(mr, owner, arg3, arg4, &errp)
>     ) {
>         ...
>         return;
>     }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/aspeed_ast2400.c | 6 ++----
>  hw/arm/aspeed_ast2600.c | 6 ++----

For my own benefit it looks like the motivating thread for this series
is: 

https://lore.kernel.org/qemu-devel/936e1ac4-cef8-08b4-c688-e5b1e057208b@linaro.org/

Anyway,

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au> # aspeed

Re: [PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_ram(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_ram(mr, owner, arg3, arg4, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/aspeed_ast2400.c | 6 ++----
>   hw/arm/aspeed_ast2600.c | 6 ++----
>   hw/arm/fsl-imx25.c      | 6 ++----
>   hw/arm/fsl-imx31.c      | 6 ++----
>   hw/arm/fsl-imx6.c       | 6 ++----
>   hw/arm/integratorcp.c   | 7 ++-----
>   hw/arm/nrf51_soc.c      | 7 ++-----
>   hw/ppc/rs6000_mc.c      | 7 ++-----
>   8 files changed, 16 insertions(+), 35 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 21/25] hw/arm: Simplify memory_region_init_rom() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_rom(mr, owner, arg3, arg4, &errp);
    if (
-       errp
+       !memory_region_init_rom(mr, owner, arg3, arg4, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/fsl-imx25.c | 13 ++++---------
 hw/arm/fsl-imx31.c | 13 ++++---------
 hw/arm/fsl-imx6.c  | 13 ++++---------
 3 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index b15435ccaf..9d2fb75a68 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -81,7 +81,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
 {
     FslIMX25State *s = FSL_IMX25(dev);
     uint8_t i;
-    Error *err = NULL;
 
     if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
         return;
@@ -281,18 +280,14 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
                                                        FSL_IMX25_WDT_IRQ));
 
     /* initialize 2 x 16 KB ROM */
-    memory_region_init_rom(&s->rom[0], OBJECT(dev), "imx25.rom0",
-                           FSL_IMX25_ROM0_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->rom[0], OBJECT(dev), "imx25.rom0",
+                                FSL_IMX25_ROM0_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX25_ROM0_ADDR,
                                 &s->rom[0]);
-    memory_region_init_rom(&s->rom[1], OBJECT(dev), "imx25.rom1",
-                           FSL_IMX25_ROM1_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->rom[1], OBJECT(dev), "imx25.rom1",
+                                FSL_IMX25_ROM1_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX25_ROM1_ADDR,
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 1d5dcd51e8..c0584e4dfc 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -63,7 +63,6 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
 {
     FslIMX31State *s = FSL_IMX31(dev);
     uint16_t i;
-    Error *err = NULL;
 
     if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
         return;
@@ -188,20 +187,16 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt), 0, FSL_IMX31_WDT_ADDR);
 
     /* On a real system, the first 16k is a `secure boot rom' */
-    memory_region_init_rom(&s->secure_rom, OBJECT(dev), "imx31.secure_rom",
-                           FSL_IMX31_SECURE_ROM_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->secure_rom, OBJECT(dev), "imx31.secure_rom",
+                                FSL_IMX31_SECURE_ROM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX31_SECURE_ROM_ADDR,
                                 &s->secure_rom);
 
     /* There is also a 16k ROM */
-    memory_region_init_rom(&s->rom, OBJECT(dev), "imx31.rom",
-                           FSL_IMX31_ROM_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->rom, OBJECT(dev), "imx31.rom",
+                                FSL_IMX31_ROM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX31_ROM_ADDR,
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 58f37e7c11..b2153022c0 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -109,7 +109,6 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
     MachineState *ms = MACHINE(qdev_get_machine());
     FslIMX6State *s = FSL_IMX6(dev);
     uint16_t i;
-    Error *err = NULL;
     unsigned int smp_cpus = ms->smp.cpus;
 
     if (smp_cpus > FSL_IMX6_NUM_CPUS) {
@@ -423,20 +422,16 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
     }
 
     /* ROM memory */
-    memory_region_init_rom(&s->rom, OBJECT(dev), "imx6.rom",
-                           FSL_IMX6_ROM_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->rom, OBJECT(dev), "imx6.rom",
+                                FSL_IMX6_ROM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_ROM_ADDR,
                                 &s->rom);
 
     /* CAAM memory */
-    memory_region_init_rom(&s->caam, OBJECT(dev), "imx6.caam",
-                           FSL_IMX6_CAAM_MEM_SIZE, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom(&s->caam, OBJECT(dev), "imx6.caam",
+                                FSL_IMX6_CAAM_MEM_SIZE, errp)) {
         return;
     }
     memory_region_add_subregion(get_system_memory(), FSL_IMX6_CAAM_MEM_ADDR,
-- 
2.41.0

Re: [PATCH-for-9.0 21/25] hw/arm: Simplify memory_region_init_rom() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Mechanical change using the following coccinelle script:
>
>@@
>expression mr, owner, arg3, arg4, errp;
>@@
>-   memory_region_init_rom(mr, owner, arg3, arg4, &errp);
>    if (
>-       errp
>+       !memory_region_init_rom(mr, owner, arg3, arg4, &errp)
>    ) {
>        ...
>        return;
>    }
>
>and removing the local Error variable.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---


Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 21/25] hw/arm: Simplify memory_region_init_rom() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_rom(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_rom(mr, owner, arg3, arg4, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/arm/fsl-imx25.c | 13 ++++---------
>   hw/arm/fsl-imx31.c | 13 ++++---------
>   hw/arm/fsl-imx6.c  | 13 ++++---------
>   3 files changed, 12 insertions(+), 27 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 22/25] hw/sparc: Simplify memory_region_init_ram_nomigrate() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp);
    if (
-       errp
+       !memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sparc/sun4m.c   | 20 ++++++--------------
 hw/sparc64/sun4u.c |  7 ++-----
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 17bf5f2879..fcf3782068 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -577,12 +577,9 @@ static void idreg_realize(DeviceState *ds, Error **errp)
 {
     IDRegState *s = MACIO_ID_REGISTER(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
-    Error *local_err = NULL;
 
-    memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.idreg",
-                                     sizeof(idreg_data), &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.idreg",
+                                          sizeof(idreg_data), errp)) {
         return;
     }
 
@@ -631,11 +628,9 @@ static void afx_realize(DeviceState *ds, Error **errp)
 {
     AFXState *s = TCX_AFX(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
-    Error *local_err = NULL;
 
-    memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.afx", 4,
-                                     &local_err);
-    if (local_err) {
+    if (!memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.afx",
+                                          4, errp)) {
         error_propagate(errp, local_err);
         return;
     }
@@ -715,12 +710,9 @@ static void prom_realize(DeviceState *ds, Error **errp)
 {
     PROMState *s = OPENPROM(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
-    Error *local_err = NULL;
 
-    memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4m.prom",
-                                     PROM_SIZE_MAX, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4m.prom",
+                                          PROM_SIZE_MAX, errp)) {
         return;
     }
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index c871170378..24d53bf5fd 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -454,12 +454,9 @@ static void prom_realize(DeviceState *ds, Error **errp)
 {
     PROMState *s = OPENPROM(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
-    Error *local_err = NULL;
 
-    memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4u.prom",
-                                     PROM_SIZE_MAX, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4u.prom",
+                                          PROM_SIZE_MAX, errp)) {
         return;
     }
 
-- 
2.41.0

Re: [PATCH-for-9.0 22/25] hw/sparc: Simplify memory_region_init_ram_nomigrate() calls

[Philippe Mathieu-Daudé]

On 20/11/23 22:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sparc/sun4m.c   | 20 ++++++--------------
>   hw/sparc64/sun4u.c |  7 ++-----
>   2 files changed, 8 insertions(+), 19 deletions(-)


> @@ -631,11 +628,9 @@ static void afx_realize(DeviceState *ds, Error **errp)
>   {
>       AFXState *s = TCX_AFX(ds);
>       SysBusDevice *dev = SYS_BUS_DEVICE(ds);
> -    Error *local_err = NULL;
>   
> -    memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.afx", 4,
> -                                     &local_err);
> -    if (local_err) {
> +    if (!memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.afx",
> +                                          4, errp)) {
>           error_propagate(errp, local_err);

I forgot to remove this error_propagate() line.

>           return;
>       }
> @@ -715,12 +710,9 @@ static void prom_realize(DeviceState *ds, Error **errp)
>   {
>       PROMState *s = OPENPROM(ds);
>       SysBusDevice *dev = SYS_BUS_DEVICE(ds);
> -    Error *local_err = NULL;
>   
> -    memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4m.prom",
> -                                     PROM_SIZE_MAX, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4m.prom",
> +                                          PROM_SIZE_MAX, errp)) {
>           return;
>       }

Re: [PATCH-for-9.0 22/25] hw/sparc: Simplify memory_region_init_ram_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> -   memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp);
>      if (
> -       errp
> +       !memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sparc/sun4m.c   | 20 ++++++--------------
>   hw/sparc64/sun4u.c |  7 ++-----
>   2 files changed, 8 insertions(+), 19 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 23/25] hw/misc: Simplify memory_region_init_ram_from_fd() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
@@
-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
    if (
-       errp
+       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/ivshmem.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 0447888029..9a8b9e2fd3 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -476,7 +476,6 @@ static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
 
 static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
 {
-    Error *local_err = NULL;
     struct stat buf;
     size_t size;
 
@@ -496,10 +495,9 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
     size = buf.st_size;
 
     /* mmap the region and map into the BAR2 */
-    memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s), "ivshmem.bar2",
-                                   size, RAM_SHARED, fd, 0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s),
+                                        "ivshmem.bar2", size, RAM_SHARED,
+                                        fd, 0, errp)) {
         return;
     }
 
-- 
2.41.0

Re: [PATCH-for-9.0 23/25] hw/misc: Simplify memory_region_init_ram_from_fd() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Mechanical change using the following coccinelle script:
>
>@@
>expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
>@@
>-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
>    if (
>-       errp
>+       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
>    ) {
>        ...
>        return;
>    }
>
>and removing the local Error variable.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---


Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 23/25] hw/misc: Simplify memory_region_init_ram_from_fd() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
> @@
> -   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp);
>      if (
> -       errp
> +       !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/misc/ivshmem.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>    

[PATCH-for-9.0 24/25] hw/nvram: Simplify memory_region_init_rom_device() calls

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, errp;
@@
-   memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp);
    if (
-       errp
+       !memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp)
    ) {
        ...
        return;
    }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/nvram/nrf51_nvm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
index 7f1db8c423..7b25becd49 100644
--- a/hw/nvram/nrf51_nvm.c
+++ b/hw/nvram/nrf51_nvm.c
@@ -336,12 +336,9 @@ static void nrf51_nvm_init(Object *obj)
 static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
 {
     NRF51NVMState *s = NRF51_NVM(dev);
-    Error *err = NULL;
 
-    memory_region_init_rom_device(&s->flash, OBJECT(dev), &flash_ops, s,
-        "nrf51_soc.flash", s->flash_size, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!memory_region_init_rom_device(&s->flash, OBJECT(dev), &flash_ops, s,
+                                       "nrf51_soc.flash", s->flash_size, errp)) {
         return;
     }
 
-- 
2.41.0

Re: [PATCH-for-9.0 24/25] hw/nvram: Simplify memory_region_init_rom_device() calls

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>Mechanical change using the following coccinelle script:
>
>@@
>expression mr, owner, arg3, arg4, arg5, arg6, errp;
>@@
>-   memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp);
>    if (
>-       errp
>+       !memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp)
>    ) {
>        ...
>        return;
>    }
>
>and removing the local Error variable.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/nvram/nrf51_nvm.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
>diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
>index 7f1db8c423..7b25becd49 100644
>--- a/hw/nvram/nrf51_nvm.c
>+++ b/hw/nvram/nrf51_nvm.c
>@@ -336,12 +336,9 @@ static void nrf51_nvm_init(Object *obj)
> static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
> {
>     NRF51NVMState *s = NRF51_NVM(dev);
>-    Error *err = NULL;
> 
>-    memory_region_init_rom_device(&s->flash, OBJECT(dev), &flash_ops, s,
>-        "nrf51_soc.flash", s->flash_size, &err);
>-    if (err) {
>-        error_propagate(errp, err);
>+    if (!memory_region_init_rom_device(&s->flash, OBJECT(dev), &flash_ops, s,
>+                                       "nrf51_soc.flash", s->flash_size, errp)) {
>         return;
>     }
> 
>-- 

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 24/25] hw/nvram: Simplify memory_region_init_rom_device() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
> 
> @@
> expression mr, owner, arg3, arg4, arg5, arg6, errp;
> @@
> -   memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp);
>      if (
> -       errp
> +       !memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, &errp)
>      ) {
>          ...
>          return;
>      }
> 
> and removing the local Error variable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/nvram/nrf51_nvm.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
Reviewed-by: Gavin Shan <gshan@redhat.com>

[PATCH-for-9.0 25/25] hw/pci-host/raven: Propagate error in raven_realize()

[Philippe Mathieu-Daudé]

When an Error** reference is available, it is better to
propagate local errors, rather then using generic ones,
which might terminate the whole QEMU process.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/raven.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 86c3a49087..e34ce47874 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -345,8 +345,10 @@ static void raven_realize(PCIDevice *d, Error **errp)
     d->config[PCI_LATENCY_TIMER] = 0x10;
     d->config[PCI_CAPABILITY_LIST] = 0x00;
 
-    memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios", BIOS_SIZE,
-                                     &error_fatal);
+    if (!memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios",
+                                          BIOS_SIZE, errp)) {
+        return;
+    }
     memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
                                 &s->bios);
     if (s->bios_name) {
-- 
2.41.0

Re: [PATCH-for-9.0 25/25] hw/pci-host/raven: Propagate error in raven_realize()

[Manos Pitsidianakis]

On Mon, 20 Nov 2023 23:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>When an Error** reference is available, it is better to
>propagate local errors, rather then using generic ones,
>which might terminate the whole QEMU process.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/pci-host/raven.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>index 86c3a49087..e34ce47874 100644
>--- a/hw/pci-host/raven.c
>+++ b/hw/pci-host/raven.c
>@@ -345,8 +345,10 @@ static void raven_realize(PCIDevice *d, Error **errp)
>     d->config[PCI_LATENCY_TIMER] = 0x10;
>     d->config[PCI_CAPABILITY_LIST] = 0x00;
> 
>-    memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios", BIOS_SIZE,
>-                                     &error_fatal);
>+    if (!memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios",
>+                                          BIOS_SIZE, errp)) {
>+        return;
>+    }
>     memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
>                                 &s->bios);
>     if (s->bios_name) {
>-- 
>2.41.0

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Re: [PATCH-for-9.0 25/25] hw/pci-host/raven: Propagate error in raven_realize()

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:
> When an Error** reference is available, it is better to
> propagate local errors, rather then using generic ones,
> which might terminate the whole QEMU process.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/pci-host/raven.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>