[PATCH 0/3] system/memory: Trivial fixes

[PATCH 0/3] system/memory: Trivial fixes

[Philippe Mathieu-Daudé]

- Include missing "exec/memory.h"
- Set machine as parent of io/mem regions

Philippe Mathieu-Daudé (3):
  cpu-target: Include missing 'exec/memory.h' header
  monitor/target: Include missing 'exec/memory.h' header
  system/physmem: Assign global system I/O Memory to machine

 cpu-target.c              | 1 +
 monitor/hmp-cmds-target.c | 1 +
 system/physmem.c          | 7 ++++---
 3 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.41.0

[PATCH 1/3] cpu-target: Include missing 'exec/memory.h' header

[Philippe Mathieu-Daudé]

Include "exec/memory.h" in order to avoid:

  cpu-target.c:201:50: error: use of undeclared identifier 'TYPE_MEMORY_REGION'
      DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
                                                   ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 cpu-target.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cpu-target.c b/cpu-target.c
index 958d63e882..86444cc2c6 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -31,6 +31,7 @@
 #else
 #include "hw/core/sysemu-cpu-ops.h"
 #include "exec/address-spaces.h"
+#include "exec/memory.h"
 #endif
 #include "sysemu/cpus.h"
 #include "sysemu/tcg.h"
-- 
2.41.0

Re: [PATCH 1/3] cpu-target: Include missing 'exec/memory.h' header

[Peter Maydell]

On Fri, 9 Feb 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Include "exec/memory.h" in order to avoid:
>
>   cpu-target.c:201:50: error: use of undeclared identifier 'TYPE_MEMORY_REGION'
>       DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
>                                                    ^

Given that we don't actually see this error, presumably
we're implicitly dragging it in via some other include?
Anyway, better to be explicit than implicit, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/3] cpu-target: Include missing 'exec/memory.h' header

[Philippe Mathieu-Daudé]

On 9/2/24 17:01, Peter Maydell wrote:
> On Fri, 9 Feb 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Include "exec/memory.h" in order to avoid:
>>
>>    cpu-target.c:201:50: error: use of undeclared identifier 'TYPE_MEMORY_REGION'
>>        DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
>>                                                     ^
> 
> Given that we don't actually see this error, presumably
> we're implicitly dragging it in via some other include?

It is pulled in by the exec/cpu-all.h header which I'm trying to
sanitize (along with others). I'll add a note about this.

> Anyway, better to be explicit than implicit, so
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!

[PATCH 2/3] monitor/target: Include missing 'exec/memory.h' header

[Philippe Mathieu-Daudé]

Include "exec/memory.h" in order to avoid:

  monitor/hmp-cmds-target.c:263:10: error: call to undeclared function 'memory_region_is_ram';
  ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
           ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 monitor/hmp-cmds-target.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index d9fbcac08d..9338ae8440 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "disas/disas.h"
 #include "exec/address-spaces.h"
+#include "exec/memory.h"
 #include "monitor/hmp-target.h"
 #include "monitor/monitor-internal.h"
 #include "qapi/error.h"
-- 
2.41.0

Re: [PATCH 2/3] monitor/target: Include missing 'exec/memory.h' header

[Peter Maydell]

On Fri, 9 Feb 2024 at 15:02, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Include "exec/memory.h" in order to avoid:
>
>   monitor/hmp-cmds-target.c:263:10: error: call to undeclared function 'memory_region_is_ram';
>   ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>       if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
>            ^
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  monitor/hmp-cmds-target.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
> index d9fbcac08d..9338ae8440 100644
> --- a/monitor/hmp-cmds-target.c
> +++ b/monitor/hmp-cmds-target.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "disas/disas.h"
>  #include "exec/address-spaces.h"
> +#include "exec/memory.h"
>  #include "monitor/hmp-target.h"
>  #include "monitor/monitor-internal.h"
>  #include "qapi/error.h"
> --
> 2.41.0

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/3] system/physmem: Assign global system I/O Memory to machine

[Philippe Mathieu-Daudé]

So far there is only one system I/O and one system
memory per machine.

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

diff --git a/system/physmem.c b/system/physmem.c
index 5e66d9ae36..50947a374e 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2554,12 +2554,13 @@ static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
 
-    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
+    memory_region_init(system_memory, OBJECT(current_machine),
+                       "system", UINT64_MAX);
     address_space_init(&address_space_memory, system_memory, "memory");
 
     system_io = g_malloc(sizeof(*system_io));
-    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
-                          65536);
+    memory_region_init_io(system_io, OBJECT(current_machine),
+                          &unassigned_io_ops, NULL, "io", 65535);
     address_space_init(&address_space_io, system_io, "I/O");
 }
 
-- 
2.41.0

Re: [PATCH 3/3] system/physmem: Assign global system I/O Memory to machine

[Peter Maydell]

On Fri, 9 Feb 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> So far there is only one system I/O and one system
> memory per machine.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  system/physmem.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 5e66d9ae36..50947a374e 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2554,12 +2554,13 @@ static void memory_map_init(void)
>  {
>      system_memory = g_malloc(sizeof(*system_memory));
>
> -    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
> +    memory_region_init(system_memory, OBJECT(current_machine),
> +                       "system", UINT64_MAX);
>      address_space_init(&address_space_memory, system_memory, "memory");
>
>      system_io = g_malloc(sizeof(*system_io));
> -    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
> -                          65536);
> +    memory_region_init_io(system_io, OBJECT(current_machine),
> +                          &unassigned_io_ops, NULL, "io", 65535);
>      address_space_init(&address_space_io, system_io, "I/O");
>  }

What's the intention in doing this? What does it change?

It seems to be OK to pass a non-Device owner in for
memory_region_init() (whereas it is *not* OK to do that
for memory_region_init_ram()), but this seems to be
getting a bit tricky.

thanks
-- PMM

Re: [PATCH 3/3] system/physmem: Assign global system I/O Memory to machine

[Philippe Mathieu-Daudé]

(+Markus I forgot to Cc)

On 9/2/24 17:06, Peter Maydell wrote:
> On Fri, 9 Feb 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> So far there is only one system I/O and one system
>> memory per machine.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   system/physmem.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/system/physmem.c b/system/physmem.c
>> index 5e66d9ae36..50947a374e 100644
>> --- a/system/physmem.c
>> +++ b/system/physmem.c
>> @@ -2554,12 +2554,13 @@ static void memory_map_init(void)
>>   {
>>       system_memory = g_malloc(sizeof(*system_memory));
>>
>> -    memory_region_init(system_memory, NULL, "system", UINT64_MAX);
>> +    memory_region_init(system_memory, OBJECT(current_machine),
>> +                       "system", UINT64_MAX);
>>       address_space_init(&address_space_memory, system_memory, "memory");
>>
>>       system_io = g_malloc(sizeof(*system_io));
>> -    memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>> -                          65536);
>> +    memory_region_init_io(system_io, OBJECT(current_machine),
>> +                          &unassigned_io_ops, NULL, "io", 65535);
>>       address_space_init(&address_space_io, system_io, "I/O");
>>   }
> 
> What's the intention in doing this? What does it change?

We want to remove access to pre-QOM and possibly hotplug QOM paths
from external API (CLI & QMP so far).

When the parent object is obvious and missing we simply have to
explicit it.

> It seems to be OK to pass a non-Device owner in for
> memory_region_init() (whereas it is *not* OK to do that
> for memory_region_init_ram()), but this seems to be
> getting a bit tricky.

Yes, memory_region_init_ram() is problematic; I'm hardly trying
to ignore it at this point.

Re: [PATCH 0/3] system/memory: Trivial fixes

[Michael Tokarev]

09.02.2024 18:00, Philippe Mathieu-Daudé :
> - Include missing "exec/memory.h"
> - Set machine as parent of io/mem regions
> 
> Philippe Mathieu-Daudé (3):
>    cpu-target: Include missing 'exec/memory.h' header
>    monitor/target: Include missing 'exec/memory.h' header
>    system/physmem: Assign global system I/O Memory to machine

Picked up patches 1 & 2, but not 3 yet.

/mjt

>   cpu-target.c              | 1 +
>   monitor/hmp-cmds-target.c | 1 +
>   system/physmem.c          | 7 ++++---
>   3 files changed, 6 insertions(+), 3 deletions(-)
>