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

[David Kiarie]

On Sat, Sep 17, 2016 at 7:59 AM, David Kiarie <address@hidden>
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.


>