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

[Michael S. Tsirkin]

On Wed, Aug 31, 2016 at 07:17:42PM +0300, David Kiarie wrote:
> +/* serialize IOMMU command processing */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;               /* command type           */
> +    uint64_t reserved:8;
> +    uint64_t store_addr:49;        /* addr to write          */
> +    uint64_t completion_flush:1;   /* allow more executions  */
> +    uint64_t completion_int:1;     /* set MMIOWAITINT        */
> +    uint64_t completion_store:1;   /* write data to address  */
> +#else
> +    uint64_t completion_store:1;
> +    uint64_t completion_int:1;
> +    uint64_t completion_flush:1;
> +    uint64_t store_addr:49;
> +    uint64_t reserved:8;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +    uint64_t store_data;           /* data to write          */
> +} CMDCompletionWait;
> +
> +/* invalidate internal caches for devid */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t devid:16;             /* device to invalidate   */
> +    uint64_t reserved_1:44;
> +    uint64_t type:4;               /* command type           */
> +#else
> +    uint64_t devid:16;
> +    uint64_t reserved_1:44;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +    uint64_t reserved_2;
> +} CMDInvalDevEntry;
> +
> +/* invalidate a range of entries in IOMMU translation cache for devid */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;               /* command type           */
> +    uint64_t reserved_2:12
> +    uint64_t domid:16;             /* domain to inval for    */
> +    uint64_t reserved_1:12;
> +    uint64_t pasid:20;
> +#else
> +    uint64_t pasid:20;
> +    uint64_t reserved_1:12;
> +    uint64_t domid:16;
> +    uint64_t reserved_2:12;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t address:51;          /* address to invalidate   */
> +    uint64_t reserved_3:10;
> +    uint64_t guest:1;             /* G/N invalidation        */
> +    uint64_t pde:1;               /* invalidate cached ptes  */
> +    uint64_t size:1               /* size of invalidation    */
> +#else
> +    uint64_t size:1;
> +    uint64_t pde:1;
> +    uint64_t guest:1;
> +    uint64_t reserved_3:10;
> +    uint64_t address:51;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +} CMDInvalIommuPages;
> +
> +/* inval specified address for devid from remote IOTLB */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;            /* command type        */
> +    uint64_t pasid_19_6:4;
> +    uint64_t pasid_7_0:8;
> +    uint64_t queuid:16;
> +    uint64_t maxpend:8;
> +    uint64_t pasid_15_8;
> +    uint64_t devid:16;         /* related devid        */
> +#else
> +    uint64_t devid:16;
> +    uint64_t pasid_15_8:8;
> +    uint64_t maxpend:8;
> +    uint64_t queuid:16;
> +    uint64_t pasid_7_0:8;
> +    uint64_t pasid_19_6:4;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t address:52;       /* invalidate addr      */
> +    uint64_t reserved_2:9;
> +    uint64_t guest:1;          /* G/N invalidate       */
> +    uint64_t reserved_1:1;
> +    uint64_t size:1;           /* size of invalidation */
> +#else
> +    uint64_t size:1;
> +    uint64_t reserved_1:1;
> +    uint64_t guest:1;
> +    uint64_t reserved_2:9;
> +    uint64_t address:52;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +} CMDInvalIOTLBPages;
> +
> +/* invalidate all cached interrupt info for devid */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;          /* command type        */
> +    uint64_t reserved_1:44;
> +    uint64_t devid:16;        /* related devid       */
> +#else
> +    uint64_t devid:16;
> +    uint64_t reserved_1:44;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +    uint64_t reserved_2;
> +} CMDInvalIntrTable;
> +
> +/* load adddress translation info for devid into translation cache */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;          /* command type       */
> +    uint64_t reserved_2:8;
> +    uint64_t pasid_19_0:20;
> +    uint64_t pfcount_7_0:8;
> +    uint64_t reserved_1:8;
> +    uint64_t devid:16;        /* related devid      */
> +#else
> +    uint64_t devid:16;
> +    uint64_t reserved_1:8;
> +    uint64_t pfcount_7_0:8;
> +    uint64_t pasid_19_0:20;
> +    uint64_t reserved_2:8;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t address:52;     /* invalidate address       */
> +    uint64_t reserved_5:7;
> +    uint64_t inval:1;        /* inval matching entries   */
> +    uint64_t reserved_4:1;
> +    uint64_t guest:1;        /* G/N invalidate           */
> +    uint64_t reserved_3:1;
> +    uint64_t size:1;         /* prefetched page size     */
> +#else
> +    uint64_t size:1;
> +    uint64_t reserved_3:1;
> +    uint64_t guest:1;
> +    uint64_t reserved_4:1;
> +    uint64_t inval:1;
> +    uint64_t reserved_5:7;
> +    uint64_t address:52;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +} CMDPrefetchPages;
> +
> +/* clear all address translation/interrupt remapping caches */
> +typedef struct QEMU_PACKED {
> +#ifdef HOST_WORDS_BIGENDIAN
> +    uint64_t type:4;              /* command type       */
> +    uint64_t reserved_1:60;
> +#else
> +    uint64_t reserved_1:60;
> +    uint64_t type:4;
> +#endif /* __BIG_ENDIAN_BITFIELD */
> +    uint64_t reserved_2;
> +} CMDInvalIommuAll;
> +
> +/* 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;


-- 
MST