| Message ID | 1357584074-10852-4-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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 ?
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
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.
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);