public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
From: Anthony Liguori <anthony@codemonkey.ws>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: Chris Wright <chrisw@redhat.com>, Gleb Natapov <gleb@redhat.com>,
	kvm@vger.kernel.org, Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org, Avi Kivity <avi@redhat.com>
Subject: Re: [Qemu-devel] KVM call minutes for Feb 8
Date: Sun, 13 Feb 2011 13:57:07 -0600	[thread overview]
Message-ID: <4D583793.10409@codemonkey.ws> (raw)
In-Reply-To: <AANLkTi=+S_HARXj3MzsB4QiuYoPvjN7w_GrO=p4F5sV-@mail.gmail.com>

On 02/13/2011 01:37 PM, Blue Swirl wrote:
> On Sun, Feb 13, 2011 at 5:31 PM, Anthony Liguori<anthony@codemonkey.ws>  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?
>>>
>>>        
>> anthony@titi:~/git/qemu/hw$ grep qdev_create *.c | wc -l
>> 73
>> anthony@titi:~/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.

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.

Regards,

Anthony Liguori

  reply	other threads:[~2011-02-13 19:57 UTC|newest]

Thread overview: 79+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-08 15:55 KVM call minutes for Feb 8 Chris Wright
2011-02-08 16:14 ` Stefan Hajnoczi
2011-02-08 16:39 ` [Qemu-devel] " Anthony Liguori
2011-02-08 17:13 ` Markus Armbruster
2011-02-08 19:02   ` Peter Maydell
2011-02-08 21:11     ` Anthony Liguori
2011-02-09  8:11     ` Markus Armbruster
2011-02-09  8:20       ` Peter Maydell
2011-02-09  9:02         ` Markus Armbruster
2011-02-08 19:30   ` Alexander Graf
2011-02-08 19:30   ` Aurelien Jarno
2011-02-09  8:23     ` Markus Armbruster
2011-02-09 10:43     ` Anthony Liguori
2011-02-09 17:38       ` Blue Swirl
2011-02-08 21:12   ` Anthony Liguori
2011-02-09  8:01     ` Markus Armbruster
2011-02-09 10:31       ` Anthony Liguori
2011-02-09 12:28         ` Markus Armbruster
2011-02-09 14:44           ` Anthony Liguori
2011-02-09 17:48             ` Blue Swirl
2011-02-09 19:53               ` Anthony Liguori
2011-02-09 19:59               ` Anthony Liguori
2011-02-09 20:15                 ` Blue Swirl
2011-02-10  7:47                   ` Anthony Liguori
2011-02-10  8:16                     ` Peter Maydell
2011-02-10  8:36                       ` Anthony Liguori
2011-02-10  9:04                         ` Peter Maydell
2011-02-10 10:13                           ` Anthony Liguori
2011-02-10 10:38                             ` Peter Maydell
2011-02-10 11:24                               ` Gleb Natapov
2011-02-10 12:23                               ` Anthony Liguori
2011-02-10 13:06                                 ` Peter Maydell
2011-02-10 19:17                       ` Scott Wood
2011-02-10 19:22                         ` Peter Maydell
2011-02-10 19:29                           ` Scott Wood
2011-02-10  9:07                     ` Gleb Natapov
2011-02-10 10:00                       ` Anthony Liguori
2011-02-10 10:10                         ` Gleb Natapov
2011-02-10 10:19                           ` Anthony Liguori
2011-02-10 10:49                             ` Gleb Natapov
2011-02-10 12:47                               ` Anthony Liguori
2011-02-10 13:12                                 ` Gleb Natapov
2011-02-10 10:25                       ` Avi Kivity
2011-02-10 11:13                         ` Gleb Natapov
2011-02-10 12:51                           ` Anthony Liguori
2011-02-10 13:00                             ` Avi Kivity
2011-02-10 13:29                               ` Gleb Natapov
2011-02-10 14:00                               ` Anthony Liguori
2011-02-10 13:27                             ` Gleb Natapov
2011-02-10 14:04                               ` Anthony Liguori
2011-02-10 14:20                                 ` Gleb Natapov
2011-02-10 16:05                                   ` Anthony Liguori
2011-02-11 18:14                                     ` Blue Swirl
2011-02-13  9:24                                       ` Gleb Natapov
2011-02-13 15:31                                       ` Anthony Liguori
2011-02-13 19:37                                         ` Blue Swirl
2011-02-13 19:57                                           ` Anthony Liguori [this message]
2011-02-13 21:00                                             ` Blue Swirl
2011-02-13 22:42                                               ` Anthony Liguori
2011-02-14 17:31                                                 ` Blue Swirl
2011-02-14 20:53                                                   ` Anthony Liguori
2011-02-14 21:25                                                     ` Blue Swirl
2011-02-14 21:47                                                       ` Anthony Liguori
2011-02-15 17:11                                                         ` Blue Swirl
2011-02-15 23:07                                                           ` Anthony Liguori
2011-02-16  9:52                                                             ` Gleb Natapov
2011-02-14  9:44                                             ` Paolo Bonzini
2011-02-10 10:29                     ` Avi Kivity
2011-02-13 15:38                       ` Anthony Liguori
2011-02-13 15:56                         ` Avi Kivity
2011-02-13 16:56                           ` Anthony Liguori
2011-02-13 18:08                             ` Gleb Natapov
2011-02-13 19:38                               ` Anthony Liguori
2011-02-14 10:23                                 ` Gleb Natapov
2011-02-13 21:24                             ` Peter Maydell
2011-02-13 22:43                               ` Anthony Liguori
2011-02-13 23:35                                 ` Peter Maydell
2011-02-13 15:39                       ` Anthony Liguori
2011-02-11 17:54                     ` Blue Swirl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D583793.10409@codemonkey.ws \
    --to=anthony@codemonkey.ws \
    --cc=armbru@redhat.com \
    --cc=avi@redhat.com \
    --cc=blauwirbel@gmail.com \
    --cc=chrisw@redhat.com \
    --cc=gleb@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox