diff mbox series

[Xen-devel,v5,01/16] x86/mm: skip incrementing mfn if it is not a valid mfn

Message ID 20180314182009.14274-2-julien.grall@arm.com
State New
Headers show
Series xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN | expand

Commit Message

Julien Grall March 14, 2018, 6:19 p.m. UTC 
From: Wei Liu <wei.liu2@citrix.com>

The function is called to fill in page table entries in
populate_pt_range. Skip incrementing mfn if it is invalid.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>

---
    Changes in v5:
        - Patch added
---
 xen/arch/x86/mm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Andrew Cooper March 14, 2018, 6:31 p.m. UTC | #1 
On 14/03/18 18:19, julien.grall@arm.com wrote:
> From: Wei Liu <wei.liu2@citrix.com>
>
> The function is called to fill in page table entries in
> populate_pt_range. Skip incrementing mfn if it is invalid.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Remind me what the purpose of this patch is?

~Andrew
Jan Beulich March 15, 2018, 7:49 a.m. UTC | #2 
>>> On 14.03.18 at 19:19, <julien.grall@arm.com> wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> The function is called to fill in page table entries in
> populate_pt_range. Skip incrementing mfn if it is invalid.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

(You should have Cc-ed Andrew and me.)

Jan
Jan Beulich March 15, 2018, 7:52 a.m. UTC | #3 
>>> On 14.03.18 at 19:31, <andrew.cooper3@citrix.com> wrote:
> On 14/03/18 18:19, julien.grall@arm.com wrote:
>> From: Wei Liu <wei.liu2@citrix.com>
>>
>> The function is called to fill in page table entries in
>> populate_pt_range. Skip incrementing mfn if it is invalid.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Remind me what the purpose of this patch is?

This is in preparation to switch callers to pass INVALID_MFN
instead of zero for non-present mappings. The incrementing
from zero was wrong here already (but couldn't be as easily
avoided, to not cause problems with possible legitimate uses of
MFN 0), but incrementing (and wrapping) from INVALID_MFN is
(imo) even worse, which is why I had asked the conversion to
INVALID_MFN to not be done without this change.

Jan
Andrew Cooper March 15, 2018, 12:16 p.m. UTC | #4 
On 15/03/18 07:52, Jan Beulich wrote:
>>>> On 14.03.18 at 19:31, <andrew.cooper3@citrix.com> wrote:
>> On 14/03/18 18:19, julien.grall@arm.com wrote:
>>> From: Wei Liu <wei.liu2@citrix.com>
>>>
>>> The function is called to fill in page table entries in
>>> populate_pt_range. Skip incrementing mfn if it is invalid.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Remind me what the purpose of this patch is?
> This is in preparation to switch callers to pass INVALID_MFN
> instead of zero for non-present mappings. The incrementing
> from zero was wrong here already (but couldn't be as easily
> avoided, to not cause problems with possible legitimate uses of
> MFN 0), but incrementing (and wrapping) from INVALID_MFN is
> (imo) even worse, which is why I had asked the conversion to
> INVALID_MFN to not be done without this change.

Yes.  My reply was a (clearly too) thinly veiled hint that a sentence to
this effect should be in the commit message.

The code itself is fine.

~Andrew
Julien Grall March 15, 2018, 3:05 p.m. UTC | #5 
On 15/03/18 07:49, Jan Beulich wrote:
>>>> On 14.03.18 at 19:19, <julien.grall@arm.com> wrote:
>> From: Wei Liu <wei.liu2@citrix.com>
>>
>> The function is called to fill in page table entries in
>> populate_pt_range. Skip incrementing mfn if it is invalid.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> (You should have Cc-ed Andrew and me.)

I forgot to update the CCs for this new patch. Sorry for that.

Cheers,
George Dunlap March 15, 2018, 4:06 p.m. UTC | #6 
On Thu, Mar 15, 2018 at 12:16 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 15/03/18 07:52, Jan Beulich wrote:
>>>>> On 14.03.18 at 19:31, <andrew.cooper3@citrix.com> wrote:
>>> On 14/03/18 18:19, julien.grall@arm.com wrote:
>>>> From: Wei Liu <wei.liu2@citrix.com>
>>>>
>>>> The function is called to fill in page table entries in
>>>> populate_pt_range. Skip incrementing mfn if it is invalid.
>>>>
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Remind me what the purpose of this patch is?
>> This is in preparation to switch callers to pass INVALID_MFN
>> instead of zero for non-present mappings. The incrementing
>> from zero was wrong here already (but couldn't be as easily
>> avoided, to not cause problems with possible legitimate uses of
>> MFN 0), but incrementing (and wrapping) from INVALID_MFN is
>> (imo) even worse, which is why I had asked the conversion to
>> INVALID_MFN to not be done without this change.
>
> Yes.  My reply was a (clearly too) thinly veiled hint that a sentence to
> this effect should be in the commit message.

You mean "a (clearly not thinly enough) veiled hint". :-)

I agree with Andy re both the code and the commit message.

 -George
diff mbox series

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 9b559448a7..5f5577c7c2 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4731,7 +4731,8 @@  int map_pages_to_xen(
             }
 
             virt    += 1UL << L3_PAGETABLE_SHIFT;
-            mfn     += 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
+            if ( !mfn_eq(_mfn(mfn), INVALID_MFN) )
+                mfn += 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
             nr_mfns -= 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
             continue;
         }
@@ -4756,7 +4757,8 @@  int map_pages_to_xen(
                 if ( i > nr_mfns )
                     i = nr_mfns;
                 virt    += i << PAGE_SHIFT;
-                mfn     += i;
+                if ( !mfn_eq(_mfn(mfn), INVALID_MFN) )
+                    mfn += i;
                 nr_mfns -= i;
                 continue;
             }
@@ -4824,7 +4826,8 @@  int map_pages_to_xen(
             }
 
             virt    += 1UL << L2_PAGETABLE_SHIFT;
-            mfn     += 1UL << PAGETABLE_ORDER;
+            if ( !mfn_eq(_mfn(mfn), INVALID_MFN) )
+                mfn += 1UL << PAGETABLE_ORDER;
             nr_mfns -= 1UL << PAGETABLE_ORDER;
         }
         else
@@ -4853,7 +4856,8 @@  int map_pages_to_xen(
                     if ( i > nr_mfns )
                         i = nr_mfns;
                     virt    += i << L1_PAGETABLE_SHIFT;
-                    mfn     += i;
+                    if ( !mfn_eq(_mfn(mfn), INVALID_MFN) )
+                        mfn += i;
                     nr_mfns -= i;
                     goto check_l3;
                 }
@@ -4898,7 +4902,8 @@  int map_pages_to_xen(
             }
 
             virt    += 1UL << L1_PAGETABLE_SHIFT;
-            mfn     += 1UL;
+            if ( !mfn_eq(_mfn(mfn), INVALID_MFN) )
+                mfn += 1UL;
             nr_mfns -= 1UL;
 
             if ( (flags == PAGE_HYPERVISOR) &&