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