[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/i386/intel_iommu: Fix endianness problems related to VTD_
From: |
Peter Xu |
Subject: |
Re: [PATCH] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry |
Date: |
Wed, 2 Aug 2023 11:12:15 -0400 |
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