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@linaro.org>
---
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]);
dev = sysbus_create_varargs("pl181", 0x10005000, pic[17], pic[18], NULL);
/* Wire up MMC card detect and read-only signals. These have
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index d10b75dfdb..7e04b23af8 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -299,7 +299,11 @@ static void versatile_init(MachineState *machine, int board_id)
/* The versatile/PB actually has a modified Color LCD controller
that includes hardware cursor support from the PL111. */
- dev = sysbus_create_simple("pl110_versatile", 0x10120000, pic[16]);
+ dev = qdev_new("pl110_versatile");
+ sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+ sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x10120000);
+ sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[16]);
+
/* Wire up the mux control signals from the SYS_CLCD register */
qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index aa5f3ca0d4..671986c21e 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -276,6 +276,7 @@ static void a9_daughterboard_init(VexpressMachineState *vms,
{
MachineState *machine = MACHINE(vms);
MemoryRegion *sysmem = get_system_memory();
+ DeviceState *dev;
if (ram_size > 0x40000000) {
/* 1GB is the maximum the address space permits */
@@ -297,7 +298,9 @@ static void a9_daughterboard_init(VexpressMachineState *vms,
/* Daughterboard peripherals : 0x10020000 .. 0x20000000 */
/* 0x10020000 PL111 CLCD (daughterboard) */
- sysbus_create_simple("pl111", 0x10020000, pic[44]);
+ dev = qdev_new("pl111");
+ sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x10020000);
+ sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[44]);
/* 0x10060000 AXI RAM */
/* 0x100e0000 PL341 Dynamic Memory Controller */
@@ -650,7 +653,10 @@ static void vexpress_common_init(MachineState *machine)
/* VE_COMPACTFLASH: not modelled */
- sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
+ dev = qdev_new("pl111");
+ sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+ sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, map[VE_CLCD]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[14]);
dinfo = drive_get(IF_PFLASH, 0, 0);
pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
--
2.41.0
On 2/16/24 05:35, 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@linaro.org> > --- > hw/arm/realview.c | 5 ++++- > hw/arm/versatilepb.c | 6 +++++- > hw/arm/vexpress.c | 10 ++++++++-- > 3 files changed, 17 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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@linaro.org>
> ---
> 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?
Regards,
BALATON Zoltan
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@linaro.org>
>> ---
>> 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.
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@linaro.org>
>>> ---
>>> 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
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.
For the big list of issues we are trying to improve, see:
https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
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@linaro.org>
>>>> ---
>>>> 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.
Regards,
BALATON Zoltan
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@linaro.org>
>>>>> ---
>>>>> 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 wonder why this is that important since you never modified
any of the files changed by this series:
$ git shortlog -es hw/arm/realview.c hw/arm/versatilepb.c
hw/arm/vexpress.c hw/display/pl110.c hw/arm/exynos4210.c
hw/display/exynos4210_fimd.c hw/i386/kvmvapic.c
66 Peter Maydell <peter.maydell@linaro.org>
34 Markus Armbruster <armbru@redhat.com>
29 Philippe Mathieu-Daudé <philmd@linaro.org>
28 Paolo Bonzini <pbonzini@redhat.com>
17 Andreas Färber <afaerber@suse.de>
13 Eduardo Habkost <ehabkost@redhat.com>
8 Greg Bellows <greg.bellows@linaro.org>
7 Krzysztof Kozlowski <krzk@kernel.org>
6 Gerd Hoffmann <kraxel@redhat.com>
5 Richard Henderson <richard.henderson@linaro.org>
5 Jan Kiszka <jan.kiszka@siemens.com>
5 Igor Mammedov <imammedo@redhat.com>
4 Xiaoqiang Zhao <zxq_yx_007@163.com>
4 Thomas Huth <thuth@redhat.com>
4 Anthony Liguori <anthony@codemonkey.ws>
3 Stefan Weil <sw@weilnetz.de>
3 Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
3 Guenter Roeck <linux@roeck-us.net>
3 Daniel P. Berrangé <berrange@redhat.com>
3 Alistair Francis <alistair.francis@xilinx.com>
2 Roy Franz <roy.franz@linaro.org>
2 Pavel Dovgaluk <pavel.dovgaluk@ispras.ru>
2 Marcel Apfelbaum <marcel.a@redhat.com>
2 Linus Walleij <linus.walleij@linaro.org>
2 Like Xu <like.xu@linux.intel.com>
2 Juan Quintela <quintela@trasno.org>
2 Igor Mitsyanko <i.mitsyanko@samsung.com>
2 Hu Tao <hutao@cn.fujitsu.com>
2 David Woodhouse <dwmw@amazon.co.uk>
1 Zongyuan Li <zongyuan.li@smartx.com>
1 Wen, Jianxian <Jianxian.Wen@verisilicon.com>
1 Vincent Palatin <vpalatin@chromium.org>
1 Tao Xu <tao3.xu@intel.com>
1 Sergey Fedorov <serge.fdrv@gmail.com>
1 Prasad J Pandit <ppandit@redhat.com>
1 Prasad J Pandit <pjp@fedoraproject.org>
1 Pranith Kumar <bobby.prani@gmail.com>
1 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
1 Nikita Belov <zodiac@ispras.ru>
1 Martin Kletzander <mkletzan@redhat.com>
1 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
1 Marcelo Tosatti <mtosatti@redhat.com>
1 Marcel Apfelbaum <marcel@redhat.com>
1 Marc-André Lureau <marcandre.lureau@redhat.com>
1 Laurent Vivier <lvivier@redhat.com>
1 Laszlo Ersek <lersek@redhat.com>
1 Kevin Wolf <kwolf@redhat.com>
1 Jean-Christophe Dubois <jcd@tribudubois.net>
1 Igor Mitsyanko <i.mitsyanko@gmail.com>
1 Grant Likely <grant.likely@linaro.org>
1 Gonglei (Arei) <arei.gonglei@huawei.com>
1 Frederic Konrad <konrad.frederic@yahoo.fr>
1 Fabian Aggeler <aggelerf@ethz.ch>
1 Eric Auger <eric.auger@linaro.org>
1 Emilio G. Cota <cota@braap.org>
1 Dirk Müller <dirk@dmllr.de>
1 David Gibson <david@gibson.dropbear.id.au>
1 Chen Qun <kuhn.chenqun@huawei.com>
1 Chen Fan <chen.fan.fnst@cn.fujitsu.com>
1 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
1 Anthony Liguori <aliguori@amazon.com>
1 Alexander Graf <agraf@csgraf.de>
1 Alex Chen <alex.chen@huawei.com>
1 Alex Bennée <alex.bennee@linaro.org>
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@linaro.org>
>>>>>> ---
>>>>>> 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 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.
Regards,
BALATON Zoltan
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@linaro.org>
>>>>>>> ---
>>>>>>> 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.
On Mon, 19 Feb 2024, Mark Cave-Ayland wrote:
> 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@linaro.org>
>>>>>>>> ---
>>>>>>>> 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".
It's not just about keeping the line count down, although that helps with
readablility, it's simpler to see what the code does if one has to go
through less QDev and QOM details, and new people are unfamiliar with
those so when they see the five lines creating the single device they
won't get what it does while a sysbus_create_simple call is very self
explaining. Maybe sysbus_create_simple is not the best API and not one we
can keep but by point is that as long as we have board code and it's the
main way to create machines that developers have to work with then we
should have some simple API to do that and don't leave them with only low
level QOM and QDev calls that are not high level enough to creare a
machine conveniently. If the direction is to eventually don't need any
code to create a machine then don't spend much time on designing that API
but at least keep what we have as long as it's possible. Removing the
device creation from sysbus_create_simple is not a big change but allows
board code to keep using it for now instead of ending up an unreadable low
level calls that makes it harder to see at a glance what a board consists
of.
> 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.
I did not say this patch should not be accepred or anything like that.
Just if there's a way with not too much work to make this simpler (as in
more readable and understandable for people not familiar with low levels
of QEMU) then I think that's worth trying and keeping at least most of the
functions of sysbus_create_simple as sysbus_realize_simple is not much
work to do but avoids blowing up the board code with a lot of low level
QOM stuff that I'd rather keep out of there unless it could be made less
overwhelming and verbose. Also keeping a higher level API for board code
would help this refactoring because if the low level calls are not all
over the board code then they would need to change less as the changes
could be done within the higher level API implementation.
But at the end this is just my opinion and Philippe is free to do what he
wants. I ust shared this view point in case he can take it into account
but if not then it's not the end of the world.
Regards,
BALATON Zoltan
On 19/2/24 14:57, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Mark Cave-Ayland wrote:
>> 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@linaro.org>
>>>>>>>>> ---
>>>>>>>>> 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".
>
> It's not just about keeping the line count down, although that helps
> with readablility, it's simpler to see what the code does if one has to
> go through less QDev and QOM details, and new people are unfamiliar with
> those so when they see the five lines creating the single device they
> won't get what it does while a sysbus_create_simple call is very self
> explaining. Maybe sysbus_create_simple is not the best API and not one
> we can keep but by point is that as long as we have board code and it's
> the main way to create machines that developers have to work with then
> we should have some simple API to do that and don't leave them with only
> low level QOM and QDev calls that are not high level enough to creare a
> machine conveniently. If the direction is to eventually don't need any
> code to create a machine then don't spend much time on designing that
> API but at least keep what we have as long as it's possible. Removing
> the device creation from sysbus_create_simple is not a big change but
> allows board code to keep using it for now instead of ending up an
> unreadable low level calls that makes it harder to see at a glance what
> a board consists of.
>
>> 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.
>
> I did not say this patch should not be accepred or anything like that.
> Just if there's a way with not too much work to make this simpler (as in
> more readable and understandable for people not familiar with low levels
> of QEMU) then I think that's worth trying and keeping at least most of
> the functions of sysbus_create_simple as sysbus_realize_simple is not
> much work to do but avoids blowing up the board code with a lot of low
> level QOM stuff that I'd rather keep out of there unless it could be
> made less overwhelming and verbose. Also keeping a higher level API for
> board code would help this refactoring because if the low level calls
> are not all over the board code then they would need to change less as
> the changes could be done within the higher level API implementation.
>
> But at the end this is just my opinion and Philippe is free to do what
> he wants. I ust shared this view point in case he can take it into
> account but if not then it's not the end of the world.
Thanks for being open to experiments. I'm not trying to attack you here,
but to not ignore any community concerns as yours. While you have a deep
knowledge of QEMU internals you also try to keep it simple for new devs,
and we really want to keep the project attractive enough to new people.
I hope a declarative HW modelling language will simplify the current
situation, so developers would focus on HW modelling rather than C
implementations and their bugs. Users would focus on the (simple) DSL
and not the C API. To get there we need to start somewhere...
On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 19/02/2024 12:00, BALATON Zoltan wrote: > > 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. Mmm. It's unfortunate that we're working with C, so we're a bit limited in what tools we have to try to make a better and lower-boilerplate interface for the "create, configure, realize and wire up devices" task. (I think you could do much better in a higher level language...) sysbus_create_simple() was handy at the time, but it doesn't work so well for more complicated SoC-based boards. It's noticeable that if you look at the code that uses it, it's almost entirely the older and less maintained board models, especially those which don't actually model an SoC and just have the board code create all the devices. -- PMM
On 19/02/2024 13:05, Peter Maydell wrote: > On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 19/02/2024 12:00, BALATON Zoltan wrote: >>> 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. > > Mmm. It's unfortunate that we're working with C, so we're a bit limited > in what tools we have to try to make a better and lower-boilerplate > interface for the "create, configure, realize and wire up devices" task. > (I think you could do much better in a higher level language...) > sysbus_create_simple() was handy at the time, but it doesn't work so > well for more complicated SoC-based boards. It's noticeable that > if you look at the code that uses it, it's almost entirely the older > and less maintained board models, especially those which don't actually > model an SoC and just have the board code create all the devices. Yeah I was thinking that you'd use the DSL (e.g. YAML templates or similar) to provide some of the boilerplating around common actions, rather than the C API itself. Even better, once everything has been moved to use a DSL then the C API shouldn't really matter so much as it is no longer directly exposed to the user. ATB, Mark.
On 19/2/24 14:33, Mark Cave-Ayland wrote: > On 19/02/2024 13:05, Peter Maydell wrote: > >> On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland >> <mark.cave-ayland@ilande.co.uk> wrote: >>> >>> On 19/02/2024 12:00, BALATON Zoltan wrote: >>>> 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. >> >> Mmm. It's unfortunate that we're working with C, so we're a bit limited >> in what tools we have to try to make a better and lower-boilerplate >> interface for the "create, configure, realize and wire up devices" task. >> (I think you could do much better in a higher level language...) >> sysbus_create_simple() was handy at the time, but it doesn't work so >> well for more complicated SoC-based boards. It's noticeable that >> if you look at the code that uses it, it's almost entirely the older >> and less maintained board models, especially those which don't actually >> model an SoC and just have the board code create all the devices. > > Yeah I was thinking that you'd use the DSL (e.g. YAML templates or > similar) to provide some of the boilerplating around common actions, > rather than the C API itself. Even better, once everything has been > moved to use a DSL then the C API shouldn't really matter so much as it > is no longer directly exposed to the user. Something similar was discussed with Markus and Manos. Although the first step we noticed is to unify the QDev API -- making it verbose -- to figure what we need to expose in the DSL. Doing it the other way (starting a DSL and trying to adapt it to all QEMU models) seemed a waste of time. At least for our current human resources.
On Mon, 19 Feb 2024, Mark Cave-Ayland wrote: > On 19/02/2024 13:05, Peter Maydell wrote: >> On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland >> <mark.cave-ayland@ilande.co.uk> wrote: >>> >>> On 19/02/2024 12:00, BALATON Zoltan wrote: >>>> 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. >> >> Mmm. It's unfortunate that we're working with C, so we're a bit limited >> in what tools we have to try to make a better and lower-boilerplate >> interface for the "create, configure, realize and wire up devices" task. >> (I think you could do much better in a higher level language...) >> sysbus_create_simple() was handy at the time, but it doesn't work so >> well for more complicated SoC-based boards. It's noticeable that >> if you look at the code that uses it, it's almost entirely the older >> and less maintained board models, especially those which don't actually >> model an SoC and just have the board code create all the devices. > > Yeah I was thinking that you'd use the DSL (e.g. YAML templates or similar) > to provide some of the boilerplating around common actions, rather than the C > API itself. Even better, once everything has been moved to use a DSL then the > C API shouldn't really matter so much as it is no longer directly exposed to > the user. That may be a few more releases away (although Philippe is doing an excellent job with doing this all alone and as efficient as he is it might be reached sooner). So I think board code will stay for a while therefore if something can be done to keep it simple with not much work then maybe that's worth considering. That's why I did not propose to keep sysbus_create_simple and add properties to it because that might need something like a properties array with values that's hard to describe in C so it would be a bit more involved to implement and defining such arrays would only make it a litle less cluttered. So just keeping the parts that work for simple devices in sysbus_realize_simple and also keep sysbus_create_simple where it's already used is probably enough now rather than converting those to low level calls everywhere now. Then we'll see how well the declarative machines will turn out and then if we no longer need to write board code these wrappers could go away then but for now it may be too early when we still have a lot of board code to maintain. Regards, BALATON Zoltan
Hi, On 19/2/24 15:05, BALATON Zoltan wrote: > On Mon, 19 Feb 2024, Mark Cave-Ayland wrote: >> On 19/02/2024 13:05, Peter Maydell wrote: >>> On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland >>> <mark.cave-ayland@ilande.co.uk> wrote: >>>> >>>> On 19/02/2024 12:00, BALATON Zoltan wrote: >>>>> 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. >>> >>> Mmm. It's unfortunate that we're working with C, so we're a bit limited >>> in what tools we have to try to make a better and lower-boilerplate >>> interface for the "create, configure, realize and wire up devices" task. >>> (I think you could do much better in a higher level language...) >>> sysbus_create_simple() was handy at the time, but it doesn't work so >>> well for more complicated SoC-based boards. It's noticeable that >>> if you look at the code that uses it, it's almost entirely the older >>> and less maintained board models, especially those which don't actually >>> model an SoC and just have the board code create all the devices. >> >> Yeah I was thinking that you'd use the DSL (e.g. YAML templates or >> similar) to provide some of the boilerplating around common actions, >> rather than the C API itself. Even better, once everything has been >> moved to use a DSL then the C API shouldn't really matter so much as >> it is no longer directly exposed to the user. > > That may be a few more releases away (although Philippe is doing an > excellent job with doing this all alone and as efficient as he is it > might be reached sooner). So I think board code will stay for a while > therefore if something can be done to keep it simple with not much work > then maybe that's worth considering. That's why I did not propose to > keep sysbus_create_simple and add properties to it because that might > need something like a properties array with values that's hard to > describe in C so it would be a bit more involved to implement and > defining such arrays would only make it a litle less cluttered. So just > keeping the parts that work for simple devices in sysbus_realize_simple > and also keep sysbus_create_simple where it's already used is probably > enough now rather than converting those to low level calls everywhere now. > > Then we'll see how well the declarative machines will turn out and then > if we no longer need to write board code these wrappers could go away > then but for now it may be too early when we still have a lot of board > code to maintain. I'll keep forward with this patch inlining sysbus_create_simple(); if we notice in few releases the DSL experiment is a failure, I don't mind going back reverting it. Regards, Phil.
On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote: > On 19/2/24 15:05, BALATON Zoltan wrote: >> On Mon, 19 Feb 2024, Mark Cave-Ayland wrote: >>> On 19/02/2024 13:05, Peter Maydell wrote: >>>> On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland >>>> <mark.cave-ayland@ilande.co.uk> wrote: >>>>> >>>>> On 19/02/2024 12:00, BALATON Zoltan wrote: >>>>>> 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. >>>> >>>> Mmm. It's unfortunate that we're working with C, so we're a bit limited >>>> in what tools we have to try to make a better and lower-boilerplate >>>> interface for the "create, configure, realize and wire up devices" task. >>>> (I think you could do much better in a higher level language...) >>>> sysbus_create_simple() was handy at the time, but it doesn't work so >>>> well for more complicated SoC-based boards. It's noticeable that >>>> if you look at the code that uses it, it's almost entirely the older >>>> and less maintained board models, especially those which don't actually >>>> model an SoC and just have the board code create all the devices. >>> >>> Yeah I was thinking that you'd use the DSL (e.g. YAML templates or >>> similar) to provide some of the boilerplating around common actions, >>> rather than the C API itself. Even better, once everything has been moved >>> to use a DSL then the C API shouldn't really matter so much as it is no >>> longer directly exposed to the user. >> >> That may be a few more releases away (although Philippe is doing an >> excellent job with doing this all alone and as efficient as he is it might >> be reached sooner). So I think board code will stay for a while therefore >> if something can be done to keep it simple with not much work then maybe >> that's worth considering. That's why I did not propose to keep >> sysbus_create_simple and add properties to it because that might need >> something like a properties array with values that's hard to describe in C >> so it would be a bit more involved to implement and defining such arrays >> would only make it a litle less cluttered. So just keeping the parts that >> work for simple devices in sysbus_realize_simple and also keep >> sysbus_create_simple where it's already used is probably enough now rather >> than converting those to low level calls everywhere now. >> >> Then we'll see how well the declarative machines will turn out and then if >> we no longer need to write board code these wrappers could go away then but >> for now it may be too early when we still have a lot of board code to >> maintain. > > I'll keep forward with this patch inlining sysbus_create_simple(); > if we notice in few releases the DSL experiment is a failure, I don't > mind going back reverting it. I'm OK with that. Just thought that keeping a sysbus_realize_simple function that's the same as sysbus_create simple minus creating the device would not be a big change and cause less churn. But if you plan te remove this completely in near future so another API would be needed anyway then maybe not worth keeping it. Having only low level functions to create and wire devices seems impractical for writing boeards in C so eventually some replacement will be needed. I doubt every board can be quickly converted to a new declarative way soon but you can prove me wrong. If this helps you to progress to that direction then I'm not attached to it to much but would like to keep some simplicity in board code wherever possible as it's already quite complex to do simple things with low level APIs so I'd prefer if convenience APIs don't go away. Regards, BALATON Zoltan
On Mon, 19 Feb 2024 at 13:33, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 19/02/2024 13:05, Peter Maydell wrote: > > > On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland > > <mark.cave-ayland@ilande.co.uk> wrote: > >> > >> On 19/02/2024 12:00, BALATON Zoltan wrote: > >>> 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. > > > > Mmm. It's unfortunate that we're working with C, so we're a bit limited > > in what tools we have to try to make a better and lower-boilerplate > > interface for the "create, configure, realize and wire up devices" task. > > (I think you could do much better in a higher level language...) > > sysbus_create_simple() was handy at the time, but it doesn't work so > > well for more complicated SoC-based boards. It's noticeable that > > if you look at the code that uses it, it's almost entirely the older > > and less maintained board models, especially those which don't actually > > model an SoC and just have the board code create all the devices. > > Yeah I was thinking that you'd use the DSL (e.g. YAML templates or similar) to > provide some of the boilerplating around common actions, rather than the C API > itself. Even better, once everything has been moved to use a DSL then the C API > shouldn't really matter so much as it is no longer directly exposed to the user. That does feel like it's rather a long way away, though, so there might be scope for improving our C APIs in the meantime. (Also, doing the boilerplating with fragments of YAML or whatever means that checking of eg typos and other syntax errors shifts from compile time to runtime, which is a shame.) -- PMM
On 19/02/2024 13:35, Peter Maydell wrote: > On Mon, 19 Feb 2024 at 13:33, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 19/02/2024 13:05, Peter Maydell wrote: >> >>> On Mon, 19 Feb 2024 at 12:49, Mark Cave-Ayland >>> <mark.cave-ayland@ilande.co.uk> wrote: >>>> >>>> On 19/02/2024 12:00, BALATON Zoltan wrote: >>>>> 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. >>> >>> Mmm. It's unfortunate that we're working with C, so we're a bit limited >>> in what tools we have to try to make a better and lower-boilerplate >>> interface for the "create, configure, realize and wire up devices" task. >>> (I think you could do much better in a higher level language...) >>> sysbus_create_simple() was handy at the time, but it doesn't work so >>> well for more complicated SoC-based boards. It's noticeable that >>> if you look at the code that uses it, it's almost entirely the older >>> and less maintained board models, especially those which don't actually >>> model an SoC and just have the board code create all the devices. >> >> Yeah I was thinking that you'd use the DSL (e.g. YAML templates or similar) to >> provide some of the boilerplating around common actions, rather than the C API >> itself. Even better, once everything has been moved to use a DSL then the C API >> shouldn't really matter so much as it is no longer directly exposed to the user. > > That does feel like it's rather a long way away, though, so there > might be scope for improving our C APIs in the meantime. (Also, > doing the boilerplating with fragments of YAML or whatever means > that checking of eg typos and other syntax errors shifts from > compile time to runtime, which is a shame.) I think most people who frequently use QOM/qdev have ideas as to how to improve the C API, but what seems clear to me is that the analysis of scenarios by Phil, Markus and others as part of the heterogeneous machine work is useful and should be used to help guide future work in this area. If there are proposed changes in the C API, I'd be keen to see a short RFC detailing the changes and their rationale to aid developers/reviewers before they are introduced into the codebase. ATB, Mark.
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@linaro.org>
>>>>>>> ---
>>>>>>> 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()?
So we need a massive refactoring of:
- dev = sysbus_create_simple(typename, addr, irq);
+ dev = qdev_new(typename);
+ // optionally set properties
+ sysbus_realize_simple(dev, addr, irq);
- 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).
>> 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.
>
> Regards,
> BALATON Zoltan
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@linaro.org>
>>>>>>>> ---
>>>>>>>> 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
© 2016 - 2024 Red Hat, Inc.