On 19/02/2024 12:00, BALATON Zoltan wrote:
On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
On 19/2/24 12:27, BALATON Zoltan wrote:
On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
On 16/2/24 20:54, Philippe Mathieu-Daudé wrote:
On 16/2/24 18:14, BALATON Zoltan wrote:
On Fri, 16 Feb 2024, Philippe Mathieu-Daudé wrote:
We want to set another qdev property (a link) for the pl110
and pl111 devices, we can not use sysbus_create_simple() which
only passes sysbus base address and IRQs as arguments. Inline
it so we can set the link property in the next commit.
Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
---
hw/arm/realview.c | 5 ++++-
hw/arm/versatilepb.c | 6 +++++-
hw/arm/vexpress.c | 10 ++++++++--
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 9058f5b414..77300e92e5 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -238,7 +238,10 @@ static void realview_init(MachineState *machine,
sysbus_create_simple("pl061", 0x10014000, pic[7]);
gpio2 = sysbus_create_simple("pl061", 0x10015000, pic[8]);
- sysbus_create_simple("pl111", 0x10020000, pic[23]);
+ dev = qdev_new("pl111");
+ sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+ sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x10020000);
+ sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[23]);
Not directly related to this patch but this blows up 1 line into 4 just to
allow setting a property. Maybe just to keep some simplicity we'd rather need
either a sysbus_realize_simple function that takes a sysbus device instead of
the name and does not create the device itself or some way to pass properties
to sysbus create simple (but the latter may not be easy to do in a generic way
so not sure about that). What do you think?
Unfortunately sysbus doesn't scale in heterogeneous setup.
Regarding the HW modelling API complexity you are pointing at, we'd
like to move from the current imperative programming paradigm to a
declarative one, likely DSL driven. Meanwhile it is being investigated
(as part of "Dynamic Machine"), I'm trying to get the HW APIs right
I'm aware of that activity but we're currently still using board code to construct
machines and probably will continue to do so for a while. Also because likely not
all current machines will be converted to new declarative way so having a
convenient API for that is still useful.
(As for the language to describe the devices of a machine and their connections
declaratively the device tree does just that but dts is not a very user friendly
descrtiption language so I haven't brought that up as a possibility. But you may
still could get some clues by looking at the problems it had to solve to at least
get a requirements for the machine description language.)
for heterogeneous emulation. Current price to pay is a verbose
imperative QDev API, hoping we'll get later a trivial declarative one
(like this single sysbus_create_simple call), where we shouldn't worry
about the order of low level calls, whether to use link or not, etc.
Having a detailed low level API does not prevent a more convenient for current use
higher level API on top so keeping that around for current machines would allow
you to chnage the low level API without having to change all the board codes
because you's only need to update the simple high level API.
So what is your suggestion here, add a new complex helper to keep
a one-line style?
DeviceState *sysbus_create_simple_dma_link(const char *typename,
hwaddr baseaddr,
const char *linkname,
Object *linkobj,
qemu_irq irq);
I think just having sysbus_realize_simple that does the same as sysbus_create_simple
minus creating the device would be enough because then the cases where you need to
set properties could still use it after qdev_new or init and property_set but hide
the realize and connecting the device behind this single call.
I can't say I'm a fan of sysbus_create_simple() because its use of varargs to
populate qdev properties is based upon the assumptions that the properties defined
with device_class_set_props() are stored in a list. I can see there could be
potential in future to store properties in other structures such as a hash, and
keeping this API would prevent this change. FWIW my personal preference would be to
remove this API completely.
I wonder why this is that important since you never modified
any of the files changed by this series:
For new people trying to contribute to QEMU QDev is overwhelming so having some way
to need less of it to do simple things would help them to get started.
It depends what how you define "simple": for QEMU developers most people search for
similar examples in the codebase and copy/paste them. I'd much rather have a slightly
longer, but consistent API for setting properties rather than coming up with many
special case wrappers that need to be maintained just to keep the line count down for
"simplicity".
I think that Phil's approach here is the best one for now, particularly given that it
allows us to take another step towards heterogeneous machines. As the work in this
area matures it might be that we can consider other approaches, but that's not a
decision that can be made right now and so shouldn't be a reason to block this change.
ATB,
Mark.
