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>
---
v2:
  * make it work with -M pc,accel=xen
---
 hw/xen/xen-common.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 15650d7f6a..a15070f7f6 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -19,6 +19,7 @@
 #include "sysemu/runstate.h"
 #include "migration/misc.h"
 #include "migration/global_state.h"
+#include "hw/boards.h"
 
 //#define DEBUG_XEN
 
@@ -151,6 +152,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 +173,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
+     */
+    mc->default_ram_id = NULL;
     return 0;
 }
 
-- 
2.18.1

On 02/04/20 16:54, 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>
> ---
> v2:
>   * make it work with -M pc,accel=xen
> ---
>  hw/xen/xen-common.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 15650d7f6a..a15070f7f6 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -19,6 +19,7 @@
>  #include "sysemu/runstate.h"
>  #include "migration/misc.h"
>  #include "migration/global_state.h"
> +#include "hw/boards.h"
>  
>  //#define DEBUG_XEN
>  
> @@ -151,6 +152,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 +173,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
> +     */
> +    mc->default_ram_id = NULL;
>      return 0;
>  }
>  
> 

Monkey patching the class is slightly disgusting. :)  It would be better
to use a new ms->default_ram_id, and assign it from the class in
instance_init, but I've queued it anyway.

Paolo

On Thu, 2 Apr 2020 20:57:14 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 02/04/20 16:54, 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>
> > ---
> > v2:
> >   * make it work with -M pc,accel=xen
> > ---
> >  hw/xen/xen-common.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> > index 15650d7f6a..a15070f7f6 100644
> > --- a/hw/xen/xen-common.c
> > +++ b/hw/xen/xen-common.c
> > @@ -19,6 +19,7 @@
> >  #include "sysemu/runstate.h"
> >  #include "migration/misc.h"
> >  #include "migration/global_state.h"
> > +#include "hw/boards.h"
> >  
> >  //#define DEBUG_XEN
> >  
> > @@ -151,6 +152,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 +173,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
> > +     */
> > +    mc->default_ram_id = NULL;
> >      return 0;
> >  }
> >  
> >   
> 
> Monkey patching the class is slightly disgusting. :)  It would be better
> to use a new ms->default_ram_id, and assign it from the class in
> instance_init, but I've queued it anyway.

it's disgusting, I hope to drop this after fixing xen properly.

> 
> Paolo
> 
>