On 15.07.22 20:15, Julien Grall wrote:
On 24/06/2022 16:31, Oleksandr wrote:
On 23.06.22 21:08, Julien Grall wrote:
Hi Oleksandr,
Hello Julien
Hi Oleksandr,
Hello Julien
On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
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
NIT: s/only and only/, this is pretty clear that this patch is
necessary for a follow-up patch.
ok
Also please add "a" in from of subsequent because the two patches may
not be committed together.
ok
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).
[...]
Suggested-by: Julien Grall <jgr...@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
---
You can find the corresponding discussion at:
https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a...@xen.org/
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.
NIT: I would simply write "If they don't match bail out early".
ok, I guess this is related to the last sentence only.
Also, it would be good to explanation how this could happen. Something
like:
"For instance, this could happen if two CPUs are requesting to unmap
the same P2M concurrently."
agree
*/
Could you please clarify, do you agree with both?
I have proposed some changes in both cases. I originally thought I
would do the update in the commit. However, this is more than simple
tweak, so would you mind to send a new version?
yes, will do
Cheers,
--
Regards,
Oleksandr Tyshchenko
