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));
return true;
}
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develHi, 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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. -- Sincerely, Andrii Anisov. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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.
--
Sincerely,
Andrii Anisov.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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.
--
Sincerely,
Andrii Anisov.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.