[PATCH for-5.0] xen: fixup RAM memory region initialization

[PATCH for-5.0] xen: fixup RAM memory region initialization

[Igor Mammedov]

Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
machine fails to start with:
   qemu-system-i386: xen: failed to populate ram at 0

The reason is that xen_ram_alloc() which is called by
memory_region_init_ram(), compares memory region with
statically allocated 'global' ram_memory memory region
that it uses for RAM, and does nothing in case it matches.

While it's possible feed machine->ram to xen_ram_alloc()
in the same manner to keep that hack working, I'd prefer
not to keep that circular dependency and try to untangle that.

However it doesn't look trivial to fix, so as temporary
fixup opt out Xen machine from memdev based RAM allocation,
and let xen_ram_alloc() do its trick for now.

Reported-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
   - compile tested only

 hw/i386/pc_piix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e6756216f9..6cb352363d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -953,6 +953,10 @@ static void xenfv_machine_options(MachineClass *m)
     m->desc = "Xen Fully-virtualized PC";
     m->max_cpus = HVM_MAX_VCPUS;
     m->default_machine_opts = "accel=xen";
+    /*
+     * opt out of system RAM being allocated by generic code
+     */
+    m->default_ram_id = NULL;
 }
 
 DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
-- 
2.18.1

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200327104828.12647-1-imammedo@redhat.com/



Hi,

This series failed build test on FreeBSD host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
if qemu-system-x86_64 --help >/dev/null 2>&1; then
  QEMU=qemu-system-x86_64
elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then
  QEMU=/usr/libexec/qemu-kvm
else
  exit 1
fi
make vm-build-freebsd J=21 QEMU=$QEMU
exit 0
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20200327104828.12647-1-imammedo@redhat.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Igor Mammedov]

Paolo,

could you take it along with Olaf's xenfv patch?


On Fri, 27 Mar 2020 06:48:28 -0400
Igor Mammedov <imammedo@redhat.com> wrote:

> Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
> machine fails to start with:
>    qemu-system-i386: xen: failed to populate ram at 0
> 
> The reason is that xen_ram_alloc() which is called by
> memory_region_init_ram(), compares memory region with
> statically allocated 'global' ram_memory memory region
> that it uses for RAM, and does nothing in case it matches.
> 
> While it's possible feed machine->ram to xen_ram_alloc()
> in the same manner to keep that hack working, I'd prefer
> not to keep that circular dependency and try to untangle that.
> 
> However it doesn't look trivial to fix, so as temporary
> fixup opt out Xen machine from memdev based RAM allocation,
> and let xen_ram_alloc() do its trick for now.
> 
> Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
>    - compile tested only
> 
>  hw/i386/pc_piix.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e6756216f9..6cb352363d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -953,6 +953,10 @@ static void xenfv_machine_options(MachineClass *m)
>      m->desc = "Xen Fully-virtualized PC";
>      m->max_cpus = HVM_MAX_VCPUS;
>      m->default_machine_opts = "accel=xen";
> +    /*
> +     * opt out of system RAM being allocated by generic code
> +     */
> +    m->default_ram_id = NULL;
>  }
>  
>  DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Paolo Bonzini]

On 27/03/20 17:36, Igor Mammedov wrote:
> Paolo,
> 
> could you take it along with Olaf's xenfv patch?

Yes, thanks.  Also your other patch.

Paolo

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Anthony PERARD]

On Fri, Mar 27, 2020 at 06:48:28AM -0400, Igor Mammedov wrote:
> Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
> machine fails to start with:
>    qemu-system-i386: xen: failed to populate ram at 0
> 
> The reason is that xen_ram_alloc() which is called by
> memory_region_init_ram(), compares memory region with
> statically allocated 'global' ram_memory memory region
> that it uses for RAM, and does nothing in case it matches.
> 
> While it's possible feed machine->ram to xen_ram_alloc()
> in the same manner to keep that hack working, I'd prefer
> not to keep that circular dependency and try to untangle that.
> 
> However it doesn't look trivial to fix, so as temporary
> fixup opt out Xen machine from memdev based RAM allocation,
> and let xen_ram_alloc() do its trick for now.
> 
> Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

That should work on most configs. But we also sometime use the "pc"
machine with accel=xen, to run without the "xen-platform" pci device,
but that would be less common.

Thanks,

-- 
Anthony PERARD

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Igor Mammedov]

On Mon, 30 Mar 2020 17:52:48 +0100
Anthony PERARD <anthony.perard@citrix.com> wrote:

> On Fri, Mar 27, 2020 at 06:48:28AM -0400, Igor Mammedov wrote:
> > Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
> > machine fails to start with:
> >    qemu-system-i386: xen: failed to populate ram at 0
> > 
> > The reason is that xen_ram_alloc() which is called by
> > memory_region_init_ram(), compares memory region with
> > statically allocated 'global' ram_memory memory region
> > that it uses for RAM, and does nothing in case it matches.
> > 
> > While it's possible feed machine->ram to xen_ram_alloc()
> > in the same manner to keep that hack working, I'd prefer
> > not to keep that circular dependency and try to untangle that.
> > 
> > However it doesn't look trivial to fix, so as temporary
> > fixup opt out Xen machine from memdev based RAM allocation,
> > and let xen_ram_alloc() do its trick for now.
> > 
> > Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> That should work on most configs. But we also sometime use the "pc"
> machine with accel=xen, to run without the "xen-platform" pci device,
> but that would be less common.

does following work for you in case of pc machine?

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 15650d7f6a..f19c0883ae 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -151,6 +151,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
 
 static int xen_init(MachineState *ms)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
         xen_pv_printf(NULL, 0, "can't open xen interface\n");
@@ -170,6 +172,10 @@ static int xen_init(MachineState *ms)
         return -1;
     }
     qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+    /*
+     * opt out of system RAM being allocated by generic code
+     */
+    m->default_ram_id = NULL;
     return 0;
 }


> Thanks,
> 

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Anthony PERARD]

On Thu, Apr 02, 2020 at 02:29:25PM +0200, Igor Mammedov wrote:
> On Mon, 30 Mar 2020 17:52:48 +0100
> Anthony PERARD <anthony.perard@citrix.com> wrote:
> 
> > On Fri, Mar 27, 2020 at 06:48:28AM -0400, Igor Mammedov wrote:
> > > Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
> > > machine fails to start with:
> > >    qemu-system-i386: xen: failed to populate ram at 0
> > > 
> > > The reason is that xen_ram_alloc() which is called by
> > > memory_region_init_ram(), compares memory region with
> > > statically allocated 'global' ram_memory memory region
> > > that it uses for RAM, and does nothing in case it matches.
> > > 
> > > While it's possible feed machine->ram to xen_ram_alloc()
> > > in the same manner to keep that hack working, I'd prefer
> > > not to keep that circular dependency and try to untangle that.
> > > 
> > > However it doesn't look trivial to fix, so as temporary
> > > fixup opt out Xen machine from memdev based RAM allocation,
> > > and let xen_ram_alloc() do its trick for now.
> > > 
> > > Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > That should work on most configs. But we also sometime use the "pc"
> > machine with accel=xen, to run without the "xen-platform" pci device,
> > but that would be less common.
> 
> does following work for you in case of pc machine?
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 15650d7f6a..f19c0883ae 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -151,6 +151,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
>  
>  static int xen_init(MachineState *ms)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
>      xen_xc = xc_interface_open(0, 0, 0);
>      if (xen_xc == NULL) {
>          xen_pv_printf(NULL, 0, "can't open xen interface\n");
> @@ -170,6 +172,10 @@ static int xen_init(MachineState *ms)
>          return -1;
>      }
>      qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> +    /*
> +     * opt out of system RAM being allocated by generic code
> +     */
> +    m->default_ram_id = NULL;
>      return 0;

After fixing the build issues, it does work, yes. I've tested both "xenfv"
and "pc,accel=xen".

Build issue:
- I've added #include "hw/boards.h"
- and s/m->/mc->/

Thanks!

-- 
Anthony PERARD

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Igor Mammedov]

On Thu, 2 Apr 2020 14:25:30 +0100
Anthony PERARD <anthony.perard@citrix.com> wrote:

> On Thu, Apr 02, 2020 at 02:29:25PM +0200, Igor Mammedov wrote:
> > On Mon, 30 Mar 2020 17:52:48 +0100
> > Anthony PERARD <anthony.perard@citrix.com> wrote:
> >   
> > > On Fri, Mar 27, 2020 at 06:48:28AM -0400, Igor Mammedov wrote:  
> > > > Since bd457782b3b0 ("x86/pc: use memdev for RAM") Xen
> > > > machine fails to start with:
> > > >    qemu-system-i386: xen: failed to populate ram at 0
> > > > 
> > > > The reason is that xen_ram_alloc() which is called by
> > > > memory_region_init_ram(), compares memory region with
> > > > statically allocated 'global' ram_memory memory region
> > > > that it uses for RAM, and does nothing in case it matches.
> > > > 
> > > > While it's possible feed machine->ram to xen_ram_alloc()
> > > > in the same manner to keep that hack working, I'd prefer
> > > > not to keep that circular dependency and try to untangle that.
> > > > 
> > > > However it doesn't look trivial to fix, so as temporary
> > > > fixup opt out Xen machine from memdev based RAM allocation,
> > > > and let xen_ram_alloc() do its trick for now.
> > > > 
> > > > Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > 
> > > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> > > 
> > > That should work on most configs. But we also sometime use the "pc"
> > > machine with accel=xen, to run without the "xen-platform" pci device,
> > > but that would be less common.  
> > 
> > does following work for you in case of pc machine?
> > 
> > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> > index 15650d7f6a..f19c0883ae 100644
> > --- a/hw/xen/xen-common.c
> > +++ b/hw/xen/xen-common.c
> > @@ -151,6 +151,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
> >  
> >  static int xen_init(MachineState *ms)
> >  {
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> >      xen_xc = xc_interface_open(0, 0, 0);
> >      if (xen_xc == NULL) {
> >          xen_pv_printf(NULL, 0, "can't open xen interface\n");
> > @@ -170,6 +172,10 @@ static int xen_init(MachineState *ms)
> >          return -1;
> >      }
> >      qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
> > +    /*
> > +     * opt out of system RAM being allocated by generic code
> > +     */
> > +    m->default_ram_id = NULL;
> >      return 0;  
> 
> After fixing the build issues, it does work, yes. I've tested both "xenfv"
> and "pc,accel=xen".

thanks, I'll post a formal patch

however it's all workarounds,
we need to fix ram allocation properly later on
so far I only have questions, hope you can help with clarifying them

1. why xen uses memory_region_init_ram() which does not allocate anything
   can it use plain memory_region_init() instead?
2. how main ram is allocated?
3. code has
           /*                                                                       
         * Xen does not allocate the memory continuously, it keeps a             
         * hole of the size computed above or passed in.                         
         */                                                                      
        block_len = (1ULL << 32) + x86ms->above_4g_mem_size; 
   which fixes up size ram memory region
   can we allocate 1 memory region of ram_size and then
   alias lower and upper memory instead of that?
4. how RAM migration works in case of xen?

> 
> Build issue:
> - I've added #include "hw/boards.h"
> - and s/m->/mc->/
ccccccgndiej> gngfiehlivuil

> Thanks!nbchihheikhjlnervve
> 

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Anthony PERARD]

On Thu, Apr 02, 2020 at 04:30:33PM +0200, Igor Mammedov wrote:
> 1. why xen uses memory_region_init_ram() which does not allocate anything

This seems to be due to history.

>    can it use plain memory_region_init() instead?

I can give it a try. And it doesn't work, I had to call qemu_ram_alloc() as
well, to set ram_memory.ram_block.

On the other and, I think memory_region_init_ram_nomigrate() would be
enough. QEMU didn't complain when I migrated the guest.

> 2. how main ram is allocated?

It's done by the toolstack, libxl. It creates a new domain in the
hypervisor, memory allocation is part of this, then QEMU is started, for
emulation of some devices.

There is one thing that QEMU does in regards to memory, it's the call
xc_domain_populate_physmap_exact() in xen_ram_alloc(). This is for when
an emulated PCI device needs some memory, like for the VGA region.

> 3. code has
>            /*                                                                       
>          * Xen does not allocate the memory continuously, it keeps a             
>          * hole of the size computed above or passed in.                         
>          */                                                                      
>         block_len = (1ULL << 32) + x86ms->above_4g_mem_size; 
>    which fixes up size ram memory region
>    can we allocate 1 memory region of ram_size and then
>    alias lower and upper memory instead of that?

I don't know, I don't think I know enough about how memory_region are
used to be able to answer that.

> 4. how RAM migration works in case of xen?

From QEMU, we only migrate devices states, we call the
"xen-save-devices-state" QMP command. Memory migration is done by the
toolstack. In QEMU, there is a bodge in xen_ram_alloc() to avoid having
QEMU doing some "allocation" during migration.

I hope that help.
Cheers,

-- 
Anthony PERARD

Re: [PATCH for-5.0] xen: fixup RAM memory region initialization

[Igor Mammedov]

On Tue, 7 Apr 2020 12:36:34 +0100
Anthony PERARD <anthony.perard@citrix.com> wrote:

> On Thu, Apr 02, 2020 at 04:30:33PM +0200, Igor Mammedov wrote:
> > 1. why xen uses memory_region_init_ram() which does not allocate anything  
> 
> This seems to be due to history.
> 
> >    can it use plain memory_region_init() instead?  
> 
> I can give it a try. And it doesn't work, I had to call qemu_ram_alloc() as
> well, to set ram_memory.c.
> 
> On the other and, I think memory_region_init_ram_nomigrate() would be
> enough. QEMU didn't complain when I migrated the guest.

Why does it need ramblock fir main ram if it does not allocate nor migrate
it using QEMU infrastructure?

> 
> > 2. how main ram is allocated?  
> 
> It's done by the toolstack, libxl. It creates a new domain in the
> hypervisor, memory allocation is part of this, then QEMU is started, for
> emulation of some devices.
> 
> There is one thing that QEMU does in regards to memory, it's the call
> xc_domain_populate_physmap_exact() in xen_ram_alloc(). This is for when
> an emulated PCI device needs some memory, like for the VGA region.
> 
> > 3. code has
> >            /*                                                                       
> >          * Xen does not allocate the memory continuously, it keeps a             
> >          * hole of the size computed above or passed in.                         
> >          */                                                                      
> >         block_len = (1ULL << 32) + x86ms->above_4g_mem_size; 
> >    which fixes up size ram memory region
> >    can we allocate 1 memory region of ram_size and then
> >    alias lower and upper memory instead of that?  
> 
> I don't know, I don't think I know enough about how memory_region are
> used to be able to answer that.
> 
> > 4. how RAM migration works in case of xen?  
> 
> From QEMU, we only migrate devices states, we call the
> "xen-save-devices-state" QMP command. Memory migration is done by the
> toolstack. In QEMU, there is a bodge in xen_ram_alloc() to avoid having
> QEMU doing some "allocation" during migration.
> 
> I hope that help.
> Cheers,
>