Re: [Qemu-devel] [RfC PATCH 05/15] virtio-gpu/2d: add hardware spec include file

[Max Reitz <mreitz@redhat.com>]

On 2015-02-23 at 05:23, Gerd Hoffmann wrote:
> This patch adds the header file with structs and defines for
> the virtio based gpu device.  Covers 2d operations only.
>
> Written by Dave Airlie and Gerd Hoffmann.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   include/hw/virtio/virtgpu_hw.h | 203 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 203 insertions(+)
>   create mode 100644 include/hw/virtio/virtgpu_hw.h
>
> diff --git a/include/hw/virtio/virtgpu_hw.h b/include/hw/virtio/virtgpu_hw.h
> new file mode 100644
> index 0000000..d6641e8
> --- /dev/null
> +++ b/include/hw/virtio/virtgpu_hw.h
> @@ -0,0 +1,203 @@
> +/*
> + * Virtio GPU Device
> + *
> + * Copyright Red Hat, Inc. 2013-2014

Hm... Well, if there haven't been any changes since 2014...

> + *
> + * Authors:
> + *     Dave Airlie <airlied@redhat.com>
> + *     Gerd Hoffmann <kraxel@redhat.com>
> + *
> + * This header is BSD licensed so anyone can use the definitions
> + * to implement compatible drivers/servers:
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors

Is the "IBM" intended?

> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#ifndef VIRTIO_GPU_HW_H
> +#define VIRTIO_GPU_HW_H
> +
> +enum virtio_gpu_ctrl_type {
> +    VIRTIO_GPU_UNDEFINED = 0,
> +
> +    /* 2d commands */
> +    VIRTIO_GPU_CMD_GET_DISPLAY_INFO = 0x0100,
> +    VIRTIO_GPU_CMD_RESOURCE_CREATE_2D,
> +    VIRTIO_GPU_CMD_RESOURCE_UNREF,
> +    VIRTIO_GPU_CMD_SET_SCANOUT,
> +    VIRTIO_GPU_CMD_RESOURCE_FLUSH,
> +    VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D,
> +    VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING,
> +    VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING,
> +
> +    /* cursor commands */
> +    VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
> +    VIRTIO_GPU_CMD_MOVE_CURSOR,
> +
> +    /* success responses */
> +    VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
> +    VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
> +
> +    /* error responses */
> +    VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
> +    VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY,
> +    VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID,
> +    VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
> +    VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
> +    VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
> +};
> +
> +#define VIRTIO_GPU_FLAG_FENCE (1 << 0)
> +
> +struct virtio_gpu_ctrl_hdr {
> +    uint32_t type;
> +    uint32_t flags;
> +    uint64_t fence_id;
> +    uint32_t ctx_id;
> +    uint32_t padding;
> +};

Shouldn't all these structures be QEMU_PACKED? In practice, it won't 
matter (I hope!), but if you're relying on it...

Usage of QEMU_PACKED doesn't seem to widespread in include/hw/virtio, so 
maybe it's fine the way it is.

(sorry I don't have any really useful comments, I'm less reviewing and 
more just reading into this series (because I don't have the knowledge 
to really review most of its patches))

Max