| Message ID | 1563456140-12180-1-git-send-email-andrii.anisov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm:cpuerrata: Align a virtual address before unmap | expand |
On 18/07/2019 14:22, Andrii Anisov wrote: > From: Andrii Anisov <andrii_anisov@epam.com> > > After changes introduced by 9cc0618 we are able to vmap/vunmap > page aligned addresses only. > So if we add a page address remainder to the mapped virtual address, > we have to mask it out before unmapping. > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > --- > xen/arch/arm/cpuerrata.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > index 8904939..6f483b2 100644 > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -75,7 +75,7 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start, > clean_dcache_va_range(dst_remapped, VECTOR_TABLE_SIZE); > invalidate_icache(); > > - vunmap(dst_remapped); > + vunmap((void *)((vaddr_t)dst_remapped & PAGE_MASK)); This really ought to be vunmap() performing the rounding, which makes it consistent with how {,un}map_domain_page() currently works. However (from inspection), there is a real bug in this code, so it needs fixing as well. dst_remapped will be the wrong page when it fills the final slot in the page, which looks to be every other slot, given that the size is 2k. A better option would be to keep the base pointer around and vunmap() that, and account for slot in a different variable. ~Andrew
On 18/07/2019 14:38, Andrew Cooper wrote: > On 18/07/2019 14:22, Andrii Anisov wrote: >> From: Andrii Anisov <andrii_anisov@epam.com> >> >> After changes introduced by 9cc0618 we are able to vmap/vunmap >> page aligned addresses only. >> So if we add a page address remainder to the mapped virtual address, >> we have to mask it out before unmapping. >> >> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> >> --- >> xen/arch/arm/cpuerrata.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >> index 8904939..6f483b2 100644 >> --- a/xen/arch/arm/cpuerrata.c >> +++ b/xen/arch/arm/cpuerrata.c >> @@ -75,7 +75,7 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start, >> clean_dcache_va_range(dst_remapped, VECTOR_TABLE_SIZE); >> invalidate_icache(); >> >> - vunmap(dst_remapped); >> + vunmap((void *)((vaddr_t)dst_remapped & PAGE_MASK)); > This really ought to be vunmap() performing the rounding, which makes it > consistent with how {,un}map_domain_page() currently works. > > However (from inspection), there is a real bug in this code Actually ignore me. I'm wrong, and clearly can't read code. My suggestion about vunmap() still stands though. ~Andrew
Hi, On 7/18/19 2:43 PM, Andrew Cooper wrote: > On 18/07/2019 14:38, Andrew Cooper wrote: >> On 18/07/2019 14:22, Andrii Anisov wrote: >>> From: Andrii Anisov <andrii_anisov@epam.com> >>> >>> After changes introduced by 9cc0618 we are able to vmap/vunmap >>> page aligned addresses only. >>> So if we add a page address remainder to the mapped virtual address, >>> we have to mask it out before unmapping. >>> >>> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> >>> --- >>> xen/arch/arm/cpuerrata.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >>> index 8904939..6f483b2 100644 >>> --- a/xen/arch/arm/cpuerrata.c >>> +++ b/xen/arch/arm/cpuerrata.c >>> @@ -75,7 +75,7 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start, >>> clean_dcache_va_range(dst_remapped, VECTOR_TABLE_SIZE); >>> invalidate_icache(); >>> >>> - vunmap(dst_remapped); >>> + vunmap((void *)((vaddr_t)dst_remapped & PAGE_MASK)); >> This really ought to be vunmap() performing the rounding, which makes it >> consistent with how {,un}map_domain_page() currently works. >> >> However (from inspection), there is a real bug in this code > > Actually ignore me. I'm wrong, and clearly can't read code. > > My suggestion about vunmap() still stands though. +1 for the vunmap solution. Cheers,
Hello Julien, On 18.07.19 16:44, Julien Grall wrote: >> My suggestion about vunmap() still stands though. > > +1 for the vunmap solution. It was my initial idea. But, the issue is introduced by change 9cc0618. In particular, by the check in `xen_pt_update()` if ( !IS_ALIGNED(virt, PAGE_SIZE) ) { mm_printk("The virtual address is not aligned to the page-size.\n"); return -EINVAL; } So I assumed you had some specific idea about that check. I'd fix `xen_pt_update()` if you are ok with it.
On 7/18/19 3:08 PM, Andrii Anisov wrote: > Hello Julien, Hi, > On 18.07.19 16:44, Julien Grall wrote: >>> My suggestion about vunmap() still stands though. >> >> +1 for the vunmap solution. > > It was my initial idea. > > But, the issue is introduced by change 9cc0618. In particular, by the > check in `xen_pt_update()` > > if ( !IS_ALIGNED(virt, PAGE_SIZE) ) > { > mm_printk("The virtual address is not aligned to the > page-size.\n"); > return -EINVAL; > } > > So I assumed you had some specific idea about that check. I am expecting all the callers to properly align the address. > > I'd fix `xen_pt_update()` if you are ok with it. Please no. The interfaces are already pretty horrible as it mixing address and frame. So this check here is partially closing the gap of passing misaligned virtual address. If you look at the x86 counter-part, they also assume the address is aligned (see ASSERT in some of the helpers). So I think the proper way to go is aligning the address in vumap. As Andrew pointed out, this also makes the interface more consistent with how {,un}map_domain_page() currently works. Cheers,
On 18.07.19 17:20, Julien Grall wrote:
> As Andrew pointed out, this also makes the interface more consistent with how {,un}map_domain_page() currently works.
OK, got it.
BTW, in the x86 code they do apply PAGE_MASK to va before passing it to `vunmap()`. I would cleanup all those places as well.
Hi, It looks like the vmap solution suggested by Andrew & I is a dead end. I still think we need to do something in the vmap regardless the alignment decision to avoid unwanted surprised (i.e the Page-table not in sync with the vmap state). We potentially want to add some ASSERT_UNREACHABLE() in the page-table code for the sanity check. So we don't continue without further on debug build. I will have a look at both. A couple of comments for the patch. Title: NIT: Missing space after the first :. On 18/07/2019 14:22, Andrii Anisov wrote: > From: Andrii Anisov <andrii_anisov@epam.com> > > After changes introduced by 9cc0618 we are able to vmap/vunmap 7-digit is not sufficient to guarantee it will be uniq in the future. You also want to specify the commit title. > page aligned addresses only. > So if we add a page address remainder to the mapped virtual address, > we have to mask it out before unmapping. > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> Acked-by: Julien Grall <julien.gralL@arm.com> If you are happy with the changes, I can do them on commit. > --- > xen/arch/arm/cpuerrata.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > index 8904939..6f483b2 100644 > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -75,7 +75,7 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start, > clean_dcache_va_range(dst_remapped, VECTOR_TABLE_SIZE); > invalidate_icache(); > > - vunmap(dst_remapped); > + vunmap((void *)((vaddr_t)dst_remapped & PAGE_MASK)); > > return true; > } > Cheers,
Hello Julien, On 26.07.19 17:02, Julien Grall wrote: > Hi, > > It looks like the vmap solution suggested by Andrew & I is a dead end. I still think we need to do something in the vmap regardless the alignment decision to avoid unwanted surprised (i.e the Page-table not in sync with the vmap state). > > We potentially want to add some ASSERT_UNREACHABLE() in the page-table code for the sanity check. So we don't continue without further on debug build. I will have a look at both. > > A couple of comments for the patch. > > Title: NIT: Missing space after the first :. > > On 18/07/2019 14:22, Andrii Anisov wrote: >> From: Andrii Anisov <andrii_anisov@epam.com> >> >> After changes introduced by 9cc0618 we are able to vmap/vunmap > > 7-digit is not sufficient to guarantee it will be uniq in the future. You also want to specify the commit title. > >> page aligned addresses only. >> So if we add a page address remainder to the mapped virtual address, >> we have to mask it out before unmapping. >> >> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> > > Acked-by: Julien Grall <julien.gralL@arm.com> > > > If you are happy with the changes, I can do them on commit. It's fine with me. Thank you.
On 7/29/19 8:33 AM, Andrii Anisov wrote: > Hello Julien, > > On 26.07.19 17:02, Julien Grall wrote: >> Hi, >> >> It looks like the vmap solution suggested by Andrew & I is a dead end. >> I still think we need to do something in the vmap regardless the >> alignment decision to avoid unwanted surprised (i.e the Page-table not >> in sync with the vmap state). >> >> We potentially want to add some ASSERT_UNREACHABLE() in the page-table >> code for the sanity check. So we don't continue without further on >> debug build. I will have a look at both. >> >> A couple of comments for the patch. >> >> Title: NIT: Missing space after the first :. >> >> On 18/07/2019 14:22, Andrii Anisov wrote: >>> From: Andrii Anisov <andrii_anisov@epam.com> >>> >>> After changes introduced by 9cc0618 we are able to vmap/vunmap >> >> 7-digit is not sufficient to guarantee it will be uniq in the future. >> You also want to specify the commit title. >> >>> page aligned addresses only. >>> So if we add a page address remainder to the mapped virtual address, >>> we have to mask it out before unmapping. >>> >>> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com> >> >> Acked-by: Julien Grall <julien.gralL@arm.com> >> >> >> If you are happy with the changes, I can do them on commit. > > It's fine with me. > Thank you. Now committed. Cheers,
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c index 8904939..6f483b2 100644 --- a/xen/arch/arm/cpuerrata.c +++ b/xen/arch/arm/cpuerrata.c @@ -75,7 +75,7 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start, clean_dcache_va_range(dst_remapped, VECTOR_TABLE_SIZE); invalidate_icache(); - vunmap(dst_remapped); + vunmap((void *)((vaddr_t)dst_remapped & PAGE_MASK)); return true; }