[Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrii Anisov]

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-devel

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Julien Grall]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrii Anisov]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Julien Grall]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrii Anisov]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Andrii Anisov]

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

Re: [Xen-devel] [PATCH] arm:cpuerrata: Align a virtual address before unmap

[Julien Grall]

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