Linux Virtualization - Sep 2014 - [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

On 09/11/2014 09:09 AM, Gerd Hoffmann wrote:
> This patch adds the header file with structs and defines for
> the virtio based gpu device.  Covers 2d operations only.
> 
> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
>  include/hw/virtio/virtgpu_hw.h | 158
+++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 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..461f452
> --- /dev/null
> +++ b/include/hw/virtio/virtgpu_hw.h
> @@ -0,0 +1,158 @@
> +#ifndef VIRTGPU_HW_H
> +#define VIRTGPU_HW_H
Non-trivial file, deserves a copyright and license notice.
> +
> +enum virtgpu_ctrl_type {
> +        VIRTGPU_UNDEFINED = 0,
> +
> +        /* 2d commands */
> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
Please consider also adding:

#define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO

and friends.  It makes it MUCH nicer for application software to probe
for later extensions if every member of the enum is also associated with
a preprocessor macro.

> +
> +struct virtgpu_ctrl_hdr {
> +        uint32_t type;
> +        uint32_t flags;
> +        uint64_t fence_id;
> +        uint32_t ctx_id;
> +        uint32_t padding;
> +};
> +
Is the padding to ensure that this is aligned regardless of 32-bit or
64-bit hosts?  Is it worth adding a compile-time assertion about the
size of the struct to ensure the compiler doesn't add any additional
padding?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20140911/30f48138/attachment.sig>

Hi,
> > @@ -0,0 +1,158 @@
> > +#ifndef VIRTGPU_HW_H
> > +#define VIRTGPU_HW_H
> 
> Non-trivial file, deserves a copyright and license notice.
Added.
> > +
> > +enum virtgpu_ctrl_type {
> > +        VIRTGPU_UNDEFINED = 0,
> > +
> > +        /* 2d commands */
> > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> 
> Please consider also adding:
> 
> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
> 
> and friends.  It makes it MUCH nicer for application software to probe
> for later extensions if every member of the enum is also associated with
> a preprocessor macro.
I don't think this will ever be shipped as library header for external
users ...
> > +struct virtgpu_ctrl_hdr {
> > +        uint32_t type;
> > +        uint32_t flags;
> > +        uint64_t fence_id;
> > +        uint32_t ctx_id;
> > +        uint32_t padding;
> > +};
> > +
> 
> Is the padding to ensure that this is aligned regardless of 32-bit or
> 64-bit hosts?
Yes.
> Is it worth adding a compile-time assertion about the
> size of the struct to ensure the compiler doesn't add any additional
> padding?
Makes sense.  What is the usual trick to do that?

thanks,
  Gerd

On 09/12/2014 04:44 AM, Gerd Hoffmann wrote:
>>> +enum virtgpu_ctrl_type {
>>> +        VIRTGPU_UNDEFINED = 0,
>>> +
>>> +        /* 2d commands */
>>> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
>>
>> Please consider also adding:
>>
>> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
>>
>> and friends.  It makes it MUCH nicer for application software to probe
>> for later extensions if every member of the enum is also associated
with
>> a preprocessor macro.
> 
> I don't think this will ever be shipped as library header for external
> users ...
Fair enough. I wasn't sure, so it didn't hurt to ask.
> 
>>> +struct virtgpu_ctrl_hdr {
>>> +        uint32_t type;
>>> +        uint32_t flags;
>>> +        uint64_t fence_id;
>>> +        uint32_t ctx_id;
>>> +        uint32_t padding;
>>> +};
>>> +
>>
>> Is the padding to ensure that this is aligned regardless of 32-bit or
>> 64-bit hosts?
> 
> Yes.
> 
>> Is it worth adding a compile-time assertion about the
>> size of the struct to ensure the compiler doesn't add any
additional
>> padding?
> 
> Makes sense.  What is the usual trick to do that?
Among others,

struct dummy {
  int size_check : (sizeof(virtgpu_ctrl_hdr) == 24 ? 1 : -1);
};

Since bitfields cannot have a negative size, the compiler will
forcefully fail compilation if the struct size ever changes.  Similar
tricks include setting up array bounds that would be negative on
failure, or (inside a function body) declaring a switch that has
colliding case statements on failure.  But depending on the set of
compiler warning options in effect, declaring an otherwise unused struct
may be too noisy.  So if you need more ideas and a good read, check out
the comments in how gnulib does it (the link mentions GPLv3+, but that
file is also shipped as LGPLv2+ so it is compatible with qemu):

git.savannah.gnu.org/cgit/gnulib.git/tree/lib/verify.h

with an end result that a user just writes:

verify(sizeof(virtgpu_ctrl_hdr) == 24);

and calls it a day.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20140912/097c9a0f/attachment.sig>

On Fri, Sep 12, 2014 at 12:44:56PM +0200, Gerd Hoffmann
wrote:
>   Hi,
> 
> > > @@ -0,0 +1,158 @@
> > > +#ifndef VIRTGPU_HW_H
> > > +#define VIRTGPU_HW_H
> > 
> > Non-trivial file, deserves a copyright and license notice.
> 
> Added.
Pls remember to make it consistent with other virtio headers,
which are 3-clause BSD, prefixed with this reminder:

 This header is BSD licensed so anyone can use the definitions to
 implement compatible drivers/servers.


> > > +
> > > +enum virtgpu_ctrl_type {
> > > +        VIRTGPU_UNDEFINED = 0,
> > > +
> > > +        /* 2d commands */
> > > +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > 
> > Please consider also adding:
VIRTIO_GPU_ everywhere to make it consistent with other
virtio headers?

> > 
> > #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
> > 
> > and friends.  It makes it MUCH nicer for application software to probe
> > for later extensions if every member of the enum is also associated
with
> > a preprocessor macro.
> 
> I don't think this will ever be shipped as library header for external
> users ...
> 
> > > +struct virtgpu_ctrl_hdr {
> > > +        uint32_t type;
> > > +        uint32_t flags;
> > > +        uint64_t fence_id;
> > > +        uint32_t ctx_id;
> > > +        uint32_t padding;
> > > +};
> > > +
> > 
> > Is the padding to ensure that this is aligned regardless of 32-bit or
> > 64-bit hosts?
> 
> Yes.
> 
> > Is it worth adding a compile-time assertion about the
> > size of the struct to ensure the compiler doesn't add any
additional
> > padding?
> 
> Makes sense.  What is the usual trick to do that?
> 
> thanks,
>   Gerd
BUILD_BUG_ON in linux, QEMU_BUILD_BUG_ON in QEMU.
You have to stick it in a C file though, so it
won't be visible in this patch.

-- 
MST