Re: [Qemu-devel] [V17 3/4] hw/i386: Introduce AMD IOMMU

[David Kiarie]

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.

>