Re: [PATCH 1/6] hw/arm: Inline sysbus_create_simple(PL110 / PL111)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
On 19/2/24 13: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.

So you suggest splitting sysbus_create_simple() as
sysbus_create_simple() + sysbus_realize_simple(), so we can set
properties between the 2 calls? IOW extract qdev_new() from
sysbus_create_varargs() and rename it as sysbus_realize_simple()?

No I suggest to keep sysbus_create_simple as it is for cases that don't need to set properties and use it now add a sysbus_realize_simple that takes a device instead of the type name and does not create the device just does the rest of realizing and connecting it. (After that sysbus_create_simple would also call sysbus_realize_simple internally to avoid code duplication but that's not something the users of this API ahve to care about.) Then cases where sysbus_create_simple cannot be used (because you need to set properties or the device is allocated statically so it needs init instead of new) can use sysbus_realize_simple to still keep the code somewhat simple so it would be:

- dev = sysbus_create_simple(typename, addr, irq);
+ dev = qdev_new(typename);
+ // optionally set properties
+ sysbus_realize_simple(dev, addr, irq);

Where you need properties, but keep sysbus_create_simple where it's already used, no need to change those places.

- dev = sysbus_create_varargs(typename, addr, irqA, irqB, ...);
+ dev = qdev_new(typename);
+ // optionally set properties
+ sysbus_realize_varargs(dev, addr, irqA, irqB, ...);

I'm not sure it is worth it because we want to move away from
sysbus, merging the non-sysbus specific API to qdev (like indexed
memory regions and IRQs to named ones).

If sysbus will be gone soon then maybe it's not worth it but if we're then left with needing five lines to create and connect a simple device (most of which is more concerning QOM and QDev than the actual device) then we'll really need to find some other way to reduce this boilerplate and let the developer create simple devices with simple calls. Littering board code with all the QOM boilerplate makes it really hard to see what it actually does so this should be hidden behind a simple API so that the board code is clean and overseeable without having to go into too much detail about the QOM and QDev implementations. If we need to do every detail of creating a device with a low level call (which may not even fit in one single line) then the board code will become unreadable and unaccessible especially for new contributors. That's why I think this is a problem we'd need to consider. (And this is not about this patch but a general issue, which I said in the beginning, I was just reminded about this by this patch.)

Regards,
BALATON Zoltan

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux