[PATCH 0/6] hw: Remove sysbus_address_space()

[PATCH 0/6] hw: Remove sysbus_address_space()

[Philippe Mathieu-Daudé]

Pass address space as link property for devices where
it seems to matter, otherwise just use get_system_memory().

Philippe Mathieu-Daudé (6):
  hw/arm: Inline sysbus_create_simple(PL110 / PL111)
  hw/display/pl110: Pass frame buffer memory region as link property
  hw/arm/exynos4210: Inline sysbus_create_varargs(EXYNOS4210_FIMD)
  hw/display/exynos4210_fimd: Pass frame buffer memory region as link
  hw/i386/kvmvapic: Inline sysbus_address_space()
  hw/sysbus: Remove now unused sysbus_address_space()

 include/hw/sysbus.h          |  1 -
 hw/arm/exynos4210.c          | 12 +++++++-----
 hw/arm/realview.c            |  7 ++++++-
 hw/arm/versatilepb.c         |  8 +++++++-
 hw/arm/vexpress.c            | 15 +++++++++++++--
 hw/core/sysbus.c             |  5 -----
 hw/display/exynos4210_fimd.c | 19 ++++++++++++++++---
 hw/display/pl110.c           | 20 ++++++++++++++++----
 hw/i386/kvmvapic.c           | 12 ++++++------
 9 files changed, 71 insertions(+), 28 deletions(-)

-- 
2.41.0

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

[Philippe Mathieu-Daudé]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Philippe Mathieu-Daudé]

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/

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

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

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

[Philippe Mathieu-Daudé]

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>

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4750 bytes --]

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

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

[Philippe Mathieu-Daudé]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 7547 bytes --]

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

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

[Mark Cave-Ayland]

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.

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

[Peter Maydell]

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

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

[Mark Cave-Ayland]

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.

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

[Peter Maydell]

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

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

[Mark Cave-Ayland]

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.

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

[BALATON Zoltan]

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

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

[Philippe Mathieu-Daudé]

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.

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4545 bytes --]

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

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

[Philippe Mathieu-Daudé]

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.

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 8526 bytes --]

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

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

[Philippe Mathieu-Daudé]

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...

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

[Richard Henderson]

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~

[PATCH 2/6] hw/display/pl110: Pass frame buffer memory region as link property

[Philippe Mathieu-Daudé]

Add the PL110::'framebuffer-memory' property. Have the different
ARM boards set it. We don't need to call sysbus_address_space()
anymore.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/realview.c    |  2 ++
 hw/arm/versatilepb.c |  2 ++
 hw/arm/vexpress.c    |  5 +++++
 hw/display/pl110.c   | 20 ++++++++++++++++----
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 77300e92e5..b186f965c6 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -239,6 +239,8 @@ static void realview_init(MachineState *machine,
     gpio2 = sysbus_create_simple("pl061", 0x10015000, pic[8]);
 
     dev = qdev_new("pl111");
+    object_property_set_link(OBJECT(dev), "framebuffer-memory",
+                             OBJECT(sysmem), &error_fatal);
     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]);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 7e04b23af8..d48235453e 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -300,6 +300,8 @@ 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 = qdev_new("pl110_versatile");
+    object_property_set_link(OBJECT(dev), "framebuffer-memory",
+                             OBJECT(sysmem), &error_fatal);
     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]);
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 671986c21e..de815d84cc 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -299,6 +299,9 @@ static void a9_daughterboard_init(VexpressMachineState *vms,
 
     /* 0x10020000 PL111 CLCD (daughterboard) */
     dev = qdev_new("pl111");
+    object_property_set_link(OBJECT(dev), "framebuffer-memory",
+                             OBJECT(sysmem), &error_fatal);
+    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[44]);
 
@@ -654,6 +657,8 @@ static void vexpress_common_init(MachineState *machine)
     /* VE_COMPACTFLASH: not modelled */
 
     dev = qdev_new("pl111");
+    object_property_set_link(OBJECT(dev), "framebuffer-memory",
+                             OBJECT(sysmem), &error_fatal);
     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]);
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index 4b83db9322..7f145bbdba 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "hw/irq.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "ui/console.h"
 #include "framebuffer.h"
@@ -17,6 +18,7 @@
 #include "qemu/timer.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 #include "qom/object.h"
 
 #define PL110_CR_EN   0x001
@@ -74,6 +76,7 @@ struct PL110State {
     uint32_t palette[256];
     uint32_t raw_palette[128];
     qemu_irq irq;
+    MemoryRegion *fbmem;
 };
 
 static int vmstate_pl110_post_load(void *opaque, int version_id);
@@ -210,7 +213,6 @@ static int pl110_enabled(PL110State *s)
 static void pl110_update_display(void *opaque)
 {
     PL110State *s = (PL110State *)opaque;
-    SysBusDevice *sbd;
     DisplaySurface *surface = qemu_console_surface(s->con);
     drawfn fn;
     int src_width;
@@ -222,8 +224,6 @@ static void pl110_update_display(void *opaque)
         return;
     }
 
-    sbd = SYS_BUS_DEVICE(s);
-
     if (s->cr & PL110_CR_BGR)
         bpp_offset = 0;
     else
@@ -290,7 +290,7 @@ static void pl110_update_display(void *opaque)
     first = 0;
     if (s->invalidate) {
         framebuffer_update_memory_section(&s->fbsection,
-                                          sysbus_address_space(sbd),
+                                          s->fbmem,
                                           s->upbase,
                                           s->rows, src_width);
     }
@@ -535,11 +535,22 @@ static const GraphicHwOps pl110_gfx_ops = {
     .gfx_update  = pl110_update_display,
 };
 
+static Property pl110_properties[] = {
+    DEFINE_PROP_LINK("framebuffer-memory", PL110State, fbmem,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pl110_realize(DeviceState *dev, Error **errp)
 {
     PL110State *s = PL110(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
+    if (!s->fbmem) {
+        error_setg(errp, "'framebuffer-memory' property was not set");
+        return;
+    }
+
     memory_region_init_io(&s->iomem, OBJECT(s), &pl110_ops, s, "pl110", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
@@ -577,6 +588,7 @@ static void pl110_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
     dc->vmsd = &vmstate_pl110;
     dc->realize = pl110_realize;
+    device_class_set_props(dc, pl110_properties);
 }
 
 static const TypeInfo pl110_info = {
-- 
2.41.0

Re: [PATCH 2/6] hw/display/pl110: Pass frame buffer memory region as link property

[Richard Henderson]

On 2/16/24 05:35, Philippe Mathieu-Daudé wrote:
> Add the PL110::'framebuffer-memory' property. Have the different
> ARM boards set it. We don't need to call sysbus_address_space()
> anymore.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/arm/realview.c    |  2 ++
>   hw/arm/versatilepb.c |  2 ++
>   hw/arm/vexpress.c    |  5 +++++
>   hw/display/pl110.c   | 20 ++++++++++++++++----
>   4 files changed, 25 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/6] hw/arm/exynos4210: Inline sysbus_create_varargs(EXYNOS4210_FIMD)

[Philippe Mathieu-Daudé]

We want to set another qdev property (a link) for the FIMD
device, we can not use sysbus_create_varargs() 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/exynos4210.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 57c77b140c..ab18836943 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -769,11 +769,13 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
     }
 
     /*** Display controller (FIMD) ***/
-    sysbus_create_varargs("exynos4210.fimd", EXYNOS4210_FIMD0_BASE_ADDR,
-            s->irq_table[exynos4210_get_irq(11, 0)],
-            s->irq_table[exynos4210_get_irq(11, 1)],
-            s->irq_table[exynos4210_get_irq(11, 2)],
-            NULL);
+    dev = qdev_new("exynos4210.fimd");
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(busdev, &error_fatal);
+    sysbus_mmio_map(busdev, 0, EXYNOS4210_FIMD0_BASE_ADDR);
+    for (n = 0; n < 3; n++) {
+        sysbus_connect_irq(busdev, n, s->irq_table[exynos4210_get_irq(11, n)]);
+    }
 
     sysbus_create_simple(TYPE_EXYNOS4210_EHCI, EXYNOS4210_EHCI_BASE_ADDR,
             s->irq_table[exynos4210_get_irq(28, 3)]);
-- 
2.41.0

Re: [PATCH 3/6] hw/arm/exynos4210: Inline sysbus_create_varargs(EXYNOS4210_FIMD)

[Richard Henderson]

On 2/16/24 05:35, Philippe Mathieu-Daudé wrote:
> We want to set another qdev property (a link) for the FIMD
> device, we can not use sysbus_create_varargs() 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/exynos4210.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/6] hw/display/exynos4210_fimd: Pass frame buffer memory region as link

[Philippe Mathieu-Daudé]

Add the Exynos4210fimdState::'framebuffer-memory' property. Have
the board set it. We don't need to call sysbus_address_space()
anymore.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/exynos4210_fimd.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
index 84687527d5..5712558e13 100644
--- a/hw/display/exynos4210_fimd.c
+++ b/hw/display/exynos4210_fimd.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
 #include "hw/hw.h"
 #include "hw/irq.h"
 #include "hw/sysbus.h"
@@ -32,6 +33,7 @@
 #include "qemu/bswap.h"
 #include "qemu/module.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "qom/object.h"
 
 /* Debug messages configuration */
@@ -302,6 +304,7 @@ struct Exynos4210fimdState {
     MemoryRegion iomem;
     QemuConsole *console;
     qemu_irq irq[3];
+    MemoryRegion *fbmem;
 
     uint32_t vidcon[4];     /* Video main control registers 0-3 */
     uint32_t vidtcon[4];    /* Video time control registers 0-3 */
@@ -1119,7 +1122,6 @@ static void exynos4210_fimd_invalidate(void *opaque)
  * VIDOSDA, VIDOSDB, VIDWADDx and SHADOWCON registers */
 static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
 {
-    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
     Exynos4210fimdWindow *w = &s->window[win];
     hwaddr fb_start_addr, fb_mapped_len;
 
@@ -1147,8 +1149,7 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
         memory_region_unref(w->mem_section.mr);
     }
 
-    w->mem_section = memory_region_find(sysbus_address_space(sbd),
-                                        fb_start_addr, w->fb_len);
+    w->mem_section = memory_region_find(s->fbmem, fb_start_addr, w->fb_len);
     assert(w->mem_section.mr);
     assert(w->mem_section.offset_within_address_space == fb_start_addr);
     DPRINT_TRACE("Window %u framebuffer changed: address=0x%08x, len=0x%x\n",
@@ -1924,6 +1925,12 @@ static const GraphicHwOps exynos4210_fimd_ops = {
     .gfx_update  = exynos4210_fimd_update,
 };
 
+static Property exynos4210_fimd_properties[] = {
+    DEFINE_PROP_LINK("framebuffer-memory", Exynos4210fimdState, fbmem,
+                     TYPE_MEMORY_REGION, MemoryRegion *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void exynos4210_fimd_init(Object *obj)
 {
     Exynos4210fimdState *s = EXYNOS4210_FIMD(obj);
@@ -1944,6 +1951,11 @@ static void exynos4210_fimd_realize(DeviceState *dev, Error **errp)
 {
     Exynos4210fimdState *s = EXYNOS4210_FIMD(dev);
 
+    if (!s->fbmem) {
+        error_setg(errp, "'framebuffer-memory' property was not set");
+        return;
+    }
+
     s->console = graphic_console_init(dev, 0, &exynos4210_fimd_ops, s);
 }
 
@@ -1954,6 +1966,7 @@ static void exynos4210_fimd_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &exynos4210_fimd_vmstate;
     dc->reset = exynos4210_fimd_reset;
     dc->realize = exynos4210_fimd_realize;
+    device_class_set_props(dc, exynos4210_fimd_properties);
 }
 
 static const TypeInfo exynos4210_fimd_info = {
-- 
2.41.0

Re: [PATCH 4/6] hw/display/exynos4210_fimd: Pass frame buffer memory region as link

[Richard Henderson]

On 2/16/24 05:35, Philippe Mathieu-Daudé wrote:
> Add the Exynos4210fimdState::'framebuffer-memory' property. Have
> the board set it. We don't need to call sysbus_address_space()
> anymore.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/display/exynos4210_fimd.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 5/6] hw/i386/kvmvapic: Inline sysbus_address_space()

[Philippe Mathieu-Daudé]

sysbus_address_space(...) is a simple wrapper to
get_system_memory(). Use it in place, since KVM
VAPIC doesn't distinct address spaces.

Rename the 'as' variable as 'mr' since it is a
MemoryRegion type, not an AddressSpace one.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/kvmvapic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index f2b0aff479..25321d4f66 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -16,6 +16,7 @@
 #include "sysemu/hw_accel.h"
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
+#include "exec/address-spaces.h"
 #include "hw/i386/apic_internal.h"
 #include "hw/sysbus.h"
 #include "hw/boards.h"
@@ -57,6 +58,7 @@ typedef struct GuestROMState {
 
 struct VAPICROMState {
     SysBusDevice busdev;
+
     MemoryRegion io;
     MemoryRegion rom;
     uint32_t state;
@@ -580,19 +582,17 @@ static int vapic_map_rom_writable(VAPICROMState *s)
 {
     hwaddr rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
     MemoryRegionSection section;
-    MemoryRegion *as;
+    MemoryRegion *mr = get_system_memory();
     size_t rom_size;
     uint8_t *ram;
 
-    as = sysbus_address_space(&s->busdev);
-
     if (s->rom_mapped_writable) {
-        memory_region_del_subregion(as, &s->rom);
+        memory_region_del_subregion(mr, &s->rom);
         object_unparent(OBJECT(&s->rom));
     }
 
     /* grab RAM memory region (region @rom_paddr may still be pc.rom) */
-    section = memory_region_find(as, 0, 1);
+    section = memory_region_find(mr, 0, 1);
 
     /* read ROM size from RAM region */
     if (rom_paddr + 2 >= memory_region_size(section.mr)) {
@@ -613,7 +613,7 @@ static int vapic_map_rom_writable(VAPICROMState *s)
 
     memory_region_init_alias(&s->rom, OBJECT(s), "kvmvapic-rom", section.mr,
                              rom_paddr, rom_size);
-    memory_region_add_subregion_overlap(as, rom_paddr, &s->rom, 1000);
+    memory_region_add_subregion_overlap(mr, rom_paddr, &s->rom, 1000);
     s->rom_mapped_writable = true;
     memory_region_unref(section.mr);
 
-- 
2.41.0

Re: [PATCH 5/6] hw/i386/kvmvapic: Inline sysbus_address_space()

[Richard Henderson]

On 2/16/24 05:35, Philippe Mathieu-Daudé wrote:
> sysbus_address_space(...) is a simple wrapper to
> get_system_memory(). Use it in place, since KVM
> VAPIC doesn't distinct address spaces.
> 
> Rename the 'as' variable as 'mr' since it is a
> MemoryRegion type, not an AddressSpace one.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i386/kvmvapic.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 6/6] hw/sysbus: Remove now unused sysbus_address_space()

[Philippe Mathieu-Daudé]

sysbus_address_space() is not more used, remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/sysbus.h | 1 -
 hw/core/sysbus.c    | 5 -----
 2 files changed, 6 deletions(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 3564b7b6a2..01d4a400c6 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -85,7 +85,6 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
 void sysbus_mmio_unmap(SysBusDevice *dev, int n);
 void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
-MemoryRegion *sysbus_address_space(SysBusDevice *dev);
 
 bool sysbus_realize(SysBusDevice *dev, Error **errp);
 bool sysbus_realize_and_unref(SysBusDevice *dev, Error **errp);
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 35f902b582..5524287730 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -304,11 +304,6 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
     memory_region_add_subregion(get_system_io(), addr, mem);
 }
 
-MemoryRegion *sysbus_address_space(SysBusDevice *dev)
-{
-    return get_system_memory();
-}
-
 static void sysbus_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
-- 
2.41.0

Re: [PATCH 6/6] hw/sysbus: Remove now unused sysbus_address_space()

[Richard Henderson]

On 2/16/24 05:35, Philippe Mathieu-Daudé wrote:
> sysbus_address_space() is not more used, remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/sysbus.h | 1 -
>   hw/core/sysbus.c    | 5 -----
>   2 files changed, 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~