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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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