diff mbox series

[4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

Message ID 20220624091146.35716-5-julien@xen.org (mailing list archive)
State Superseded
Headers show
Series xen/arm: mm: Bunch of clean-ups | expand

Commit Message

Julien Grall June 24, 2022, 9:11 a.m. UTC 
From: Julien Grall <jgrall@amazon.com>

Both destroy_xen_mappings() and modify_xen_mappings() will take in
parameter a range [start, end[. Both end should be page aligned.

Add extra ASSERT() to ensure start and end are page aligned. Take the
opportunity to rename 'v' to 's' to be consistent with the other helper.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/mm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Michal Orzel June 27, 2022, 6:45 a.m. UTC | #1 
Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Both destroy_xen_mappings() and modify_xen_mappings() will take in
> parameter a range [start, end[. Both end should be page aligned.
> 
> Add extra ASSERT() to ensure start and end are page aligned. Take the
> opportunity to rename 'v' to 's' to be consistent with the other helper.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/mm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0607c65f95cd..20733afebce4 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1371,14 +1371,18 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>      return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
>  }
>  
> -int destroy_xen_mappings(unsigned long v, unsigned long e)
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
I think the prototype wants to be updated as well in include/xen/mm.h.
x86 mm.c already uses s instead of v so it is a good opportunity to fix the prototype.

Cheers,
Michal
Bertrand Marquis July 4, 2022, 12:35 p.m. UTC | #2 
Hi Julien,

> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Both destroy_xen_mappings() and modify_xen_mappings() will take in
> parameter a range [start, end[. Both end should be page aligned.
> 
> Add extra ASSERT() to ensure start and end are page aligned. Take the
> opportunity to rename 'v' to 's' to be consistent with the other helper.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

With the prototype updated in mm.h as suggested by Michal:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/mm.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0607c65f95cd..20733afebce4 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1371,14 +1371,18 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>     return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> }
> 
> -int destroy_xen_mappings(unsigned long v, unsigned long e)
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> {
> -    ASSERT(v <= e);
> -    return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +    ASSERT(s <= e);
> +    return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
> }
> 
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>     ASSERT(s <= e);
>     return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> }
> -- 
> 2.32.0
>
Julien Grall July 16, 2022, 2:38 p.m. UTC | #3 
Hi Bertrand,

On 04/07/2022 13:35, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>> parameter a range [start, end[. Both end should be page aligned.
>>
>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> With the prototype updated in mm.h as suggested by Michal:

Done. I will send a new version shortly.

> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

Cheers,
Jan Beulich July 18, 2022, 8:47 a.m. UTC | #4 
On 16.07.2022 16:38, Julien Grall wrote:
> On 04/07/2022 13:35, Bertrand Marquis wrote:
>>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>>> parameter a range [start, end[. Both end should be page aligned.
>>>
>>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> With the prototype updated in mm.h as suggested by Michal:
> 
> Done. I will send a new version shortly.

I notice you had applied and then reverted this, with the revert saying
an x86 ack was missing. I don't see any need for an x86 ack here though,
so I'm puzzled ...

Jan
Julien Grall July 18, 2022, 2:06 p.m. UTC | #5 
Hi Jan,

On 18/07/2022 09:47, Jan Beulich wrote:
> On 16.07.2022 16:38, Julien Grall wrote:
>> On 04/07/2022 13:35, Bertrand Marquis wrote:
>>>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>>>> parameter a range [start, end[. Both end should be page aligned.
>>>>
>>>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>>>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> With the prototype updated in mm.h as suggested by Michal:
>>
>> Done. I will send a new version shortly.
> 
> I notice you had applied and then reverted this, with the revert saying
> an x86 ack was missing. I don't see any need for an x86 ack here though,
> so I'm puzzled ...

Sorry, I am not sure why I thought this needed an x86 ack. I will 
(re-)commit it shortly.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0607c65f95cd..20733afebce4 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1371,14 +1371,18 @@  int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
     return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
 }
 
-int destroy_xen_mappings(unsigned long v, unsigned long e)
+int destroy_xen_mappings(unsigned long s, unsigned long e)
 {
-    ASSERT(v <= e);
-    return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+    ASSERT(s <= e);
+    return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
 }
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 {
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
     ASSERT(s <= e);
     return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
 }