| Message ID | 20200327104828.12647-1-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-5.0] xen: fixup RAM memory region initialization | expand |
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
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,
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
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,
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, >
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!
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 >
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,
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, >
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,
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(+)