All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device
@ 2023-08-02 13:57 Thomas Huth
  2023-08-02 13:57 ` [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems Thomas Huth
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

The intel-iommu device is currently unusable on big endian hosts.
When doing something like this on a s390x host:

 wget https://download.fedoraproject.org/pub/fedora/linux/releases/38/Server/x86_64/images/Fedora-Server-KVM-38-1.6.x86_64.qcow2
 ./qemu-system-x86_64 -M q35 -device intel-iommu -m 2G \
        -hda ~/Fedora-Server-KVM-38-1.6.x86_64.qcow2 -trace "vtd_*"

... the guest kernel crashes during boot, complaining about some
problems with the iommu, and you can see clearly in the traces that
some values are wrong when compared to running this on a x86 host.

After spending quite some time hunting down the problems one by one,
I think I now found them all - at least I can successfully boot the
above kernel after I applied these patches.

Thomas Huth (6):
  hw/i386/intel_iommu: Fix trivial endianness problems
  hw/i386/intel_iommu: Fix endianness problems related to
    VTD_IR_TableEntry
  hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts
  hw/i386/intel_iommu: Fix index calculation in
    vtd_interrupt_remap_msi()
  hw/i386/x86-iommu: Fix endianness issue in
    x86_iommu_irq_to_msi_message()
  include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big
    endian hosts

 hw/i386/intel_iommu_internal.h |  9 ++++++
 include/hw/i386/intel_iommu.h  | 50 ++++++++++++++++++----------------
 include/hw/i386/x86-iommu.h    | 50 ++++++++++++++++++----------------
 hw/i386/intel_iommu.c          | 23 ++++++++++------
 hw/i386/x86-iommu.c            |  2 +-
 5 files changed, 76 insertions(+), 58 deletions(-)

-- 
2.39.3



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 21:13   ` Philippe Mathieu-Daudé
  2023-08-02 13:57 ` [PATCH 2/6] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry Thomas Huth
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

After reading the guest memory with dma_memory_read(), we have
to make sure that we byteswap the little endian data to the host's
byte order.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/intel_iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index dcc334060c..13fcde8e91 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -756,6 +756,8 @@ static int vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base,
         return -VTD_FR_PASID_TABLE_INV;
     }
 
+    pdire->val = le64_to_cpu(pdire->val);
+
     return 0;
 }
 
@@ -780,6 +782,9 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
                         pe, entry_size, MEMTXATTRS_UNSPECIFIED)) {
         return -VTD_FR_PASID_TABLE_INV;
     }
+    for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) {
+        pe->val[i] = le64_to_cpu(pe->val[i]);
+    }
 
     /* Do translation type check */
     if (!vtd_pe_type_check(x86_iommu, pe)) {
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/6] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
  2023-08-02 13:57 ` [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 13:57 ` [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts Thomas Huth
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

The code already tries to do some endianness handling here, but
currently fails badly:
- While it already swaps the data when logging errors / tracing, it fails
  to byteswap the value before e.g. accessing entry->irte.present
- entry->irte.source_id is swapped with le32_to_cpu(), though this is
  a 16-bit value
- The whole union is apparently supposed to be swapped via the 64-bit
  data[2] array, but the struct is a mixture between 32 bit values
  (the first 8 bytes) and 64 bit values (the second 8 bytes), so this
  cannot work as expected.

Fix it by converting the struct to two proper 64-bit bitfields, and
by swapping the values only once for everybody right after reading
the data from memory.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/intel_iommu.h | 50 ++++++++++++++++++-----------------
 hw/i386/intel_iommu.c         | 16 +++++------
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 89dcbc5e1e..7fa0a695c8 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -178,37 +178,39 @@ enum {
 union VTD_IR_TableEntry {
     struct {
 #if HOST_BIG_ENDIAN
-        uint32_t __reserved_1:8;     /* Reserved 1 */
-        uint32_t vector:8;           /* Interrupt Vector */
-        uint32_t irte_mode:1;        /* IRTE Mode */
-        uint32_t __reserved_0:3;     /* Reserved 0 */
-        uint32_t __avail:4;          /* Available spaces for software */
-        uint32_t delivery_mode:3;    /* Delivery Mode */
-        uint32_t trigger_mode:1;     /* Trigger Mode */
-        uint32_t redir_hint:1;       /* Redirection Hint */
-        uint32_t dest_mode:1;        /* Destination Mode */
-        uint32_t fault_disable:1;    /* Fault Processing Disable */
-        uint32_t present:1;          /* Whether entry present/available */
+        uint64_t dest_id:32;         /* Destination ID */
+        uint64_t __reserved_1:8;     /* Reserved 1 */
+        uint64_t vector:8;           /* Interrupt Vector */
+        uint64_t irte_mode:1;        /* IRTE Mode */
+        uint64_t __reserved_0:3;     /* Reserved 0 */
+        uint64_t __avail:4;          /* Available spaces for software */
+        uint64_t delivery_mode:3;    /* Delivery Mode */
+        uint64_t trigger_mode:1;     /* Trigger Mode */
+        uint64_t redir_hint:1;       /* Redirection Hint */
+        uint64_t dest_mode:1;        /* Destination Mode */
+        uint64_t fault_disable:1;    /* Fault Processing Disable */
+        uint64_t present:1;          /* Whether entry present/available */
 #else
-        uint32_t present:1;          /* Whether entry present/available */
-        uint32_t fault_disable:1;    /* Fault Processing Disable */
-        uint32_t dest_mode:1;        /* Destination Mode */
-        uint32_t redir_hint:1;       /* Redirection Hint */
-        uint32_t trigger_mode:1;     /* Trigger Mode */
-        uint32_t delivery_mode:3;    /* Delivery Mode */
-        uint32_t __avail:4;          /* Available spaces for software */
-        uint32_t __reserved_0:3;     /* Reserved 0 */
-        uint32_t irte_mode:1;        /* IRTE Mode */
-        uint32_t vector:8;           /* Interrupt Vector */
-        uint32_t __reserved_1:8;     /* Reserved 1 */
+        uint64_t present:1;          /* Whether entry present/available */
+        uint64_t fault_disable:1;    /* Fault Processing Disable */
+        uint64_t dest_mode:1;        /* Destination Mode */
+        uint64_t redir_hint:1;       /* Redirection Hint */
+        uint64_t trigger_mode:1;     /* Trigger Mode */
+        uint64_t delivery_mode:3;    /* Delivery Mode */
+        uint64_t __avail:4;          /* Available spaces for software */
+        uint64_t __reserved_0:3;     /* Reserved 0 */
+        uint64_t irte_mode:1;        /* IRTE Mode */
+        uint64_t vector:8;           /* Interrupt Vector */
+        uint64_t __reserved_1:8;     /* Reserved 1 */
+        uint64_t dest_id:32;         /* Destination ID */
 #endif
-        uint32_t dest_id;            /* Destination ID */
-        uint16_t source_id;          /* Source-ID */
 #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 */
+        uint64_t source_id:16;       /* Source-ID */
 #else
+        uint64_t source_id:16;       /* Source-ID */
         uint64_t sid_q:2;            /* Source-ID Qualifier */
         uint64_t sid_vtype:2;        /* Source-ID Validation Type */
         uint64_t __reserved_2:44;    /* Reserved 2 */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 13fcde8e91..4028e32701 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3328,14 +3328,15 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
         return -VTD_FR_IR_ROOT_INVAL;
     }
 
-    trace_vtd_ir_irte_get(index, le64_to_cpu(entry->data[1]),
-                          le64_to_cpu(entry->data[0]));
+    entry->data[0] = le64_to_cpu(entry->data[0]);
+    entry->data[1] = le64_to_cpu(entry->data[1]);
+
+    trace_vtd_ir_irte_get(index, entry->data[1], entry->data[0]);
 
     if (!entry->irte.present) {
         error_report_once("%s: detected non-present IRTE "
                           "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")",
-                          __func__, index, le64_to_cpu(entry->data[1]),
-                          le64_to_cpu(entry->data[0]));
+                          __func__, index, entry->data[1], entry->data[0]);
         return -VTD_FR_IR_ENTRY_P;
     }
 
@@ -3343,14 +3344,13 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
         entry->irte.__reserved_2) {
         error_report_once("%s: detected non-zero reserved IRTE "
                           "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")",
-                          __func__, index, le64_to_cpu(entry->data[1]),
-                          le64_to_cpu(entry->data[0]));
+                          __func__, index, entry->data[1], entry->data[0]);
         return -VTD_FR_IR_IRTE_RSVD;
     }
 
     if (sid != X86_IOMMU_SID_INVALID) {
         /* Validate IRTE SID */
-        source_id = le32_to_cpu(entry->irte.source_id);
+        source_id = entry->irte.source_id;
         switch (entry->irte.sid_vtype) {
         case VTD_SVT_NONE:
             break;
@@ -3404,7 +3404,7 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index,
     irq->trigger_mode = irte.irte.trigger_mode;
     irq->vector = irte.irte.vector;
     irq->delivery_mode = irte.irte.delivery_mode;
-    irq->dest = le32_to_cpu(irte.irte.dest_id);
+    irq->dest = irte.irte.dest_id;
     if (!iommu->intr_eime) {
 #define  VTD_IR_APIC_DEST_MASK         (0xff00ULL)
 #define  VTD_IR_APIC_DEST_SHIFT        (8)
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
  2023-08-02 13:57 ` [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems Thomas Huth
  2023-08-02 13:57 ` [PATCH 2/6] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 21:16   ` Philippe Mathieu-Daudé
  2023-08-02 13:57 ` [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi() Thomas Huth
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On big endian hosts, we need to reverse the bitfield order in the
struct VTDInvDescIEC, just like it is already done for the other
bitfields in the various structs of the intel-iommu device.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/intel_iommu_internal.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2e61eec2f5..e1450c5cfe 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -321,12 +321,21 @@ typedef enum VTDFaultReason {
 
 /* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */
 struct VTDInvDescIEC {
+#if HOST_BIG_ENDIAN
+    uint64_t reserved_2:16;
+    uint64_t index:16;          /* Start index to invalidate */
+    uint64_t index_mask:5;      /* 2^N for continuous int invalidation */
+    uint64_t resved_1:22;
+    uint64_t granularity:1;     /* If set, it's global IR invalidation */
+    uint64_t type:4;            /* Should always be 0x4 */
+#else
     uint32_t type:4;            /* Should always be 0x4 */
     uint32_t granularity:1;     /* If set, it's global IR invalidation */
     uint32_t resved_1:22;
     uint32_t index_mask:5;      /* 2^N for continuous int invalidation */
     uint32_t index:16;          /* Start index to invalidate */
     uint32_t reserved_2:16;
+#endif
 };
 typedef struct VTDInvDescIEC VTDInvDescIEC;
 
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi()
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
                   ` (2 preceding siblings ...)
  2023-08-02 13:57 ` [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 21:12   ` Philippe Mathieu-Daudé
  2023-08-02 13:57 ` [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message() Thomas Huth
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

The values in "addr" are populated locally in this function in host
endian byte order, so we must not swap the index_l field here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/intel_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4028e32701..3ca71df369 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3459,7 +3459,7 @@ static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
         goto out;
     }
 
-    index = addr.addr.index_h << 15 | le16_to_cpu(addr.addr.index_l);
+    index = addr.addr.index_h << 15 | addr.addr.index_l;
 
 #define  VTD_IR_MSI_DATA_SUBHANDLE       (0x0000ffff)
 #define  VTD_IR_MSI_DATA_RESERVED        (0xffff0000)
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message()
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
                   ` (3 preceding siblings ...)
  2023-08-02 13:57 ` [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi() Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 21:16   ` Philippe Mathieu-Daudé
  2023-08-02 13:57 ` [PATCH 6/6] include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big endian hosts Thomas Huth
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

The values in "msg" are assembled in host endian byte order (the other
field are also not swapped), so we must not swap the __addr_head here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i386/x86-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 01d11325a6..726e9e1d16 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -63,7 +63,7 @@ void x86_iommu_irq_to_msi_message(X86IOMMUIrq *irq, MSIMessage *msg_out)
     msg.redir_hint = irq->redir_hint;
     msg.dest = irq->dest;
     msg.__addr_hi = irq->dest & 0xffffff00;
-    msg.__addr_head = cpu_to_le32(0xfee);
+    msg.__addr_head = 0xfee;
     /* Keep this from original MSI address bits */
     msg.__not_used = irq->msi_addr_last_bits;
 
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 6/6] include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big endian hosts
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
                   ` (4 preceding siblings ...)
  2023-08-02 13:57 ` [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message() Thomas Huth
@ 2023-08-02 13:57 ` Thomas Huth
  2023-08-02 15:36 ` [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Peter Xu
  2023-08-02 15:37 ` Michael S. Tsirkin
  7 siblings, 0 replies; 13+ messages in thread
From: Thomas Huth @ 2023-08-02 13:57 UTC (permalink / raw)
  To: Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

The first bitfield here is supposed to be used as a 64-bit equivalent
to the "uint64_t msi_addr" in the union. To make this work correctly
on big endian hosts, too, the __addr_hi field has to be part of the
bitfield, and the the bitfield members must be declared with "uint64_t"
instead of "uint32_t" - otherwise the values are placed in the wrong
bytes on big endian hosts.

Same applies to the 32-bit "msi_data" field: __resved1 must be part
of the bitfield, and the members must be declared with "uint32_t"
instead of "uint16_t".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/i386/x86-iommu.h | 50 +++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index 8d8d53b18b..bfd21649d0 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -87,40 +87,42 @@ struct X86IOMMU_MSIMessage {
     union {
         struct {
 #if HOST_BIG_ENDIAN
-            uint32_t __addr_head:12; /* 0xfee */
-            uint32_t dest:8;
-            uint32_t __reserved:8;
-            uint32_t redir_hint:1;
-            uint32_t dest_mode:1;
-            uint32_t __not_used:2;
+            uint64_t __addr_hi:32;
+            uint64_t __addr_head:12; /* 0xfee */
+            uint64_t dest:8;
+            uint64_t __reserved:8;
+            uint64_t redir_hint:1;
+            uint64_t dest_mode:1;
+            uint64_t __not_used:2;
 #else
-            uint32_t __not_used:2;
-            uint32_t dest_mode:1;
-            uint32_t redir_hint:1;
-            uint32_t __reserved:8;
-            uint32_t dest:8;
-            uint32_t __addr_head:12; /* 0xfee */
+            uint64_t __not_used:2;
+            uint64_t dest_mode:1;
+            uint64_t redir_hint:1;
+            uint64_t __reserved:8;
+            uint64_t dest:8;
+            uint64_t __addr_head:12; /* 0xfee */
+            uint64_t __addr_hi:32;
 #endif
-            uint32_t __addr_hi;
         } QEMU_PACKED;
         uint64_t msi_addr;
     };
     union {
         struct {
 #if HOST_BIG_ENDIAN
-            uint16_t trigger_mode:1;
-            uint16_t level:1;
-            uint16_t __resved:3;
-            uint16_t delivery_mode:3;
-            uint16_t vector:8;
+            uint32_t __resved1:16;
+            uint32_t trigger_mode:1;
+            uint32_t level:1;
+            uint32_t __resved:3;
+            uint32_t delivery_mode:3;
+            uint32_t vector:8;
 #else
-            uint16_t vector:8;
-            uint16_t delivery_mode:3;
-            uint16_t __resved:3;
-            uint16_t level:1;
-            uint16_t trigger_mode:1;
+            uint32_t vector:8;
+            uint32_t delivery_mode:3;
+            uint32_t __resved:3;
+            uint32_t level:1;
+            uint32_t trigger_mode:1;
+            uint32_t __resved1:16;
 #endif
-            uint16_t __resved1;
         } QEMU_PACKED;
         uint32_t msi_data;
     };
-- 
2.39.3



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
                   ` (5 preceding siblings ...)
  2023-08-02 13:57 ` [PATCH 6/6] include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big endian hosts Thomas Huth
@ 2023-08-02 15:36 ` Peter Xu
  2023-08-02 15:37 ` Michael S. Tsirkin
  7 siblings, 0 replies; 13+ messages in thread
From: Peter Xu @ 2023-08-02 15:36 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Michael S. Tsirkin, Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On Wed, Aug 02, 2023 at 03:57:17PM +0200, Thomas Huth wrote:
> The intel-iommu device is currently unusable on big endian hosts.
> When doing something like this on a s390x host:
> 
>  wget https://download.fedoraproject.org/pub/fedora/linux/releases/38/Server/x86_64/images/Fedora-Server-KVM-38-1.6.x86_64.qcow2
>  ./qemu-system-x86_64 -M q35 -device intel-iommu -m 2G \
>         -hda ~/Fedora-Server-KVM-38-1.6.x86_64.qcow2 -trace "vtd_*"
> 
> ... the guest kernel crashes during boot, complaining about some
> problems with the iommu, and you can see clearly in the traces that
> some values are wrong when compared to running this on a x86 host.
> 
> After spending quite some time hunting down the problems one by one,
> I think I now found them all - at least I can successfully boot the
> above kernel after I applied these patches.
> 
> Thomas Huth (6):
>   hw/i386/intel_iommu: Fix trivial endianness problems
>   hw/i386/intel_iommu: Fix endianness problems related to
>     VTD_IR_TableEntry
>   hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts
>   hw/i386/intel_iommu: Fix index calculation in
>     vtd_interrupt_remap_msi()
>   hw/i386/x86-iommu: Fix endianness issue in
>     x86_iommu_irq_to_msi_message()
>   include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big
>     endian hosts

All look right here. This was the 1st project I did after I worked on QEMU,
thanks again for fixing the issues for years!

Reviewed-by: Peter Xu <peterx@redhat.com>

This will need to be 8.2 material though, am I right?

Thanks,

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device
  2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
                   ` (6 preceding siblings ...)
  2023-08-02 15:36 ` [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Peter Xu
@ 2023-08-02 15:37 ` Michael S. Tsirkin
  7 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-08-02 15:37 UTC (permalink / raw)
  To: Thomas Huth
  Cc: Peter Xu, Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On Wed, Aug 02, 2023 at 03:57:17PM +0200, Thomas Huth wrote:
> The intel-iommu device is currently unusable on big endian hosts.
> When doing something like this on a s390x host:
> 
>  wget https://download.fedoraproject.org/pub/fedora/linux/releases/38/Server/x86_64/images/Fedora-Server-KVM-38-1.6.x86_64.qcow2
>  ./qemu-system-x86_64 -M q35 -device intel-iommu -m 2G \
>         -hda ~/Fedora-Server-KVM-38-1.6.x86_64.qcow2 -trace "vtd_*"
> 
> ... the guest kernel crashes during boot, complaining about some
> problems with the iommu, and you can see clearly in the traces that
> some values are wrong when compared to running this on a x86 host.
> 
> After spending quite some time hunting down the problems one by one,
> I think I now found them all - at least I can successfully boot the
> above kernel after I applied these patches.
> 
> Thomas Huth (6):
>   hw/i386/intel_iommu: Fix trivial endianness problems
>   hw/i386/intel_iommu: Fix endianness problems related to
>     VTD_IR_TableEntry
>   hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts
>   hw/i386/intel_iommu: Fix index calculation in
>     vtd_interrupt_remap_msi()
>   hw/i386/x86-iommu: Fix endianness issue in
>     x86_iommu_irq_to_msi_message()
>   include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big
>     endian hosts
> 
>  hw/i386/intel_iommu_internal.h |  9 ++++++
>  include/hw/i386/intel_iommu.h  | 50 ++++++++++++++++++----------------
>  include/hw/i386/x86-iommu.h    | 50 ++++++++++++++++++----------------
>  hw/i386/intel_iommu.c          | 23 ++++++++++------
>  hw/i386/x86-iommu.c            |  2 +-
>  5 files changed, 76 insertions(+), 58 deletions(-)

I guess it's ok as a hack for 8.1. But we really need better
APIs. Annotating field endianness with __bitwise would be
a start.


> -- 
> 2.39.3



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi()
  2023-08-02 13:57 ` [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi() Thomas Huth
@ 2023-08-02 21:12   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-02 21:12 UTC (permalink / raw)
  To: Thomas Huth, Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On 2/8/23 15:57, Thomas Huth wrote:
> The values in "addr" are populated locally in this function in host
> endian byte order, so we must not swap the index_l field here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems
  2023-08-02 13:57 ` [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems Thomas Huth
@ 2023-08-02 21:13   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-02 21:13 UTC (permalink / raw)
  To: Thomas Huth, Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On 2/8/23 15:57, Thomas Huth wrote:
> After reading the guest memory with dma_memory_read(), we have
> to make sure that we byteswap the little endian data to the host's
> byte order.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 5 +++++
>   1 file changed, 5 insertions(+)

Maybe worth adding a comment in "hw/i386/intel_iommu.h"
around VTDPASIDEntry::val mentioning little endianness.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts
  2023-08-02 13:57 ` [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts Thomas Huth
@ 2023-08-02 21:16   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-02 21:16 UTC (permalink / raw)
  To: Thomas Huth, Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On 2/8/23 15:57, Thomas Huth wrote:
> On big endian hosts, we need to reverse the bitfield order in the
> struct VTDInvDescIEC, just like it is already done for the other
> bitfields in the various structs of the intel-iommu device.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/intel_iommu_internal.h | 9 +++++++++
>   1 file changed, 9 insertions(+)

Isn't it a bit hacky? Bitfields can be accessed in a portable
way using the deposit/extract API, right?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message()
  2023-08-02 13:57 ` [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message() Thomas Huth
@ 2023-08-02 21:16   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-02 21:16 UTC (permalink / raw)
  To: Thomas Huth, Michael S. Tsirkin, Peter Xu
  Cc: Jason Wang, Richard Henderson, qemu-devel,
	Daniel P . Berrangé,
	qemu-stable, Paolo Bonzini

On 2/8/23 15:57, Thomas Huth wrote:
> The values in "msg" are assembled in host endian byte order (the other
> field are also not swapped), so we must not swap the __addr_head here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/i386/x86-iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>




^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-08-02 21:17 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-02 13:57 [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Thomas Huth
2023-08-02 13:57 ` [PATCH 1/6] hw/i386/intel_iommu: Fix trivial endianness problems Thomas Huth
2023-08-02 21:13   ` Philippe Mathieu-Daudé
2023-08-02 13:57 ` [PATCH 2/6] hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry Thomas Huth
2023-08-02 13:57 ` [PATCH 3/6] hw/i386/intel_iommu: Fix struct VTDInvDescIEC on big endian hosts Thomas Huth
2023-08-02 21:16   ` Philippe Mathieu-Daudé
2023-08-02 13:57 ` [PATCH 4/6] hw/i386/intel_iommu: Fix index calculation in vtd_interrupt_remap_msi() Thomas Huth
2023-08-02 21:12   ` Philippe Mathieu-Daudé
2023-08-02 13:57 ` [PATCH 5/6] hw/i386/x86-iommu: Fix endianness issue in x86_iommu_irq_to_msi_message() Thomas Huth
2023-08-02 21:16   ` Philippe Mathieu-Daudé
2023-08-02 13:57 ` [PATCH 6/6] include/hw/i386/x86-iommu: Fix struct X86IOMMU_MSIMessage for big endian hosts Thomas Huth
2023-08-02 15:36 ` [PATCH for-8.1 0/6] Fix endianness issues in the intel-iommu device Peter Xu
2023-08-02 15:37 ` Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.