On 20.07.19 21:25, Julien Grall wrote:
Hi,
Hi, Julien.
Apologies for the late answer. I have been traveling for the past few
weeks.
Absolutely no problem. Thank you for your review.
On 6/26/19 11:30 AM, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <[email protected]>
The IPMMU-VMSA is VMSA-compatible I/O Memory Management Unit (IOMMU)
which provides address translation and access protection functionalities
to processing units and interconnect networks.
Do you have a link to the specification?
All I have is a TRM. Unfortunately, I can't share it.
My knowledge about the IPMMU is quite limited, so for now, I will
mostly look at Xen specific code. It would be good if someone with a
better knowledge of the driver could have a look.
I understand your point. Make sense. I will CC a person who is familiar
with H/W.
[...]
diff --git a/xen/drivers/passthrough/Kconfig
b/xen/drivers/passthrough/Kconfig
index a3c0649..e57aa29 100644
--- a/xen/drivers/passthrough/Kconfig
+++ b/xen/drivers/passthrough/Kconfig
@@ -12,4 +12,17 @@ config ARM_SMMU
Say Y here if your SoC includes an IOMMU device
implementing the
ARM SMMU architecture.
+
+config IPMMU_VMSA
+ bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs"
+ default y
I would prefer this to be a default N for the time-being.
ok
+ depends on ARM_64
+ ---help---
+ Support for implementations of the Renesas IPMMU-VMSA found
+ in R-Car Gen3 SoCs.
+
+ Say Y here if you are using newest R-Car Gen3 SoCs revisions
which IPMMU
What new now will be old soon ;). So it would be best if you give a
precise revision here.
yes ;). Will clarify.
+ hardware supports stage 2 translation table format and is able
to use
+ CPU's P2M table as is.
+
endif
diff --git a/xen/drivers/passthrough/arm/Makefile
b/xen/drivers/passthrough/arm/Makefile
index b3efcfd..40ac7a9 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,3 @@
obj-y += iommu.o
obj-$(CONFIG_ARM_SMMU) += smmu.o
+obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
new file mode 100644
index 0000000..5091c61
--- /dev/null
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -0,0 +1,1487 @@
+/*
+ * xen/drivers/passthrough/arm/ipmmu-vmsa.c
+ *
+ * Driver for the Renesas IPMMU-VMSA found in R-Car Gen3 SoCs.
+ *
+ * The IPMMU-VMSA is VMSA-compatible I/O Memory Management Unit (IOMMU)
+ * which provides address translation and access protection
functionalities
+ * to processing units and interconnect networks.
+ *
+ * Please note, current driver is supposed to work only with newest
Gen3 SoCs
+ * revisions which IPMMU hardware supports stage 2 translation table
format and
+ * is able to use CPU's P2M table as is.
+ *
+ * Based on Linux's IPMMU-VMSA driver from Renesas BSP:
+ * drivers/iommu/ipmmu-vmsa.c
What are the major differences compare the Linux driver?
Well, the major differences are:
1. Stage 1/Stage 2 translation. Linux driver supports Stage 1
translation only (with Stage 1 translation table format). It manages
page table by itself. But Xen driver supports Stage 2 translation (with
Stage 2 translation table format) to be able to share the page table
with the CPU. Stage 1 translation is always bypassed in Xen driver.
So, Xen driver is supposed to be used with newest Gen3 SoC revisions
only (H3 ES3.0, M3 ES3.0, etc.) which IPMMU hardware does support stage
2 translation table format.
2. AArch64 support. Linux driver uses VMSAv8-32 mode, while Xen driver
enables Armv8 VMSAv8-64 mode to cover up to 40 bit input address.
3. Context bank (sets of page table) usage. In Xen, each context bank is
mapped to one Xen domain. So, all devices being pass throughed to the
same Xen domain share the same context bank.
+ * you can found at:
+ * url:
git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git
+ * branch: v4.14.75-ltsi/rcar-3.9.2
+ * commit: a5266d298124874c2c06b8b13d073f6ecc2ee355
Is there any reason to use the BSP driver and not the one provided by
Linux directly?
I was thinking the BSP driver is a *little bit* more updated than Linux
one. Sometime it was a big difference between mainline and BSP driver.
But now
the difference is not big and mostly in DDR_BACKUP and WHITELIST
support. I looked at mainline driver as well when implementing Xen driver.
[...]
+#define dev_archdata(dev) ((struct ipmmu_vmsa_xen_device
*)dev->archdata.iommu)
+
+/* Root/Cache IPMMU device's information */
+struct ipmmu_vmsa_device
+{
AFAICT, this file was converted to Xen coding style. If so, the style
for struct is
struct ... {
...
};
Yes, will correct everywhere in this file.
+ struct device *dev;
+ void __iomem *base;
+ struct ipmmu_vmsa_device *root;
+ struct list_head list;
+ unsigned int num_utlbs;
+ unsigned int num_ctx;
+ spinlock_t lock; /* Protects ctx and domains[] */
+ DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+ struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
+};
+
+/*
+ * Root/Cache IPMMU domain's information
+ *
+ * Root IPMMU device is assigned to Root IPMMU domain while Cache
IPMMU device
+ * is assigned to Cache IPMMU domain. Master devices are connected
to Cache
+ * IPMMU devices through specific ports called micro-TLBs.
+ * All Cache IPMMU devices, in turn, are connected to Root IPMMU device
+ * which manages IPMMU context.
+ */
+struct ipmmu_vmsa_domain
+{
Ditto.
ok
+ /*
+ * IPMMU device assigned to this IPMMU domain.
+ * Either Root device which is located at the main memory bus
domain or
+ * Cache device which is located at each hierarchy bus domain.
+ */
+ struct ipmmu_vmsa_device *mmu;
+
+ /* Context used for this IPMMU domain */
+ unsigned int context_id;
+
+ /* Xen domain associated with this IPMMU domain */
+ struct domain *d;
+
+ /* The fields below are used for Cache IPMMU domain only */
+
+ /*
+ * Used to keep track of the master devices which are attached
to this
+ * IPMMU domain (domain users). Master devices behind the same
IPMMU device
+ * are grouped together by putting into the same IPMMU domain.
+ * Only when the refcount reaches 0 this IPMMU domain can be
destroyed.
+ */
+ int refcount;
If the refcount cannot be 0, then I would prefer an unsigned int here.
It can be >= 0.
+ /* Used to link this IPMMU domain for the same Xen domain */
+ struct list_head list;
+};
[...]
+/* Read/Write Access */
+static u32 ipmmu_read(struct ipmmu_vmsa_device *mmu, unsigned int
offset)
If you are going to use Xen coding style, then this should be
"uint32_t". The same is valid everywhere in this file, I am not going
to mention all of them :).
Yes, will change everywhere in this file.
+{
+ return readl(mmu->base + offset);
+}
+
+static void ipmmu_write(struct ipmmu_vmsa_device *mmu, unsigned int
offset,
+ u32 data)
+{
+ writel(data, mmu->base + offset);
+}
+
+static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain,
+ unsigned int reg)
+{
+ return ipmmu_read(domain->mmu->root,
+ domain->context_id * IM_CTX_SIZE + reg);
+}
+
+static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain,
+ unsigned int reg, u32 data)
+{
+ ipmmu_write(domain->mmu->root,
+ domain->context_id * IM_CTX_SIZE + reg, data);
+}
+
+static void ipmmu_ctx_write_cache(struct ipmmu_vmsa_domain *domain,
+ unsigned int reg, u32 data)
+{
+ ASSERT(reg == IMCTR);
What's the rationale of passing reg in parameter if it can only be
equal to IMCTR?
Good question. I tried to retain the same interface as for
ipmmu_ctx_write_root(_all) for visibility.
Cache IPMMU device has other than IMCTR context registers, but they are
not used by this driver.
Shall I drop reg parameter?
+
+ /* Mask fields which are implemented in IPMMU-MM only. */
+ if ( !ipmmu_is_root(domain->mmu) )
+ ipmmu_write(domain->mmu, domain->context_id * IM_CTX_SIZE +
reg,
+ data & IMCTR_COMMON_MASK);
+}
[...]
+/* Enable MMU translation for the micro-TLB. */
+static void ipmmu_utlb_enable(struct ipmmu_vmsa_domain *domain,
+ unsigned int utlb)
+{
+ struct ipmmu_vmsa_device *mmu = domain->mmu;
+
+ /*
+ * TODO: Reference-count the micro-TLB as several bus masters
can be
+ * connected to the same micro-TLB.
+ */
What would be the exact problem if this is not handled? Could a utlb
shared between multiple domain?
Actually, this TODO as well as the whole "TLB and micro-TLB Management"
code I tried to retain as much as possible.
The large amount of devices have unique micro-TLB connection, but there
is a case, when not. At the moment, I don't see any problem if these
potential devices (which share the same utlb) are assigned to the same
Xen domain.
In this case we would just enable the same utlb more than once at the
domain creation time (assign a device) and disable the same utlb more
than once at the domain destruction time (deassign a device).
The worst case scenario would be when these devices are assigned to
different Xen domains. So, I think, the same utlb *can't* be shared
between multiple Xen domains, since it points to the context bank to use
for the page walk.
+ ipmmu_write(mmu, IMUASID(utlb), 0);
+ ipmmu_write(mmu, IMUCTR(utlb), ipmmu_read(mmu, IMUCTR(utlb)) |
+ IMUCTR_TTSEL_MMU(domain->context_id) | IMUCTR_MMUEN);
+}
[...]
+static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
+{
+ u64 ttbr;
s/u64/uint64_t/
ok
+ u32 tsz0;
+ int ret;
+
+ /* Find an unused context. */
+ ret = ipmmu_domain_allocate_context(domain->mmu->root, domain);
+ if ( ret < 0 )
+ return ret;
+
+ domain->context_id = ret;
+
+ /*
+ * TTBR0
+ * Use P2M table for this Xen domain.
+ */
+ ASSERT(domain->d != NULL);
+ ttbr = page_to_maddr(domain->d->arch.p2m.root);
+
+ dev_info(domain->mmu->root->dev, "d%d: Set IPMMU context %u (pgd
0x%"PRIx64")\n",
We introduce a format specifier to print a domain. So you can use %pd
in combination of just domain->d.
Such a convenient thing. Will use.
+ domain->d->domain_id, domain->context_id, ttbr);
+
+ ipmmu_ctx_write_root(domain, IMTTLBR0, ttbr & IMTTLBR0_TTBR_MASK);
+ ipmmu_ctx_write_root(domain, IMTTUBR0, (ttbr >> 32) &
IMTTUBR0_TTBR_MASK);
+
+ /*
+ * TTBCR
+ * We use long descriptors with inner-shareable WBWA tables and
allocate
+ * the whole "p2m_ipa_bits" IPA space to TTBR0. Use 4KB page
granule.
+ * Start page table walks at first level. Bypass stage 1
translation
+ * when only stage 2 translation is performed.
I am not sure to understand the last sentence. You only use stage2
right, so it shouldn't it be "Always bypass stage 1 translation"?
Yes, you are right. Will rephrase.
+ */
+ tsz0 = (64 - p2m_ipa_bits) << IMTTBCR_TSZ0_SHIFT;
+ ipmmu_ctx_write_root(domain, IMTTBCR, IMTTBCR_EAE | IMTTBCR_PMB |
+ IMTTBCR_SH0_INNER_SHAREABLE |
IMTTBCR_ORGN0_WB_WA |
+ IMTTBCR_IRGN0_WB_WA | IMTTBCR_SL0_LVL_1 |
tsz0);
[...]
+/* Fault Handling */
+static void ipmmu_domain_irq(struct ipmmu_vmsa_domain *domain)
+{
+ const u32 err_mask = IMSTR_MHIT | IMSTR_ABORT | IMSTR_PF |
IMSTR_TF;
+ struct ipmmu_vmsa_device *mmu = domain->mmu;
+ u32 status;
+ u64 iova;
+
+ status = ipmmu_ctx_read_root(domain, IMSTR);
+ if ( !(status & err_mask) )
+ return;
+
+ iova = ipmmu_ctx_read_root(domain, IMELAR) |
+ ((u64)ipmmu_ctx_read_root(domain, IMEUAR) << 32);
+
+ /*
+ * Clear the error status flags. Unlike traditional interrupt flag
+ * registers that must be cleared by writing 1, this status
register
+ * seems to require 0. The error address register must be read
before,
+ * otherwise its value will be 0.
+ */
+ ipmmu_ctx_write_root(domain, IMSTR, 0);
+
+ /* Log fatal errors. */
+ if ( status & IMSTR_MHIT )
+ dev_err_ratelimited(mmu->dev, "d%d: Multiple TLB hits
@0x%"PRIx64"\n",
Similar remark for d%d here ...
ok
+ domain->d->domain_id, iova);
+ if ( status & IMSTR_ABORT )
+ dev_err_ratelimited(mmu->dev, "d%d: Page Table Walk Abort
@0x%"PRIx64"\n",
... here and ...
ok
+ domain->d->domain_id, iova);
+
+ /* Return if it is neither Permission Fault nor Translation
Fault. */
+ if ( !(status & (IMSTR_PF | IMSTR_TF)) )
+ return;
+
+ /* Flush the TLB as required when IPMMU translation error
occurred. */
+ ipmmu_tlb_invalidate(domain);
+
+ dev_err_ratelimited(mmu->dev, "d%d: Unhandled fault: status
0x%08x iova 0x%"PRIx64"\n",
... here.
ok
+ domain->d->domain_id, status, iova);
+}
+
+static void ipmmu_irq(int irq, void *dev, struct cpu_user_regs *regs)
+{
+ struct ipmmu_vmsa_device *mmu = dev;
+ unsigned int i;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mmu->lock, flags);
+
+ /*
+ * When interrupt arrives, we don't know the context it is
related to.
+ * So, check interrupts for all active contexts to locate a context
+ * with status bits set.
+ */
+ for ( i = 0; i < mmu->num_ctx; i++ )
+ {
+ if ( !mmu->domains[i] )
+ continue;
+ ipmmu_domain_irq(mmu->domains[i]);
+ }
+
+ spin_unlock_irqrestore(&mmu->lock, flags);
+}
+
+/* Master devices management */
+static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
+ struct device *dev)
+{
+ struct ipmmu_vmsa_master_cfg *cfg = dev_archdata(dev)->cfg;
+ struct ipmmu_vmsa_device *mmu = cfg->mmu;
+ unsigned int i;
+
+ if ( !mmu )
+ {
+ dev_err(dev, "Cannot attach to IPMMU\n");
+ return -ENXIO;
+ }
+
+ if ( !domain->mmu )
So you read domain->mmu here and ...
+ {
+ /* The domain hasn't been used yet, initialize it. */
+ domain->mmu = mmu;
+
+ /*
+ * We have already enabled context for Root IPMMU assigned
to this
+ * Xen domain in ipmmu_domain_init_context().
+ * Enable the context for Cache IPMMU only. Flush the TLB as
required
+ * when modifying the context registers.
+ */
+ ipmmu_ctx_write_cache(domain, IMCTR,
+ ipmmu_ctx_read_root(domain, IMCTR) |
IMCTR_FLUSH);
+
+ dev_info(dev, "Using IPMMU context %u\n", domain->context_id);
+ }
+ else if ( domain->mmu != mmu )
... here. What actually promise that domain->mmu can't change in
parallel?
ipmmu_attach_device is protected by xen_domain->lock
+ {
+ /*
+ * Something is wrong, we can't attach two master devices using
+ * different IOMMUs to the same IPMMU domain.
+ */
+ dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n",
+ dev_name(mmu->dev), dev_name(domain->mmu->dev));
+ return -EINVAL;
+ }
+ else
+ dev_info(dev, "Reusing IPMMU context %u\n",
domain->context_id);
+
+ for ( i = 0; i < cfg->num_utlbs; ++i )
+ ipmmu_utlb_enable(domain, cfg->utlbs[i]);
+
+ return 0;
+}
[...]
+static void ipmmu_protect_masters(struct ipmmu_vmsa_device *mmu)
+{
+ struct dt_device_node *node;
+
+ dt_for_each_device_node( dt_host, node )
+ {
+ if ( mmu->dev->of_node != dt_parse_phandle(node, "iommus", 0) )
+ continue;
+
+ /* Let Xen know that the master device is protected by an
IOMMU. */
+ dt_device_set_protected(node);
+
+ dev_info(mmu->dev, "Found master device %s\n",
dt_node_full_name(node));
+ }
+}
I don't much like this. You are going to go through the whole DT quite
a few times.
+1
The IOMMU interface in Xen has not been designed with the new IOMMU
bindings in mind. I would prefer if we look for extending add_device
callback to support platform device.
This would allow to probe the device later on and therefore avoid to
go through the device-tree multiple.
I completely agree with you that current implementation is not optimal
and should be reworked in order not to scan the whole DT many times, but
I am not completely understand what we should do and how exactly.
Could you, please, add more details?
+
+static void ipmmu_device_reset(struct ipmmu_vmsa_device *mmu)
+{
+ unsigned int i;
+
+ /* Disable all contexts. */
+ for ( i = 0; i < mmu->num_ctx; ++i )
+ ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
+}
+
+/*
+ * This function relies on the fact that Root IPMMU device is being
probed
+ * the first. If not the case, it denies further Cache IPMMU device
probes
+ * (returns the -ENODEV) until the Root IPMMU device has been
registered
+ * for sure.
Can we look at handling -EDEFER in Xen instead?
I am not sure this is something we should implement at this stage (while
only IPMMU driver would be a user). I have already resolved that
possible issue by trying to locate a Root IPMMU device and probe it the
first
to avoid the case described above. So now, we don't depend on how IPMMU
devices are located in DT. Please, see ipmmu_init(). So, I tend to live
with it some time.
+ */
[...]
+static int __must_check ipmmu_map_page(struct domain *d, dfn_t dfn,
mfn_t mfn,
+ unsigned int flags,
+ unsigned int *flush_flags)
The function is exactly the same as for the SMMU driver. Could we
implement common helpers in a separate file?
Sounds indeed reasonable. I will look at it.
+{
+ p2m_type_t t;
+
+ /*
+ * Grant mappings can be used for DMA requests. The dev_bus_addr
+ * returned by the hypercall is the MFN (not the IPA). For device
+ * protected by an IOMMU, Xen needs to add a 1:1 mapping in the
domain
+ * p2m to allow DMA request to work.
+ * This is only valid when the domain is directed mapped. Hence
this
+ * function should only be used by gnttab code with gfn == mfn
== dfn.
+ */
+ BUG_ON(!is_domain_direct_mapped(d));
+ BUG_ON(mfn_x(mfn) != dfn_x(dfn));
+
+ /* We only support readable and writable flags */
+ if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
+ return -EINVAL;
+
+ t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw :
p2m_iommu_map_ro;
+
+ /*
+ * The function guest_physmap_add_entry replaces the current
mapping
+ * if there is already one...
+ */
+ return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)),
_mfn(dfn_x(dfn)), 0, t);
+}
+
+static int __must_check ipmmu_unmap_page(struct domain *d, dfn_t dfn,
+ unsigned int *flush_flags)
Same here.
ok
----------
Julien, what we should do with the fact that IPMMU supports only 3-level
page table?
I left a TODO regarding that, but we need to work out some usable
solution if possible.
/*
* As 4-level translation table is not supported in IPMMU, we need
* to check IPA size used for P2M table beforehand to be sure it is
* 3-level and the IPMMU will be able to use it.
*
* In case of using 4KB page granule we should use two concatenated
* translation tables at level 1 in order to support 40 bit IPA
* with 3-level translation table.
*
* TODO: Probably, when determing the "pa_range" in
setup_virt_paging()
* we should take into the account the IPMMU ability as well.
*/
if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
{
dev_err(&node->dev, "P2M IPA size is not supported (P2M=%u
IPMMU=%u)!\n",
p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
return -EOPNOTSUPP;
}
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
