Re: [PATCH] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry

[Thomas Huth <thuth@redhat.com>]

On 02/08/2023 16.02, Peter Xu wrote:
> On Wed, Aug 02, 2023 at 11:28:37AM +0200, Thomas Huth wrote:
>>   #if HOST_BIG_ENDIAN
>> -        uint32_t __reserved_1:8;     /* Reserved 1 */
>> -        uint32_t vector:8;           /* Interrupt Vector */
>> -        uint32_t irte_mode:1;        /* IRTE Mode */
>> -        uint32_t __reserved_0:3;     /* Reserved 0 */
>> -        uint32_t __avail:4;          /* Available spaces for software */
>> -        uint32_t delivery_mode:3;    /* Delivery Mode */
>> -        uint32_t trigger_mode:1;     /* Trigger Mode */
>> -        uint32_t redir_hint:1;       /* Redirection Hint */
>> -        uint32_t dest_mode:1;        /* Destination Mode */
>> -        uint32_t fault_disable:1;    /* Fault Processing Disable */
>> -        uint32_t present:1;          /* Whether entry present/available */
>> +        uint64_t dest_id:32;         /* Destination ID */
>> +        uint64_t __reserved_1:8;     /* Reserved 1 */
>> +        uint64_t vector:8;           /* Interrupt Vector */
>> +        uint64_t irte_mode:1;        /* IRTE Mode */
>> +        uint64_t __reserved_0:3;     /* Reserved 0 */
>> +        uint64_t __avail:4;          /* Available spaces for software */
>> +        uint64_t delivery_mode:3;    /* Delivery Mode */
>> +        uint64_t trigger_mode:1;     /* Trigger Mode */
>> +        uint64_t redir_hint:1;       /* Redirection Hint */
>> +        uint64_t dest_mode:1;        /* Destination Mode */
>> +        uint64_t fault_disable:1;    /* Fault Processing Disable */
>> +        uint64_t present:1;          /* Whether entry present/available */
>>   #else
>> -        uint32_t present:1;          /* Whether entry present/available */
>> -        uint32_t fault_disable:1;    /* Fault Processing Disable */
>> -        uint32_t dest_mode:1;        /* Destination Mode */
>> -        uint32_t redir_hint:1;       /* Redirection Hint */
>> -        uint32_t trigger_mode:1;     /* Trigger Mode */
>> -        uint32_t delivery_mode:3;    /* Delivery Mode */
>> -        uint32_t __avail:4;          /* Available spaces for software */
>> -        uint32_t __reserved_0:3;     /* Reserved 0 */
>> -        uint32_t irte_mode:1;        /* IRTE Mode */
>> -        uint32_t vector:8;           /* Interrupt Vector */
>> -        uint32_t __reserved_1:8;     /* Reserved 1 */
>> +        uint64_t present:1;          /* Whether entry present/available */
>> +        uint64_t fault_disable:1;    /* Fault Processing Disable */
>> +        uint64_t dest_mode:1;        /* Destination Mode */
>> +        uint64_t redir_hint:1;       /* Redirection Hint */
>> +        uint64_t trigger_mode:1;     /* Trigger Mode */
>> +        uint64_t delivery_mode:3;    /* Delivery Mode */
>> +        uint64_t __avail:4;          /* Available spaces for software */
>> +        uint64_t __reserved_0:3;     /* Reserved 0 */
>> +        uint64_t irte_mode:1;        /* IRTE Mode */
>> +        uint64_t vector:8;           /* Interrupt Vector */
>> +        uint64_t __reserved_1:8;     /* Reserved 1 */
>> +        uint64_t dest_id:32;         /* Destination ID */
>>   #endif
>> -        uint32_t dest_id;            /* Destination ID */
>> -        uint16_t source_id;          /* Source-ID */
>>   #if HOST_BIG_ENDIAN
>>           uint64_t __reserved_2:44;    /* Reserved 2 */
>>           uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>>           uint64_t sid_q:2;            /* Source-ID Qualifier */
>> +        uint64_t source_id:16;       /* Source-ID */
>>   #else
>> +        uint64_t source_id:16;       /* Source-ID */
>>           uint64_t sid_q:2;            /* Source-ID Qualifier */
>>           uint64_t sid_vtype:2;        /* Source-ID Validation Type */
>>           uint64_t __reserved_2:44;    /* Reserved 2 */
> 
> A quick comment before a repost: we can merge the two HOST_BIG_ENDIAN
> blocks into one now?

We could, and I also considered it while working on this. But I think it 
would rather decrease the readability of this code. These are two separete 
64-bit fields, and you might want to compare the big endian version of a 
bitfield to the little endian version next to it. If we merge, it looks 
rather like one big 128-bitfield if you don't look carefully enough, and 
comparision gets worse. So I'd prefer to keep them separate.

  Thomas