Re: Re: [PATCH 1/2] iommu/riscv: Map IMSIC addresses for paging domains

[fangyu.yu@linux.alibaba.com]

>> >When IOMMU_DMA is enabled, devices get paging domains and MSI writes
>> >to IMSIC interrupt files must be handled correctly in the s-stage.
>> >As the device always writes to the host physical IMSIC addresses,
>> >which the IMSIC irqchip programs directly, install s-stage identity
>> >mappings for the host IMSICs. But, use IOMMU_RESV_DIRECT_RELAXABLE
>> >since the 1:1 mappings aren't required for device assignment.
>> >
>> >Loop over the cpus rather than imsic groups to handle asymmetric
>> >configurations.
>> >
>> >Signed-off-by: Andrew Jones <andrew.jones@oss.qualcomm.com>
>> >---
>> > drivers/iommu/riscv/iommu.c         | 34 +++++++++++++++++++++++++++++
>> > include/linux/irqchip/riscv-imsic.h |  7 ++++++
>> > 2 files changed, 41 insertions(+)
>> >
>> >diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>> >index a31f50bbad35..3c6aa9d69f95 100644
>> >--- a/drivers/iommu/riscv/iommu.c
>> >+++ b/drivers/iommu/riscv/iommu.c
>> >@@ -19,6 +19,7 @@
>> > #include <linux/init.h>
>> > #include <linux/iommu.h>
>> > #include <linux/iopoll.h>
>> >+#include <linux/irqchip/riscv-imsic.h>
>> > #include <linux/kernel.h>
>> > #include <linux/pci.h>
>> > #include <linux/generic_pt/iommu.h>
>> >@@ -1286,6 +1287,38 @@ static struct iommu_domain *riscv_iommu_alloc_paging_domain(struct device *dev)
>> > 	return &domain->domain;
>> > }
>> >
>> >+static void riscv_iommu_get_resv_regions(struct device *dev, struct list_head *head)
>> >+{
>> >+	const struct imsic_global_config *imsic_global;
>> >+	unsigned int cpu;
>> >+
>> >+	if (!imsic_enabled())
>> >+		return;
>> >+
>> >+	imsic_global = imsic_get_global_config();
>> >+
>> >+	for_each_possible_cpu(cpu) {
>> >+		const struct imsic_local_config *local;
>> >+		struct iommu_resv_region *reg;
>> >+
>> >+		local = per_cpu_ptr(imsic_global->local, cpu);
>> >+		if (!local->msi_va)
>> >+			continue;
>> >+
>> >+		/*
>> >+		 * The device always writes to the host physical IMSIC address, so install
>> >+		 * identity mappings directly. Use IOMMU_RESV_DIRECT_RELAXABLE instead of
>> >+		 * IOMMU_RESV_DIRECT since these 1:1 mappings are not required for assigned
>> >+		 * devices.
>> >+		 */
>> >+		reg = iommu_alloc_resv_region(local->msi_pa, IMSIC_MMIO_PAGE_SZ,
>> >+					      IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO,
>> >+					      IOMMU_RESV_DIRECT_RELAXABLE, GFP_KERNEL);
>> >+		if (reg)
>> >+			list_add_tail(&reg->list, head);
>> >+	}
>> >+}
>> >+
>> 
>> Hi Andrew,
>> 
>> Thanks for picking this up -- enabling IOMMU_DMA on RISC-V has been
>> along-standing gap, and handling the IMSIC MSI mapping is the missing
>> piece that finally unblocks it.
>> 
>> One concern is that the current implementation emits one 4 KiB
>> RESV_DIRECT_RELAXABLE region for each possible CPU. On platforms
>> with hundreds of harts, this noticeably increases the cost of both
>> .get_resv_regions() and the iommu_create_device_direct_mappings()
>> walk.
>> 
>> Since interrupt files within one IMSIC group occupy a physically
>> contiguous range, would it make sense to emit one region per IMSIC
>> group covering the full group stride, aligned down/up to 2 MiB so
>> the core can map it as a superpage? This would over-map some padding
>> within the IMSIC PA window, but RESV_DIRECT_RELAXABLE keeps the
>> padding out of assigned-device IOVA space, so it looks harmless.
>> 
>
>Hi Fangyu,
>
>Thanks for pointing out this issue. We'll need to decide how much we
>want to isolate the devices from VMs in order to address it, though,
>because, if we do group mappings, then we'll also be exposing the guest
>interrupt files to the devices.
>
>I'll certainly send a v2 to do larger mappings when s-mode imsics are
>contiguous (nr-guest-files = 0) and then we can consider creating a
>way to opt-in to group mappings even when nr-guest-files != 0. How's
>that sound?
>

Hi Andrew,

As you mentioned, my suggestion could also expose the guest interrupt
files to the device-visible/mappable range, but the current approach
already appears to provide only limited isolation. So I think this
change would not materially increase the risk.

In any case, I respect your decision, and we can certainly proceed
with your current suggestion for now.

Thanks,
Fangyu

>Thanks,
>drew