On 23.06.22 21:08, Julien Grall wrote:
Hi Oleksandr,
Hello Julien
On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <[email protected]>
Borrow the x86's check from p2m_remove_page() which was added
by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e
"x86/p2m: don't assert that the passed in MFN matches for a remove"
and adjust it to the Arm code base.
Basically, this check is strictly needed for the xenheap pages only
since there are several non-protected read accesses to our simplified
xenheap based M2P approach on Arm (most calls to page_get_xenheap_gfn()
are not protected by the P2M lock).
To me, this read as you introduced a bug in patch #1 and now you are
fixing it. So this patch should have been first.
Sounds like yes, I agree. But, in that case I propose to rewrite this
text like the following:
Basically, this check will be strictly needed for the xenheap pages only
*and* only after applying subsequent
commit which will introduce xenheap based M2P approach on Arm. But, it
will be a good opportunity
to harden the P2M code for *every* RAM pages since it is possible to
remove any GFN - MFN mapping
currently on Arm (even with the wrong helpers).
And ...
But, it will be a good opportunity to harden the P2M code for *every*
RAM pages since it is possible to remove any GFN - MFN mapping
currently on Arm (even with the wrong helpers).
This can result in
a few issues when mapping is overridden silently (in particular when
building dom0).
Hmmm... AFAIU, in such situation p2m_remove_mapping() wouldn't be
called. Instead, we would call the mapping helper twice and the
override would still happen.
... drop this one.
Suggested-by: Julien Grall <[email protected]>
Signed-off-by: Oleksandr Tyshchenko <[email protected]>
---
You can find the corresponding discussion at:
https://lore.kernel.org/xen-devel/[email protected]/
Changes V5 -> V6:
- new patch
---
xen/arch/arm/p2m.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f87b48e..635e474 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1311,11 +1311,32 @@ static inline int p2m_remove_mapping(struct
domain *d,
mfn_t mfn)
{
struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ unsigned long i;
int rc;
p2m_write_lock(p2m);
+ for ( i = 0; i < nr; )
One bit I really hate in the x86 code is the lack of in-code
documentation. It makes really difficult to understand the logic.
I know this code was taken from x86, but I would like to avoid making
same mistake (this code is definitely not trivial). So can we document
the logic?
ok, I propose the following text right after acquiring the p2m lock:
/*
* Before removing the GFN - MFN mapping for any RAM pages make sure
* that there is no difference between what is already mapped and what
* is requested to be unmapped. If passed mapping doesn't match
* the existing one bail out early.
*/
Could you please clarify, do you agree with both?
The code itself looks good to me.
Thanks!
+ {
+ unsigned int cur_order;
+ p2m_type_t t;
+ mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i),
&t, NULL,
+ &cur_order, NULL);
+
+ if ( p2m_is_any_ram(t) &&
+ (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i),
mfn_return)) )
+ {
+ rc = -EILSEQ;
+ goto out;
+ }
+
+ i += (1UL << cur_order) -
+ ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1));
+ }
+
rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN,
p2m_invalid, p2m_access_rwx);
+
+out:
p2m_write_unlock(p2m);
return rc;
Cheers,
--
Regards,
Oleksandr Tyshchenko
