Re: [Qemu-devel] [RFC PATCH v2 2/3] virtio-pci : add a virtio-bus interface

[Anthony Liguori]

address@hidden writes:

> From: KONRAD Frederic <address@hidden>
>
> 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 <address@hidden>
> ---
>  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