From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38961) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTA5A-0006qw-C3 for qemu-devel@nongnu.org; Sun, 14 Sep 2014 09:43:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTA54-0007mA-6U for qemu-devel@nongnu.org; Sun, 14 Sep 2014 09:43:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTA53-0007m0-Tn for qemu-devel@nongnu.org; Sun, 14 Sep 2014 09:43:06 -0400 Date: Sun, 14 Sep 2014 16:46:22 +0300 From: "Michael S. Tsirkin" Message-ID: <20140914134622.GB27315@redhat.com> References: <1410448173-12960-1-git-send-email-kraxel@redhat.com> <1410448173-12960-2-git-send-email-kraxel@redhat.com> <5411BD74.80608@redhat.com> <1410518696.30411.9.camel@nilsson.home.kraxel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1410518696.30411.9.camel@nilsson.home.kraxel.org> Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: Dave Airlie , virtualization@lists.linux-foundation.org, qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org 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