Re: [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

["Michael S. Tsirkin" <mst@redhat.com>]

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