Re: [Qemu-devel] KVM call minutes for Feb 8

[Blue Swirl]

On Mon, Feb 14, 2011 at 12:42 AM, Anthony Liguori <address@hidden> wrote:
> On 02/13/2011 03:00 PM, Blue Swirl wrote:
>>
>> On Sun, Feb 13, 2011 at 9:57 PM, Anthony Liguori<address@hidden>
>>  wrote:
>>
>>>
>>> On 02/13/2011 01:37 PM, Blue Swirl wrote:
>>>
>>>>
>>>> On Sun, Feb 13, 2011 at 5:31 PM, Anthony Liguori<address@hidden>
>>>>  wrote:
>>>>
>>>>
>>>>>
>>>>> qdev doesn't expose any state today.  qdev properties are
>>>>> construction-only
>>>>> properties that happen to be stored in each device state.
>>>>>
>>>>> What we really need is a full property framework that includes
>>>>> properties
>>>>> with hookable getters and setters along with the ability to mark
>>>>> properties
>>>>> as construct-only, read-only, or read-write.
>>>>>
>>>>> But I think it's reasonable to expose construct-only properties as just
>>>>> an
>>>>> initfn argument.
>>>>>
>>>>>
>>>>
>>>> Sounds OK. About read-write properties, what happens if we one day
>>>> have extensive threading, and locks are pushed to device level? I can
>>>> imagine a deadlock involving one thread running in IO thread for a
>>>> device and another trying to access that device's properties. Maybe
>>>> that is not different from function call version.
>>>>
>>>>
>>>
>>> You need hookable setters/getters that can acquire a lock and do the
>>> right
>>> thing.  It shouldn't be able to dead lock if the locking is designed
>>> right.
>>>
>>>
>>>
>>>>>
>>>>> Yes, but qemu_irq is very restricted as it only models a signal bit of
>>>>> information and doesn't really have a mechanism to attach/detach in any
>>>>> generic way.
>>>>>
>>>>>
>>>>
>>>> Basic signals are already very useful for many purposes, since they
>>>> match digital logic signals in real HW. In theory, whole machines
>>>> could be constructed with just qemu_irq and NAND gate emulator. ;-)
>>>>
>>>>
>>>
>>> It's not just in theory.  In the C++ port of QEMU that I wrote, I
>>> implemented an AND, OR, and XOR gate and implemented a full 32-bit adder
>>> by
>>> just using a device config file.
>>>
>>> If done correctly, using referencing can be extremely powerful.  A full
>>> adder is a good example.  The gates really don't have any concept of bus
>>> and
>>> the relationship between gates is definitely not a tree.
>>>
>>>
>>>>
>>>> In the message passing IRQ discussion earlier, it was IIRC decided
>>>> that the one bit version would not be changed but a separate message
>>>> passing version would be created if ever needed.
>>>>
>>>>
>>>
>>> C already has a message passing interface that supports type safety
>>> called
>>> function pointers :-)
>>>
>>> An object that implements multiple interfaces where the interface becomes
>>> the "message passing interface" is exactly what I've been saying we need.
>>>  It's flexible and the compiler helps us enforce typing.
>>>
>>>
>>>>>
>>>>> Any interfaces of a base class should make sense even for derived
>>>>> classes.
>>>>>
>>>>> That means if the base class is going to expose essentially a pin-out
>>>>> interface, that if I have a PCIDevice and cast it to Device, I should
>>>>> be
>>>>> able to interact with the GPIO interface to interact with the PCI
>>>>> device.
>>>>>  Presumably, that means interfacing at the PCI signalling level.
>>>>>  That's
>>>>> insane to model in QEMU :-)
>>>>>
>>>>>
>>>>
>>>> This would be doable, if we built buses from a bunch of signals, like
>>>> in VHDL or Verilog. It would simplify aliased MMIO addresses nicely,
>>>> the undecoded address pins would be ignored. I don't think it would be
>>>> useful, but a separate interface could be added for connecting to
>>>> PCIBus with just qemu_irqs.
>>>>
>>>>
>>>
>>> Yeah, it's possible, but I don't want to spend my time doing this.
>>>
>>>
>>>>>
>>>>> In reality, GPIO only makes sense for a small class of simple devices
>>>>> where
>>>>> modelling the pin-out interface makes sense (like a 7-segment LCD).
>>>>>  That
>>>>> suggests that GPIO should not be in the DeviceState interface but
>>>>> instead
>>>>> should be in a SimpleDevice subclass or something like that.
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Could you point to examples of SystemBus overuse?
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> address@hidden:~/git/qemu/hw$ grep qdev_create *.c | wc -l
>>>>> 73
>>>>> address@hidden:~/git/qemu/hw$ grep 'qdev_create(NULL' *.c | wc -l
>>>>> 56
>>>>>
>>>>> SystemBus has become a catch-all for shallow qdev conversions.  We've
>>>>> got
>>>>> Northbridges, RAM, and network devices sitting on the same bus...
>>>>>
>>>>>
>>>>
>>>> On Sparc32 I have not bothered to create a SBus bus. Now it would be
>>>> useful to get bootindex corrected. Most devices (even on-board IO)
>>>> should use SBus.
>>>>
>>>> The only other bus (MBus) would exist between CPU, IOMMU and memory.
>>>>
>>>> But SysBus fitted the need until recently.
>>>>
>>>>
>>>
>>> A good way to judge where a device is using a bus interface correct: does
>>> all of it's interactions with any other part of the guest state involve
>>> method calls to the bus?
>>>
>>> Right now, the answer is no for just about every device in QEMU.  This is
>>> the problem that qdev really was meant to solve and we're not really any
>>> further along solving it unfortunately.
>>>
>>>
>>>>>
>>>>> I'm not arguing against a generic factory interface, I'm arguing that
>>>>> it
>>>>> should be separate.
>>>>>
>>>>> IOW:
>>>>>
>>>>> SerialState *serial_create(int iobase, int irq, ...);
>>>>>
>>>>> static DeviceState *qdev_serial_create(QemuOpts *opts);
>>>>>
>>>>> static void serial_init(void)
>>>>> {
>>>>>     qdev_register("serial", qdev_serial_create);
>>>>> }
>>>>>
>>>>> The key point is that when we create devices internally, we should have
>>>>> a
>>>>> C-friendly, type-safe interface to interact with.  This will encourage
>>>>> composition and a richer device model than what we have today.
>>>>>
>>>>>
>>>>
>>>> Isn't this what we have now in most cases?
>>>>
>>>>
>>>
>>> No.  The common pattern of shallow conversion is:
>>>
>>> struct SerialState
>>> {
>>>    // device state
>>> };
>>>
>>> struct ISASerialState
>>> {
>>>   ISADeviceState parent;
>>>   SerialState serial;
>>> };
>>>
>>> void serial_init(SerialState *s);
>>>
>>> void isa_serial_init(ISADevice *dev)
>>> {
>>>    ISASerialState *s = DO_UPCAST(dev);
>>>    serial_init(&s->serial);
>>> }
>>>
>>> The problem with this is that you cannot use serial_init() if you want to
>>> have the device be reflected in the device tree.
>>>
>>
>> But why would serial_init() be used anymore since isa_serial_init() is as
>> good?
>>
>
> The point is that it's not.
>
> Instead of doing error checking in one call, you've gotta do error checking
> in a half dozen calls because each of the set calls can fail.

I don't understand. The caller just does
if (isa_serial_init()) {
  error();
}
or
if (serial_init()) {
  error();
}

If you mean inside isa_serial_init() vs. serial_init(), that may be
true since isa_serial_init has to check for qdev failures, but the to
the caller both should be identical.

>>> GObject takes a different approach than I'm suggesting that is equally
>>> valid.  It supports a vararg constructor form and then provides
>>> C-friendly
>>> interfaces that use the vararg form.  For instance:
>>>
>>> SerialState *serial_init(int iobase, int irq, ...)
>>> {
>>>     return gobject_new(QEMU_SERIAL, "iobase", iobase, "irq", irq, ...,
>>> NULL);
>>> }
>>>
>>> This is not a bad solution but what I was trying to avoid in my
>>> suggestion
>>> is a lot of the ugliness of supporting a factory initializer.  It may be
>>> a
>>> better approach in the long run though.
>>>
>>
>> Producing varargs lists may get messy if the list is filled
>> differently based on some conditions. It's also not easy to do things
>> in between:
>> dev = isa_create();
>> qdev_prop_set_uint32(dev);
>> if (flag1) {
>>   qdev_prop_set_uint32(dev);
>> }
>> if (flag2) {
>>   int f = func();
>>   qdev_prop_set_uint32(dev, f);
>> }
>> qdev_prop_set_uint32(dev);
>> qdev_init_nofail(dev);
>>
>
> Vararg is designed for direct invocation.  You can still build a list of
> construction properties if you're so inclined.
>
> Regards,
>
> Anthony Liguori
>
>
>