Patchwork [03/61] virtio-pci-bus : introduce virtio-pci-bus.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date Jan. 7, 2013, 6:40 p.m.
Message ID <1357584074-10852-4-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/210094/
State New
Headers show

Comments

fred.konrad@greensocs.com - Jan. 7, 2013, 6:40 p.m.
From: KONRAD Frederic <fred.konrad@greensocs.com>

Introduce virtio-pci-bus, which extends virtio-bus. It is used with virtio-pci
transport device.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-pci.c | 37 +++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h | 18 ++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)
Peter Maydell - Jan. 8, 2013, 6:08 p.m.
On 7 January 2013 18:40,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Introduce virtio-pci-bus, which extends virtio-bus. It is used with virtio-pci
> transport device.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>

This isn't quite right, I think (somebody correct me if I'm wrong!)
The virtio-pci-bus subclass doesn't have any state of its own, so it
doesn't need a struct, but it does still want a type, so virtio-pci.h should
have a
   typedef VirtioBusClass VirtioPCIBusClass;
   typedef VirtioBusState VirtioPCIBusState;
and we then use VirtioPCIBusClass/State where appropriate
(notably in the typeinfo for class/instance size and in the get-class/
class-check/etc macros).

This lets us easily add state later if we need to by turning the
typedef into a typedef'd struct without having to hunt down all
the places that now need to say 'VirtioPCIBus*' rather than
'VirtioBus*'.

-- PMM
fred.konrad@greensocs.com - Jan. 9, 2013, 8:37 a.m.
On 08/01/2013 19:08, Peter Maydell wrote:
> On 7 January 2013 18:40,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> Introduce virtio-pci-bus, which extends virtio-bus. It is used with virtio-pci
>> transport device.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> This isn't quite right, I think (somebody correct me if I'm wrong!)
> The virtio-pci-bus subclass doesn't have any state of its own, so it
> doesn't need a struct, but it does still want a type, so virtio-pci.h should
> have a
>     typedef VirtioBusClass VirtioPCIBusClass;
>     typedef VirtioBusState VirtioPCIBusState;
> and we then use VirtioPCIBusClass/State where appropriate
> (notably in the typeinfo for class/instance size and in the get-class/
> class-check/etc macros).
>
> This lets us easily add state later if we need to by turning the
> typedef into a typedef'd struct without having to hunt down all
> the places that now need to say 'VirtioPCIBus*' rather than
> 'VirtioBus*'.
>
> -- PMM
>
Ok, I'll make the change.
fred.konrad@greensocs.com - Jan. 9, 2013, 9:14 a.m.
On 09/01/2013 09:37, KONRAD Frédéric wrote:
> On 08/01/2013 19:08, Peter Maydell wrote:
>> On 7 January 2013 18:40, <fred.konrad@greensocs.com> wrote:
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> Introduce virtio-pci-bus, which extends virtio-bus. It is used with 
>>> virtio-pci
>>> transport device.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> This isn't quite right, I think (somebody correct me if I'm wrong!)
>> The virtio-pci-bus subclass doesn't have any state of its own, so it
>> doesn't need a struct, but it does still want a type, so virtio-pci.h 
>> should
>> have a
>>     typedef VirtioBusClass VirtioPCIBusClass;
>>     typedef VirtioBusState VirtioPCIBusState;
>> and we then use VirtioPCIBusClass/State where appropriate
>> (notably in the typeinfo for class/instance size and in the get-class/
>> class-check/etc macros).
>>
>> This lets us easily add state later if we need to by turning the
>> typedef into a typedef'd struct without having to hunt down all
>> the places that now need to say 'VirtioPCIBus*' rather than
>> 'VirtioBus*'.
>>
>> -- PMM
>>
> Ok, I'll make the change.
>
>
Though, I'm not really sure we will add something here..
The virtio-pci-bus is only added to implement VirtioBusClass functions.
There are no differences between pci, s390, or mmio.

unless it is mandatory to have another name in the QOM macros ?
Peter Maydell - Jan. 9, 2013, 2:53 p.m.
On 9 January 2013 09:14, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 09/01/2013 09:37, KONRAD Frédéric wrote:
>> On 08/01/2013 19:08, Peter Maydell wrote:
>>> This isn't quite right, I think (somebody correct me if I'm wrong!)
>>> The virtio-pci-bus subclass doesn't have any state of its own, so it
>>> doesn't need a struct, but it does still want a type, so virtio-pci.h
>>> should
>>> have a
>>>     typedef VirtioBusClass VirtioPCIBusClass;
>>>     typedef VirtioBusState VirtioPCIBusState;
>>> and we then use VirtioPCIBusClass/State where appropriate
>>> (notably in the typeinfo for class/instance size and in the get-class/
>>> class-check/etc macros).
>>>
>>> This lets us easily add state later if we need to by turning the
>>> typedef into a typedef'd struct without having to hunt down all
>>> the places that now need to say 'VirtioPCIBus*' rather than
>>> 'VirtioBus*'.

>> Ok, I'll make the change.

> Though, I'm not really sure we will add something here..
> The virtio-pci-bus is only added to implement VirtioBusClass functions.
> There are no differences between pci, s390, or mmio.

It's just consistency, and as I say it avoids surprises later. We're
basically inventing our own object system in C here, which means
that there's no guard preventing you from doing something in a
way which is inconsistent with our object system but still valid
and functioning C; but it's better style not to do that.

-- PMM
fred.konrad@greensocs.com - Jan. 9, 2013, 2:55 p.m.
On 09/01/2013 15:53, Peter Maydell wrote:
> On 9 January 2013 09:14, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> On 09/01/2013 09:37, KONRAD Frédéric wrote:
>>> On 08/01/2013 19:08, Peter Maydell wrote:
>>>> This isn't quite right, I think (somebody correct me if I'm wrong!)
>>>> The virtio-pci-bus subclass doesn't have any state of its own, so it
>>>> doesn't need a struct, but it does still want a type, so virtio-pci.h
>>>> should
>>>> have a
>>>>      typedef VirtioBusClass VirtioPCIBusClass;
>>>>      typedef VirtioBusState VirtioPCIBusState;
>>>> and we then use VirtioPCIBusClass/State where appropriate
>>>> (notably in the typeinfo for class/instance size and in the get-class/
>>>> class-check/etc macros).
>>>>
>>>> This lets us easily add state later if we need to by turning the
>>>> typedef into a typedef'd struct without having to hunt down all
>>>> the places that now need to say 'VirtioPCIBus*' rather than
>>>> 'VirtioBus*'.
>>> Ok, I'll make the change.
>> Though, I'm not really sure we will add something here..
>> The virtio-pci-bus is only added to implement VirtioBusClass functions.
>> There are no differences between pci, s390, or mmio.
> It's just consistency, and as I say it avoids surprises later. We're
> basically inventing our own object system in C here, which means
> that there's no guard preventing you from doing something in a
> way which is inconsistent with our object system but still valid
> and functioning C; but it's better style not to do that.
>
> -- PMM
ok then, I'll change it. And for s390 too.

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index c7f0c4d..5ff03e8 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -31,6 +31,7 @@ 
 #include "sysemu/blockdev.h"
 #include "virtio-pci.h"
 #include "qemu/range.h"
+#include "virtio-bus.h"
 
 /* from Linux's linux/virtio_pci.h */
 
@@ -1162,6 +1163,41 @@  static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/* virtio-pci-bus */
+
+VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev)
+{
+    DeviceState *qdev = DEVICE(dev);
+    BusState *qbus = qbus_create(TYPE_VIRTIO_PCI_BUS, qdev, NULL);
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    qbus->allow_hotplug = 0;
+    return bus;
+}
+
+static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *bus_class = BUS_CLASS(klass);
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+    bus_class->max_dev = 1;
+    k->notify = virtio_pci_notify;
+    k->save_config = virtio_pci_save_config;
+    k->load_config = virtio_pci_load_config;
+    k->save_queue = virtio_pci_save_queue;
+    k->load_queue = virtio_pci_load_queue;
+    k->get_features = virtio_pci_get_features;
+    k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
+    k->set_host_notifier = virtio_pci_set_host_notifier;
+    k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
+    k->vmstate_change = virtio_pci_vmstate_change;
+}
+
+static const TypeInfo virtio_pci_bus_info = {
+    .name          = TYPE_VIRTIO_PCI_BUS,
+    .parent        = TYPE_VIRTIO_BUS,
+    .instance_size = sizeof(VirtioBusState),
+    .class_init    = virtio_pci_bus_class_init,
+};
+
 static void virtio_pci_register_types(void)
 {
     type_register_static(&virtio_blk_info);
@@ -1170,6 +1206,7 @@  static void virtio_pci_register_types(void)
     type_register_static(&virtio_balloon_info);
     type_register_static(&virtio_scsi_info);
     type_register_static(&virtio_rng_info);
+    type_register_static(&virtio_pci_bus_info);
 }
 
 type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index b58d9a2..2ac18b3 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -20,6 +20,20 @@ 
 #include "virtio-rng.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
+#include "virtio-bus.h"
+
+typedef struct VirtIOPCIProxy VirtIOPCIProxy;
+
+/* virtio-pci-bus */
+#define TYPE_VIRTIO_PCI_BUS "virtio-pci-bus"
+#define VIRTIO_PCI_BUS_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioBusClass, obj, TYPE_VIRTIO_PCI_BUS)
+#define VIRTIO_PCI_BUS_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioBusClass, klass, TYPE_VIRTIO_PCI_BUS)
+#define VIRTIO_PCI_BUS(obj) \
+        OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_PCI_BUS)
+
+VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev);
 
 /* Performance improves when virtqueue kick processing is decoupled from the
  * vcpu thread using ioeventfd for some devices. */
@@ -31,7 +45,7 @@  typedef struct {
     unsigned int users;
 } VirtIOIRQFD;
 
-typedef struct {
+struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     MemoryRegion bar;
@@ -51,7 +65,7 @@  typedef struct {
     bool ioeventfd_disabled;
     bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
-} VirtIOPCIProxy;
+};
 
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
 void virtio_pci_reset(DeviceState *d);