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

[Peter Xu]

On Wed, Aug 02, 2023 at 04:14:01PM +0200, Thomas Huth wrote:
> 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.

We can have a comment for each uint64_t at the top, IMHO better than having
two continuous block having the same "#ifdef" which is OTOH confusing.

Not a huge deal - I saw that the new version is already there.  I'll read
first.  Thanks a lot for fixing these problems.

-- 
Peter Xu