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é <berra...@redhat.com>
> > > Signed-off-by: Thomas Huth <th...@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,
--
Peter Xu
