From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tuqez-0006QK-Bc for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:29:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tuqer-0006wS-J9 for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:29:33 -0500 Received: from e9.ny.us.ibm.com ([32.97.182.139]:54595) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tuqer-0006wI-EN for qemu-devel@nongnu.org; Mon, 14 Jan 2013 15:29:25 -0500 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 14 Jan 2013 15:29:24 -0500 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 0EB15C90026 for ; Mon, 14 Jan 2013 15:29:22 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0EKTLLm270116 for ; Mon, 14 Jan 2013 15:29:21 -0500 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r0EKTLtq021846 for ; Mon, 14 Jan 2013 15:29:21 -0500 From: Anthony Liguori In-Reply-To: <50F4651B.3010701@greensocs.com> 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> <50F4651B.3010701@greensocs.com> Date: Mon, 14 Jan 2013 14:29:14 -0600 Message-ID: <87a9sbfqzp.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: KONRAD =?utf-8?B?RnLDqWTDqXJpYw==?= 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 KONRAD Fr=C3=A9d=C3=A9ric writes: > 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 e= xtend >>> 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; >>> } >>>=20=20=20 >>> +/* Get the features of the plugged device. */ >>> +uint32_t virtio_device_get_features(VirtioBusState *bus, >>> + uint32_t requested_features) >>> +{ >>> + VirtioDeviceClass *k; >>> + assert(bus->vdev !=3D NULL); >>> + k =3D VIRTIO_DEVICE_GET_CLASS(bus->vdev); >>> + assert(k->get_features !=3D 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 !=3D NULL); >>> + k =3D VIRTIO_DEVICE_GET_CLASS(bus->vdev); >>> + if (k->bad_features !=3D 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 !=3D NULL); >>> + k =3D VIRTIO_DEVICE_GET_CLASS(bus->vdev); >>> + if (k->get_config !=3D 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. Can you refer me to the mail? I searched and I can't find it... Regards, Anthony Liguori > > But I can remove this function and use=20 > virtio_device(virtio_bus_get_vdev(...)) >> >>> static const TypeInfo virtio_bus_info =3D { >>> .name =3D TYPE_VIRTIO_BUS, >>> .parent =3D 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); >>>=20=20=20 >>> #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" >>>=20=20=20 >>> /* 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; >>> } >>>=20=20=20 >>> -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); >>> } >>>=20=20=20 >>> @@ -902,14 +908,10 @@ static void virtio_vmstate_change(void *opaque, i= nt running, RunState state) >>> } >>> } >>>=20=20=20 >>> -VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >>> - size_t config_size, size_t struct_siz= e) >>> +void virtio_init(VirtIODevice *vdev, const char *name, >>> + uint16_t device_id, size_t config_size) >>> { >>> - VirtIODevice *vdev; >>> int i; >>> - >>> - vdev =3D g_malloc0(struct_size); >>> - >>> vdev->device_id =3D device_id; >>> vdev->status =3D 0; >>> vdev->isr =3D 0; >>> @@ -917,20 +919,28 @@ VirtIODevice *virtio_common_init(const char *name= , uint16_t device_id, >>> vdev->config_vector =3D VIRTIO_NO_VECTOR; >>> vdev->vq =3D g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); >>> vdev->vm_running =3D runstate_is_running(); >>> - for(i =3D 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { >>> + for (i =3D 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { >>> vdev->vq[i].vector =3D VIRTIO_NO_VECTOR; >>> vdev->vq[i].vdev =3D vdev; >>> } >>>=20=20=20 >>> vdev->name =3D name; >>> vdev->config_len =3D config_size; >>> - if (vdev->config_len) >>> + if (vdev->config_len) { >>> vdev->config =3D g_malloc0(config_size); >>> - else >>> + } else { >>> vdev->config =3D NULL; >>> + } >>> + vdev->vmstate =3D qemu_add_vm_change_state_handler(virtio_vmstate_= change, >>> + vdev); >>> +} >>>=20=20=20 >>> - vdev->vmstate =3D 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_siz= e) >>> +{ >>> + VirtIODevice *vdev; >>> + vdev =3D g_malloc0(struct_size); >>> + virtio_init(vdev, name, device_id, config_size); >>> return vdev; >>> } >>>=20=20=20 >>> @@ -1056,3 +1066,39 @@ EventNotifier *virtio_queue_get_host_notifier(Vi= rtQueue *vq) >>> { >>> return &vq->host_notifier; >>> } >>> + >>> +static int virtio_device_init(DeviceState *qdev) >>> +{ >>> + VirtIODevice *vdev =3D VIRTIO_DEVICE(qdev); >>> + VirtioDeviceClass *k =3D VIRTIO_DEVICE_GET_CLASS(qdev); >>> + assert(k->init !=3D 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 =3D DEVICE_CLASS(klass); >>> + dc->init =3D virtio_device_init; >>> + dc->bus_type =3D TYPE_VIRTIO_BUS; >>> +} >>> + >>> +static const TypeInfo virtio_device_info =3D { >>> + .name =3D TYPE_VIRTIO_DEVICE, >>> + .parent =3D TYPE_DEVICE, >>> + .instance_size =3D sizeof(VirtIODevice), >>> + .class_init =3D virtio_device_class_init, >>> + .abstract =3D true, >>> + .class_size =3D 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 { >>>=20=20=20 >>> #define VIRTIO_NO_VECTOR 0xffff >>>=20=20=20 >>> +#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_DEVIC= E) >>> +#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 t= hey are in >>> + * VirtioDeviceClass. >>> + */ >>> uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_f= eatures); >>> uint32_t (*bad_features)(VirtIODevice *vdev); >>> void (*set_features)(VirtIODevice *vdev, uint32_t val); >>> @@ -134,6 +147,23 @@ struct VirtIODevice >>> VMChangeStateEntry *vmstate; >>> }; >>>=20=20=20 >>> +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_fe= atures); >>> + 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 *)); >>> --=20 >>> 1.7.11.7