[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH v1 2/7] iommu/arm: ipmmu-vmsa: Add Xen changes for main driver
Hi,
On 10/08/17 15:27, Oleksandr Tyshchenko wrote:
On Tue, Aug 8, 2017 at 2:34 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
On 26/07/17 16:09, Oleksandr Tyshchenko wrote:
@@ -355,6 +557,10 @@ static struct hw_register
*root_pgtable[IPMMU_CTX_MAX] = {
static bool ipmmu_is_root(struct ipmmu_vmsa_device *mmu)
{
+ /* Xen: Fix */
Hmmm. Can we get a bit more details?
These is a case when ipmmu_is_root is called with "mmu" being NULL.
https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2330
In ipmmu_vmsa_alloc_page_table() we need to find "root mmu", but we
doesn't have argument to pass.
So, I had two options:
1. Add code searching for it.
...
spin_lock(&ipmmu_devices_lock);
list_for_each_entry(mmu, &ipmmu_devices, list) {
if (ipmmu_is_root(mmu))
break;
}
spin_unlock(&ipmmu_devices_lock);
...
2. Use existing ipmmu_find_root() with adding this check for a valid value.
So, if we call ipmmu_find_root() with argument being NULL we will
actually get searching the list.
I decided to use 2 option.
Can you please expand the comment then?
+ if (!mmu)
+ return false;
+
if (mmu->features->has_cache_leaf_nodes)
return mmu->is_leaf ? false : true;
else
@@ -405,14 +611,28 @@ static void ipmmu_ctx_write(struct ipmmu_vmsa_domain
*domain, unsigned int reg,
ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg,
data);
}
-static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned
int reg,
+/* Xen: Write the context for cache IPMMU only. */
Same here. Why does it need to be different with Xen?
Well, let me elaborate a bit more about this.
I feel that I need to explain in a few words about IPMMU itself:
Generally speaking,
The IPMMU hardware (R-Car Gen3) has 8 context banks and consists of next parts:
- root IPMMU
- a number of cache IPMMUs
Each cache IPMMU is connected to root IPMMU and has uTLB ports the
master devices can be tied to.
Something, like this:
master device1 ---> cache IPMMU1 [8 ctx] ---> root IPMMU [8 ctx] -> memory
| |
master device2 -- |
|
master device3 ---> cache IPMMU2 [8 ctx] --
Each context bank has registers.
Some registers exist for both root IPMMU and cache IPMMUs -> IMCTR
Some registers exist only for root IPMMU -> IMTTLBRx/IMTTUBRx, IMMAIR0, etc
So, original driver has two helpers:
1. ipmmu_ctx_write() - is intended to write a register in context bank
N* for root IPMMU only.
2. ipmmu_ctx_write2() - is intended to write a register in context
bank N for both root IPMMU and cache IPMMU.
*where N=0-7
AFAIU, original Linux driver provides each IOMMU domain with a
separate IPMMU context:
master device1 + master device2 are in IOMMU domain1 and use IPMMU context 0
master device3 is in IOMMU domain2 and uses IPMMU context 1
So, when attaching device to new IOMMU domain in Linux we have to
initialize context for root IPMMU and enable context (IMCTR register)
for both root and cache IPMMUs.
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/drivers/iommu/ipmmu-vmsa.c?h=v4.9/rcar-3.5.3#n620
In Xen we need additional helper "ipmmu_ctx_write1" for writing a
register in context bank N for cache IPMMU only.
The reason is that we need a way to control cache IPMMU separately
since we have a little bit another model.
All IOMMU domains within a single Xen domain (dom_iommu(d)->arch.priv)
use the same IPMMU context N
which was initialized and enabled at the domain creation time. This
means that all master devices
that are assigned to the guest domain "d" use only this IPMMU context
N which actually contains P2M mapping for domain "d":
master device1 + master device2 are in IOMMU domain1 and use IPMMU context 0
master device3 is in IOMMU domain2 and also uses IPMMU context 0
So, when attaching device to new IOMMU domain in Xen we don't have to
initialize and enable context,
because it has been already done at domain initialization time:
https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2380
we just have to enable context for corresponding cache IPMMU only:
https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L1083
This is the main difference between drivers in Linux and Xen.
So, as you can see there is a need to manipulate context registers for
cache IPMMU without touching root IPMMU,
that's why I added this helper.
Does this make sense?
I think it does.
+static void ipmmu_ctx_write1(struct ipmmu_vmsa_domain *domain, unsigned
int reg,
u32 data)
{
if (domain->mmu != domain->root)
- ipmmu_write(domain->mmu,
- domain->context_id * IM_CTX_SIZE + reg, data);
+ ipmmu_write(domain->mmu, domain->context_id * IM_CTX_SIZE
+ reg, data);
+}
- ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg,
data);
+/*
+ * Xen: Write the context for both root IPMMU and all cache IPMMUs
+ * that assigned to this Xen domain.
+ */
+static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned
int reg,
+ u32 data)
+{
+ struct ipmmu_vmsa_xen_domain *xen_domain =
dom_iommu(domain->d)->arch.priv;
+ struct iommu_domain *io_domain;
+
+ list_for_each_entry(io_domain, &xen_domain->contexts, list)
+ ipmmu_ctx_write1(to_vmsa_domain(io_domain), reg, data);
+
+ ipmmu_ctx_write(domain, reg, data);
}
/*
-----------------------------------------------------------------------------
@@ -488,6 +708,10 @@ static void ipmmu_tlb_flush_all(void *cookie)
{
struct ipmmu_vmsa_domain *domain = cookie;
+ /* Xen: Just return if context_id has non-existent value */
Same here.
I think that there is a possible race.
In ipmmu_domain_init_context() we are trying to allocate context and
if allocation fails we will call free_io_pgtable_ops(),
but "domain->context_id" hasn't been initialized yet (likely it is 0).
https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L799
And having following call stack:
free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
we will get a mistaken cache flush for a context pointed by
uninitialized "domain->context_id".
That's why I initialized context_id with non-existent value before
allocating context
https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L792
and checked it for a valid value here
https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L712
and everywhere it is need to checked.
The race is in the code added or the one from Linux? If the latter, then
you should have an action to fix it there. If the former, the I'd like
to understand how come we introduced a race compare to Linux.
[...]
+
/*
* Find an unused context.
*/
@@ -578,6 +807,11 @@ static int ipmmu_domain_init_context(struct
ipmmu_vmsa_domain *domain)
/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
+
+ /* Xen: */
+ dev_notice(domain->root->dev, "d%d: Set IPMMU context %u (pgd
0x%"PRIx64")\n",
+ domain->d->domain_id, domain->context_id, ttbr);
If you want to keep driver close to Linux, then you need to avoid unecessary
change.
Shall I drop it?
Depends. How useful is it? If it is, then may you want to upstream that?
[...]
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
@@ -787,7 +1042,20 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
/* The domain hasn't been used yet, initialize it. */
domain->mmu = mmu;
domain->root = root;
+
+/*
+ * Xen: We have already initialized and enabled context for root IPMMU
+ * for this Xen domain. Enable context for given cache IPMMU only.
+ * Flush the TLB as required when modifying the context registers.
Why?
Original Linux driver provides each IOMMU domain with a separate IPMMU context.
So, when attaching device to IOMMU domain which hasn't been
initialized yet we have to
call ipmmu_domain_init_context() for initializing (root only) and
enabling (root + cache * ) context for this IOMMU domain.
* You can see at the end of the "original" ipmmu_domain_init_context()
implementation, that context is enabled for both cache and root IPMMUs
because of "ipmmu_ctx_write2".
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/drivers/iommu/ipmmu-vmsa.c?h=v4.9/rcar-3.5.3#n620
From my point of view, we don't have to do the same when we are
attaching device in Xen, as we keep only one IPMMU context (P2M
mappings) per Xen domain
for using by all assigned to this guest devices.
What is more a number of context banks is limited (8), and if we
followed Linux way here, we would be quickly run out of available
contexts.
But having one IPMMU context per Xen domain allow us to passthrough
devices to 8 guest domain.
The way you describe it give an impression that the driver is
fundamentally different in Xen compare to Linux. Am I right?
Taking into the account described above, we initialize (root only) and
enable (root only ** ) context at the domain creation time
if IOMMU is expected to be used for this guest.
https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2380
** You can see at the end of the "modified"
ipmmu_domain_init_context() implementation, that context is enabled
for root IPMMU only
because of "ipmmu_ctx_write".
https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L882
That's why, here, in ipmmu_attach_device() we don't have to call
ipmmu_domain_init_context() anymore, because
the context has been already initialized and enabled. All what we need
here is to enable this context for cache IPMMU the device
is physically connected to.
Does this make sense?
+ */
+#if 0
ret = ipmmu_domain_init_context(domain);
+#endif
+ ipmmu_ctx_write1(domain, IMCTR,
+ ipmmu_ctx_read(domain, IMCTR) |
IMCTR_FLUSH);
+
+ dev_info(dev, "Using IPMMU context %u\n",
domain->context_id);
+#if 0 /* Xen: Not needed */
if (ret < 0) {
dev_err(dev, "Unable to initialize IPMMU
context\n");
domain->mmu = NULL;
@@ -795,6 +1063,7 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
dev_info(dev, "Using IPMMU context %u\n",
domain->context_id);
}
+#endif
} else if (domain->mmu != mmu) {
/*
* Something is wrong, we can't attach two devices using
@@ -834,6 +1103,14 @@ static void ipmmu_detach_device(struct iommu_domain
*io_domain,
*/
}
+/*
+ * Xen: The current implementation of these callbacks is insufficient for
us
+ * since they are intended to be called from Linux IOMMU core that
+ * has already done all required actions such as doing various checks,
+ * splitting into memory block the hardware supports and so on.
Can you expand it here? Why can't our IOMMU framework could do that?
If add all required support to IOMMU framework and modify all existing
IOMMU drivers
to follow this support, then yes, it will avoid IOMMU drivers such as
IPMMU-VMSA from having these stuff in.
To be honest, I was trying to touch IOMMU common code and other IOMMU
drivers as little as possible,
but I had to introduce a few changes ("non-shared IOMMU").
What I am looking is something we can easily maintain in the future. If
it requires change in the common code then we should do it. If it
happens to be too complex, then maybe we should not take it from Linux.
IHMO, if we want to get driver from Linux, we need to get an interface very
close to it. Otherwise it is not worth it because you would have to
implement for each IOMMU.
You are right.
My overall feeling at the moment is Xen is not ready to welcome this driver
directly from Linux. This is also a BSP driver, so no thorough review done
by the community.
As I said in a cover letter the BSP driver had more complete support
than the mainline one.
I know. But this means we are going to bring code in Xen that was not
fully reviewed and don't know the quality of the code.
I would like to clarify what need to be done from my side.
Should I wait for the missing things reach upsteam and then rebase on
the mainline driver?
Or should I rewrite this driver without following Linux?
I don't have a clear answer here. As I said, we need to weight pros and
cons to use Linux driver over our own.
At the moment, you are using a BSP driver which has more features but
modified quite a lot. We don't even know when this is going to be merged
in Linux.
Keeping code close to Linux requires some hacks that are acceptable if
you can benefits from the community (bug fix, review...). As the driver
is taken from the BSP, we don't know if the code will stay in the
current form nor be able to get bug fix.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
