| Message ID | 20220426154904.GA11482@DESKTOP-NK4TH6S.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen/arm: p2m_set_entry reuse mask variables | expand |
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,
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
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;
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(-)