From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TuqIC-0005Xa-M9 for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:06:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TuqIA-0001Ka-Rz for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:06:00 -0500 Received: from greensocs.com ([87.106.252.221]:54190 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TuqIA-0001Jf-ES for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:05:58 -0500 Message-ID: <50F4651B.3010701@greensocs.com> Date: Mon, 14 Jan 2013 21:05:47 +0100 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 References: <1357747019-20580-1-git-send-email-fred.konrad@greensocs.com> <1357747019-20580-4-git-send-email-fred.konrad@greensocs.com> <87ehhnwpmb.fsf@codemonkey.ws> In-Reply-To: <87ehhnwpmb.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 3/7] virtio-device: refactor virtio-device. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: kwolf@redhat.com, peter.maydell@linaro.org, e.voevodin@samsung.com, mst@redhat.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, agraf@suse.de, cornelia.huck@de.ibm.com, aneesh.kumar@linux.vnet.ibm.com, stefanha@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, afaerber@suse.de On 14/01/2013 20:06, Anthony Liguori wrote: > fred.konrad@greensocs.com writes: > >> From: KONRAD Frederic >> >> Create the virtio-device which is abstract. All the virtio-device can extend >> this class. It also add some functions to virtio-bus. >> >> Signed-off-by: KONRAD Frederic >> --- >> hw/virtio-bus.c | 35 +++++++++++++++++++++++++++++ >> hw/virtio-bus.h | 7 ++++++ >> hw/virtio.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++---------- >> hw/virtio.h | 30 +++++++++++++++++++++++++ >> 4 files changed, 130 insertions(+), 12 deletions(-) >> >> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c >> index 7813a89..ec43185 100644 >> --- a/hw/virtio-bus.c >> +++ b/hw/virtio-bus.c >> @@ -127,6 +127,41 @@ size_t virtio_device_get_config_len(VirtioBusState *bus) >> return bus->vdev->config_len; >> } >> >> +/* Get the features of the plugged device. */ >> +uint32_t virtio_device_get_features(VirtioBusState *bus, >> + uint32_t requested_features) >> +{ >> + VirtioDeviceClass *k; >> + assert(bus->vdev != NULL); >> + k = VIRTIO_DEVICE_GET_CLASS(bus->vdev); >> + assert(k->get_features != NULL); >> + return k->get_features(bus->vdev, requested_features); >> +} >> + >> +/* Get bad features of the plugged device. */ >> +uint32_t virtio_device_get_bad_features(VirtioBusState *bus) >> +{ >> + VirtioDeviceClass *k; >> + assert(bus->vdev != NULL); >> + k = VIRTIO_DEVICE_GET_CLASS(bus->vdev); >> + if (k->bad_features != NULL) { >> + return k->bad_features(bus->vdev); >> + } else { >> + return 0; >> + } >> +} >> + >> +/* Get config of the plugged device. */ >> +void virtio_device_get_config(VirtioBusState *bus, uint8_t *config) >> +{ >> + VirtioDeviceClass *k; >> + assert(bus->vdev != NULL); >> + k = VIRTIO_DEVICE_GET_CLASS(bus->vdev); >> + if (k->get_config != NULL) { >> + k->get_config(bus->vdev, config); >> + } >> +} >> + > If the prefix is "virtio_device" the first argument should be a > VirtIODevice not a virtio_bus. > > You probably should introduce a virtio_bus_get_vdev() and then call > virtio_device(virtio_bus_get_vdev(bus), ...) > > Or alternatively, rename these to: > > virtio_bus_get_vdev_* You suggest prefix this function with virtio_device last time. But I can remove this function and use virtio_device(virtio_bus_get_vdev(...)) > >> static const TypeInfo virtio_bus_info = { >> .name = TYPE_VIRTIO_BUS, >> .parent = TYPE_BUS, >> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h >> index 8aa71b2..7bea64a 100644 >> --- a/hw/virtio-bus.h >> +++ b/hw/virtio-bus.h >> @@ -87,5 +87,12 @@ int virtio_device_get_nvectors(VirtioBusState *bus); >> void virtio_device_set_nvectors(VirtioBusState *bus, int nvectors); >> /* Get the config_len field of the plugged device. */ >> size_t virtio_device_get_config_len(VirtioBusState *bus); >> +/* Get the features of the plugged device. */ >> +uint32_t virtio_device_get_features(VirtioBusState *bus, >> + uint32_t requested_features); >> +/* Get bad features of the plugged device. */ >> +uint32_t virtio_device_get_bad_features(VirtioBusState *bus); >> +/* Get config of the plugged device. */ >> +void virtio_device_get_config(VirtioBusState *bus, uint8_t *config); >> >> #endif /* VIRTIO_BUS_H */ >> diff --git a/hw/virtio.c b/hw/virtio.c >> index 77b53a9..ca170c3 100644 >> --- a/hw/virtio.c >> +++ b/hw/virtio.c >> @@ -17,6 +17,7 @@ >> #include "qemu/error-report.h" >> #include "virtio.h" >> #include "qemu/atomic.h" >> +#include "virtio-bus.h" >> >> /* The alignment to use between consumer and producer parts of vring. >> * x86 pagesize again. */ >> @@ -875,11 +876,16 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) >> return 0; >> } >> >> -void virtio_cleanup(VirtIODevice *vdev) >> +void virtio_common_cleanup(VirtIODevice *vdev) >> { >> qemu_del_vm_change_state_handler(vdev->vmstate); >> g_free(vdev->config); >> g_free(vdev->vq); >> +} >> + >> +void virtio_cleanup(VirtIODevice *vdev) >> +{ >> + virtio_common_cleanup(vdev); >> g_free(vdev); >> } >> >> @@ -902,14 +908,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state) >> } >> } >> >> -VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >> - size_t config_size, size_t struct_size) >> +void virtio_init(VirtIODevice *vdev, const char *name, >> + uint16_t device_id, size_t config_size) >> { >> - VirtIODevice *vdev; >> int i; >> - >> - vdev = g_malloc0(struct_size); >> - >> vdev->device_id = device_id; >> vdev->status = 0; >> vdev->isr = 0; >> @@ -917,20 +919,28 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >> vdev->config_vector = VIRTIO_NO_VECTOR; >> vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); >> vdev->vm_running = runstate_is_running(); >> - for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { >> + for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { >> vdev->vq[i].vector = VIRTIO_NO_VECTOR; >> vdev->vq[i].vdev = vdev; >> } >> >> vdev->name = name; >> vdev->config_len = config_size; >> - if (vdev->config_len) >> + if (vdev->config_len) { >> vdev->config = g_malloc0(config_size); >> - else >> + } else { >> vdev->config = NULL; >> + } >> + vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change, >> + vdev); >> +} >> >> - vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change, vdev); >> - >> +VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >> + size_t config_size, size_t struct_size) >> +{ >> + VirtIODevice *vdev; >> + vdev = g_malloc0(struct_size); >> + virtio_init(vdev, name, device_id, config_size); >> return vdev; >> } >> >> @@ -1056,3 +1066,39 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq) >> { >> return &vq->host_notifier; >> } >> + >> +static int virtio_device_init(DeviceState *qdev) >> +{ >> + VirtIODevice *vdev = VIRTIO_DEVICE(qdev); >> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev); >> + assert(k->init != NULL); >> + if (k->init(vdev) < 0) { >> + return -1; >> + } >> + virtio_bus_plug_device(vdev); >> + return 0; >> +} >> + >> +static void virtio_device_class_init(ObjectClass *klass, void *data) >> +{ >> + /* Set the default value here. */ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + dc->init = virtio_device_init; >> + dc->bus_type = TYPE_VIRTIO_BUS; >> +} >> + >> +static const TypeInfo virtio_device_info = { >> + .name = TYPE_VIRTIO_DEVICE, >> + .parent = TYPE_DEVICE, >> + .instance_size = sizeof(VirtIODevice), >> + .class_init = virtio_device_class_init, >> + .abstract = true, >> + .class_size = sizeof(VirtioDeviceClass), >> +}; >> + >> +static void virtio_register_types(void) >> +{ >> + type_register_static(&virtio_device_info); >> +} >> + >> +type_init(virtio_register_types) >> diff --git a/hw/virtio.h b/hw/virtio.h >> index 1dec9dc..a321fb2 100644 >> --- a/hw/virtio.h >> +++ b/hw/virtio.h >> @@ -108,8 +108,17 @@ typedef struct { >> >> #define VIRTIO_NO_VECTOR 0xffff >> >> +#define TYPE_VIRTIO_DEVICE "virtio-device" >> +#define VIRTIO_DEVICE_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(VirtioDeviceClass, obj, TYPE_VIRTIO_DEVICE) >> +#define VIRTIO_DEVICE_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(VirtioDeviceClass, klass, TYPE_VIRTIO_DEVICE) >> +#define VIRTIO_DEVICE(obj) \ >> + OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE) >> + >> struct VirtIODevice >> { >> + DeviceState parent_obj; >> const char *name; >> uint8_t status; >> uint8_t isr; >> @@ -119,6 +128,10 @@ struct VirtIODevice >> void *config; >> uint16_t config_vector; >> int nvectors; >> + /* >> + * Function pointers will be removed at the end of the series as they are in >> + * VirtioDeviceClass. >> + */ >> uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features); >> uint32_t (*bad_features)(VirtIODevice *vdev); >> void (*set_features)(VirtIODevice *vdev, uint32_t val); >> @@ -134,6 +147,23 @@ struct VirtIODevice >> VMChangeStateEntry *vmstate; >> }; >> >> +typedef struct { > It's good form to not make this an anonymous struct. > > Regards, > > Anthony Liguori > >> + /* This is what a VirtioDevice must implement */ >> + DeviceClass parent; >> + int (*init)(VirtIODevice *vdev); >> + uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features); >> + uint32_t (*bad_features)(VirtIODevice *vdev); >> + void (*set_features)(VirtIODevice *vdev, uint32_t val); >> + void (*get_config)(VirtIODevice *vdev, uint8_t *config); >> + void (*set_config)(VirtIODevice *vdev, const uint8_t *config); >> + void (*reset)(VirtIODevice *vdev); >> + void (*set_status)(VirtIODevice *vdev, uint8_t val); >> +} VirtioDeviceClass; >> + >> +void virtio_init(VirtIODevice *vdev, const char *name, >> + uint16_t device_id, size_t config_size); >> +void virtio_common_cleanup(VirtIODevice *vdev); >> + >> VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, >> void (*handle_output)(VirtIODevice *, >> VirtQueue *)); >> -- >> 1.7.11.7