On 16/07/2025 10:56, Julien Grall wrote:
On 15/07/2025 16:58, Stewart Hildebrand wrote:
On 7/14/25 18:47, Julien Grall wrote:
Hi Stewards,
On 14/07/2025 22:12, Stewart Hildebrand wrote:
On 7/12/25 06:08, Julien Grall wrote:
Hi Stefano,
On 11/07/2025 01:25, Stefano Stabellini wrote:
diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/
drivers/passthrough/arm/iommu_helpers.c
index 5cb1987481..dae5fc0caa 100644
--- a/xen/drivers/passthrough/arm/iommu_helpers.c
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -36,17 +36,6 @@ int __must_check arm_iommu_map_page(struct
domain *d, dfn_t dfn, mfn_t mfn,
{
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));
-
Shouldn't arm_iommu_unmap_page() also be updated? It would not
result to a crash, but we would not be able to
remove the mapping.
f9f6b22abf1d didn't add any calls to iommu_unmap(). As this is still
only hwdom for now, hwdom is not expected to be destroyed, so the
mapping is not expected to be removed for now.
I already gathered that by looking at the fixes tag in my previous
answer. Maybe I should have been clearer at that point. Even though
iommu_unmap() is not called today, this is meant to be the reverse of
what was done by iommu_map(). So it looks very odd to update one but
not the other.
Furthermore, AFAIU, this patch is going a bit further than just
fixing the bug introduced by f9f6b22abf1d. At which point, we should
properly
fix it in the same patch rather than hoping that someone else will
remember that this needed be updated.
I'd like to suggest splitting this into two patches then, so we don't
let preparation for future work get in the way of fixing the reported
issue:
Patch #1 to fix the reported issue with a simple
s/is_domain_direct_mapped/domain_use_host_layout/ in both
arm_iommu_map_page() and arm_iommu_unmap_page().
Patch #2 to allow translated mappings in preparation for future work.
This sounds good to me.
With that said, in the future when we expose vITS to domU, you'd be
right. In the xlnx downstream we have something like this:
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ef8bd4b6ab33..2f5b79279ff9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -133,7 +133,8 @@ static inline int p2m_remove_mapping(struct
domain *d,
mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn,
i), &t, NULL,
&cur_order, NULL);
- if ( p2m_is_any_ram(t) &&
+ if ( !mfn_eq(mfn, INVALID_MFN) &&
+ p2m_is_any_ram(t) &&
I don't quite understand why you need to update this function. Can
you clarify?
Since arm_iommu_unmap_page() doesn't have the mfn, we needed a way to
remove a p2m mapping without the mfn available. INVALID_MFN is a
sentinel/placeholder in lieu of the missing mfn.
Ah, I didn't spot you changed the MFN passed in
guest_physmap_remove_page() below. Hmmm... The code in
p2m_remove_mapping() is checking MFN to avoid any race. IIRC this is to
close a race in the grant-table mapping.
So I am a bit uncomfortable to allow bypassing the check when
INVALID_MFN is passed. Looking at the code, I see the check is also
gated with p2m_is_any_ram(). I would argue that none of the IOMMU
mapping we are creating should be considered as RAM (the grant mapping
is arguable, but definitely not the doorbell). So I think it would be
better to use a different p2m type for the IOMMU mapping.
Actually, looking at the code, IOMMU mapping will use
p2m_iommu_map_{rw,ro}. If I am not mistaken, neither of them are
included in p2m_is_any_ram(). So I don't see why this check is needed in
upstream.
Did I miss anything? Do you happen to have downstream change?
Cheers,
--
Julien Grall
