Reuse mask variables on order shift duplicated calculation.

Signed-off-by: Paran Lee <p4ranlee@gmail.com>
---
 xen/arch/arm/p2m.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1d1059f7d2..cdb3b56aa1 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1118,11 +1118,12 @@ int p2m_set_entry(struct p2m_domain *p2m,
         if ( rc )
             break;
 
-        sgfn = gfn_add(sgfn, (1 << order));
+        mask = 1UL << order;
+        sgfn = gfn_add(sgfn, mask);
         if ( !mfn_eq(smfn, INVALID_MFN) )
-           smfn = mfn_add(smfn, (1 << order));
+           smfn = mfn_add(smfn, mask);
 
-        nr -= (1 << order);
+        nr -= mask;
     }
 
     return rc;
-- 
2.25.1

Hi,

On 26/04/2022 16:49, Paran Lee wrote:
> Reuse mask variables on order shift duplicated calculation.
> 
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
> ---
>   xen/arch/arm/p2m.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

It is common to add a changelog after "---". This helps the reviewer to 
know what changed in your patch.

> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 1d1059f7d2..cdb3b56aa1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1118,11 +1118,12 @@ int p2m_set_entry(struct p2m_domain *p2m,
>           if ( rc )
>               break;
>   
> -        sgfn = gfn_add(sgfn, (1 << order));
> +        mask = 1UL << order;

"1UL << order" refers to the number of pages and not a mask. So I don't 
think re-using the local variable 'mask' is a good idea because the code 
is a lot more confusing.

Instead, I think your other patch is the way to go with a small tweak to 
use 1UL (which BTW should be mentioned in the commit message).

Either Stefano or I can deal with the change on commit.

Cheers,

-- 
Julien Grall

Thansk!

> It is common to add a changelog after "---". This helps the reviewer to
> know what changed in your patch.

I think this experience will be very helpful for the next more
meaningful patch.

>
> "1UL << order" refers to the number of pages and not a mask. So I don't
> think re-using the local variable 'mask' is a good idea because the code
> is a lot more confusing.

I agree too.

> Instead, I think your other patch is the way to go with a small tweak to
> use 1UL (which BTW should be mentioned in the commit message).
>
> Either Stefano or I can deal with the change on commit.

Thank you so much for your detailed patch review.

BR
Paran Lee