| Message ID | 1499438577-7674-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Make memory_region_init_ram() and friends handle migration | expand |
On 07/07/2017 16:42, Peter Maydell wrote: > This patchset changes the memory region functions > - memory_region_init_ram() > - memory_region_init_rom() > - memory_region_init_rom_device() > to all automatically register the backing memory they allocate > for migration using vmstate_register_ram(). Renamed functions > - memory_region_init_ram_nomigrate() > - memory_region_init_rom_nomigrate() > - memory_region_init_rom_device_nomigrate() > are provided which only do the MR init, for the oddball > cases which want to manage migration of the backing memory > themselves (and to avoid behavioural changes for callers > which weren't managing correctly migration themselves...) > > The idea is based on discussion from a previous patchset: > https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00764.html > > The series includes a largish coccinelle-scripted patch and a > few by-hand conversions which change callsites which previously > manually created the region and registered its backing ram in > two separate steps to use the new functions. > > This series does not include any patches to fix bugs like: > * caller forgot to call vmstate_register_ram() so region > is not actually migrated (eg hw/arm/highbank.c) > * caller used vmstate_register_ram_global() even though it is > a device, so you can't create 2 copies of the device (eg sm501) > because I wanted to stick to strictly no-behaviour-change > for this patch -- we can fix the bugs separately (fixes will > tend to imply migration compat breaks so can only be done > for some boards.) Most of the remaining callers of the > _nomigrate functions are buggy, I think (and a demonstration > that our current API does not score well on the "hard to > get wrong by accident" scale). Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, Paolo > > thanks > -- PMM > > Peter Maydell (11): > include/hw/boards.h: Document memory_region_allocate_system_memory() > memory: Document that the RAM MR initializers do not handle migration > memory: Rename memory_region_init_ram() to > memory_region_init_ram_nomigrate() > memory: Rename memory_region_init_rom() and _rom_device() to > _nomigrate() > memory.h: Add memory_region_init_{ram,rom,rom_device}() handling > migration > scripts/coccinelle/memory-region-init-ram.cocci: New script > hw: Use new memory_region_init_{ram,rom,rom_device}() functions > hw/block/pflash_cfi01, pflash_cfi02: Use > memory_region_init_rom_device() > hw/pci/pci.c: Use memory_region_init_rom() > hw/display/qxl.c Use memory_region_init_ram() > docs/devel/memory.txt: Add section about RAM migration > > docs/devel/memory.txt | 31 +++++ > include/exec/memory.h | 161 ++++++++++++++++++++---- > include/hw/boards.h | 29 +++++ > backends/hostmem-ram.c | 2 +- > hw/arm/aspeed.c | 2 +- > hw/arm/aspeed_soc.c | 2 +- > hw/arm/exynos4210.c | 2 - > hw/arm/exynos4_boards.c | 2 - > hw/arm/fsl-imx25.c | 5 +- > hw/arm/fsl-imx31.c | 5 +- > hw/arm/fsl-imx6.c | 5 +- > hw/arm/highbank.c | 2 +- > hw/arm/integratorcp.c | 2 +- > hw/arm/mainstone.c | 1 - > hw/arm/musicpal.c | 1 - > hw/arm/omap1.c | 1 - > hw/arm/omap2.c | 1 - > hw/arm/omap_sx1.c | 6 +- > hw/arm/palm.c | 1 - > hw/arm/pxa2xx.c | 4 - > hw/arm/realview.c | 3 - > hw/arm/spitz.c | 1 - > hw/arm/stellaris.c | 2 - > hw/arm/stm32f205_soc.c | 3 - > hw/arm/tosa.c | 1 - > hw/arm/vexpress.c | 3 - > hw/arm/virt.c | 4 +- > hw/arm/xilinx_zynq.c | 1 - > hw/arm/xlnx-zynqmp.c | 1 - > hw/block/onenand.c | 2 +- > hw/block/pflash_cfi01.c | 1 - > hw/block/pflash_cfi02.c | 1 - > hw/cris/axis_dev88.c | 5 +- > hw/display/cg3.c | 3 +- > hw/display/qxl.c | 3 - > hw/display/sm501.c | 2 +- > hw/display/tc6393xb.c | 1 - > hw/display/tcx.c | 4 +- > hw/display/vga.c | 2 +- > hw/display/vmware_vga.c | 1 - > hw/i386/pc.c | 1 - > hw/i386/pc_sysfw.c | 2 - > hw/i386/pci-assign-load-rom.c | 2 +- > hw/i386/xen/xen-hvm.c | 1 - > hw/input/milkymist-softusb.c | 4 +- > hw/m68k/an5206.c | 1 - > hw/m68k/mcf5208.c | 1 - > hw/microblaze/petalogix_ml605_mmu.c | 2 - > hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 - > hw/mips/boston.c | 2 +- > hw/mips/mips_fulong2e.c | 1 - > hw/mips/mips_jazz.c | 2 - > hw/mips/mips_malta.c | 2 +- > hw/mips/mips_mipssim.c | 1 - > hw/mips/mips_r4k.c | 1 - > hw/moxie/moxiesim.c | 4 +- > hw/net/dp8393x.c | 2 +- > hw/net/milkymist-minimac2.c | 2 +- > hw/nios2/10m50_devboard.c | 8 +- > hw/openrisc/openrisc_sim.c | 1 - > hw/pci-host/prep.c | 2 +- > hw/pci-host/xilinx-pcie.c | 2 +- > hw/pci/pci.c | 1 - > hw/ppc/mac_newworld.c | 1 - > hw/ppc/mac_oldworld.c | 1 - > hw/ppc/ppc405_boards.c | 3 - > hw/ppc/ppc405_uc.c | 1 - > hw/s390x/sclp.c | 1 - > hw/sh4/r2d.c | 1 - > hw/sh4/shix.c | 3 - > hw/sparc/leon3.c | 1 - > hw/sparc/sun4m.c | 6 +- > hw/sparc64/sun4u.c | 4 +- > hw/tricore/tricore_testboard.c | 26 ++-- > hw/unicore32/puv3.c | 1 - > hw/xtensa/sim.c | 4 +- > hw/xtensa/xtfpga.c | 4 +- > memory.c | 110 +++++++++++++--- > numa.c | 4 +- > scripts/coccinelle/memory-region-init-ram.cocci | 38 ++++++ > 80 files changed, 385 insertions(+), 180 deletions(-) > create mode 100644 scripts/coccinelle/memory-region-init-ram.cocci >
On 10 July 2017 at 11:05, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/07/2017 16:42, Peter Maydell wrote: >> This patchset changes the memory region functions >> - memory_region_init_ram() >> - memory_region_init_rom() >> - memory_region_init_rom_device() >> to all automatically register the backing memory they allocate >> for migration using vmstate_register_ram(). Renamed functions >> - memory_region_init_ram_nomigrate() >> - memory_region_init_rom_nomigrate() >> - memory_region_init_rom_device_nomigrate() >> are provided which only do the MR init, for the oddball >> cases which want to manage migration of the backing memory >> themselves (and to avoid behavioural changes for callers >> which weren't managing correctly migration themselves...) >> >> The idea is based on discussion from a previous patchset: >> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00764.html >> >> The series includes a largish coccinelle-scripted patch and a >> few by-hand conversions which change callsites which previously >> manually created the region and registered its backing ram in >> two separate steps to use the new functions. >> >> This series does not include any patches to fix bugs like: >> * caller forgot to call vmstate_register_ram() so region >> is not actually migrated (eg hw/arm/highbank.c) >> * caller used vmstate_register_ram_global() even though it is >> a device, so you can't create 2 copies of the device (eg sm501) >> because I wanted to stick to strictly no-behaviour-change >> for this patch -- we can fix the bugs separately (fixes will >> tend to imply migration compat breaks so can only be done >> for some boards.) Most of the remaining callers of the >> _nomigrate functions are buggy, I think (and a demonstration >> that our current API does not score well on the "hard to >> get wrong by accident" scale). > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks. I'm going to apply this directly to master (so then I can check that nothing has got applied to it since which uses the old semantics for these functions; nothing in fact has.) thanks -- PMM
On 14 July 2017 at 18:01, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 July 2017 at 11:05, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 07/07/2017 16:42, Peter Maydell wrote: >>> This patchset changes the memory region functions >>> - memory_region_init_ram() >>> - memory_region_init_rom() >>> - memory_region_init_rom_device() >>> to all automatically register the backing memory they allocate >>> for migration using vmstate_register_ram(). Renamed functions >>> - memory_region_init_ram_nomigrate() >>> - memory_region_init_rom_nomigrate() >>> - memory_region_init_rom_device_nomigrate() >>> are provided which only do the MR init, for the oddball >>> cases which want to manage migration of the backing memory >>> themselves (and to avoid behavioural changes for callers >>> which weren't managing correctly migration themselves...) >>> >>> The idea is based on discussion from a previous patchset: >>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00764.html >>> >>> The series includes a largish coccinelle-scripted patch and a >>> few by-hand conversions which change callsites which previously >>> manually created the region and registered its backing ram in >>> two separate steps to use the new functions. >>> >>> This series does not include any patches to fix bugs like: >>> * caller forgot to call vmstate_register_ram() so region >>> is not actually migrated (eg hw/arm/highbank.c) >>> * caller used vmstate_register_ram_global() even though it is >>> a device, so you can't create 2 copies of the device (eg sm501) >>> because I wanted to stick to strictly no-behaviour-change >>> for this patch -- we can fix the bugs separately (fixes will >>> tend to imply migration compat breaks so can only be done >>> for some boards.) Most of the remaining callers of the >>> _nomigrate functions are buggy, I think (and a demonstration >>> that our current API does not score well on the "hard to >>> get wrong by accident" scale). >> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Thanks. I'm going to apply this directly to master (so then > I can check that nothing has got applied to it since which > uses the old semantics for these functions; nothing in > fact has.) ...now applied (with the minor comment fixes requested). thanks -- PMM
Hi Peter, On 07/07/2017 11:42 AM, Peter Maydell wrote: > This patchset changes the memory region functions > - memory_region_init_ram() > - memory_region_init_rom() > - memory_region_init_rom_device() > to all automatically register the backing memory they allocate > for migration using vmstate_register_ram(). Renamed functions > - memory_region_init_ram_nomigrate() > - memory_region_init_rom_nomigrate() > - memory_region_init_rom_device_nomigrate() > are provided which only do the MR init, for the oddball > cases which want to manage migration of the backing memory > themselves (and to avoid behavioural changes for callers > which weren't managing correctly migration themselves...) [...] I'm seeing memleaks using the malta machine, they come from the smbus_eeprom_init() in hw/i2c/smbus_eeprom.c which does: uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */ Question for 2.11: is this device a candidate to use one of the function you enumerated? My guess is memory_region_init_rom_device() Question for 2.10: does that mean than the malta machine is not migratable? Thanks! Phil.
On 22 July 2017 at 05:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > I'm seeing memleaks using the malta machine, they come from the > smbus_eeprom_init() in hw/i2c/smbus_eeprom.c which does: > > uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent > */ (This isn't actually a leak because the eeprom device can't be hotplugged/unplugged, so once created it will live for the life of the simulation.) > Question for 2.11: is this device a candidate to use one of the function you > enumerated? My guess is memory_region_init_rom_device() A quick look at that device suggests that it's doing some pretty weird stuff which probably should be being cleaned up to use a memory region to handle the backing store for the EEPROM (although there are other ways to do it; since there's only 2K of data there it could also be handled by a byte-array in the device migrated via VMState). Do you know what the intention is of the code which passes the device an offset into the eeprom_buf buffer rather than just passing the start of the buffer ? > Question for 2.10: does that mean than the malta machine is not migratable? Depends whether the guest is actually writing to the EEPROM :-) The smbus_eeprom device doesn't have any support for migration at all -- it is neither migrating the data, nor the internal state (the offset). But if the guest doesn't ever write to the EEPROM and it doesn't happen to be relying on the offset having a particular value at the point of migration, you won't notice. In particular, the pc_piix and pc_q35 machines both create one of these devices, so this is an issue for the pc machine migration too... (and any fix we make to smbus_eeprom needs to handle migration compat as a result) thanks -- PMM
On 07/23/2017 04:58 PM, Peter Maydell wrote: > On 22 July 2017 at 05:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> I'm seeing memleaks using the malta machine, they come from the >> smbus_eeprom_init() in hw/i2c/smbus_eeprom.c which does: >> >> uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent >> */ > > (This isn't actually a leak because the eeprom device > can't be hotplugged/unplugged, so once created it will > live for the life of the simulation.) > >> Question for 2.11: is this device a candidate to use one of the function you >> enumerated? My guess is memory_region_init_rom_device() > > A quick look at that device suggests that it's doing some > pretty weird stuff which probably should be being > cleaned up to use a memory region to handle the backing > store for the EEPROM (although there are other ways to do it; > since there's only 2K of data there it could also be > handled by a byte-array in the device migrated via > VMState). > > Do you know what the intention is of the code which > passes the device an offset into the eeprom_buf buffer > rather than just passing the start of the buffer ? > >> Question for 2.10: does that mean than the malta machine is not migratable? > > Depends whether the guest is actually writing to the EEPROM :-) > The smbus_eeprom device doesn't have any support for migration > at all -- it is neither migrating the data, nor the internal > state (the offset). But if the guest doesn't ever write to > the EEPROM and it doesn't happen to be relying on the offset > having a particular value at the point of migration, you > won't notice. > > In particular, the pc_piix and pc_q35 machines both create > one of these devices, so this is an issue for the pc > machine migration too... (and any fix we make to > smbus_eeprom needs to handle migration compat as a result) Thank for your clear explanation, it seems quite some work to fix this device (due to the migration part I'm not common yet) so I'll just add this in my TODO list for 2.13 and will probably come back to you about that in a far future. Regards, Phil.
This patchset changes the memory region functions - memory_region_init_ram() - memory_region_init_rom() - memory_region_init_rom_device() to all automatically register the backing memory they allocate for migration using vmstate_register_ram(). Renamed functions - memory_region_init_ram_nomigrate() - memory_region_init_rom_nomigrate() - memory_region_init_rom_device_nomigrate() are provided which only do the MR init, for the oddball cases which want to manage migration of the backing memory themselves (and to avoid behavioural changes for callers which weren't managing correctly migration themselves...) The idea is based on discussion from a previous patchset: https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00764.html The series includes a largish coccinelle-scripted patch and a few by-hand conversions which change callsites which previously manually created the region and registered its backing ram in two separate steps to use the new functions. This series does not include any patches to fix bugs like: * caller forgot to call vmstate_register_ram() so region is not actually migrated (eg hw/arm/highbank.c) * caller used vmstate_register_ram_global() even though it is a device, so you can't create 2 copies of the device (eg sm501) because I wanted to stick to strictly no-behaviour-change for this patch -- we can fix the bugs separately (fixes will tend to imply migration compat breaks so can only be done for some boards.) Most of the remaining callers of the _nomigrate functions are buggy, I think (and a demonstration that our current API does not score well on the "hard to get wrong by accident" scale). thanks -- PMM Peter Maydell (11): include/hw/boards.h: Document memory_region_allocate_system_memory() memory: Document that the RAM MR initializers do not handle migration memory: Rename memory_region_init_ram() to memory_region_init_ram_nomigrate() memory: Rename memory_region_init_rom() and _rom_device() to _nomigrate() memory.h: Add memory_region_init_{ram,rom,rom_device}() handling migration scripts/coccinelle/memory-region-init-ram.cocci: New script hw: Use new memory_region_init_{ram,rom,rom_device}() functions hw/block/pflash_cfi01, pflash_cfi02: Use memory_region_init_rom_device() hw/pci/pci.c: Use memory_region_init_rom() hw/display/qxl.c Use memory_region_init_ram() docs/devel/memory.txt: Add section about RAM migration docs/devel/memory.txt | 31 +++++ include/exec/memory.h | 161 ++++++++++++++++++++---- include/hw/boards.h | 29 +++++ backends/hostmem-ram.c | 2 +- hw/arm/aspeed.c | 2 +- hw/arm/aspeed_soc.c | 2 +- hw/arm/exynos4210.c | 2 - hw/arm/exynos4_boards.c | 2 - hw/arm/fsl-imx25.c | 5 +- hw/arm/fsl-imx31.c | 5 +- hw/arm/fsl-imx6.c | 5 +- hw/arm/highbank.c | 2 +- hw/arm/integratorcp.c | 2 +- hw/arm/mainstone.c | 1 - hw/arm/musicpal.c | 1 - hw/arm/omap1.c | 1 - hw/arm/omap2.c | 1 - hw/arm/omap_sx1.c | 6 +- hw/arm/palm.c | 1 - hw/arm/pxa2xx.c | 4 - hw/arm/realview.c | 3 - hw/arm/spitz.c | 1 - hw/arm/stellaris.c | 2 - hw/arm/stm32f205_soc.c | 3 - hw/arm/tosa.c | 1 - hw/arm/vexpress.c | 3 - hw/arm/virt.c | 4 +- hw/arm/xilinx_zynq.c | 1 - hw/arm/xlnx-zynqmp.c | 1 - hw/block/onenand.c | 2 +- hw/block/pflash_cfi01.c | 1 - hw/block/pflash_cfi02.c | 1 - hw/cris/axis_dev88.c | 5 +- hw/display/cg3.c | 3 +- hw/display/qxl.c | 3 - hw/display/sm501.c | 2 +- hw/display/tc6393xb.c | 1 - hw/display/tcx.c | 4 +- hw/display/vga.c | 2 +- hw/display/vmware_vga.c | 1 - hw/i386/pc.c | 1 - hw/i386/pc_sysfw.c | 2 - hw/i386/pci-assign-load-rom.c | 2 +- hw/i386/xen/xen-hvm.c | 1 - hw/input/milkymist-softusb.c | 4 +- hw/m68k/an5206.c | 1 - hw/m68k/mcf5208.c | 1 - hw/microblaze/petalogix_ml605_mmu.c | 2 - hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 - hw/mips/boston.c | 2 +- hw/mips/mips_fulong2e.c | 1 - hw/mips/mips_jazz.c | 2 - hw/mips/mips_malta.c | 2 +- hw/mips/mips_mipssim.c | 1 - hw/mips/mips_r4k.c | 1 - hw/moxie/moxiesim.c | 4 +- hw/net/dp8393x.c | 2 +- hw/net/milkymist-minimac2.c | 2 +- hw/nios2/10m50_devboard.c | 8 +- hw/openrisc/openrisc_sim.c | 1 - hw/pci-host/prep.c | 2 +- hw/pci-host/xilinx-pcie.c | 2 +- hw/pci/pci.c | 1 - hw/ppc/mac_newworld.c | 1 - hw/ppc/mac_oldworld.c | 1 - hw/ppc/ppc405_boards.c | 3 - hw/ppc/ppc405_uc.c | 1 - hw/s390x/sclp.c | 1 - hw/sh4/r2d.c | 1 - hw/sh4/shix.c | 3 - hw/sparc/leon3.c | 1 - hw/sparc/sun4m.c | 6 +- hw/sparc64/sun4u.c | 4 +- hw/tricore/tricore_testboard.c | 26 ++-- hw/unicore32/puv3.c | 1 - hw/xtensa/sim.c | 4 +- hw/xtensa/xtfpga.c | 4 +- memory.c | 110 +++++++++++++--- numa.c | 4 +- scripts/coccinelle/memory-region-init-ram.cocci | 38 ++++++ 80 files changed, 385 insertions(+), 180 deletions(-) create mode 100644 scripts/coccinelle/memory-region-init-ram.cocci -- 2.7.4