[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 12/13] xen/iommu: smmu: Add Xen specific code to be able to use the driver
Hi Stefano,
On 06/02/2015 21:20, Stefano Stabellini wrote:
On Fri, 30 Jan 2015, Julien Grall wrote:
-static int force_stage;
-module_param_named(force_stage, force_stage, int, S_IRUGO | S_IWUSR);
-MODULE_PARM_DESC(force_stage,
- "Force SMMU mappings to be installed at a particular stage of translation. A
value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no
stage is forced). Note that selecting a specific stage will disable support for nested
translation.");
for the sake of minimizing the diff, I would add
#define module_param_named(a, b, c, d)
#define MODULE_PARM_DESC(a, b)
to the Xen definitions above
We still have to drop force_stage because it will get unused and won't
compile.
So it's not too bad to remove the 2 other lines.
[..]
+static struct iommu_group *iommu_group_get(struct device *dev)
+{
+ struct iommu_group *group = dev_iommu_group(dev);
+
+ if (group)
+ atomic_inc(&group->ref);
+
+ return group;
+}
+
+#define iommu_group_get_iommudata(group) (group)->cfg
I would move all the Xen stuff at the beginning of the file or maybe to
a separate file. You could #include the linux smmu driver into it.
Maybe we cannot have runtime patching of this file, but at least we can
keep Xen and Linux stuff clearly separate.
Rather than using a void * in iommu_group, I directly use the real type
(arm_smmu_master_cfg). It's defined a bit earlier.
I would prefer to keep the real type because it's easier to understand
the code and catch possible error.
All the Xen code is beginning with /* Xen: */. Though, I could add a /*
Xen: End */ to make the separation clearer.
static DEFINE_SPINLOCK(arm_smmu_devices_lock);
static LIST_HEAD(arm_smmu_devices);
@@ -455,6 +690,8 @@ static void parse_driver_options(struct arm_smmu_device
*smmu)
static struct device_node *dev_get_dev_node(struct device *dev)
{
+ /* Xen: TODO: Add support for PCI */
+#if 0
if (dev_is_pci(dev)) {
struct pci_bus *bus = to_pci_dev(dev)->bus;
@@ -462,7 +699,7 @@ static struct device_node *dev_get_dev_node(struct device
*dev)
bus = bus->parent;
return bus->bridge->parent->of_node;
}
-
+#endif
Would it be possible instead to add:
#define dev_is_pci (0)
above among the Xen definitions?
dev_is_pci is already defined to 0 (see include/asm-arm/device.h), but
it won't prevent the compiler to check the validity of the code inside...
pci_dev doesn't contain a structure pci_bus on Xen. So it won't compile.
return dev->of_node;
}
@@ -556,6 +793,9 @@ static int register_smmu_master(struct arm_smmu_device
*smmu,
master->of_node = masterspec->np;
master->cfg.num_streamids = masterspec->args_count;
+ /* Xen: Let Xen knows that the device is protected by an SMMU */
+ dt_device_set_protected(masterspec->np);
+
for (i = 0; i < master->cfg.num_streamids; ++i) {
u16 streamid = masterspec->args[i];
@@ -651,11 +891,12 @@ static void arm_smmu_tlb_inv_context(struct
arm_smmu_domain *smmu_domain)
arm_smmu_tlb_sync(smmu);
}
-static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
+static void arm_smmu_context_fault(int irq, void *dev,
+ struct cpu_user_regs *regs)
{
- int flags, ret;
+ int flags;
u32 fsr, far, fsynr, resume;
- unsigned long iova;
+ paddr_t iova;
struct iommu_domain *domain = dev;
struct arm_smmu_domain *smmu_domain = domain->priv;
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
@@ -666,7 +907,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
*dev)
fsr = readl_relaxed(cb_base + ARM_SMMU_CB_FSR);
if (!(fsr & FSR_FAULT))
- return IRQ_NONE;
+ return;
if (fsr & FSR_IGN)
dev_err_ratelimited(smmu->dev,
@@ -678,19 +919,16 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
*dev)
far = readl_relaxed(cb_base + ARM_SMMU_CB_FAR_LO);
iova = far;
-#ifdef CONFIG_64BIT
+ /* Xen: The fault address maybe higher than 32 bits on arm32 */
far = readl_relaxed(cb_base + ARM_SMMU_CB_FAR_HI);
- iova |= ((unsigned long)far << 32);
-#endif
+ iova |= ((paddr_t)far << 32);
if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
- ret = IRQ_HANDLED;
resume = RESUME_RETRY;
} else {
dev_err_ratelimited(smmu->dev,
- "Unhandled context fault: iova=0x%08lx, fsynr=0x%x,
cb=%d\n",
+ "Unhandled context fault: iova=0x%"PRIpaddr", fsynr=0x%x,
cb=%d\n",
iova, fsynr, cfg->cbndx);
- ret = IRQ_NONE;
resume = RESUME_TERMINATE;
}
@@ -700,11 +938,10 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
*dev)
/* Retry or terminate any stalled transactions */
if (fsr & FSR_SS)
writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
-
- return ret;
}
For the sake of minimizing the changes to Linux functions, could you
write a wrapper around this function instead? That might allow you to
remove the changes related to the return value.
We should avoid to try to minimize the code against the clarity of the
code...
The change you suggest would require to modify the caller of this
function, which is less clearer than this one.
-static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
+static void arm_smmu_global_fault(int irq, void *dev,
+ struct cpu_user_regs *regs)
{
u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
struct arm_smmu_device *smmu = dev;
@@ -716,7 +953,7 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
if (!gfsr)
- return IRQ_NONE;
+ return;
dev_err_ratelimited(smmu->dev,
"Unexpected global fault, this could be serious\n");
@@ -725,9 +962,10 @@ static irqreturn_t arm_smmu_global_fault(int irq, void
*dev)
gfsr, gfsynr0, gfsynr1, gfsynr2);
writel(gfsr, gr0_base + ARM_SMMU_GR0_sGFSR);
- return IRQ_HANDLED;
}
Same here
Ditto.
+/* Xen: Page tables are shared with the processor */
+#if 0
static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
size_t size)
{
@@ -749,6 +987,7 @@ static void arm_smmu_flush_pgtable(struct arm_smmu_device
*smmu, void *addr,
DMA_TO_DEVICE);
}
}
+#endif
But then you need to fix all the call sites. I think it is best to add a
return at the beginning of the function. Again the goal is to minimize
changes to the linux driver.
There is only one call site and the field pgd is drop because pgt_t
doesn't exist on Xen. We would have to add a comment on the caller to
explain what means PTRS_PER_PGD which is meaningful on Xe.
Again, we need to find a fair trade between clarity and the number of
changes size.
If I really wanted to minize the code, I would have avoid the #if 0 and
add hundreds typedef/macro to compile the code. We need to find a fair
trade between the clarity and the size changes.
At the moment, I believe the current patch is the best we can have.
static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
{
@@ -757,6 +996,7 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
struct arm_smmu_device *smmu = smmu_domain->smmu;
void __iomem *cb_base, *gr0_base, *gr1_base;
+ paddr_t p2maddr;
gr0_base = ARM_SMMU_GR0(smmu);
gr1_base = ARM_SMMU_GR1(smmu);
@@ -840,11 +1080,16 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
}
/* TTBR0 */
- arm_smmu_flush_pgtable(smmu, cfg->pgd,
- PTRS_PER_PGD * sizeof(pgd_t));
- reg = __pa(cfg->pgd);
+ /* Xen: The page table is shared with the P2M code */
+ ASSERT(smmu_domain->cfg.domain != NULL);
+ p2maddr = page_to_maddr(smmu_domain->cfg.domain->arch.p2m.root);
+
+ dev_notice(smmu->dev, "d%u: p2maddr 0x%"PRIpaddr"\n",
+ smmu_domain->cfg.domain->domain_id, p2maddr);
+
+ reg = (p2maddr & ((1ULL << 32) - 1));
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
- reg = (phys_addr_t)__pa(cfg->pgd) >> 32;
+ reg = (p2maddr >> 32);
if (stage1)
reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
Not sure how we could get rid of this change.
Maybe we could keep pgd in arm_smmu_cfg and make it point to
smmu_domain->cfg.domain->arch.p2m.root?
Maybe, I will look at it.
@@ -886,9 +1131,21 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
reg |= (64 - smmu->s1_input_size) << TTBCR_T0SZ_SHIFT;
}
} else {
+#if CONFIG_ARM_32
+ /* Xen: Midway is using 40-bit address space. Though it may
+ * not work on other ARM32 platform with SMMU-v1.
+ * TODO: A quirk may be necessary if we have to support
+ * other ARM32 platform with SMMU-v1.
+ */
+ reg = 0x18 << TTBCR_T0SZ_SHIFT;
+#else
reg = 0;
+#endif
}
+ /* Xen: The attributes to walk the page table should be the same as
+ * VTCR_EL2. Currently doesn't differ from Linux ones.
+ */
reg |= TTBCR_EAE |
(TTBCR_SH_IS << TTBCR_SH0_SHIFT) |
(TTBCR_RGN_WBWA << TTBCR_ORGN0_SHIFT) |
@@ -1031,7 +1288,6 @@ static void arm_smmu_destroy_domain_context(struct
iommu_domain *domain)
static int arm_smmu_domain_init(struct iommu_domain *domain)
{
struct arm_smmu_domain *smmu_domain;
- pgd_t *pgd;
/*
* Allocate the domain and initialise some of its data structures.
@@ -1042,20 +1298,12 @@ static int arm_smmu_domain_init(struct iommu_domain
*domain)
if (!smmu_domain)
return -ENOMEM;
- pgd = kcalloc(PTRS_PER_PGD, sizeof(pgd_t), GFP_KERNEL);
- if (!pgd)
- goto out_free_domain;
- smmu_domain->cfg.pgd = pgd;
-
spin_lock_init(&smmu_domain->lock);
domain->priv = smmu_domain;
return 0;
-
-out_free_domain:
- kfree(smmu_domain);
- return -ENOMEM;
I would consider using explicit if (0) or #if 0 blocks instead: next
time you'll see right away what to remove.
Which will add more diff, in this case 3 #if 0 / #endif plus some
comments to explain the #if 0. Because you want to understand what is
the variable pgd.
}
+#if 0
static void arm_smmu_free_ptes(pmd_t *pmd)
{
pgtable_t table = pmd_pgtable(*pmd);
@@ -1118,6 +1366,7 @@ static void arm_smmu_free_pgtables(struct arm_smmu_domain
*smmu_domain)
kfree(pgd_base);
}
+#endif
If you use return at the beginning of the functions instead, you can
avoid changing the call sites, like below:
Calling this function would be very confusing for someone which will
debug the code. I think the #if 0/#endif of the whole functions make
more sense.
[..]
@@ -1454,10 +1704,14 @@ static void arm_smmu_detach_dev(struct iommu_domain
*domain, struct device *dev)
if (!cfg)
return;
- dev->archdata.iommu = NULL;
+ dev_iommu_domain(dev) = NULL;
arm_smmu_domain_remove_master(smmu_domain, cfg);
}
%s/dev->archdata.iommu/dev_iommu_domain(dev)/g is not too bad I guess
We have to store more data for Xen. So the only solution is to change
this code.
[..]
static int arm_smmu_add_device(struct device *dev)
{
@@ -1784,6 +2042,10 @@ static int arm_smmu_add_device(struct device *dev)
void (*releasefn)(void *) = NULL;
int ret;
+ /* Xen: Check if the device has already been added */
+ if (dev_iommu_group(dev))
+ return -EBUSY;
This is strange. Why is this check missing in Linux?
IIRC it's done in the iommu common code. But actually this check is
already done arm_smmu_add_device (Xen specific function). It may be a
left-over my development.
I will drop it.
smmu = find_smmu_for_device(dev);
if (!smmu)
return -ENODEV;
@@ -1795,6 +2057,9 @@ static int arm_smmu_add_device(struct device *dev)
}
if (dev_is_pci(dev)) {
+ /* Xen: TODO: Add PCI support */
+ BUG();
+#if 0
struct pci_dev *pdev = to_pci_dev(dev);
cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
@@ -1811,6 +2076,7 @@ static int arm_smmu_add_device(struct device *dev)
pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
&cfg->streamids[0]);
releasefn = __arm_smmu_release_pci_iommudata;
+#endif
just make dev_is_pci be (0)
dev_is_pci is already set to 0 but a if (0) won't prevent any
compilation error...
[..]
-MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
#define MODULE_DEVICE_TABLE(a, b)
Will do.
[..]
+out_free:
+ kfree(smmu->irqs);
+ if (!IS_ERR(smmu->base))
+ iounmap(smmu->base);
+ kfree(smmu);
+
return err;
It would require fewer changes to add a wrapper function.
But we won't be able to get the smmu pointer as it's not stored if the
function fail.
[..]
+DT_DEVICE_START(smmu, "ARM SMMU", DEVICE_IOMMU)
+ .dt_match = arm_smmu_of_match,
+ .init = arm_smmu_dt_init,
+DT_DEVICE_END
If possible, I would move them all together at the beginning of the
file or maybe to a separate file.
Not possible. Some code for Xen depends on the functions defined in the
Linux part. We would have to add prototype for those functions but they
are using structure defined within this file. So we would end up to add
all the code in the middle of the file.
We could do a separate file but we also need to modify some bits of the
Linux code... it's pointless.
Overall, the position of the Xen functions/definitions are placed where
they fit the most and make the whole code still clear.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
