[RFC,v2,2/3] virtio-pci : add a virtio-bus interface

From: KONRAD Frederic <fred.konrad@greensocs.com>

This patch add a VirtioBus in the VirtIOPCIProxy structure. It creates a new
device : "virtio-pci" which init the VirtioBus. Two callback are written :

    * void virtio_pci_init_cb(DeviceState *dev) to initialize the PCI interface
      after the VirtIODevice init, it is approximately the same as
      virtio_init_pci.

    * void virtio_pci_exit_cb(DeviceState *dev) to stop the ioevent before the
      VirtIODevice exit.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-pci.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h |   6 +++
 2 files changed, 158 insertions(+)

On Thu, 22 Nov 2012 15:50:51 +0100
fred.konrad@greensocs.com wrote:


> +static void virtiopci_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
> +
> +    pc->init = virtiopci_qdev_init;
> +    pc->exit = virtio_exit_pci;

Doesn't the exit function need to be symmetric to the init function?

> +    pc->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> +    pc->revision = VIRTIO_PCI_ABI_VERSION;
> +    pc->class_id = PCI_CLASS_OTHERS;
> +    /* TODO : Add the correct device information below */
> +    /* pc->exit =
> +     * pc->device_id =
> +     * pc->subsystem_vendor_id =
> +     * pc->subsystem_id =
> +     * dc->reset =
> +     * dc->vmsd =
> +     */
> +    dc->props = virtiopci_properties;
> +    dc->desc = "virtio-pci transport.";
> +}

On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
> +static const struct VirtioBusInfo virtio_bus_info = {
> +    .init_cb = virtio_pci_init_cb,
> +    .exit_cb = virtio_pci_exit_cb,
> +
> +    .virtio_bindings = {

Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
don't see a need for separate structs.

> +        .notify = virtio_pci_notify,
> +        .save_config = virtio_pci_save_config,
> +        .load_config = virtio_pci_load_config,
> +        .save_queue = virtio_pci_save_queue,
> +        .load_queue = virtio_pci_load_queue,
> +        .get_features = virtio_pci_get_features,
> +        .query_guest_notifiers = virtio_pci_query_guest_notifiers,
> +        .set_host_notifier = virtio_pci_set_host_notifier,
> +        .set_guest_notifiers = virtio_pci_set_guest_notifiers,
> +        .vmstate_change = virtio_pci_vmstate_change,
> +    }
> +};

On 23 November 2012 12:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
>> +static const struct VirtioBusInfo virtio_bus_info = {
>> +    .init_cb = virtio_pci_init_cb,
>> +    .exit_cb = virtio_pci_exit_cb,
>> +
>> +    .virtio_bindings = {
>
> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
> don't see a need for separate structs.

I agree. It might (or might not) be convenient to retain it
temporarily while converting all the transports, but
VirtIOBindings is part of the old code which we're trying
to refactor here, and I'd expect it to go away when we're done.

-- PMM

On 23/11/2012 13:34, Peter Maydell wrote:
> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>  wrote:
>> On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
>>> +static const struct VirtioBusInfo virtio_bus_info = {
>>> +    .init_cb = virtio_pci_init_cb,
>>> +    .exit_cb = virtio_pci_exit_cb,
>>> +
>>> +    .virtio_bindings = {
>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>> don't see a need for separate structs.
> I agree. It might (or might not) be convenient to retain it
> temporarily while converting all the transports, but
> VirtIOBindings is part of the old code which we're trying
> to refactor here, and I'd expect it to go away when we're done.
>
> -- PMM
Yes, for the moment, I didn't refactor this VirtIOBindings, so it is 
better to separate struct to keep the virtiodevice binding function.

On 23 November 2012 14:23, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 23/11/2012 13:34, Peter Maydell wrote:
>> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>  wrote:
>>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>>> don't see a need for separate structs.
>>
>> I agree. It might (or might not) be convenient to retain it
>> temporarily while converting all the transports, but
>> VirtIOBindings is part of the old code which we're trying
>> to refactor here, and I'd expect it to go away when we're done.

> Yes, for the moment, I didn't refactor this VirtIOBindings, so it
> is better to separate struct to keep the virtiodevice binding function.

Where you're deliberately not changing something as a temporary
step you need to comment it to make that clear. Otherwise
people trying to review the code won't be able to tell...

-- PMM

On 23/11/2012 15:26, Peter Maydell wrote:
> On 23 November 2012 14:23, Konrad Frederic<fred.konrad@greensocs.com>  wrote:
>> On 23/11/2012 13:34, Peter Maydell wrote:
>>> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>   wrote:
>>>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>>>> don't see a need for separate structs.
>>> I agree. It might (or might not) be convenient to retain it
>>> temporarily while converting all the transports, but
>>> VirtIOBindings is part of the old code which we're trying
>>> to refactor here, and I'd expect it to go away when we're done.
>> Yes, for the moment, I didn't refactor this VirtIOBindings, so it
>> is better to separate struct to keep the virtiodevice binding function.
> Where you're deliberately not changing something as a temporary
> step you need to comment it to make that clear. Otherwise
> people trying to review the code won't be able to tell...
>
> -- PMM
Ok, sorry for that.

Fred

fred.konrad@greensocs.com writes:

> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This patch add a VirtioBus in the VirtIOPCIProxy structure. It creates a new
> device : "virtio-pci" which init the VirtioBus. Two callback are written :
>
>     * void virtio_pci_init_cb(DeviceState *dev) to initialize the PCI interface
>       after the VirtIODevice init, it is approximately the same as
>       virtio_init_pci.
>
>     * void virtio_pci_exit_cb(DeviceState *dev) to stop the ioevent before the
>       VirtIODevice exit.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/virtio-pci.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-pci.h |   6 +++
>  2 files changed, 158 insertions(+)
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 71f4fb5..78aa5e8 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -22,6 +22,7 @@
>  #include "virtio-net.h"
>  #include "virtio-serial.h"
>  #include "virtio-scsi.h"
> +#include "virtio-bus.h"
>  #include "pci.h"
>  #include "qemu-error.h"
>  #include "msi.h"
> @@ -1117,15 +1118,166 @@ static TypeInfo virtio_scsi_info = {
>      .instance_size = sizeof(VirtIOPCIProxy),
>      .class_init    = virtio_scsi_class_init,
>  };
> +/* The new virtio-pci device */
> +
> +void virtio_pci_init_cb(DeviceState *dev)
> +{
> +    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
> +    uint8_t *config;
> +    uint32_t size;
> +
> +    /* Put the PCI IDs */
> +    switch (get_virtio_device_id(proxy->bus)) {
> +
> +    case VIRTIO_ID_BLOCK:
> +        pci_config_set_device_id(proxy->pci_dev.config,
> +                                 PCI_DEVICE_ID_VIRTIO_BLOCK);
> +        pci_config_set_class(proxy->pci_dev.config, PCI_CLASS_STORAGE_SCSI);
> +    break;
> +    default:
> +        error_report("unknown device id\n");
> +    break;
> +
> +    }
> +
> +    /* virtio_init_pci code without bindings */
> +
> +    /* This should disappear */
> +    proxy->vdev = proxy->bus->vdev;
> +
> +    config = proxy->pci_dev.config;
> +
> +    if (proxy->class_code) {
> +        pci_config_set_class(config, proxy->class_code);
> +    }
> +    pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
> +                 pci_get_word(config + PCI_VENDOR_ID));
> +    pci_set_word(config + PCI_SUBSYSTEM_ID, proxy->bus->vdev->device_id);
> +    config[PCI_INTERRUPT_PIN] = 1;
> +
> +    if (proxy->bus->vdev->nvectors &&
> +        msix_init_exclusive_bar(&proxy->pci_dev, proxy->bus->vdev->nvectors,
> +                                1)) {
> +        proxy->bus->vdev->nvectors = 0;
> +    }
> +
> +    proxy->pci_dev.config_write = virtio_write_config;
> +
> +    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> +         + proxy->bus->vdev->config_len;
> +    if (size & (size-1)) {
> +        size = 1 << qemu_fls(size);
> +    }
> +
> +    memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy,
> +                          "virtio-pci", size);
> +    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
> +                     &proxy->bar);
> +
> +    if (!kvm_has_many_ioeventfds()) {
> +        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> +    }
> +
> +    /* Bind the VirtIODevice to the VirtioBus. */
> +    virtio_bus_bind_device(proxy->bus);
> +
> +    proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> +    proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> +    /* Should be modified */
> +    proxy->host_features = proxy->bus->vdev->get_features(proxy->bus->vdev,
> +                                                         proxy->host_features);
> +}
> +
> +void virtio_pci_exit_cb(DeviceState *dev)
> +{
> +    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
> +    /* Put the PCI IDs */
> +    pci_config_set_device_id(proxy->pci_dev.config, 0x0000);
> +    pci_config_set_class(proxy->pci_dev.config, PCI_CLASS_OTHERS);
> +
> +    virtio_pci_stop_ioeventfd(proxy);
> +}
> +
> +static const struct VirtioBusInfo virtio_bus_info = {
> +    .init_cb = virtio_pci_init_cb,
> +    .exit_cb = virtio_pci_exit_cb,
> +
> +    .virtio_bindings = {
> +        .notify = virtio_pci_notify,
> +        .save_config = virtio_pci_save_config,
> +        .load_config = virtio_pci_load_config,
> +        .save_queue = virtio_pci_save_queue,
> +        .load_queue = virtio_pci_load_queue,
> +        .get_features = virtio_pci_get_features,
> +        .query_guest_notifiers = virtio_pci_query_guest_notifiers,
> +        .set_host_notifier = virtio_pci_set_host_notifier,
> +        .set_guest_notifiers = virtio_pci_set_guest_notifiers,
> +        .vmstate_change = virtio_pci_vmstate_change,
> +    }
> +};

So this looks wrong.

The interactions should roughly look like:

(1) VirtioDevice only interacts with VirtioBus through it's class
    methods.  'notify' is a method.

(2) Since VirtioBus is a descrete object, it needs to interact with
    whatever the actual bus is.  There are two ways to do this:

    (a) Each bus-type can sub-class VirtioBus and implement each method
        using a private interface.  This is probably the easiest
        approach because you can just give VirtioPCIBus a pointer to the
        PCIDevice, and then implement the methods within VirtioPCIBus.

    (b) There can be only one VirtioBus object type with a well defined
        proxy interface.  This should basically mirror the VirtioBus
        class methods.  VirtioPCI can then implement this proxy
        interface.

Seeing how the code is turning out, I think 2.a would require the least
amount of coding.

> +
> +static int virtiopci_qdev_init(PCIDevice *dev)
> +{
> +    VirtIOPCIProxy *s = DO_UPCAST(VirtIOPCIProxy, pci_dev, dev);
> +    DeviceState *qdev = DEVICE(dev);
> +
> +    /* create a virtio-bus and attach virtio-pci to it */
> +    s->bus = virtio_bus_new(qdev, &virtio_bus_info);

You basically end up with virtio_pci_bus_new() here and you pass 'dev'
to it.  VirtioPCIBus is a 'friend' to VirtioPCI so it can access it's
private data.

> +
> +    return 0;
> +}
> +

Regards,

Anthony Liguori

> +static Property virtiopci_properties[] = {
> +    /* TODO : Add the correct properties */
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virtiopci_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
> +
> +    pc->init = virtiopci_qdev_init;
> +    pc->exit = virtio_exit_pci;
> +    pc->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> +    pc->revision = VIRTIO_PCI_ABI_VERSION;
> +    pc->class_id = PCI_CLASS_OTHERS;
> +    /* TODO : Add the correct device information below */
> +    /* pc->exit =
> +     * pc->device_id =
> +     * pc->subsystem_vendor_id =
> +     * pc->subsystem_id =
> +     * dc->reset =
> +     * dc->vmsd =
> +     */
> +    dc->props = virtiopci_properties;
> +    dc->desc = "virtio-pci transport.";
> +}
> +
> +static const TypeInfo virtio_pci_info = {
> +    .name  = "virtio-pci",
> +    .parent = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(VirtIOPCIProxy),
> +    .class_init = virtiopci_class_init,
> +};
> +
> +
> +/************************************/
> +
>  
>  static void virtio_pci_register_types(void)
>  {
> +    /* This should disappear */
>      type_register_static(&virtio_blk_info);
>      type_register_static(&virtio_net_info);
>      type_register_static(&virtio_serial_info);
>      type_register_static(&virtio_balloon_info);
>      type_register_static(&virtio_scsi_info);
>      type_register_static(&virtio_rng_info);
> +    /* new virtio-pci device */
> +    type_register_static(&virtio_pci_info);
>  }
>  
>  type_init(virtio_pci_register_types)
> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
> index b58d9a2..a7a9847 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -20,6 +20,7 @@
>  #include "virtio-rng.h"
>  #include "virtio-serial.h"
>  #include "virtio-scsi.h"
> +#include "virtio-bus.h"
>  
>  /* Performance improves when virtqueue kick processing is decoupled from the
>   * vcpu thread using ioeventfd for some devices. */
> @@ -33,6 +34,7 @@ typedef struct {
>  
>  typedef struct {
>      PCIDevice pci_dev;
> +    /* This should be removed */
>      VirtIODevice *vdev;
>      MemoryRegion bar;
>      uint32_t flags;
> @@ -51,10 +53,14 @@ typedef struct {
>      bool ioeventfd_disabled;
>      bool ioeventfd_started;
>      VirtIOIRQFD *vector_irqfd;
> +    /* VirtIOBus to connect the VirtIODevice */
> +    VirtioBus *bus;
>  } VirtIOPCIProxy;
>  
>  void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
>  void virtio_pci_reset(DeviceState *d);
> +void virtio_pci_init_cb(DeviceState *dev);
> +void virtio_pci_exit_cb(DeviceState *dev);
>  
>  /* Virtio ABI version, if we increment this, we break the guest driver. */
>  #define VIRTIO_PCI_ABI_VERSION          0
> -- 
> 1.7.11.7