Re: [PATCH v3 12/35] x86/msi: Provide msi message shadow structs

2022-04-06 Thread Thomas Gleixner
On Wed, Apr 06 2022 at 10:36, Reto Buerki wrote:
> While working on some out-of-tree patches, we noticed that assignment to
> dmar_subhandle of struct x86_msi_data lead to a QEMU warning about
> reserved bits in MSI data being set:
>
> qemu-system-x86_64: vtd_interrupt_remap_msi: invalid IR MSI (sid=256, 
> address=0xfee003d8, data=0x1)
>
> This message originates from hw/i386/intel_iommu.c in QEMU:
>
> #define VTD_IR_MSI_DATA_RESERVED (0x)
> if (origin->data & VTD_IR_MSI_DATA_RESERVED) { ... }
>
> Looking at struct x86_msi_data, it appears that it is actually 48-bits in size
> since the bitfield containing the vector, delivery_mode etc is 2 bytes wide
> followed by dmar_subhandle which is 32 bits. Thus assignment to dmar_subhandle
> leads to bits > 16 being set.
>
> If I am not mistaken, the MSI data field should be 32-bits wide for all
> platforms (struct msi_msg, include/linux/msi.h). Is this analysis
> correct or did I miss something wrt. handling of dmar_subhandle?

It's correct and I'm completely surprised that this went unnoticed
for more than a year. Where is that brown paperbag...

Thanks,

tglx

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 12/35] x86/msi: Provide msi message shadow structs

2022-04-06 Thread Reto Buerki
While working on some out-of-tree patches, we noticed that assignment to
dmar_subhandle of struct x86_msi_data lead to a QEMU warning about
reserved bits in MSI data being set:

qemu-system-x86_64: vtd_interrupt_remap_msi: invalid IR MSI (sid=256, 
address=0xfee003d8, data=0x1)

This message originates from hw/i386/intel_iommu.c in QEMU:

#define VTD_IR_MSI_DATA_RESERVED (0x)
if (origin->data & VTD_IR_MSI_DATA_RESERVED) { ... }

Looking at struct x86_msi_data, it appears that it is actually 48-bits in size
since the bitfield containing the vector, delivery_mode etc is 2 bytes wide
followed by dmar_subhandle which is 32 bits. Thus assignment to dmar_subhandle
leads to bits > 16 being set.

If I am not mistaken, the MSI data field should be 32-bits wide for all
platforms (struct msi_msg, include/linux/msi.h). Is this analysis
correct or did I miss something wrt. handling of dmar_subhandle?

The attached patch fixes the issue for us.

Regards,
- reto
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 12/35] x86/msi: Provide msi message shadow structs

2020-10-24 Thread David Woodhouse
From: Thomas Gleixner 

Create shadow structs with named bitfields for msi_msg data, address_lo and
address_hi and use them in the MSI message composer.

Provide a function to retrieve the destination ID. This could be inline,
but that'd create a circular header dependency.

[dwmw2: fix bitfields not all to be a union]
Signed-off-by: Thomas Gleixner 
Signed-off-by: David Woodhouse 
---
 arch/x86/include/asm/msi.h  | 49 +
 arch/x86/kernel/apic/apic.c | 35 ++
 2 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/msi.h b/arch/x86/include/asm/msi.h
index cd30013d15d3..322fd905da9c 100644
--- a/arch/x86/include/asm/msi.h
+++ b/arch/x86/include/asm/msi.h
@@ -9,4 +9,53 @@ typedef struct irq_alloc_info msi_alloc_info_t;
 int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec,
msi_alloc_info_t *arg);
 
+/* Structs and defines for the X86 specific MSI message format */
+
+typedef struct x86_msi_data {
+   u32 vector  :  8,
+   delivery_mode   :  3,
+   dest_mode_logical   :  1,
+   reserved:  2,
+   active_low  :  1,
+   is_level:  1;
+
+   u32 dmar_subhandle;
+} __attribute__ ((packed)) arch_msi_msg_data_t;
+#define arch_msi_msg_data  x86_msi_data
+
+typedef struct x86_msi_addr_lo {
+   union {
+   struct {
+   u32 reserved_0  :  2,
+   dest_mode_logical   :  1,
+   redirect_hint   :  1,
+   reserved_1  :  8,
+   destid_0_7  :  8,
+   base_address: 12;
+   };
+   struct {
+   u32 dmar_reserved_0 :  2,
+   dmar_index_15   :  1,
+   dmar_subhandle_valid:  1,
+   dmar_format :  1,
+   dmar_index_0_14 : 15,
+   dmar_base_address   : 12;
+   };
+   };
+} __attribute__ ((packed)) arch_msi_msg_addr_lo_t;
+#define arch_msi_msg_addr_lo   x86_msi_addr_lo
+
+#define X86_MSI_BASE_ADDRESS_LOW   (0xfee0 >> 20)
+
+typedef struct x86_msi_addr_hi {
+   u32 reserved:  8,
+   destid_8_31 : 24;
+} __attribute__ ((packed)) arch_msi_msg_addr_hi_t;
+#define arch_msi_msg_addr_hi   x86_msi_addr_hi
+
+#define X86_MSI_BASE_ADDRESS_HIGH  (0)
+
+struct msi_msg;
+u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid);
+
 #endif /* _ASM_X86_MSI_H */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4c15bf29ea2c..f7196ee0f005 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -50,7 +50,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2484,22 +2483,16 @@ int hard_smp_processor_id(void)
 void __irq_msi_compose_msg(struct irq_cfg *cfg, struct msi_msg *msg,
   bool dmar)
 {
-   msg->address_hi = MSI_ADDR_BASE_HI;
+   memset(msg, 0, sizeof(*msg));
 
-   msg->address_lo =
-   MSI_ADDR_BASE_LO |
-   (apic->dest_mode_logical ?
-   MSI_ADDR_DEST_MODE_LOGICAL :
-   MSI_ADDR_DEST_MODE_PHYSICAL) |
-   MSI_ADDR_REDIRECTION_CPU |
-   MSI_ADDR_DEST_ID(cfg->dest_apicid);
+   msg->arch_addr_lo.base_address = X86_MSI_BASE_ADDRESS_LOW;
+   msg->arch_addr_lo.dest_mode_logical = apic->dest_mode_logical;
+   msg->arch_addr_lo.destid_0_7 = cfg->dest_apicid & 0xFF;
 
-   msg->data =
-   MSI_DATA_TRIGGER_EDGE |
-   MSI_DATA_LEVEL_ASSERT |
-   MSI_DATA_DELIVERY_FIXED |
-   MSI_DATA_VECTOR(cfg->vector);
+   msg->arch_data.delivery_mode = APIC_DELIVERY_MODE_FIXED;
+   msg->arch_data.vector = cfg->vector;
 
+   msg->address_hi = X86_MSI_BASE_ADDRESS_HIGH;
/*
 * Only the IOMMU itself can use the trick of putting destination
 * APIC ID into the high bits of the address. Anything else would
@@ -2507,11 +2500,21 @@ void __irq_msi_compose_msg(struct irq_cfg *cfg, struct 
msi_msg *msg,
 * address higher APIC IDs.
 */
if (dmar)
-   msg->address_hi |= MSI_ADDR_EXT_DEST_ID(cfg->dest_apicid);
+   msg->arch_addr_hi.destid_8_31 = cfg->dest_apicid >> 8;
else
-   WARN_ON_ONCE(MSI_ADDR_EXT_DEST_ID(cfg->dest_apicid));
+   WARN_ON_ONCE(cfg->dest_apicid > 0xFF);
 }
 
+u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
+{
+   u32 dest = msg->arch_addr_lo.destid_0_7;
+
+