From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41545) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bl8ov-0002WK-K5 for qemu-devel@nongnu.org; Sat, 17 Sep 2016 02:09:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bl8ot-0004NH-7q for qemu-devel@nongnu.org; Sat, 17 Sep 2016 02:09:48 -0400 Received: from mail-yw0-x233.google.com ([2607:f8b0:4002:c05::233]:36715) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bl8ot-0004N9-2i for qemu-devel@nongnu.org; Sat, 17 Sep 2016 02:09:47 -0400 Received: by mail-yw0-x233.google.com with SMTP id t67so98788823ywg.3 for ; Fri, 16 Sep 2016 23:09:47 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <57DCCDAD.10105@gmail.com> References: <1472660263-4524-1-git-send-email-davidkiarie4@gmail.com> <1472660263-4524-4-git-send-email-davidkiarie4@gmail.com> <20160916215555-mutt-send-email-mst@kernel.org> <57DCCDAD.10105@gmail.com> From: David Kiarie Date: Sat, 17 Sep 2016 09:09:43 +0300 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [V17 3/4] hw/i386: Introduce AMD IOMMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: QEMU Developers , Jan Kiszka , rkrcmar@redhat.com, Peter Xu , Eduardo Habkost On Sat, Sep 17, 2016 at 7:59 AM, David Kiarie wrote: > > > On 16/09/16 21:58, Michael S. Tsirkin wrote: > >> On Wed, Aug 31, 2016 at 07:17:42PM +0300, David Kiarie wrote: >> > Hi Michael, > > + >> +/* issue a PCIe completion packet for devid */ >> +typedef struct QEMU_PACKED { >> + uint32_t reserved_1:16; >> + uint32_t devid:16; >> + >> +#ifdef HOST_WORDS_BIGENDIAN >> + uint32_t type:4; /* command type */ >> + uint32_t reserved_2:8; >> + uint32_t pasid_19_0:20 >> +#else >> + uint32_t pasid_19_0:20; >> + uint32_t reserved_2:8; >> + uint32_t type:4; >> +#endif /* __BIG_ENDIAN_BITFIELD */ >> + >> +#ifdef HOST_WORDS_BIGENDIAN >> + uint32_t reserved_3:29; >> + uint32_t guest:1; >> + uint32_t reserved_4:2; >> +#else >> + uint32_t reserved_3:2; >> + uint32_t guest:1; >> + uint32_t reserved_4:29; >> +#endif /* __BIG_ENDIAN_BITFIELD */ >> + >> + uint32_t completion_tag:16; /* PCIe PRI information */ >> + uint32_t reserved_5:16; >> +} CMDCompletePPR; >> So as Peter points, out, we don't do this kind of >> thing. Pls drop this ifdefery and rework using >> masks and cpu_to_le. E.g. >> >> > +#ifdef HOST_WORDS_BIGENDIAN >> > + uint32_t reserved_3:29; >> > + uint32_t guest:1; >> > + uint32_t reserved_4:2; >> > +#else >> > + uint32_t reserved_3:2; >> > + uint32_t guest:1; >> > + uint32_t reserved_4:29; >> > +#endif /* __BIG_ENDIAN_BITFIELD */ >> >> guest = 1; >> >> -> >> >> #define GUEST cpu_to_le32(0x1 << 2) >> uint32_t guest; >> >> guest = GUEST; >> > > This might not solve the problem we are encountering here. I don't intent > to write any values to these fields. I only intend to read. > > I read into these structs using 'dma_memory_read' which gives me a memory > order dependent on the host. Byte swapping the read buffer will lead into > some of the fields being permanently corrupted since they span across byte > boundaries. I actually didn't have any bitfields(and was not taking care of > BE) but then there were some complains that the code was barely readable > hence the bitfields and later the ifdefinery but I've never checked that > the BE code complies. 'extract64' seems to solve everything though so I can get rid of all the host dependent defines: didn't know we had this before. >