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