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é <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,
Let's just fix it properly please. Bad code proliferates.
--
MST
