Re: [RFC PATCH 4/6] hw/i386/intel_iommu: Fix VTD_IR_TableEntry for ms_struct layout

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 31, 2023 at 04:26:44PM -0400, Peter Xu wrote:
> On Mon, Jul 31, 2023 at 10:20:40AM +0200, Thomas Huth wrote:
> > On 28/07/2023 16.37, Daniel P. Berrangé wrote:
> > > On Fri, Jul 28, 2023 at 04:27:46PM +0200, Thomas Huth wrote:
> > > > We might want to compile QEMU with Clang on Windows - but it
> > > > does not support the __attribute__((gcc_struct)) yet. So we
> > > > have to make sure that the structs will stay the same when
> > > > the compiler uses the "ms_struct" layout. The VTD_IR_TableEntry
> > > > struct is affected - rewrite it a little bit so that it works
> > > > fine with both struct layouts.
> > > > 
> > > > Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >   include/hw/i386/intel_iommu.h | 14 ++++++++------
> > > >   hw/i386/intel_iommu.c         |  2 +-
> > > >   2 files changed, 9 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > > > index 89dcbc5e1e..08bf220393 100644
> > > > --- a/include/hw/i386/intel_iommu.h
> > > > +++ b/include/hw/i386/intel_iommu.h
> > > > @@ -204,18 +204,20 @@ union VTD_IR_TableEntry {
> > > >   #endif
> > > >           uint32_t dest_id;            /* Destination ID */
> > > >           uint16_t source_id;          /* Source-ID */
> > > > +        uint16_t __reserved_2;       /* Reserved 2 */
> > > >   #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 */
> > > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > > >   #else
> > > > -        uint64_t sid_q:2;            /* Source-ID Qualifier */
> > > > -        uint64_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > -        uint64_t __reserved_2:44;    /* Reserved 2 */
> > > > +        uint32_t sid_q:2;            /* Source-ID Qualifier */
> > > > +        uint32_t sid_vtype:2;        /* Source-ID Validation Type */
> > > > +        uint32_t __reserved_3:28;    /* Reserved 3 */
> > > 
> > > Hasn't this has changed the struct layout in the else clause
> > > 
> > >   Old layout:
> > > 
> > >     source_id : 16
> > >     sid_q : 2
> > >     sid_vtype : 2
> > >     reserved_2 : 44
> > > 
> > >   New layout
> > > 
> > >     source_id : 16
> > >     reserved_2 : 16
> > >     sid_q : 2
> > >     sid_vtype : 2
> > >     reserved_3 : 28
> > 
> > Drat, you're right, I missed the fact that the whole stuff is read and
> > written via the uint64_t data[2] part from the union in the code ... :-(
> 
> Yes, that's actually part of the VT-d spec.
> 
> > 
> > > Was there something wrong with the change I suggested to
> > > just make source_id be a bitfield too:
> > > 
> > >         uint64_t source_id: 16;          /* Source-ID */
> > > 
> > > which could make ms_struct layout avoid padding to the following
> > > bitfields.
> > 
> > That likely works, but I think we then need to add it then twice, one time
> > in the HOST_BIG_ENDIAN at the end, and one time in the #else part?
> > 
> > Anyway, that whole code looks like it's completely wrong on big endian
> > machines. The struct is read via dma_memory_read() from guest memory, but
> > then the values are never byte-swapped, except for the error_report and
> > trace functions, e.g. entry->irte.present is used without calling
> > le64_to_cpu() first.
> > entry->irte.source_id is swapped with le32_to_cpu() which looks also wrong
> > since this is a 16 bit field.
> > 
> > Sigh. This is another good example why we shouldn't use bitfields at all in
> > structures that exchange data. As Richard suggested in his reply, this
> > really should be rewritten, e.g. with the stuff from hw/registerfields.h.
> 
> I can definitely review the iommu-side changes if someone would like to
> finally enable that for either clang or whatever purpose.  Sorry if it
> never worked..
> 
> But then if it's broken for 7 years since the start, it probably also means
> no one ever used it on big endian hosts, either, as a functionality.. so
> another approach is we can opt-out VT-d as a whole for big endian, if
> that's easier.
> 
> Thanks,

Let's just fix it properly please. Bad code proliferates.


-- 
MST