[PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()

Igor Mammedov posted 3 patches 4 years, 6 months ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191008113318.7012-1-imammedo@redhat.com
Maintainers: Artyom Tarasenko <atar4qemu@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Richard Henderson <rth@twiddle.net>, "Hervé Poussineau" <hpoussin@reactos.org>, Helge Deller <deller@gmx.de>, David Gibson <david@gibson.dropbear.id.au>
hw/hppa/machine.c    |  5 ++---
hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
hw/sparc64/niagara.c | 25 +++++++++++++------------
3 files changed, 25 insertions(+), 20 deletions(-)
[PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Igor Mammedov 4 years, 6 months ago
Series cleans up remaining boards that call memory_region_allocate_system_memory()
multiple times, violating interface contract (the function should be called only
once).

With that cleaned up, it should be possible to switch from adhoc RAM allocation
in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
memory-backend based allocation, remaining roadblock for doing it is deprecated
-mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
merge window. So remaining patches to consolidate system RAM allocation around
memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
then.


Igor Mammedov (3):
  sparc64: use memory_region_allocate_system_memory() only for '-m'
    specified RAM
  ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
  hppa: drop usage of memory_region_allocate_system_memory() for ROM

 hw/hppa/machine.c    |  5 ++---
 hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
 hw/sparc64/niagara.c | 25 +++++++++++++------------
 3 files changed, 25 insertions(+), 20 deletions(-)

-- 
2.18.1


Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Igor Mammedov 4 years, 6 months ago
On Tue,  8 Oct 2019 07:33:15 -0400
Igor Mammedov <imammedo@redhat.com> wrote:

> 
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.

Eduardo,

This patches are fixing various machines across tree, so series does not belong
to any particular arch specific tree, can you merge it via generic machine tree?


> 
> 
> Igor Mammedov (3):
>   sparc64: use memory_region_allocate_system_memory() only for '-m'
>     specified RAM
>   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
>   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> 
>  hw/hppa/machine.c    |  5 ++---
>  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
>  hw/sparc64/niagara.c | 25 +++++++++++++------------
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 


Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Igor Mammedov 4 years, 6 months ago
On Thu, 10 Oct 2019 19:35:03 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

Forgot to actually CC Eduardo,

> On Tue,  8 Oct 2019 07:33:15 -0400
> Igor Mammedov <imammedo@redhat.com> wrote:
...
> Eduardo,
> 
> This patches are fixing various machines across tree, so series does not belong
> to any particular arch specific tree, can you merge it via generic machine tree?


> > 
> > 
> > Igor Mammedov (3):
> >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> >     specified RAM
> >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > 
> >  hw/hppa/machine.c    |  5 ++---
> >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> >  3 files changed, 25 insertions(+), 20 deletions(-)
> >   
> 
> 


Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago
Ping?

On Fri, Oct 11, 2019 at 5:23 PM Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 10 Oct 2019 19:35:03 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> Forgot to actually CC Eduardo,
>
> > On Tue,  8 Oct 2019 07:33:15 -0400
> > Igor Mammedov <imammedo@redhat.com> wrote:
> ...
> > Eduardo,
> >
> > This patches are fixing various machines across tree, so series does not belong
> > to any particular arch specific tree, can you merge it via generic machine tree?
>
>
> > >
> > >
> > > Igor Mammedov (3):
> > >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> > >     specified RAM
> > >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> > >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > >
> > >  hw/hppa/machine.c    |  5 ++---
> > >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> > >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> > >  3 files changed, 25 insertions(+), 20 deletions(-)
> > >

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Eduardo Habkost 4 years, 5 months ago
On Sun, Oct 20, 2019 at 04:38:06PM +0200, Philippe Mathieu-Daudé wrote:
> Ping?

Sorry, missed this.  queued on machine-next.  Pull request will
be submitted today or tomorrow.

> 
> On Fri, Oct 11, 2019 at 5:23 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > On Thu, 10 Oct 2019 19:35:03 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Forgot to actually CC Eduardo,
> >
> > > On Tue,  8 Oct 2019 07:33:15 -0400
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > ...
> > > Eduardo,
> > >
> > > This patches are fixing various machines across tree, so series does not belong
> > > to any particular arch specific tree, can you merge it via generic machine tree?
> >
> >
> > > >
> > > >
> > > > Igor Mammedov (3):
> > > >   sparc64: use memory_region_allocate_system_memory() only for '-m'
> > > >     specified RAM
> > > >   ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> > > >   hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > > >
> > > >  hw/hppa/machine.c    |  5 ++---
> > > >  hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> > > >  hw/sparc64/niagara.c | 25 +++++++++++++------------
> > > >  3 files changed, 25 insertions(+), 20 deletions(-)
> > > >

-- 
Eduardo


Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Philippe Mathieu-Daudé 4 years, 5 months ago
Hi Igor,

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.
> 
> 
> Igor Mammedov (3):
>    sparc64: use memory_region_allocate_system_memory() only for '-m'
>      specified RAM
>    ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
>    hppa: drop usage of memory_region_allocate_system_memory() for ROM
> 
>   hw/hppa/machine.c    |  5 ++---
>   hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
>   hw/sparc64/niagara.c | 25 +++++++++++++------------

What about the TYPE_SUN4M_MEMORY device in hw/sparc/sun4m.c?

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Igor Mammedov 4 years, 5 months ago
On Mon, 21 Oct 2019 10:59:56 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor,
> 
> On 10/8/19 1:33 PM, Igor Mammedov wrote:
> > Series cleans up remaining boards that call memory_region_allocate_system_memory()
> > multiple times, violating interface contract (the function should be called only
> > once).
> > 
> > With that cleaned up, it should be possible to switch from adhoc RAM allocation
> > in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> > memory-backend based allocation, remaining roadblock for doing it is deprecated
> > -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> > merge window. So remaining patches to consolidate system RAM allocation around
> > memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> > then.
> > 
> > 
> > Igor Mammedov (3):
> >    sparc64: use memory_region_allocate_system_memory() only for '-m'
> >      specified RAM
> >    ppc: rs6000_mc: drop usage of memory_region_allocate_system_memory()
> >    hppa: drop usage of memory_region_allocate_system_memory() for ROM
> > 
> >   hw/hppa/machine.c    |  5 ++---
> >   hw/ppc/rs6000_mc.c   | 15 ++++++++++-----
> >   hw/sparc64/niagara.c | 25 +++++++++++++------------  
> 
> What about the TYPE_SUN4M_MEMORY device in hw/sparc/sun4m.c?

it has only one call site so it's not issue this series targets.
I'll take care of it in follow up series, which will deal with
converting memory_region_allocate_system_memory() to memdev only
(probably I'll drop TYPE_SUN4M_MEMORY altogether)


> 


Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago
Hi Igor,

On 10/8/19 1:33 PM, Igor Mammedov wrote:
> Series cleans up remaining boards that call memory_region_allocate_system_memory()
> multiple times, violating interface contract (the function should be called only
> once).
> 
> With that cleaned up, it should be possible to switch from adhoc RAM allocation
> in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> memory-backend based allocation, remaining roadblock for doing it is deprecated
> -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> merge window. So remaining patches to consolidate system RAM allocation around
> memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> then.

How do we protect the codebase for new boards to not make the same mistake?

What about some code like this snippet (or nicer, but since this is a 
developer error, and assert is enough IMO):

-- >8 --

diff --git a/hw/core/numa.c b/hw/core/numa.c
index 4dfec5c95b..a487677672 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -484,6 +484,11 @@ static void 
allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
                                             const char *name,
                                             uint64_t ram_size)
  {
+    static bool nonnuma_system_memory_allocated;
+
+    g_assert(!nonnuma_system_memory_allocated);
+    nonnuma_system_memory_allocated = true;
+
      if (mem_path) {
  #ifdef __linux__
          Error *err = NULL;
---

$ hppa-softmmu/qemu-system-hppa
**
ERROR:/home/phil/source/qemu/hw/core/numa.c:489:allocate_system_memory_nonnuma: 
assertion failed: (!nonnuma_system_memory_allocated)
Aborted (core dumped)

Re: [PATCH 0/3] eliminate remaining places that abuse memory_region_allocate_system_memory()
Posted by Igor Mammedov 4 years, 6 months ago
On Tue, 8 Oct 2019 14:41:25 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Hi Igor,
> 
> On 10/8/19 1:33 PM, Igor Mammedov wrote:
> > Series cleans up remaining boards that call memory_region_allocate_system_memory()
> > multiple times, violating interface contract (the function should be called only
> > once).
> > 
> > With that cleaned up, it should be possible to switch from adhoc RAM allocation
> > in memory_region_allocate_system_memory()->allocate_system_memory_nonnuma() to
> > memory-backend based allocation, remaining roadblock for doing it is deprecated
> > -mem-path fallback to RAM allocation, which is scheduled for removal at 4.3
> > merge window. So remaining patches to consolidate system RAM allocation around
> > memory-backends and aliasing -mem-path/mem-prealloc to it are postponed till
> > then.  
> 
> How do we protect the codebase for new boards to not make the same mistake?
> 
> What about some code like this snippet (or nicer, but since this is a 
> developer error, and assert is enough IMO):
probably it's not worth effort (it's not too long till 4.2 softfreeze).

Like cover letter say, I'm planing to finish refactoring of
memory_region_allocate_system_memory() and I hope this function
will be gone during 4.3 and most boards will only need to map
pre-created (by common code) memory-backend wherever they used to
map RAM memory region.

> -- >8 --  
> 
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 4dfec5c95b..a487677672 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -484,6 +484,11 @@ static void 
> allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                              const char *name,
>                                              uint64_t ram_size)
>   {
> +    static bool nonnuma_system_memory_allocated;
> +
> +    g_assert(!nonnuma_system_memory_allocated);
> +    nonnuma_system_memory_allocated = true;
> +
>       if (mem_path) {
>   #ifdef __linux__
>           Error *err = NULL;
> ---
> 
> $ hppa-softmmu/qemu-system-hppa
> **
> ERROR:/home/phil/source/qemu/hw/core/numa.c:489:allocate_system_memory_nonnuma: 
> assertion failed: (!nonnuma_system_memory_allocated)
> Aborted (core dumped)