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

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

On 14 September 2014 06:46, Michael S. Tsirkin <mst at redhat.com>
wrote:
> 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.
Why do you think that? We have several header files which
use QEMU_BUILD_BUG_ON and I don't see any reason why
it would have to be invoked from a .c file.

-- PMM

On Sun, Sep 14, 2014 at 07:04:11AM -0700, Peter Maydell
wrote:
> On 14 September 2014 06:46, Michael S. Tsirkin <mst at redhat.com>
wrote:
> > 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.
> 
> Why do you think that? We have several header files which
> use QEMU_BUILD_BUG_ON and I don't see any reason why
> it would have to be invoked from a .c file.
> 
> -- PMM
Because Gerd wants to share this with linux uapi, and
you can't use BUILD_BUG_ON in uapi headers on linux.

> > > > +
> > > > +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?
The names are already pretty long as-is, especially considering the
80cols rule in our codestyle.  I'd prefer to keep them the way they are
now.

cheers,
  Gerd

On Mon, Sep 15, 2014 at 12:40:07PM +0200, Gerd Hoffmann
wrote:
> > > > > +
> > > > > +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?
> 
> The names are already pretty long as-is, especially considering the
> 80cols rule in our codestyle.  I'd prefer to keep them the way they are
> now.
> 
> cheers,
>   Gerd
> 
VIRTIO_GPU_ is not longer than VIRTIO_NET_ is it?

VIRTGRU seems too generic.

-- 
MST