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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
