Re: [Qemu-devel] [PATCH 4/9] virtio-9p: remove PCI dependencies from hw/9pfs/

[Aneesh Kumar K.V]

On Thu, 24 Nov 2011 13:38:23 +0100, Paolo Bonzini <address@hidden> wrote:
> Also move the 9p.h file to 9pfs/virtio-9p-device.h, for consistency
> with the corresponding .c file.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/9pfs/virtio-9p-device.c           |   35 
> +---------------------------------
>  hw/{9p.h => 9pfs/virtio-9p-device.h} |    4 +-
>  hw/9pfs/virtio-9p.c                  |    1 -
>  hw/virtio-pci.c                      |   35 
> ++++++++++++++++++++++++++++++++++
>  hw/virtio-pci.h                      |    1 +
>  hw/virtio.h                          |    2 +-
>  6 files changed, 40 insertions(+), 38 deletions(-)
>  rename hw/{9p.h => 9pfs/virtio-9p-device.h} (85%)
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index bba4c54..509faa8 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -14,9 +14,9 @@
>  #include "hw/virtio.h"
>  #include "hw/pc.h"
>  #include "qemu_socket.h"
> -#include "hw/virtio-pci.h"
>  #include "virtio-9p.h"
>  #include "fsdev/qemu-fsdev.h"
> +#include "virtio-9p-device.h"
>  #include "virtio-9p-xattr.h"
>  #include "virtio-9p-coth.h"
> 
> @@ -147,41 +147,8 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf 
> *conf)
>      return &s->vdev;
>  }
> 
> -static int virtio_9p_init_pci(PCIDevice *pci_dev)
> -{
> -    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> -    VirtIODevice *vdev;
> -
> -    vdev = virtio_9p_init(&pci_dev->qdev, &proxy->fsconf);
> -    vdev->nvectors = proxy->nvectors;
> -    virtio_init_pci(proxy, vdev);
> -    /* make the actual value visible */
> -    proxy->nvectors = vdev->nvectors;
> -    return 0;
> -}
> -
> -static PCIDeviceInfo virtio_9p_info = {
> -    .qdev.name = "virtio-9p-pci",
> -    .qdev.size = sizeof(VirtIOPCIProxy),
> -    .init      = virtio_9p_init_pci,
> -    .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> -    .device_id = 0x1009,
> -    .revision  = VIRTIO_PCI_ABI_VERSION,
> -    .class_id  = 0x2,
> -    .qdev.props = (Property[]) {
> -        DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> -                        VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> -        DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> -        DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> -        DEFINE_PROP_STRING("mount_tag", VirtIOPCIProxy, fsconf.tag),
> -        DEFINE_PROP_STRING("fsdev", VirtIOPCIProxy, fsconf.fsdev_id),
> -        DEFINE_PROP_END_OF_LIST(),
> -    }
> -};
> -
>  static void virtio_9p_register_devices(void)
>  {
> -    pci_qdev_register(&virtio_9p_info);
>      virtio_9p_set_fd_limit();
>  }
> 
> diff --git a/hw/9p.h b/hw/9pfs/virtio-9p-device.h
> similarity index 85%
> rename from hw/9p.h
> rename to hw/9pfs/virtio-9p-device.h
> index d9951d6..65789db 100644
> --- a/hw/9p.h
> +++ b/hw/9pfs/virtio-9p-device.h
> @@ -11,8 +11,8 @@
>   *
>   */
> 
> -#ifndef QEMU_9P_H
> -#define QEMU_9P_H
> +#ifndef QEMU_VIRTIO_9P_DEVICE_H
> +#define QEMU_VIRTIO_9P_DEVICE_H
> 
>  typedef struct V9fsConf
>  {
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 1b2fc5d..7b786e5 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -17,7 +17,6 @@
>  #include "hw/virtio.h"
>  #include "hw/pc.h"
>  #include "qemu_socket.h"
> -#include "hw/virtio-pci.h"
>  #include "virtio-9p.h"
>  #include "fsdev/qemu-fsdev.h"
>  #include "virtio-9p-xattr.h"
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index d5cdc04..78c1790 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -779,6 +779,21 @@ static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
>      return virtio_exit_pci(pci_dev);
>  }
> 
> +#ifdef CONFIG_LINUX
> +static int virtio_9p_init_pci(PCIDevice *pci_dev)
> +{
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> +    VirtIODevice *vdev;
> +
> +    vdev = virtio_9p_init(&pci_dev->qdev, &proxy->fsconf);
> +    vdev->nvectors = proxy->nvectors;
> +    virtio_init_pci(proxy, vdev);
> +    /* make the actual value visible */
> +    proxy->nvectors = vdev->nvectors;
> +    return 0;
> +}
> +#endif
> +
>  static PCIDeviceInfo virtio_info[] = {
>      {
>          .qdev.name = "virtio-blk-pci",
> @@ -855,6 +870,26 @@ static PCIDeviceInfo virtio_info[] = {
>              DEFINE_PROP_END_OF_LIST(),
>          },
>          .qdev.reset = virtio_pci_reset,
> +#ifdef CONFIG_LINUX
> +    }, {
> +        .qdev.name = "virtio-9p-pci",
> +        .qdev.size = sizeof(VirtIOPCIProxy),
> +        .init      = virtio_9p_init_pci,
> +        .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> +        .device_id = 0x1009,
> +        .revision  = VIRTIO_PCI_ABI_VERSION,
> +        .class_id  = 0x2,
> +        .qdev.props = (Property[]) {
> +            DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> +                            VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> +            DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> +            DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> +            DEFINE_PROP_STRING("mount_tag", VirtIOPCIProxy, fsconf.tag),
> +            DEFINE_PROP_STRING("fsdev", VirtIOPCIProxy, fsconf.fsdev_id),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
> +        .qdev.reset = virtio_pci_reset,
> +#endif
>      },{
>          /* end of list */
>      }
> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
> index 4126705..7654c84 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -17,6 +17,7 @@
> 
>  #include "virtio-net.h"
>  #include "virtio-serial.h"
> +#include "9pfs/virtio-9p-device.h"
> 
>  /* Performance improves when virtqueue kick processing is decoupled from the
>   * vcpu thread using ioeventfd for some devices. */
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 7303b6c..9ed8dfc 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -21,7 +21,7 @@
>  #include "block.h"
>  #include "event_notifier.h"
>  #ifdef CONFIG_LINUX
> -#include "9p.h"
> +#include "9pfs/virtio-9p-device.h"
>  #endif
> 
>  /* from Linux's linux/virtio_config.h */

That adds few #ifdef to virtio-pci.c. Is that ok ? I actually moved
that code from virtio-pci.c as a part of
9fe1ebebd0c427fd9b3d1ffffa77a9969d873d93 . Regarding PCI dependency,
I was thinking it would be better for 9p to explicitly state that
dependency. When virtio can support other than pci devices, we would
list then using #ifdef in virtio-9p-device.c That way all 9p device
related details can be found at one place. That is one of the reason
i did 9fe1ebebd0c427fd9b3d1ffffa77a9969d873d93

Part of the above patch would still be needed if we don't want to
apply the full patch above. I have inlined it below.

commit 2cb7289031b2646d6a107610967e90d2320791fa
Author: Aneesh Kumar K.V <address@hidden>
Date:   Mon Nov 28 12:23:49 2011 +0530

    hw/9pfs: Add qdev.reset callback for virtio-9p-pci device
    
    Add the device reset callback
    
    Signed-off-by: Aneesh Kumar K.V <address@hidden>

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index bba4c54..57dc77c 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -176,7 +176,8 @@ static PCIDeviceInfo virtio_9p_info = {
         DEFINE_PROP_STRING("mount_tag", VirtIOPCIProxy, fsconf.tag),
         DEFINE_PROP_STRING("fsdev", VirtIOPCIProxy, fsconf.fsdev_id),
         DEFINE_PROP_END_OF_LIST(),
-    }
+    },
+    .qdev.reset = virtio_pci_reset,
 };
 
 static void virtio_9p_register_devices(void)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index ca5923c..07f5861 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -266,7 +266,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
     proxy->ioeventfd_started = false;
 }
 
-static void virtio_pci_reset(DeviceState *d)
+void virtio_pci_reset(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     virtio_pci_stop_ioeventfd(proxy);
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index f8404de..344c22b 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -45,6 +45,7 @@ typedef struct {
 } VirtIOPCIProxy;
 
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
+void virtio_pci_reset(DeviceState *d);
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION          0