xen/common/grant_table.c | 10 ++++++++-- xen/drivers/passthrough/arm/iommu_helpers.c | 13 +------------ 2 files changed, 9 insertions(+), 14 deletions(-)
Up until f9f6b22ab "xen/arm: Map ITS doorbell register to IOMMU page
tables" the only caller of iommu_map on ARM was grant_table.c which has
a specific usage model and restrictions as described by the in-code
comment in arm_iommu_map_page.
f9f6b22ab introduced a second caller to iommu_map on ARM:
vgic_v3_its_init_virtual. This specific statement in the
f9f6b22ab commit message is wrong:
"Note that the 1:1 check in arm_iommu_map_page remains for now, as
virtual ITSes are currently only created for hwdom where the doorbell
mapping is always 1:1."
Leading to crashes any time the hardware domain is not direct-mapped
(e.g. cache coloring and non-Dom0 hardware domain):
(XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49
[...]
(XEN) Xen call trace:
(XEN) [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
(XEN) [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
(XEN) [<00000a0000250884>] iommu_map+0xcc/0x29c
(XEN) [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
(XEN) [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
(XEN) [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
(XEN) [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
(XEN) [<00000a00002055e8>] domain_create+0x47c/0x6c0
(XEN) [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
(XEN) [<00000a00002eb588>] start_xen+0x8f4/0x998
(XEN) [<00000a000020018c>] head.o#primary_switched+0x4/0x10
Specifically, non-1:1 hardware domain exists with cache coloring
enabled. For that, is_domain_direct_mapped(d) is false but
domain_use_host_layout(d) is true.
At a minimum, we need to change the is_domain_direct_mapped(d) check in
arm_iommu_map_page into a domain_use_host_layout(d) check. But in
preparation of exposing vITS to domUs, let's take this opportunity to
generalize the arm_iommu_map_page function further to be more generic
and unopinionated. Move the in-code comment specific to the grant table
can live in grant-table.c instead.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
xen/common/grant_table.c | 10 ++++++++--
xen/drivers/passthrough/arm/iommu_helpers.c | 13 +------------
2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index cf131c43a1..2e08dac656 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1274,8 +1274,14 @@ map_grant_ref(
}
/*
- * We're not translated, so we know that dfns and mfns are
- * the same things, so the IOMMU entry is always 1-to-1.
+ * 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.
+ *
+ * We're not translated, so we know that dfns and mfns are the
+ * same things, so the IOMMU entry is always 1-to-1.
*/
if ( !(op->flags & GNTMAP_readonly) && node.cnt.wr == 1 )
kind = IOMMUF_readable | IOMMUF_writable;
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));
-
/* We only support readable and writable flags */
if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
return -EINVAL;
@@ -57,7 +46,7 @@ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
* 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)),
+ return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), mfn,
IOMMUF_order(flags), t);
}
--
2.25.1Hi Stefano,
On 11/07/2025 01:25, Stefano Stabellini wrote:
> Up until f9f6b22ab "xen/arm: Map ITS doorbell register to IOMMU page
Everywhere in this commit message, we are using 12 characters commit ID.
> tables" the only caller of iommu_map on ARM was grant_table.c which has
> a specific usage model and restrictions as described by the in-code
> comment in arm_iommu_map_page.
>
> f9f6b22ab introduced a second caller to iommu_map on ARM:
> vgic_v3_its_init_virtual. This specific statement in the
> f9f6b22ab commit message is wrong:
>
> "Note that the 1:1 check in arm_iommu_map_page remains for now, as
> virtual ITSes are currently only created for hwdom where the doorbell
> mapping is always 1:1."
>
> Leading to crashes any time the hardware domain is not direct-mapped
> (e.g. cache coloring and non-Dom0 hardware domain):
>
> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49
Are you using the last staging? Asking because line 49 is a blank line:
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/passthrough/arm/iommu_helpers.c;h=5cb19874819357b5cd58638864b56c505d07b37a;hb=HEAD#l49
> [...]
> (XEN) Xen call trace:
> (XEN) [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
> (XEN) [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
> (XEN) [<00000a0000250884>] iommu_map+0xcc/0x29c
> (XEN) [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
> (XEN) [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
> (XEN) [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
> (XEN) [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
> (XEN) [<00000a00002055e8>] domain_create+0x47c/0x6c0
> (XEN) [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
> (XEN) [<00000a00002eb588>] start_xen+0x8f4/0x998
> (XEN) [<00000a000020018c>] head.o#primary_switched+0x4/0x10
>
> Specifically, non-1:1 hardware domain exists with cache coloring
> enabled. For that, is_domain_direct_mapped(d) is false but
> domain_use_host_layout(d) is true.
>
> At a minimum, we need to change the is_domain_direct_mapped(d) check in
> arm_iommu_map_page into a domain_use_host_layout(d) check. But in
> preparation of exposing vITS to domUs, let's take this opportunity to
> generalize the arm_iommu_map_page function further to be more generic
> and unopinionated. Move the in-code comment specific to the grant table
> can live in grant-table.c instead.
>
Given this is a bug fix, a Fixes tag should be added.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> xen/common/grant_table.c | 10 ++++++++--
> xen/drivers/passthrough/arm/iommu_helpers.c | 13 +------------
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index cf131c43a1..2e08dac656 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1274,8 +1274,14 @@ map_grant_ref(
> }
>
> /*
> - * We're not translated, so we know that dfns and mfns are
> - * the same things, so the IOMMU entry is always 1-to-1.
> + * Grant mappings can be used for DMA requests. The dev_bus_addr
> + * returned by the hypercall is the MFN (not the IPA). For
I was under the impression that IPA is an Arm termonology. Yet this is
common code. Also, it would be preferable if we don't mix frame and
address. In this case, I think you want GFN (I think it would work for
all the domains.
> + * 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.
> + *> + * We're not translated, so we know that dfns
and mfns are the
> + * same things, so the IOMMU entry is always 1-to-1.
> */
> if ( !(op->flags & GNTMAP_readonly) && node.cnt.wr == 1 )
> kind = IOMMUF_readable | IOMMUF_writable;
> 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.
Cheers,
--
Julien Grall
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.
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) &&
(!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
{
rc = -EILSEQ;
diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
index 04b7e98e4aae..d59a9cf5a48b 100644
--- a/xen/drivers/passthrough/arm/iommu_helpers.c
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -63,14 +63,7 @@ int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
unsigned int order,
unsigned int *flush_flags)
{
- /*
- * This function should only be used by gnttab code when the domain
- * is direct mapped (i.e. gfn == mfn == dfn).
- */
- if ( !is_domain_direct_mapped(d) )
- return -EINVAL;
-
- return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
+ return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), INVALID_MFN,
order);
}
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.
>
> 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?
> (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
> {
> rc = -EILSEQ;
> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
> index 04b7e98e4aae..d59a9cf5a48b 100644
> --- a/xen/drivers/passthrough/arm/iommu_helpers.c
> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
> @@ -63,14 +63,7 @@ int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
> unsigned int order,
> unsigned int *flush_flags)
> {
> - /*
> - * This function should only be used by gnttab code when the domain
> - * is direct mapped (i.e. gfn == mfn == dfn).
> - */
> - if ( !is_domain_direct_mapped(d) )
> - return -EINVAL;
> -
> - return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
> + return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), INVALID_MFN,
> order);
> }
Cheers,
--
Julien Grall
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.
>>
>> 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.
>> (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) )
>> {
>> rc = -EILSEQ;
>> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
>> index 04b7e98e4aae..d59a9cf5a48b 100644
>> --- a/xen/drivers/passthrough/arm/iommu_helpers.c
>> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
>> @@ -63,14 +63,7 @@ int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
>> unsigned int order,
>> unsigned int *flush_flags)
>> {
>> - /*
>> - * This function should only be used by gnttab code when the domain
>> - * is direct mapped (i.e. gfn == mfn == dfn).
>> - */
>> - if ( !is_domain_direct_mapped(d) )
>> - return -EINVAL;
>> -
>> - return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
>> + return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), INVALID_MFN,
>> order);
>> }
>
> Cheers,
>
Hi Stewart,
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.
Cheers,
--
Julien Grall
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
On 7/16/25 05:59, Julien Grall wrote:
> 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?
Ah, no, I think you're right. Since the p2m_is_any_ram() check does not
include p2m_iommu_map_{rw,ro}, no change to p2m_remove_mapping() should
be needed. This was an oversight in our downstream.
On 2025-07-12 06:08, Julien Grall wrote: > Hi Stefano, > > On 11/07/2025 01:25, Stefano Stabellini wrote: >> Up until f9f6b22ab "xen/arm: Map ITS doorbell register to IOMMU page > > Everywhere in this commit message, we are using 12 characters commit ID. > >> tables" the only caller of iommu_map on ARM was grant_table.c which has >> a specific usage model and restrictions as described by the in-code >> comment in arm_iommu_map_page. >> >> f9f6b22ab introduced a second caller to iommu_map on ARM: >> vgic_v3_its_init_virtual. This specific statement in the >> f9f6b22ab commit message is wrong: >> >> "Note that the 1:1 check in arm_iommu_map_page remains for now, as >> virtual ITSes are currently only created for hwdom where the doorbell >> mapping is always 1:1." >> >> Leading to crashes any time the hardware domain is not direct-mapped >> (e.g. cache coloring and non-Dom0 hardware domain): >> >> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49 > > Are you using the last staging? Asking because line 49 is a blank line: > > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/ > passthrough/arm/ > iommu_helpers.c;h=5cb19874819357b5cd58638864b56c505d07b37a;hb=HEAD#l49 This is my fault. I added some debugging code and shifted the line numbers in my original reporting. 47 is the correct line: BUG_ON(!is_domain_direct_mapped(d)); Regards, Jason
On 7/10/25 20:25, Stefano Stabellini wrote:
> Up until f9f6b22ab "xen/arm: Map ITS doorbell register to IOMMU page
> tables" the only caller of iommu_map on ARM was grant_table.c which has
> a specific usage model and restrictions as described by the in-code
> comment in arm_iommu_map_page.
>
> f9f6b22ab introduced a second caller to iommu_map on ARM:
> vgic_v3_its_init_virtual. This specific statement in the
> f9f6b22ab commit message is wrong:
>
> "Note that the 1:1 check in arm_iommu_map_page remains for now, as
> virtual ITSes are currently only created for hwdom where the doorbell
> mapping is always 1:1."
>
> Leading to crashes any time the hardware domain is not direct-mapped
> (e.g. cache coloring and non-Dom0 hardware domain):
>
> (XEN) Xen BUG at drivers/passthrough/arm/iommu_helpers.c:49
> [...]
> (XEN) Xen call trace:
> (XEN) [<00000a000024c758>] arm_iommu_map_page+0x80/0x90 (PC)
> (XEN) [<00000a000024c750>] arm_iommu_map_page+0x78/0x90 (LR)
> (XEN) [<00000a0000250884>] iommu_map+0xcc/0x29c
> (XEN) [<00000a0000288024>] vgic_v3_its_init_domain+0x18c/0x1e8
> (XEN) [<00000a0000285228>] vgic-v3.c#vgic_v3_domain_init+0x168/0x21c
> (XEN) [<00000a0000281dcc>] domain_vgic_init+0x14c/0x210
> (XEN) [<00000a00002705a4>] arch_domain_create+0x150/0x1f0
> (XEN) [<00000a00002055e8>] domain_create+0x47c/0x6c0
> (XEN) [<00000a00002cf090>] create_domUs+0x7f8/0x8cc
> (XEN) [<00000a00002eb588>] start_xen+0x8f4/0x998
> (XEN) [<00000a000020018c>] head.o#primary_switched+0x4/0x10
>
> Specifically, non-1:1 hardware domain exists with cache coloring
> enabled. For that, is_domain_direct_mapped(d) is false but
> domain_use_host_layout(d) is true.
>
> At a minimum, we need to change the is_domain_direct_mapped(d) check in
> arm_iommu_map_page into a domain_use_host_layout(d) check. But in
> preparation of exposing vITS to domUs, let's take this opportunity to
> generalize the arm_iommu_map_page function further to be more generic
> and unopinionated. Move the in-code comment specific to the grant table
> can live in grant-table.c instead.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Reviewed-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> xen/common/grant_table.c | 10 ++++++++--
> xen/drivers/passthrough/arm/iommu_helpers.c | 13 +------------
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index cf131c43a1..2e08dac656 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1274,8 +1274,14 @@ map_grant_ref(
> }
>
> /*
> - * We're not translated, so we know that dfns and mfns are
> - * the same things, so the IOMMU entry is always 1-to-1.
> + * 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.
> + *
> + * We're not translated, so we know that dfns and mfns are the
> + * same things, so the IOMMU entry is always 1-to-1.
> */
> if ( !(op->flags & GNTMAP_readonly) && node.cnt.wr == 1 )
> kind = IOMMUF_readable | IOMMUF_writable;
> 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));
> -
> /* We only support readable and writable flags */
> if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
> return -EINVAL;
> @@ -57,7 +46,7 @@ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
> * 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)),
> + return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), mfn,
> IOMMUF_order(flags), t);
> }
>
© 2016 - 2025 Red Hat, Inc.