Re: [Xen-devel] [PATCH v3 1/4] iommu/amd: support all delivery modes with x2APIC
On 15.10.2019 17:47, Roger Pau Monne wrote:
> Current AMD IOMMU code will attempt to create remapping entries for
> all IO-APIC pins, regardless of the delivery mode. AMD IOMMU
> implementation doesn't support remapping entries for IO-APIC pins with
> delivery mode set to SMI, MNI, INIT or ExtINT, instead those entries
Nit: "NMI"
> are not translated provided the right bits are set in the device table
> entry.
>
> This change checks whether the device table entry has the correct bits
> set in order to delivery the requested interrupt or a warning message
> is printed. It also translates the 32bit destination field into a
> physical or logical IO-APIC entry format. Note that if the 32bit
> destination cannot fit into the legacy format a message is printed and
> the entry is masked.
>
> Signed-off-by: Roger Pau Monné
For this to have an effect on firmware initialized RTEs, it also
requires patch 4 aiui. In fact I think it should be _only_ this
case where we allow delivery modes other than fixed and lowest
priority ("arbitrated" in AMD terminology). Hence I think this
patch wants to go last in the series, and the code be changed to
reject runtime requests to fiddle with non-"normal" delivery
modes (this may go further and actually disallow changing such
RTEs at runtime alongside disallowing their production).
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -439,6 +439,80 @@ int __init amd_iommu_setup_ioapic_remapping(void)
> return 0;
> }
>
> +void setup_forced_ioapic_rte(unsigned int apic, unsigned int pin,
> + struct amd_iommu *iommu,
> + struct IO_APIC_route_entry *rte)
> +{
> +unsigned int idx = ioapic_id_to_index(IO_APIC_ID(apic));
> +struct amd_iommu_dte *table = iommu->dev_table.buffer;
> +unsigned int req_id, dest, offset;
> +union irte_ptr entry;
> +
> +ASSERT(x2apic_enabled);
> +
> +if ( idx == MAX_IO_APICS )
Better >= ?
> +{
> +rte->mask = true;
> +return;
> +}
> +
> +req_id = get_intremap_requestor_id(ioapic_sbdf[idx].seg,
> + ioapic_sbdf[idx].bdf);
> +
> +switch ( rte->delivery_mode )
> +{
> +case dest_SMI:
> +break;
Don't you want to check the sys_mgt field here, along the lines of the
other ones below?
> +#define DEL_CHECK(type, dte_field) \
> +case dest_ ## type: \
> +if ( !table[req_id].dte_field ) \
> +printk(XENLOG_WARNING \
> + STR(type) " on IO-APIC %u pin %u will be aborted\n", \
> + apic, pin); \
> +break;
Please omit the final ; here, such that ...
> +DEL_CHECK(NMI, nmi_pass);
> +DEL_CHECK(INIT, init_pass);
> +DEL_CHECK(ExtINT, ext_int_pass);
... the ones here become an actual requirement.
> +#undef DEL_CHECK
> +
> +default:
> +ASSERT_UNREACHABLE();
> +return;
> +}
> +
> +offset = ioapic_sbdf[idx].pin_2_idx[pin];
> +if ( offset >= INTREMAP_MAX_ENTRIES )
> +{
> +rte->mask = true;
> +return;
> +}
> +
> +entry = get_intremap_entry(iommu, req_id, offset);
> +dest = get_full_dest(entry.ptr128);
> +
> +#define SET_DEST(name, dest_mask) {
> \
> +if ( dest & ~(dest_mask) )
> \
> +{
> \
> +printk(XENLOG_WARNING
> \
> + "IO-APIC %u pin %u " STR(name) " destination (%x) > %x\n",
> \
> + apic, pin, dest, dest_mask);
> \
> +rte->mask = true;
> \
> +return;
> \
> +}
> \
> +rte->dest.name.name ## _dest = dest;
> \
> +}
> +
> +if ( rte->dest_mode )
> +SET_DEST(physical, 0xf)
> +else
> +SET_DEST(logical, 0xff)
This reads as if the code was broken. Please add () around the outermost
{} of the macro, allowing you to put ; here. Or otherwise, just like
above for DEL_CHECK(), omit the {} as well as the final semicolon.
Furthermore - are you sure about this distinction? See e.g.
update_intremap_entry_from_ioapic() and
amd_iommu_setup_ioapic_remapping(), which unilaterally use
rte->dest.logical.logical_dest. Likewise io_apic.c's SET_DEST() users,
which generally pass "logical" for as middle argument. I thought one of
my half way recent patches (IRQ handling or
[Xen-devel] [PATCH v3 1/4] iommu/amd: support all delivery modes with x2APIC
Current AMD IOMMU code will attempt to create remapping entries for
all IO-APIC pins, regardless of the delivery mode. AMD IOMMU
implementation doesn't support remapping entries for IO-APIC pins with
delivery mode set to SMI, MNI, INIT or ExtINT, instead those entries
are not translated provided the right bits are set in the device table
entry.
This change checks whether the device table entry has the correct bits
set in order to delivery the requested interrupt or a warning message
is printed. It also translates the 32bit destination field into a
physical or logical IO-APIC entry format. Note that if the 32bit
destination cannot fit into the legacy format a message is printed and
the entry is masked.
Signed-off-by: Roger Pau Monné
---
My AMD hardware doesn't have any of such entries, hence I've only been
able to compile test this change and assert it doesn't affect the
functionality of other interrupt delivery modes.
---
xen/drivers/passthrough/amd/iommu_intr.c | 81
1 file changed, 81 insertions(+)
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c
b/xen/drivers/passthrough/amd/iommu_intr.c
index fb71073c84..02dd3737c7 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -439,6 +439,80 @@ int __init amd_iommu_setup_ioapic_remapping(void)
return 0;
}
+void setup_forced_ioapic_rte(unsigned int apic, unsigned int pin,
+ struct amd_iommu *iommu,
+ struct IO_APIC_route_entry *rte)
+{
+unsigned int idx = ioapic_id_to_index(IO_APIC_ID(apic));
+struct amd_iommu_dte *table = iommu->dev_table.buffer;
+unsigned int req_id, dest, offset;
+union irte_ptr entry;
+
+ASSERT(x2apic_enabled);
+
+if ( idx == MAX_IO_APICS )
+{
+rte->mask = true;
+return;
+}
+
+req_id = get_intremap_requestor_id(ioapic_sbdf[idx].seg,
+ ioapic_sbdf[idx].bdf);
+
+switch ( rte->delivery_mode )
+{
+case dest_SMI:
+break;
+
+#define DEL_CHECK(type, dte_field) \
+case dest_ ## type: \
+if ( !table[req_id].dte_field ) \
+printk(XENLOG_WARNING \
+ STR(type) " on IO-APIC %u pin %u will be aborted\n", \
+ apic, pin); \
+break;
+
+DEL_CHECK(NMI, nmi_pass);
+DEL_CHECK(INIT, init_pass);
+DEL_CHECK(ExtINT, ext_int_pass);
+#undef DEL_CHECK
+
+default:
+ASSERT_UNREACHABLE();
+return;
+}
+
+offset = ioapic_sbdf[idx].pin_2_idx[pin];
+if ( offset >= INTREMAP_MAX_ENTRIES )
+{
+rte->mask = true;
+return;
+}
+
+entry = get_intremap_entry(iommu, req_id, offset);
+dest = get_full_dest(entry.ptr128);
+
+#define SET_DEST(name, dest_mask) {
\
+if ( dest & ~(dest_mask) )
\
+{
\
+printk(XENLOG_WARNING
\
+ "IO-APIC %u pin %u " STR(name) " destination (%x) > %x\n",
\
+ apic, pin, dest, dest_mask);
\
+rte->mask = true;
\
+return;
\
+}
\
+rte->dest.name.name ## _dest = dest;
\
+}
+
+if ( rte->dest_mode )
+SET_DEST(physical, 0xf)
+else
+SET_DEST(logical, 0xff)
+#undef SET_DEST
+
+return;
+}
+
void amd_iommu_ioapic_update_ire(
unsigned int apic, unsigned int reg, unsigned int value)
{
@@ -482,6 +556,13 @@ void amd_iommu_ioapic_update_ire(
*((u32 *)_rte) = value;
/* read upper 32 bits from io-apic rte */
*(((u32 *)_rte) + 1) = __io_apic_read(apic, reg + 1);
+
+if ( new_rte.delivery_mode > 1 && x2apic_enabled )
+{
+setup_forced_ioapic_rte(apic, pin, iommu, _rte);
+__ioapic_write_entry(apic, pin, true, new_rte);
+return;
+}
}
else
{
--
2.23.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
