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

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

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