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 ... :-(
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.
Thomas
