Re: [RFC PATCH v5 0/3] Sysbus device generic QAPI plug support

[David Hildenbrand]

On 01.06.22 10:39, Damien Hedde wrote:
> 
> 
> On 5/31/22 22:43, Mark Cave-Ayland wrote:
>> On 31/05/2022 10:22, Damien Hedde wrote:
>>
>>> On 5/31/22 10:00, Mark Cave-Ayland wrote:
>>>> On 30/05/2022 15:05, Damien Hedde wrote:
>>>>
>>>>> On 5/30/22 12:25, Peter Maydell wrote:
>>>>>> On Mon, 30 May 2022 at 10:50, Damien Hedde 
>>>>>> <damien.hedde@greensocs.com> wrote:
>>>>>>> TYPE_SYS_BUS_DEVICE also comes with reset support.
>>>>>>> If a device is on not on any bus it is not reached by the root sysbus
>>>>>>> reset which propagates to every device (and other sub-buses).
>>>>>>> Even if we move all the mmio/sysbus-irq logic into TYPE_DEVICE, we 
>>>>>>> will
>>>>>>> still miss that. The bus is needed to handle the reset.
>>>>>>> For devices created in machine init code, we have the option to do 
>>>>>>> it in
>>>>>>> the machine reset handler. But for user created device, this is an 
>>>>>>> issue.
>>>>>>
>>>>>> Yes, the missing reset support in TYPE_DEVICE is a design
>>>>>> flaw that we really should try to address.
>>>>
>>>> I think the easiest way to handle this would be just after calling 
>>>> dc->realize; if the device has bus == NULL and dc->reset != NULL then 
>>>> manually call qemu_register_reset() for dc->reset. In a qdev world 
>>>> dc->reset is intended to be a bus-level reset, but I can't see an 
>>>> issue with manual registration for individual devices in this way, 
>>>> particularly as there are no reset ordering guarantees for sysbus.
>>>
>>> I'm a bit afraid calling qemu_register_reset() outside dc->realize 
>>> might modify the behavior for existing devices. Does any device end up 
>>> having a non-NULL bus right now when using "-device" CLI ?
>>
>> If you take a look at "info qtree" then that will show you all devices 
>> that are attached to a bus i.e. ones where bus is not NULL.
>>
>>>>>>> If we end up putting in TYPE_DEVICE support for mmios, interrupts and
>>>>>>> some way to do the bus reset. What would be the difference between 
>>>>>>> the
>>>>>>> current TYPE_SYS_BUS_DEVICE ?
>>>>>>
>>>>>> There would be none, and the idea would be to get rid of
>>>>>> TYPE_SYS_BUS_DEVICE entirely...
>>>>>>
>>>>> Do you expect the bus object to disappear in the process (bus-less 
>>>>> system) or transforming the sysbus into a ~TYPE_BUS thing ?
>>>>
>>>> I'd probably lean towards removing sysbus completely since in real 
>>>> life devices can exist outside of a bus. If a device needs a bus then 
>>>> it should already be modelled in QEMU, and anything that requires a 
>>>> hierarchy can already be represented via QOM children
>>>
>>> For me, a "memory bus" is a bus. But I understand in QEMU, this is 
>>> modeled by a memory region and we do not want to represent it anymore 
>>> by a qdev/qbus hierarchy.
>>>
>>>>
>>>>> Assuming we manage to sort out this does cold plugging using the 
>>>>> following scenario looks ok ? (regarding having to issue one command 
>>>>> to create the device AND some commands to handle memory-region and 
>>>>> interrupt lines)
>>>>>
>>>>>  > device_add driver=ibex-uart id=uart chardev=serial0
>>>>>  > sysbus-mmio-map device=uart addr=1073741824
>>>>>  > qom-set path=uart property=sysbus-irq[0] 
>>>>> value=plic/unnamed-gpio-in[1]
>>>>>
>>>>> TYPE_DEVICE or TYPE_SYS_BUS_DEVICE, my goal is still to be able to 
>>>>> cold-plug a "ibex-uart" define its memory map and choose which irq I 
>>>>> wire where.
>>>>
>>>> Anyhow getting back on topic: my main objection here is that you're 
>>>> adding a command "sysbus-mmio-map" when we don't want the concept of 
>>>> SysBusDevice to be exposed outside of QEMU at all. Referring back to 
>>>> my last email I think we should extend the device concept in the 
>>>> monitor to handle the additional functionality perhaps along the 
>>>> lines of:
>>>>
>>>> - A monitor command such as "device_map" which is effectively a 
>>>> wrapper around
>>>>    memory_region_add_subregion(). Do we also want a "device_unmap"? 
>>>> We should
>>>>    consider allow mapping to other memory regions other than the 
>>>> system root.
>>>>
>>>> - A monitor command such as "device_connect" which can be used to 
>>>> simplify your IRQ
>>>>    wiring, perhaps also with a "device_disconnect"?
>>>>
>>>> - An outline of the monitor commands showing the complete workflow 
>>>> from introspection
>>>>    of a device to mapping its memory region(s) and connecting its gpios
>>>>
>>>> Does that give you enough information to come up with a more detailed 
>>>> proposal?
>>>>
>>>
>>> Yes. Sorry for being not clear enough. I did not wanted to insist on 
>>> specific command names. I've no issues regarding the modifications you 
>>> request about having a device_connect or a device_map.
>>>
>>> My question was more about the workflow which does not rely on issuing 
>>> a single 'device_add' command handling mapping/connection using 
>>> parameters. Note that since we are talking supporting of map/connect 
>>> for the base type TYPE_DEVICE, I don't really see how we could have 
>>> parameters for these without impacting subtypes.
>>
>> I'm not sure I understand what you are saying here? Can you give an 
>> example?
> 
> There are 2 possible workflows:
> 1. several commands
>  > device_add ...
>  > device_map ...
>  > device_connect ...
> 
> 2. single command
>  > device_add ... map={...} connect={...}
> 
> The 2nd one is more like how we connect devices with '-device': all is 
> done at once. But if this is supposed to apply to TYPE_DEVICE (versus 
> TYPE_SYS_BUS_DEVICE), it becomes IMHO hard to prevent using them on 
> devices where it does not makes sense (for example: a virtio or pci 
> device for which everything is already handled).

Can't we flag devices for which map/connect is supported in the device
class somehow?


-- 
Thanks,

David / dhildenb