Message ID | 20230802092837.153689-1-thuth@redhat.com |
---|---|
State | New |
Headers | show |
Series | hw/i386/intel_iommu: Fix endianness problems related to VTD_IR_TableEntry | expand |
On 02/08/2023 11.28, Thomas Huth wrote: > 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> > --- > Note: There are more endianness issues in the code, I haven't figured > out all of them yet, Linux fails to boot in the guest when I use > this device on a s390x host. But I wanted to publish this patch > now already since this should also fix the "issue" with the Clang > ms_struct packing that we recently discussed on the mailing list. I just found the all remaining issues (I hope). I can now run a Linux guest with -device intel-iommu and it works without crashing now. I'll send out the patches shorty, after cleaning them up a little bit. Thomas
On Wed, Aug 02, 2023 at 11:28:37AM +0200, Thomas Huth wrote: > #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 */ A quick comment before a repost: we can merge the two HOST_BIG_ENDIAN blocks into one now? Thanks,
On 02/08/2023 16.02, Peter Xu wrote: > On Wed, Aug 02, 2023 at 11:28:37AM +0200, Thomas Huth wrote: >> #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 */ > > A quick comment before a repost: we can merge the two HOST_BIG_ENDIAN > blocks into one now? We could, and I also considered it while working on this. But I think it would rather decrease the readability of this code. These are two separete 64-bit fields, and you might want to compare the big endian version of a bitfield to the little endian version next to it. If we merge, it looks rather like one big 128-bitfield if you don't look carefully enough, and comparision gets worse. So I'd prefer to keep them separate. Thomas
On Wed, Aug 02, 2023 at 04:14:01PM +0200, Thomas Huth wrote: > On 02/08/2023 16.02, Peter Xu wrote: > > On Wed, Aug 02, 2023 at 11:28:37AM +0200, Thomas Huth wrote: > > > #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 */ > > > > A quick comment before a repost: we can merge the two HOST_BIG_ENDIAN > > blocks into one now? > > We could, and I also considered it while working on this. But I think it > would rather decrease the readability of this code. These are two separete > 64-bit fields, and you might want to compare the big endian version of a > bitfield to the little endian version next to it. If we merge, it looks > rather like one big 128-bitfield if you don't look carefully enough, and > comparision gets worse. So I'd prefer to keep them separate. We can have a comment for each uint64_t at the top, IMHO better than having two continuous block having the same "#ifdef" which is OTOH confusing. Not a huge deal - I saw that the new version is already there. I'll read first. Thanks a lot for fixing these problems.
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 b9b629d1b1..3ca71df369 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)
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> --- Note: There are more endianness issues in the code, I haven't figured out all of them yet, Linux fails to boot in the guest when I use this device on a s390x host. But I wanted to publish this patch now already since this should also fix the "issue" with the Clang ms_struct packing that we recently discussed on the mailing list. include/hw/i386/intel_iommu.h | 50 ++++++++++++++++++----------------- hw/i386/intel_iommu.c | 16 +++++------ 2 files changed, 34 insertions(+), 32 deletions(-)