On 23.07.19 16:36, Julien Grall wrote:
Hi Oleksandr,
Hi, Julien.
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.
Does anyone in the community has access to the spec?
Yes. I believe, there are persons from the Linux community who have
access to the spec. I have already asked for review.
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.
Can this be written in the commit message? This is helpful for anyone
reviewing the driver today and future developer.
Sure. Will update.
+ * 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.
What is the review process for patches to be merged in the BSP? Is it
the same as Linux upstream?
I don't know at the moment, I will try to clarify this question.
It can be >= 0.
Ok, so please switch to unsigned int here please.
ok
+{
+ 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.
Could the function be able to deal with those other registers without
any change?
No. "data & IMCTR_COMMON_MASK" should be moved out of the function at
least.
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.
Thank you for the explanation. What can actually happen? Could it lead
to a security issue (e.g the IPMMU is bypassed)?
Yes, using context bank for IPA to PA translation from Domain A for a
device running in Domain B could lead to something very bad. The best
case scenario would be when IPA supplied by a device in Domain B is not
effectively mapped in a context for Domain A. In this case we would just
get a page fault...
Also, the question is whether this is worth to try to implement it. Do
we have cases where devices use the same micro-TLB but assigned to
different domains?
No. At least I am not aware of. What is more that devices (which share
the same utlb) can't be easily separated from each other the first in
order to be assigned to different Xen domains then, I think.
Such DMA devices as AVB, SATA, eMMC/SD, USB, DU, GPU, etc (which really
could be located in different Xen domains according to the particular
use case) don't share utlbs.
If not, then maybe you could just add check in the driver to prevent
that use cases. The work around the iommu_group done by Paul [1] might
be useful.
Anyway, from upstream perspective this is not a massive concern for
now as platform device-passthrough is not security supported. So I
would be happy if the TODO is addressed in a follow-up series.
Agree.
So, the following actions:
1. TODO remains for this driver series.
2. TODO will be addressed in a follow-up series by *preventing* the use
cases where the same utlb could be shared between multiple Xen domains.
[...]
+/* 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
I find confusing to rely on xen_domain->lock to serialize access to a
field from a different structure. It would be good if this is written
in the document of the structure.
Also is this always read behind the same lock?
I think, yes. I couldn't find the case when not.
Will clarify documentation.
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?
It would be good to have an abstract way to add new device to IOMMU
based on the generic IOMMU DT binding. I am quite keen to seen
something similar to iommu_fwspec in Xen so this can be used for both
DT and ACPI.
From an high level perspective, we would have some code add a new
device to the IOMMU. The generic code would:
1) Parse the binding and prepare iommu_fwspec with the correct
information
2) Call the IOMMU driver to register the new device
The new function would be either called from handle_device or a new
loop over the DT nodes.
In the whole, I understand your point. I will come up with questions if any.
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.
The reason I asked the question is the current solution feels like
papering over an API that does not fit for the new driver. So it would
be worth investigating whether a -EDEFER like could be easily used in Xen.
Well, I will definitely investigate the possibility.
----------
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;
}
We have similar problem with the SMMU. The only strict requirement for
the IOMMU is to have a valid p2m_ipa_bits at the time a domain is
built. Note that the SMMU will store the value when probing the SMMU,
but that could be reworked.
So rather than initializing the P2M first and then the IOMMU, I would
first initialize the IOMMU so we can gather the requirements and then
initialize the P2M.
In the P2M code, you can take into account the IOMMU requirements and
further restrict if necessary. What do you think?
I think, this sounds reasonable and worth trying. Could this TODO be
addressed in a follow-up series?
Cheers,
[1] <[email protected]>
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
