Message ID | 20220919231720.163121-1-shentey@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Deprecate sysbus_get_default() and get_system_memory() et. al | expand |
On 20/9/22 01:17, Bernhard Beschow wrote: > In the next steps, these singletons will be resolved by turning them > into attributes of the system bus. The system bus is already accessible > via the global current_machine variable which will be made use of later > in the wrapper functions. > > All changes have been performed with search-and-replace: > * s/&address_space_memory/get_address_space_memory()/ > * s/&address_space_io/get_address_space_io()/ > The only exceptions were exec/address-spaces.h and softmmu/physmem.c > which have been manually changed. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > accel/hvf/hvf-accel-ops.c | 2 +- > accel/kvm/kvm-all.c | 12 ++++++------ > hw/alpha/dp264.c | 4 ++-- > hw/alpha/typhoon.c | 4 ++-- > hw/arm/smmu-common.c | 4 ++-- > hw/arm/smmuv3.c | 14 +++++++------- > hw/arm/virt.c | 2 +- > hw/char/goldfish_tty.c | 4 ++-- > hw/core/loader.c | 2 +- > hw/dma/pl330.c | 2 +- > hw/dma/rc4030.c | 2 +- > hw/dma/xlnx-zynq-devcfg.c | 4 ++-- > hw/dma/xlnx_dpdma.c | 8 ++++---- > hw/hppa/machine.c | 4 ++-- > hw/hyperv/hyperv.c | 2 +- > hw/hyperv/vmbus.c | 2 +- > hw/i386/amd_iommu.c | 18 +++++++++--------- > hw/i386/fw_cfg.c | 2 +- > hw/i386/intel_iommu.c | 24 ++++++++++++------------ > hw/i386/microvm.c | 4 ++-- > hw/i386/pc.c | 2 +- > hw/i386/xen/xen-hvm.c | 4 ++-- > hw/ide/ahci.c | 2 +- > hw/ide/macio.c | 10 +++++----- > hw/intc/apic.c | 2 +- > hw/intc/openpic_kvm.c | 2 +- > hw/intc/pnv_xive.c | 6 +++--- > hw/intc/pnv_xive2.c | 6 +++--- > hw/intc/riscv_aplic.c | 2 +- > hw/intc/spapr_xive.c | 2 +- > hw/intc/xive.c | 4 ++-- > hw/intc/xive2.c | 4 ++-- > hw/mips/jazz.c | 4 ++-- > hw/misc/lasi.c | 2 +- > hw/misc/macio/mac_dbdma.c | 8 ++++---- > hw/net/ftgmac100.c | 16 ++++++++-------- > hw/net/i82596.c | 24 ++++++++++++------------ > hw/net/imx_fec.c | 22 +++++++++++----------- > hw/net/lasi_i82596.c | 2 +- > hw/net/npcm7xx_emc.c | 14 +++++++------- > hw/openrisc/boot.c | 2 +- > hw/pci-host/dino.c | 6 +++--- > hw/pci-host/pnv_phb3.c | 6 +++--- > hw/pci-host/pnv_phb3_msi.c | 6 +++--- > hw/pci-host/pnv_phb4.c | 10 +++++----- > hw/pci/pci.c | 2 +- > hw/ppc/pnv_psi.c | 2 +- > hw/ppc/spapr.c | 4 ++-- > hw/ppc/spapr_events.c | 2 +- > hw/ppc/spapr_hcall.c | 4 ++-- > hw/ppc/spapr_iommu.c | 4 ++-- > hw/ppc/spapr_ovec.c | 8 ++++---- > hw/ppc/spapr_rtas.c | 2 +- > hw/remote/iommu.c | 2 +- > hw/remote/message.c | 4 ++-- > hw/remote/proxy-memory-listener.c | 2 +- > hw/riscv/boot.c | 6 +++--- > hw/riscv/sifive_e.c | 2 +- > hw/riscv/sifive_u.c | 2 +- > hw/riscv/virt.c | 2 +- > hw/s390x/css.c | 16 ++++++++-------- > hw/s390x/ipl.h | 2 +- > hw/s390x/s390-pci-bus.c | 4 ++-- > hw/s390x/s390-pci-inst.c | 10 +++++----- > hw/s390x/s390-skeys.c | 2 +- > hw/s390x/virtio-ccw.c | 10 +++++----- > hw/sd/sdhci.c | 2 +- > hw/sh4/r2d.c | 4 ++-- > hw/sparc/sun4m.c | 2 +- > hw/sparc/sun4m_iommu.c | 4 ++-- > hw/sparc64/sun4u_iommu.c | 4 ++-- > hw/timer/hpet.c | 2 +- > hw/usb/hcd-ehci-pci.c | 2 +- > hw/usb/hcd-ehci-sysbus.c | 2 +- > hw/usb/hcd-ohci.c | 2 +- > hw/usb/hcd-xhci-sysbus.c | 2 +- > hw/vfio/ap.c | 2 +- > hw/vfio/ccw.c | 2 +- > hw/vfio/common.c | 8 ++++---- > hw/vfio/platform.c | 2 +- > hw/virtio/vhost-vdpa.c | 2 +- > hw/virtio/vhost.c | 2 +- > hw/virtio/virtio-bus.c | 4 ++-- > hw/virtio/virtio-iommu.c | 6 +++--- > hw/virtio/virtio-pci.c | 2 +- > hw/xen/xen_pt.c | 4 ++-- > include/exec/address-spaces.h | 4 ++-- > include/hw/elf_ops.h | 4 ++-- > include/hw/ppc/spapr.h | 5 +++-- > include/hw/ppc/vof.h | 4 ++-- > monitor/misc.c | 4 ++-- > softmmu/ioport.c | 12 ++++++------ > softmmu/memory_mapping.c | 2 +- > softmmu/physmem.c | 17 ++++++++++++++--- > target/arm/hvf/hvf.c | 4 ++-- > target/arm/kvm.c | 4 ++-- > target/avr/helper.c | 8 ++++---- > target/i386/hax/hax-all.c | 2 +- > target/i386/hax/hax-mem.c | 2 +- > target/i386/hvf/hvf.c | 2 +- > target/i386/hvf/vmx.h | 2 +- > target/i386/hvf/x86_mmu.c | 6 +++--- > target/i386/nvmm/nvmm-all.c | 4 ++-- > target/i386/sev.c | 4 ++-- > target/i386/tcg/sysemu/misc_helper.c | 12 ++++++------ > target/i386/whpx/whpx-all.c | 4 ++-- > target/s390x/diag.c | 2 +- > target/s390x/mmu_helper.c | 2 +- > target/s390x/sigp.c | 2 +- > target/xtensa/dbg_helper.c | 2 +- > tests/qtest/fuzz/generic_fuzz.c | 4 ++-- > 111 files changed, 285 insertions(+), 273 deletions(-) Please consider using scripts/git.orderfile for tree-wide refactors, it helps reviewers. > diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h > index db8bfa9a92..d5c8cbd718 100644 > --- a/include/exec/address-spaces.h > +++ b/include/exec/address-spaces.h > @@ -33,8 +33,8 @@ MemoryRegion *get_system_memory(void); > */ > MemoryRegion *get_system_io(void); > > -extern AddressSpace address_space_memory; > -extern AddressSpace address_space_io; > +AddressSpace *get_address_space_memory(void); > +AddressSpace *get_address_space_io(void); > > #endif > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 56e03e07b5..0ac920d446 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -89,8 +89,8 @@ RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) }; > static MemoryRegion *system_memory; > static MemoryRegion *system_io; > > -AddressSpace address_space_io; > -AddressSpace address_space_memory; > +static AddressSpace address_space_io; > +static AddressSpace address_space_memory; > > static MemoryRegion io_mem_unassigned; > > @@ -2690,6 +2690,16 @@ MemoryRegion *get_system_io(void) > return system_io; > } > > +AddressSpace *get_address_space_memory(void) > +{ > + return &address_space_memory; > +} > + > +AddressSpace *get_address_space_io(void) > +{ > + return &address_space_io; > +} Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote: > > In address-spaces.h it can be read that get_system_memory() and > get_system_io() are temporary interfaces which "should only be used temporarily > until a proper bus interface is available". This statement certainly extends to > the address_space_memory and address_space_io singletons. This is a long standing "we never really completed a cleanup"... > This series attempts > to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an > object-oriented, "proper bus interface" inspired by PCIBus. > > While at it, also the main_system_bus singleton is turned into an attribute of > MachineState. Together, this resolves five singletons in total, making the > ownership relations much more obvious which helps comprehension. ...but I don't think this is the direction we want to go. Overall the reason that the "system memory" and "system IO" singletons are weird is that in theory they should not be necessary at all -- board code should create devices and map them into an entirely arbitrary MemoryRegion or set of MemoryRegions corresponding to address space(s) for the CPU and for DMA-capable devices. But we keep them around because (a) there is a ton of legacy code that assumes there's only one address space in the system and this is it (b) when modelling the kind of board where there really is only one address space, having the 'system memory' global makes the APIs for creating and connecting devices a lot simpler Retaining the whole-system singleton but shoving it into MachineState doesn't really change much, IMHO. More generally, sysbus is rather weird because it isn't really a bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're all in the same address space or that in real hardware they'd all be on the same bus. sysbus has essentially degraded into a hack for having devices get reset. I really really need to make some time to have another look at reset handling. If we get that right then I think it's probably possible to collapse the few things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down into TYPE_DEVICE and get rid of sysbus altogether... thanks -- PMM
On 20/09/2022 10:55, Peter Maydell wrote: > On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote: >> >> In address-spaces.h it can be read that get_system_memory() and >> get_system_io() are temporary interfaces which "should only be used temporarily >> until a proper bus interface is available". This statement certainly extends to >> the address_space_memory and address_space_io singletons. > > This is a long standing "we never really completed a cleanup"... > >> This series attempts >> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an >> object-oriented, "proper bus interface" inspired by PCIBus. >> >> While at it, also the main_system_bus singleton is turned into an attribute of >> MachineState. Together, this resolves five singletons in total, making the >> ownership relations much more obvious which helps comprehension. > > ...but I don't think this is the direction we want to go. > Overall the reason that the "system memory" and "system IO" > singletons are weird is that in theory they should not be necessary > at all -- board code should create devices and map them into an > entirely arbitrary MemoryRegion or set of MemoryRegions corresponding > to address space(s) for the CPU and for DMA-capable devices. But we > keep them around because > (a) there is a ton of legacy code that assumes there's only one > address space in the system and this is it > (b) when modelling the kind of board where there really is only > one address space, having the 'system memory' global makes > the APIs for creating and connecting devices a lot simpler > > Retaining the whole-system singleton but shoving it into MachineState > doesn't really change much, IMHO. > > More generally, sysbus is rather weird because it isn't really a > bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" > the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're > all in the same address space or that in real hardware they'd > all be on the same bus. sysbus has essentially degraded into a > hack for having devices get reset. I really really need to make > some time to have another look at reset handling. If we get that > right then I think it's probably possible to collapse the few > things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down > into TYPE_DEVICE and get rid of sysbus altogether... Following on from one of the discussion points from Alex's KVM Forum BoF session: I think longer term what we need to aim for is for QEMU machines to define their own address spaces, and then bind those address spaces containing memory-mapped devices to one or more CPUs. Once this in place, as Peter notes above it just remains to solve the reset problem and then it becomes possible to eliminate sysbus altogether as everything else can already be managed by qdev/QOM. ATB, Mark.
Am 20. September 2022 09:55:37 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: >On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote: >> >> In address-spaces.h it can be read that get_system_memory() and >> get_system_io() are temporary interfaces which "should only be used temporarily >> until a proper bus interface is available". This statement certainly extends to >> the address_space_memory and address_space_io singletons. > >This is a long standing "we never really completed a cleanup"... > >> This series attempts >> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an >> object-oriented, "proper bus interface" inspired by PCIBus. >> >> While at it, also the main_system_bus singleton is turned into an attribute of >> MachineState. Together, this resolves five singletons in total, making the >> ownership relations much more obvious which helps comprehension. > >...but I don't think this is the direction we want to go. >Overall the reason that the "system memory" and "system IO" >singletons are weird is that in theory they should not be necessary >at all -- board code should create devices and map them into an >entirely arbitrary MemoryRegion or set of MemoryRegions corresponding >to address space(s) for the CPU and for DMA-capable devices. My intention was to allow exactly that: By turning sytem memory and system IO into non-singletons, one could have many of them, thus allowing boards to create arbitrary mappings of memory and IO. Since QEMU currently assumes one set (memory and IO) of addresses, I for now instantiated the SysBus once in the machine class to preserve behavior. >But we >keep them around because > (a) there is a ton of legacy code that assumes there's only one > address space in the system and this is it > (b) when modelling the kind of board where there really is only > one address space, having the 'system memory' global makes > the APIs for creating and connecting devices a lot simpler Indeed, the APIs may look simpler. The issue I see here though is that devices may make assumptions about these globals which makes the code hard to change in the long run. If devices are given their dependencies by the framework, they must make less assumptions, putting the framework into control. This makes the code more homogenious and therefore easier to change. >Retaining the whole-system singleton but shoving it into MachineState >doesn't really change much, IMHO. > >More generally, sysbus is rather weird because it isn't really a >bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" >the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're >all in the same address space or that in real hardware they'd >all be on the same bus. Again, having multiple SysBuses may solve that issue. >sysbus has essentially degraded into a >hack for having devices get reset. I really really need to make >some time to have another look at reset handling. If we get that >right then I think it's probably possible to collapse the few >things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down >into TYPE_DEVICE and get rid of sysbus altogether... There are many SysBusDevices which directly access the globals I intended to deprecate. If those devices could be changed to use the SysBus equivalents instead, this would put the boards in control of memory mappings. Best regards, Bernhard > >thanks >-- PMM
Am 20. September 2022 15:36:26 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 20/09/2022 10:55, Peter Maydell wrote: > >> On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote: >>> >>> In address-spaces.h it can be read that get_system_memory() and >>> get_system_io() are temporary interfaces which "should only be used temporarily >>> until a proper bus interface is available". This statement certainly extends to >>> the address_space_memory and address_space_io singletons. >> >> This is a long standing "we never really completed a cleanup"... >> >>> This series attempts >>> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an >>> object-oriented, "proper bus interface" inspired by PCIBus. >>> >>> While at it, also the main_system_bus singleton is turned into an attribute of >>> MachineState. Together, this resolves five singletons in total, making the >>> ownership relations much more obvious which helps comprehension. >> >> ...but I don't think this is the direction we want to go. >> Overall the reason that the "system memory" and "system IO" >> singletons are weird is that in theory they should not be necessary >> at all -- board code should create devices and map them into an >> entirely arbitrary MemoryRegion or set of MemoryRegions corresponding >> to address space(s) for the CPU and for DMA-capable devices. But we >> keep them around because >> (a) there is a ton of legacy code that assumes there's only one >> address space in the system and this is it >> (b) when modelling the kind of board where there really is only >> one address space, having the 'system memory' global makes >> the APIs for creating and connecting devices a lot simpler >> >> Retaining the whole-system singleton but shoving it into MachineState >> doesn't really change much, IMHO. >> >> More generally, sysbus is rather weird because it isn't really a >> bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" >> the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're >> all in the same address space or that in real hardware they'd >> all be on the same bus. sysbus has essentially degraded into a >> hack for having devices get reset. I really really need to make >> some time to have another look at reset handling. If we get that >> right then I think it's probably possible to collapse the few >> things TYPE_SYS_BUS_DEVICE does that TYPE_DEVICE does not down >> into TYPE_DEVICE and get rid of sysbus altogether... > >Following on from one of the discussion points from Alex's KVM Forum BoF session: I think longer term what we need to aim for is for QEMU machines to define their own address spaces, and then bind those address spaces containing memory-mapped devices to one or more CPUs. Isn't that more or less impossible with singletons? > >Once this in place, as Peter notes above it just remains to solve the reset problem and then it becomes possible to eliminate sysbus altogether as everything else can already be managed by qdev/QOM. Also see my reply to Peter. Thanks, Bernhard > > >ATB, > >Mark.
On Tue, 20 Sept 2022 at 23:50, Bernhard Beschow <shentey@gmail.com> wrote: > > Am 20. September 2022 09:55:37 UTC schrieb Peter Maydell <peter.maydell@linaro.org>: > >On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow <shentey@gmail.com> wrote: > >> > >> In address-spaces.h it can be read that get_system_memory() and > >> get_system_io() are temporary interfaces which "should only be used temporarily > >> until a proper bus interface is available". This statement certainly extends to > >> the address_space_memory and address_space_io singletons. > > > >This is a long standing "we never really completed a cleanup"... > > > >> This series attempts > >> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into an > >> object-oriented, "proper bus interface" inspired by PCIBus. > >> > >> While at it, also the main_system_bus singleton is turned into an attribute of > >> MachineState. Together, this resolves five singletons in total, making the > >> ownership relations much more obvious which helps comprehension. > > > >...but I don't think this is the direction we want to go. > >Overall the reason that the "system memory" and "system IO" > >singletons are weird is that in theory they should not be necessary > >at all -- board code should create devices and map them into an > >entirely arbitrary MemoryRegion or set of MemoryRegions corresponding > >to address space(s) for the CPU and for DMA-capable devices. > > My intention was to allow exactly that: By turning sytem memory and system IO into non-singletons, one could have many of them, thus allowing boards to create arbitrary mappings of memory and IO. Since QEMU currently assumes one set (memory and IO) of addresses, I for now instantiated the SysBus once in the machine class to preserve behavior. You can already create arbitrary mappings of memory and IO (look at the virt board for an example). The existence of the legacy singleton system-memory and system-io doesn't prevent that, and stuffing the singletons into the MachineState doesn't do anything to change the code that is relying on the singletons. > >Retaining the whole-system singleton but shoving it into MachineState > >doesn't really change much, IMHO. > > > >More generally, sysbus is rather weird because it isn't really a > >bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" > >the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're > >all in the same address space or that in real hardware they'd > >all be on the same bus. > > Again, having multiple SysBuses may solve that issue. We definitely don't want multiple sysbuses. thanks -- PMM