| Message ID | 1652294845-13980-2-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V6,1/2] xen/gnttab: Store frame GFN in struct page_info on Arm | expand |
Hi Oleksandr, On 11/05/2022 19:47, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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. > > 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. > > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > You can find the corresponding discussion at: > https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@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? The code itself looks good to me. > + { > + 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,
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 <oleksandr_tyshchenko@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 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 <jgrall@amazon.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> You can find the corresponding discussion at: >> https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@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. */ 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, >
On 24/06/2022 16:31, Oleksandr wrote: > > On 23.06.22 21:08, Julien Grall wrote: >> Hi Oleksandr, > > > Hello Julien Hi Oleksandr, > >> >> On 11/05/2022 19:47, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@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. Also please add "a" in from of subsequent because the two patches may not be committed together. > 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 <jgrall@amazon.com> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> You can find the corresponding discussion at: >>> https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@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". 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." > */ > > > 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? Cheers,
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_tyshchenko@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 <jgrall@amazon.com> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> You can find the corresponding discussion at: >>>> https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@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, >
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; ) + { + 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;