[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V1] iommu/arm: Add Renesas IPMMU-VMSA support



Hi Oleksandr,

On 7/24/19 11:54 AM, Oleksandr wrote:
On 23.07.19 16:36, Julien Grall wrote:
On 6/26/19 11:30 AM, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
+{
+    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.

I will leave up to you whether you want to drop the parameter. However, I'd like a comment explaining why the ASSERT() if you decide to keep it.


[...]


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.

The 2 actions looks good to me.

[...]

I think, this sounds reasonable and worth trying. Could this TODO be addressed in a follow-up series?

I am fine with that.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.