Re: [PATCH v2 3/7] iommu/vt-d: Expose ISA direct mapping region via iommu_get_resv_regions
From: Lu Baolu
Date: Mon Mar 25 2019 - 21:16:12 EST
Hi,
On 3/25/19 8:57 PM, James Sewart wrote:
Hey Lu,
On 25 Mar 2019, at 02:03, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
Hi James,
On 3/22/19 5:57 PM, James Sewart wrote:
Hey Lu,
On 15 Mar 2019, at 02:19, Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
Hi James,
On 3/14/19 7:58 PM, James Sewart wrote:
To support mapping ISA region via iommu_group_create_direct_mappings,
make sure its exposed by iommu_get_resv_regions. This allows
deduplication of reserved region mappings
Signed-off-by: James Sewart <jamessewart@xxxxxxxxxx>
---
drivers/iommu/intel-iommu.c | 42 +++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 8e0a4e2ff77f..2e00e8708f06 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -337,6 +337,8 @@ static LIST_HEAD(dmar_rmrr_units);
#define for_each_rmrr_units(rmrr) \
list_for_each_entry(rmrr, &dmar_rmrr_units, list)
+static struct iommu_resv_region *isa_resv_region;
+
/* bitmap for indexing intel_iommus */
static int g_num_of_iommus;
@@ -2780,26 +2782,34 @@ static inline int iommu_prepare_rmrr_dev(struct dmar_rmrr_unit *rmrr,
rmrr->end_address);
}
+static inline struct iommu_resv_region *iommu_get_isa_resv_region(void)
+{
+ if (!isa_resv_region)
+ isa_resv_region = iommu_alloc_resv_region(0,
+ 16*1024*1024,
+ 0, IOMMU_RESV_DIRECT);
+
+ return isa_resv_region;
+}
+
#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
-static inline void iommu_prepare_isa(void)
+static inline void iommu_prepare_isa(struct pci_dev *pdev)
{
- struct pci_dev *pdev;
int ret;
+ struct iommu_resv_region *reg = iommu_get_isa_resv_region();
- pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
- if (!pdev)
+ if (!reg)
return;
pr_info("Prepare 0-16MiB unity mapping for LPC\n");
- ret = iommu_prepare_identity_map(&pdev->dev, 0, 16*1024*1024 - 1);
+ ret = iommu_prepare_identity_map(&pdev->dev, reg->start,
+ reg->start + reg->length - 1);
if (ret)
pr_err("Failed to create 0-16MiB identity map - floppy might not work\n");
-
- pci_dev_put(pdev);
}
#else
-static inline void iommu_prepare_isa(void)
+static inline void iommu_prepare_isa(struct pci_dev *pdev)
{
return;
}
@@ -3289,6 +3299,7 @@ static int __init init_dmars(void)
struct dmar_rmrr_unit *rmrr;
bool copied_tables = false;
struct device *dev;
+ struct pci_dev *pdev;
struct intel_iommu *iommu;
int i, ret;
@@ -3469,7 +3480,11 @@ static int __init init_dmars(void)
}
}
- iommu_prepare_isa();
+ pdev = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
+ if (pdev) {
+ iommu_prepare_isa(pdev);
+ pci_dev_put(pdev);
+ }
domains_done:
@@ -5266,6 +5281,7 @@ static void intel_iommu_get_resv_regions(struct device *device,
struct iommu_resv_region *reg;
struct dmar_rmrr_unit *rmrr;
struct device *i_dev;
+ struct pci_dev *pdev;
int i;
rcu_read_lock();
@@ -5280,6 +5296,14 @@ static void intel_iommu_get_resv_regions(struct device *device,
}
rcu_read_unlock();
+ if (dev_is_pci(device)) {
+ pdev = to_pci_dev(device);
+ if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
+ reg = iommu_get_isa_resv_region();
+ list_add_tail(®->list, head);
+ }
+ }
+
Just wondering why not just
+#ifdef CONFIG_INTEL_IOMMU_FLOPPY_WA
+ if (dev_is_pci(device)) {
+ pdev = to_pci_dev(device);
+ if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
+ reg = iommu_alloc_resv_region(0,
+ 16*1024*1024,
+ 0, IOMMU_RESV_DIRECT);
+ if (reg)
+ list_add_tail(®->list, head);
+ }
+ }
+#endif
and, remove all other related code?
At this point in the patchset if we remove iommu_prepare_isa then the ISA
region wonât be mapped to the device. Only once the dma domain is allocable
will the reserved regions be mapped by iommu_group_create_direct_mappings.
Yes. So if we put the allocation code here, it won't impact anything and
will take effect as soon as the dma domain is allocatable.
Theres an issue that if we choose to alloc a new resv_region with type
IOMMU_RESV_DIRECT, we will need to refactor intel_iommu_put_resv_regions
to free this entry type which means refactoring the rmrr regions in
get_resv_regions. Should this work be in this patchset?
Do you mean the rmrr regions are not allocated in get_resv_regions, but
are freed in put_resv_regions? I think we should fix this in this patch
set since this might impact the device passthrough if we don't do it.
Theyâre not allocated and not freed currently, only type IOMMU_RESV_MSI is
freed in put_resv_regions. If we allocate a new resv_region with type
IOMMU_RESV_DIRECT for the isa region, then it wonât be freed. If we modify
put_resv_regions to free type IOMMU_RESV_DIRECT, then we will try to free
the static RMRR regions.
Ah yes! Thanks for the explanation.
Either the ISA region is static and not freed as with my implementation,
or the RMRR regions are converted to be allocated on each call to
get_resv_regions and freed in put_resv_regions.
Okay, I second your implementation now.
Best regards,
Lu Baolu
Cheers,
James.
Best regards,
Lu Baolu