All of lore.kernel.org
 help / color / mirror / Atom feed
From: Anthony Liguori <anthony@codemonkey.ws>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Jan Kiszka <jan.kiszka@siemens.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	Markus Armbruster <armbru@redhat.com>,
	Gerd Hoffmann <kraxel@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Subject: Re: [Qemu-devel] [RFC] Plan for moving forward with QOM
Date: Thu, 15 Sep 2011 08:26:49 -0500	[thread overview]
Message-ID: <4E71FD19.6050606@codemonkey.ws> (raw)
In-Reply-To: <4E719F7C.10700@redhat.com>

On 09/15/2011 01:47 AM, Paolo Bonzini wrote:
> On 09/14/2011 08:04 PM, Anthony Liguori wrote:
>> The concept of busses are implemented as an
>> interface that a device implements.
>
> I noticed that you haven't written in the document how to make devices reside on
> a particular bus (PCI, ISA, I2C, ...).
>
> The three possibilities for this are:
>
> * a device implements an interface. I would rule this out because for most buses
> the devices will need to store some data (PCI: configuration data, pointer to
> the parent bus; ISA: pointer to the parent bus). Interfaces are
> implementation-only, so you have to place the data in each device and cause
> massive code duplication.

I agree.

>
> * a device inherits from an abstract class, e.g. PCIDevice. It is useful to see
> how the inheritance tree would look like for two devices with a common chipset
> and multiple interfaces:
>
> Device
> NE2000
> PCIDevice
> PCI_NE2000 ------> includes a NE2000
> ISA_NE2000 ------> includes a NE2000

I think this model is the closest to what we have today and is the most obvious. 
  For something like ne2k, I would expect:

class NE2000 : public Device
{
   // ne2k public functions
};

class PCI_NE2000 : public PciDevice
{
   // implement PCI functions by calling ne2k public functions
   NE2000 ne2k;
};

class ISA_NE2000 : public IsaDevice
{
   // implement ISA functions by calling ne2k public functions
   NE2000 ne2k;
};

> * a device is composed with a connector object. There is no PCIDevice class
> anymore, so the bus has an array of socket<PCIConnector> instead. The case above
> would look like this
>
> Device
> NE2000 (abstract)
> PCI_NE2000 ------> includes a PCIConnector
> ISA_NE2000 ------> includes an ISAConnector
>
> Or, if you insist on a closer mapping of real hardware, where there are no
> abstract classes, it would look like this:
>
> Device
> NE2000
> PCI_NE2000 ------> includes an NE2000 and a PCIConnector
> ISA_NE2000 ------> includes an NE2000 and an ISAConnector

I think there are two ways to view this:

class PciDevice : public Device
{
    PciConnector connector;
    // init function registers closures with connector that dispatch
    // to abstract functions
};

Or:

class PciConnector : public PciDevice
{
    // provides interfaces to register closures which implement
    // PCI abstract functions
};

I personally lean toward the later as I don't think the PciConnector model 
really does map all that well to hardware (normally, at least).  I think this is 
much closer to how real hardware actually works.

>
> Advantages of abstract classes are pretty obvious, so I will just list them: it
> is more similar to what is done in QDev, and perhaps it is more intuitive.
>
>
> Advantages of connectors include:
>
> * it is more flexible: it lets you choose between a more abstract and a more
> low-level representation (the two hierarchies above);
>
> * you have the option of showing a simpler device tree to the user, without the
> internal composition. This is important because, unlike QDev, composition in QOM
> is explicit. So QOM would place NIC properties in NE2000, not in *_NE2000 (right?).
>
> * related to this, it keeps property names more stable. If I understand
> correctly, if the device starts as ISA-only or PCI-only, i.e.:
>
> Device
> PCIDevice
> PCI_NE2000
>
> and later you change it to support multiple buses, suddenly properties would
> have to be addressed differently to account for the composition of NE2000 inside
> PCI_NE2000. You could I guess implement "forwarder properties", but that would
> also lead to more boilerplate code.
>
> Any other opinions?

The properties thing is definitely an interesting point, but I'm not sure how 
far you can push it.  If you start out with a NE2000 device that is ISA and you 
decide to abstract it to a shared model, all you need to do is keep the ISA 
NE2000 device named NE2000 and call the common chip and PCI bridge something else.

I really think it's important to keep the simple cases simple.  I think any 
model where you don't do:

class E1000 : public PciDevice
{
};

Is unnecessarily complicated.  If it's too complicated, conversions will be much 
slower to do and will be more likely to be done wrong.

Regards,

Anthony Liguori

> Paolo
>

  reply	other threads:[~2011-09-15 13:26 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-14 18:04 [Qemu-devel] [RFC] Plan for moving forward with QOM Anthony Liguori
2011-09-14 18:49 ` Anthony Liguori
2011-09-14 19:30 ` Jan Kiszka
2011-09-14 19:42   ` Anthony Liguori
2011-09-14 21:15     ` Jan Kiszka
2011-09-14 22:11       ` Anthony Liguori
2011-09-15 13:43         ` Jan Kiszka
2011-09-15 14:11           ` Anthony Liguori
2011-09-15 16:38             ` Jan Kiszka
2011-09-15 18:01               ` Anthony Liguori
2011-09-16 10:12             ` Kevin Wolf
2011-09-16 13:00               ` Anthony Liguori
2011-09-14 20:00 ` Edgar E. Iglesias
2011-09-14 20:22   ` Anthony Liguori
2011-09-14 20:27     ` Edgar E. Iglesias
2011-09-14 20:37     ` Blue Swirl
2011-09-14 21:25       ` Anthony Liguori
2011-09-15  6:31 ` Gleb Natapov
2011-09-15 10:49   ` Stefan Hajnoczi
2011-09-15 13:08     ` Anthony Liguori
2011-09-15 13:17   ` Anthony Liguori
2011-09-15 14:23     ` Gleb Natapov
2011-09-16 14:46     ` John Williams
2011-09-16 16:10       ` Anthony Liguori
2011-09-17  1:11         ` Edgar E. Iglesias
2011-09-17  2:12           ` Anthony Liguori
2011-09-17  2:35             ` Edgar E. Iglesias
2011-09-15 13:57   ` Anthony Liguori
2011-09-15 14:14     ` Paolo Bonzini
2011-09-15 14:25       ` Gleb Natapov
2011-09-15 15:28         ` Anthony Liguori
2011-09-15 15:38           ` Gleb Natapov
2011-09-15 16:33             ` Anthony Liguori
2011-09-15 16:59               ` Gleb Natapov
2011-09-15 17:51                 ` Anthony Liguori
2011-09-15 20:29                   ` Gleb Natapov
2011-09-15 20:45                     ` Peter Maydell
2011-09-15 21:15                       ` Anthony Liguori
2011-09-16 16:33                       ` Gleb Natapov
2011-09-16 17:47                         ` Peter Maydell
2011-09-16 18:08                           ` Anthony Liguori
2011-09-16 18:22                             ` Gleb Natapov
2011-09-16 18:42                               ` Anthony Liguori
2011-09-16 19:13                                 ` Gleb Natapov
2011-09-16 19:29                                   ` Anthony Liguori
2011-09-16 20:48                                     ` Gleb Natapov
2011-09-16 21:03                                       ` Anthony Liguori
2011-09-17  0:01                                 ` Edgar E. Iglesias
2011-09-16 18:18                           ` Gleb Natapov
2011-09-15 20:50                     ` Anthony Liguori
2011-09-16 16:47                       ` Gleb Natapov
2011-09-17  0:48                         ` Edgar E. Iglesias
2011-09-17  2:17                           ` Anthony Liguori
2011-09-17  2:29                             ` Anthony Liguori
2011-09-17  2:41                             ` Edgar E. Iglesias
2011-09-15  6:47 ` Paolo Bonzini
2011-09-15 13:26   ` Anthony Liguori [this message]
2011-09-15 13:35     ` Paolo Bonzini
2011-09-15 13:54       ` Peter Maydell
2011-09-15 14:18         ` Anthony Liguori
2011-09-15 14:33           ` Paolo Bonzini
2011-09-15 14:48             ` Peter Maydell
2011-09-15 15:31             ` Anthony Liguori
2011-09-15 15:47               ` Paolo Bonzini
2011-09-15 20:23     ` Avi Kivity
2011-09-15 20:52       ` Anthony Liguori
2011-09-18  7:56         ` Avi Kivity
2011-09-18 14:00           ` Avi Kivity
2011-09-16  9:36       ` Gerd Hoffmann
2011-12-13  4:47 ` Paul Brook
2011-12-13 13:22   ` Anthony Liguori
2011-12-13 17:40     ` Paul Brook
2011-12-13 18:00       ` Anthony Liguori
2011-12-13 20:36         ` Paul Brook
2011-12-13 21:53           ` Anthony Liguori
2011-12-14  0:39             ` Paul Brook
2011-12-14 13:53               ` Anthony Liguori
2011-12-14 14:01                 ` Avi Kivity
2011-12-14 14:11                   ` Anthony Liguori
2011-12-14 14:35                     ` Avi Kivity
2011-12-14 14:46                       ` Anthony Liguori
2011-12-14 14:50                         ` Avi Kivity
2011-12-15 18:59                 ` Paul Brook
2011-12-15 19:12                   ` Anthony Liguori
2011-12-15 21:28                     ` Paul Brook
2011-12-16  2:08                       ` Anthony Liguori
2011-12-16  5:11                         ` Paul Brook
2011-12-14  9:11             ` Andreas Färber

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=4E71FD19.6050606@codemonkey.ws \
    --to=anthony@codemonkey.ws \
    --cc=armbru@redhat.com \
    --cc=edgar.iglesias@gmail.com \
    --cc=jan.kiszka@siemens.com \
    --cc=kraxel@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.