[Xen-devel,v4,08/16] xen/mm: Drop the parameter mfn from populate_pt_range

Message ID 20180221140259.29360-9-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN
Related show

Commit Message

Julien Grall Feb. 21, 2018, 2:02 p.m.
The function populate_pt_range is used to populate in advance the
page-table but it will not do the actual mapping. So passing the MFN in
parameter is pointless. Note that the only caller pass 0...

At the same time replace 0 by INVALID_MFN to make clear the MFN is
invalid.

Signed-off-by: Julien Grall <julien.grall@arm.com>

--

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

The move from 0 to INVALID_MFN will work on Arm. I am not entirely sure
for the x86 version.

    Changes in v4:
        - Patch added.
---
 xen/arch/arm/mm.c    | 5 ++---
 xen/arch/x86/mm.c    | 5 ++---
 xen/common/vmap.c    | 2 +-
 xen/include/xen/mm.h | 3 +--
 4 files changed, 6 insertions(+), 9 deletions(-)

Comments

Wei Liu Feb. 22, 2018, 4:35 p.m. | #1
On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
> The function populate_pt_range is used to populate in advance the
> page-table but it will not do the actual mapping. So passing the MFN in
> parameter is pointless. Note that the only caller pass 0...
> 
> At the same time replace 0 by INVALID_MFN to make clear the MFN is
> invalid.
> 

The mfn parameter is the first mfn of a consecutive nr MFNs passed to
map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
to page table(s) will wrap around to 0.

And I think starting from 0 to avoid overflow is probably a better
behaviour. If you really want to make sure all entries are filled with
INVALID_MFN you should call map_pages_to_xen for nr times with each
page.

Wei.
Julien Grall Feb. 22, 2018, 4:40 p.m. | #2
Hi Wei,

On 22/02/18 16:35, Wei Liu wrote:
> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>> The function populate_pt_range is used to populate in advance the
>> page-table but it will not do the actual mapping. So passing the MFN in
>> parameter is pointless. Note that the only caller pass 0...
>>
>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>> invalid.
>>
> 
> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
> to page table(s) will wrap around to 0.
> 
> And I think starting from 0 to avoid overflow is probably a better
> behaviour. If you really want to make sure all entries are filled with
> INVALID_MFN you should call map_pages_to_xen for nr times with each
> page.

I am not sure to understand this. From its name, populate_pt_range 
should only create the intermediate tables. The leaf entry will stay 
invalid. So how the value of mfn matters? Is it because the code is 
written in a such way that passing INVALID_MFN will result to undefined 
behavior?

Cheers,
Wei Liu Feb. 22, 2018, 4:51 p.m. | #3
On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
> Hi Wei,
> 
> On 22/02/18 16:35, Wei Liu wrote:
> > On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
> > > The function populate_pt_range is used to populate in advance the
> > > page-table but it will not do the actual mapping. So passing the MFN in
> > > parameter is pointless. Note that the only caller pass 0...
> > > 
> > > At the same time replace 0 by INVALID_MFN to make clear the MFN is
> > > invalid.
> > > 
> > 
> > The mfn parameter is the first mfn of a consecutive nr MFNs passed to
> > map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
> > to page table(s) will wrap around to 0.
> > 
> > And I think starting from 0 to avoid overflow is probably a better
> > behaviour. If you really want to make sure all entries are filled with
> > INVALID_MFN you should call map_pages_to_xen for nr times with each
> > page.
> 
> I am not sure to understand this. From its name, populate_pt_range should
> only create the intermediate tables. The leaf entry will stay invalid. So
> how the value of mfn matters? Is it because the code is written in a such
> way that passing INVALID_MFN will result to undefined behavior?

Right, that's what I meant. It doesn't matter whether you use 0 or
INVALID_MFN.

Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
safe.

But your intention seemed to be filling all entries with INVALID_MFN to
aid debugging, so the function doesn't do what I think you wanted it to
do. It could be I misunderstood your intention.

Wei.
Julien Grall Feb. 22, 2018, 4:55 p.m. | #4
Hi Wei,

On 22/02/18 16:51, Wei Liu wrote:
> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>> Hi Wei,
>>
>> On 22/02/18 16:35, Wei Liu wrote:
>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>> The function populate_pt_range is used to populate in advance the
>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>> parameter is pointless. Note that the only caller pass 0...
>>>>
>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>> invalid.
>>>>
>>>
>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>> to page table(s) will wrap around to 0.
>>>
>>> And I think starting from 0 to avoid overflow is probably a better
>>> behaviour. If you really want to make sure all entries are filled with
>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>> page.
>>
>> I am not sure to understand this. From its name, populate_pt_range should
>> only create the intermediate tables. The leaf entry will stay invalid. So
>> how the value of mfn matters? Is it because the code is written in a such
>> way that passing INVALID_MFN will result to undefined behavior?
> 
> Right, that's what I meant. It doesn't matter whether you use 0 or
> INVALID_MFN.
> 
> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
> safe.
> 
> But your intention seemed to be filling all entries with INVALID_MFN to
> aid debugging, so the function doesn't do what I think you wanted it to
> do. It could be I misunderstood your intention.

That was not my intention. I replaced 0 by INVALID_MFN because from the 
name you know the MFN is invalid. 0 could potentially be valid (at least 
on Arm) and make the code confusing to understand.

I can make it clearer in the commit message.

Cheers,
Wei Liu Feb. 22, 2018, 5:10 p.m. | #5
On Thu, Feb 22, 2018 at 04:55:10PM +0000, Julien Grall wrote:
> Hi Wei,
> 
> On 22/02/18 16:51, Wei Liu wrote:
> > On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
> > > Hi Wei,
> > > 
> > > On 22/02/18 16:35, Wei Liu wrote:
> > > > On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
> > > > > The function populate_pt_range is used to populate in advance the
> > > > > page-table but it will not do the actual mapping. So passing the MFN in
> > > > > parameter is pointless. Note that the only caller pass 0...
> > > > > 
> > > > > At the same time replace 0 by INVALID_MFN to make clear the MFN is
> > > > > invalid.
> > > > > 
> > > > 
> > > > The mfn parameter is the first mfn of a consecutive nr MFNs passed to
> > > > map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
> > > > to page table(s) will wrap around to 0.
> > > > 
> > > > And I think starting from 0 to avoid overflow is probably a better
> > > > behaviour. If you really want to make sure all entries are filled with
> > > > INVALID_MFN you should call map_pages_to_xen for nr times with each
> > > > page.
> > > 
> > > I am not sure to understand this. From its name, populate_pt_range should
> > > only create the intermediate tables. The leaf entry will stay invalid. So
> > > how the value of mfn matters? Is it because the code is written in a such
> > > way that passing INVALID_MFN will result to undefined behavior?
> > 
> > Right, that's what I meant. It doesn't matter whether you use 0 or
> > INVALID_MFN.
> > 
> > Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
> > safe.
> > 
> > But your intention seemed to be filling all entries with INVALID_MFN to
> > aid debugging, so the function doesn't do what I think you wanted it to
> > do. It could be I misunderstood your intention.
> 
> That was not my intention. I replaced 0 by INVALID_MFN because from the name
> you know the MFN is invalid. 0 could potentially be valid (at least on Arm)
> and make the code confusing to understand.
> 
> I can make it clearer in the commit message.

Alright. That would be good.

Wei.
Jan Beulich March 2, 2018, 2:55 p.m. | #6
>>> On 22.02.18 at 17:55, <julien.grall@arm.com> wrote:
> On 22/02/18 16:51, Wei Liu wrote:
>> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>>> On 22/02/18 16:35, Wei Liu wrote:
>>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>>> The function populate_pt_range is used to populate in advance the
>>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>>> parameter is pointless. Note that the only caller pass 0...
>>>>>
>>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>>> invalid.
>>>>>
>>>>
>>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>>> to page table(s) will wrap around to 0.
>>>>
>>>> And I think starting from 0 to avoid overflow is probably a better
>>>> behaviour. If you really want to make sure all entries are filled with
>>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>>> page.
>>>
>>> I am not sure to understand this. From its name, populate_pt_range should
>>> only create the intermediate tables. The leaf entry will stay invalid. So
>>> how the value of mfn matters? Is it because the code is written in a such
>>> way that passing INVALID_MFN will result to undefined behavior?
>> 
>> Right, that's what I meant. It doesn't matter whether you use 0 or
>> INVALID_MFN.
>> 
>> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
>> safe.
>> 
>> But your intention seemed to be filling all entries with INVALID_MFN to
>> aid debugging, so the function doesn't do what I think you wanted it to
>> do. It could be I misunderstood your intention.
> 
> That was not my intention. I replaced 0 by INVALID_MFN because from the 
> name you know the MFN is invalid. 0 could potentially be valid (at least 
> on Arm) and make the code confusing to understand.
> 
> I can make it clearer in the commit message.

I don't think that'll be much better; I agree with Wei that you
don't want the wrapping behavior here. What you want to do
is skip the increments in x86's map_pages_to_xen() when
mfn is INVALID_MFN. Granted this should have been done
before (so that there wouldn't have been incrementing from
zero), but as you say MFN 0 isn't fundamentally invalid (albeit
on x86 we almost make it invalid).

As to your earlier argument - please don't forget that on x86
the function still fills all leaf entries in the range, just that they
all will be non-present ones.

Jan
Julien Grall March 5, 2018, 1:43 p.m. | #7
Hi Jan,

On 02/03/18 14:55, Jan Beulich wrote:
>>>> On 22.02.18 at 17:55, <julien.grall@arm.com> wrote:
>> On 22/02/18 16:51, Wei Liu wrote:
>>> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>>>> On 22/02/18 16:35, Wei Liu wrote:
>>>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>>>> The function populate_pt_range is used to populate in advance the
>>>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>>>> parameter is pointless. Note that the only caller pass 0...
>>>>>>
>>>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>>>> invalid.
>>>>>>
>>>>>
>>>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>>>> to page table(s) will wrap around to 0.
>>>>>
>>>>> And I think starting from 0 to avoid overflow is probably a better
>>>>> behaviour. If you really want to make sure all entries are filled with
>>>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>>>> page.
>>>>
>>>> I am not sure to understand this. From its name, populate_pt_range should
>>>> only create the intermediate tables. The leaf entry will stay invalid. So
>>>> how the value of mfn matters? Is it because the code is written in a such
>>>> way that passing INVALID_MFN will result to undefined behavior?
>>>
>>> Right, that's what I meant. It doesn't matter whether you use 0 or
>>> INVALID_MFN.
>>>
>>> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
>>> safe.
>>>
>>> But your intention seemed to be filling all entries with INVALID_MFN to
>>> aid debugging, so the function doesn't do what I think you wanted it to
>>> do. It could be I misunderstood your intention.
>>
>> That was not my intention. I replaced 0 by INVALID_MFN because from the
>> name you know the MFN is invalid. 0 could potentially be valid (at least
>> on Arm) and make the code confusing to understand.
>>
>> I can make it clearer in the commit message.
> 
> I don't think that'll be much better; I agree with Wei that you
> don't want the wrapping behavior here. What you want to do
> is skip the increments in x86's map_pages_to_xen() when
> mfn is INVALID_MFN. Granted this should have been done
> before (so that there wouldn't have been incrementing from
> zero), but as you say MFN 0 isn't fundamentally invalid (albeit
> on x86 we almost make it invalid).
> 
> As to your earlier argument - please don't forget that on x86
> the function still fills all leaf entries in the range, just that they
> all will be non-present ones.

I still don't understand why it matters. The entry is not present so the 
address is going to ignore. 0 or MFN_INVALID are just dummy value that 
are going to be replaced on the entry is made present.

Furthermore, as Wei pointed out unsigned integer overflow is not UB in 
C, so passing INVALID_MFN is safe.

Anyway, I don't have much knowledge on the x86 to make the modification 
that you suggested. So I am going to revert to _mfn(0) for x86.

Cheers,
Jan Beulich March 5, 2018, 2 p.m. | #8
>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
> On 02/03/18 14:55, Jan Beulich wrote:
>>>>> On 22.02.18 at 17:55, <julien.grall@arm.com> wrote:
>>> On 22/02/18 16:51, Wei Liu wrote:
>>>> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>>>>> On 22/02/18 16:35, Wei Liu wrote:
>>>>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>>>>> The function populate_pt_range is used to populate in advance the
>>>>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>>>>> parameter is pointless. Note that the only caller pass 0...
>>>>>>>
>>>>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>>>>> invalid.
>>>>>>>
>>>>>>
>>>>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>>>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>>>>> to page table(s) will wrap around to 0.
>>>>>>
>>>>>> And I think starting from 0 to avoid overflow is probably a better
>>>>>> behaviour. If you really want to make sure all entries are filled with
>>>>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>>>>> page.
>>>>>
>>>>> I am not sure to understand this. From its name, populate_pt_range should
>>>>> only create the intermediate tables. The leaf entry will stay invalid. So
>>>>> how the value of mfn matters? Is it because the code is written in a such
>>>>> way that passing INVALID_MFN will result to undefined behavior?
>>>>
>>>> Right, that's what I meant. It doesn't matter whether you use 0 or
>>>> INVALID_MFN.
>>>>
>>>> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
>>>> safe.
>>>>
>>>> But your intention seemed to be filling all entries with INVALID_MFN to
>>>> aid debugging, so the function doesn't do what I think you wanted it to
>>>> do. It could be I misunderstood your intention.
>>>
>>> That was not my intention. I replaced 0 by INVALID_MFN because from the
>>> name you know the MFN is invalid. 0 could potentially be valid (at least
>>> on Arm) and make the code confusing to understand.
>>>
>>> I can make it clearer in the commit message.
>> 
>> I don't think that'll be much better; I agree with Wei that you
>> don't want the wrapping behavior here. What you want to do
>> is skip the increments in x86's map_pages_to_xen() when
>> mfn is INVALID_MFN. Granted this should have been done
>> before (so that there wouldn't have been incrementing from
>> zero), but as you say MFN 0 isn't fundamentally invalid (albeit
>> on x86 we almost make it invalid).
>> 
>> As to your earlier argument - please don't forget that on x86
>> the function still fills all leaf entries in the range, just that they
>> all will be non-present ones.
> 
> I still don't understand why it matters. The entry is not present so the 
> address is going to ignore. 0 or MFN_INVALID are just dummy value that 
> are going to be replaced on the entry is made present.
> 
> Furthermore, as Wei pointed out unsigned integer overflow is not UB in 
> C, so passing INVALID_MFN is safe.

I didn't say it's unsafe. It's clumsy and misleading.

> Anyway, I don't have much knowledge on the x86 to make the modification 
> that you suggested. So I am going to revert to _mfn(0) for x86.

I'd prefer if you didn't, but well, it'll be one of us to clean it up
then.

Jan
Julien Grall March 5, 2018, 2:11 p.m. | #9
Hi Jan,

On 05/03/18 14:00, Jan Beulich wrote:
>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
>> On 02/03/18 14:55, Jan Beulich wrote:
>>>>>> On 22.02.18 at 17:55, <julien.grall@arm.com> wrote:
>>>> On 22/02/18 16:51, Wei Liu wrote:
>>>>> On Thu, Feb 22, 2018 at 04:40:04PM +0000, Julien Grall wrote:
>>>>>> On 22/02/18 16:35, Wei Liu wrote:
>>>>>>> On Wed, Feb 21, 2018 at 02:02:51PM +0000, Julien Grall wrote:
>>>>>>>> The function populate_pt_range is used to populate in advance the
>>>>>>>> page-table but it will not do the actual mapping. So passing the MFN in
>>>>>>>> parameter is pointless. Note that the only caller pass 0...
>>>>>>>>
>>>>>>>> At the same time replace 0 by INVALID_MFN to make clear the MFN is
>>>>>>>> invalid.
>>>>>>>>
>>>>>>>
>>>>>>> The mfn parameter is the first mfn of a consecutive nr MFNs passed to
>>>>>>> map_pages_to_xen. Putting INVALID_MFN isn't helping -- the value written
>>>>>>> to page table(s) will wrap around to 0.
>>>>>>>
>>>>>>> And I think starting from 0 to avoid overflow is probably a better
>>>>>>> behaviour. If you really want to make sure all entries are filled with
>>>>>>> INVALID_MFN you should call map_pages_to_xen for nr times with each
>>>>>>> page.
>>>>>>
>>>>>> I am not sure to understand this. From its name, populate_pt_range should
>>>>>> only create the intermediate tables. The leaf entry will stay invalid. So
>>>>>> how the value of mfn matters? Is it because the code is written in a such
>>>>>> way that passing INVALID_MFN will result to undefined behavior?
>>>>>
>>>>> Right, that's what I meant. It doesn't matter whether you use 0 or
>>>>> INVALID_MFN.
>>>>>
>>>>> Unsigned integer overflow is not UB in C, so passing INVALID_MFN is
>>>>> safe.
>>>>>
>>>>> But your intention seemed to be filling all entries with INVALID_MFN to
>>>>> aid debugging, so the function doesn't do what I think you wanted it to
>>>>> do. It could be I misunderstood your intention.
>>>>
>>>> That was not my intention. I replaced 0 by INVALID_MFN because from the
>>>> name you know the MFN is invalid. 0 could potentially be valid (at least
>>>> on Arm) and make the code confusing to understand.
>>>>
>>>> I can make it clearer in the commit message.
>>>
>>> I don't think that'll be much better; I agree with Wei that you
>>> don't want the wrapping behavior here. What you want to do
>>> is skip the increments in x86's map_pages_to_xen() when
>>> mfn is INVALID_MFN. Granted this should have been done
>>> before (so that there wouldn't have been incrementing from
>>> zero), but as you say MFN 0 isn't fundamentally invalid (albeit
>>> on x86 we almost make it invalid).
>>>
>>> As to your earlier argument - please don't forget that on x86
>>> the function still fills all leaf entries in the range, just that they
>>> all will be non-present ones.
>>
>> I still don't understand why it matters. The entry is not present so the
>> address is going to ignore. 0 or MFN_INVALID are just dummy value that
>> are going to be replaced on the entry is made present.
>>
>> Furthermore, as Wei pointed out unsigned integer overflow is not UB in
>> C, so passing INVALID_MFN is safe.
> 
> I didn't say it's unsafe. It's clumsy and misleading.

Well, that's just the way the page-table code has been written on x86. 
On Arm, the code is much clearer with INVALID_MFN than _mfn(0).

> 
>> Anyway, I don't have much knowledge on the x86 to make the modification
>> that you suggested. So I am going to revert to _mfn(0) for x86.
> 
> I'd prefer if you didn't, but well, it'll be one of us to clean it up
> then.
I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86 
folks) would have to provide the patch to skip incrementing invalid MFN 
(if I understood correctly your request).

Cheers,
Jan Beulich March 5, 2018, 2:38 p.m. | #10
>>> On 05.03.18 at 15:11, <julien.grall@arm.com> wrote:
> On 05/03/18 14:00, Jan Beulich wrote:
>>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
>>> Anyway, I don't have much knowledge on the x86 to make the modification
>>> that you suggested. So I am going to revert to _mfn(0) for x86.
>> 
>> I'd prefer if you didn't, but well, it'll be one of us to clean it up
>> then.
> I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86 
> folks) would have to provide the patch to skip incrementing invalid MFN 
> (if I understood correctly your request).

Sigh - this should go together imo. While wrongly incrementing from
zero was bad, wrongly wrapping from INVALID_MFN makes things
worse.

Jan
Wei Liu March 9, 2018, 5:29 p.m. | #11
On Mon, Mar 05, 2018 at 07:38:36AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 15:11, <julien.grall@arm.com> wrote:
> > On 05/03/18 14:00, Jan Beulich wrote:
> >>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
> >>> Anyway, I don't have much knowledge on the x86 to make the modification
> >>> that you suggested. So I am going to revert to _mfn(0) for x86.
> >> 
> >> I'd prefer if you didn't, but well, it'll be one of us to clean it up
> >> then.
> > I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86 
> > folks) would have to provide the patch to skip incrementing invalid MFN 
> > (if I understood correctly your request).
> 
> Sigh - this should go together imo. While wrongly incrementing from
> zero was bad, wrongly wrapping from INVALID_MFN makes things
> worse.
> 

Try this patch?

---8<---
From 8f0024c690c736d17adde0fa765cbbf6fa2846dc Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Fri, 9 Mar 2018 17:20:14 +0000
Subject: [PATCH] x86/mm: skip incrementing mfn if it is not a valid mfn

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>
---
 xen/arch/x86/mm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

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) &&
Julien Grall March 11, 2018, 7:30 p.m. | #12
Hi Wei,

On 03/09/2018 05:29 PM, Wei Liu wrote:
> On Mon, Mar 05, 2018 at 07:38:36AM -0700, Jan Beulich wrote:
>>>>> On 05.03.18 at 15:11, <julien.grall@arm.com> wrote:
>>> On 05/03/18 14:00, Jan Beulich wrote:
>>>>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
>>>>> Anyway, I don't have much knowledge on the x86 to make the modification
>>>>> that you suggested. So I am going to revert to _mfn(0) for x86.
>>>>
>>>> I'd prefer if you didn't, but well, it'll be one of us to clean it up
>>>> then.
>>> I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86
>>> folks) would have to provide the patch to skip incrementing invalid MFN
>>> (if I understood correctly your request).
>>
>> Sigh - this should go together imo. While wrongly incrementing from
>> zero was bad, wrongly wrapping from INVALID_MFN makes things
>> worse.
>>
> 
> Try this patch?

I am happy to carry this patch at the beginning of my series if you want.

> 
> ---8<---
>  From 8f0024c690c736d17adde0fa765cbbf6fa2846dc Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Fri, 9 Mar 2018 17:20:14 +0000
> Subject: [PATCH] x86/mm: skip incrementing mfn if it is not a valid mfn
> 
> 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>
> ---
>   xen/arch/x86/mm.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> 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) &&
> 

Cheers,
Jan Beulich March 12, 2018, 6:36 a.m. | #13
>>> Wei Liu <wei.liu2@citrix.com> 03/09/18 6:30 PM >>>
>On Mon, Mar 05, 2018 at 07:38:36AM -0700, Jan Beulich wrote:
>> >>> On 05.03.18 at 15:11, <julien.grall@arm.com> wrote:
>> > On 05/03/18 14:00, Jan Beulich wrote:
>> >>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
>> >>> Anyway, I don't have much knowledge on the x86 to make the modification
>> >>> that you suggested. So I am going to revert to _mfn(0) for x86.
>> >> 
>> >> I'd prefer if you didn't, but well, it'll be one of us to clean it up
>> >> then.
>> > I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86 
>> > folks) would have to provide the patch to skip incrementing invalid MFN 
>> > (if I understood correctly your request).
>> 
>> Sigh - this should go together imo. While wrongly incrementing from
>> zero was bad, wrongly wrapping from INVALID_MFN makes things
>> worse.
>
>Try this patch?

Looks fine; Julien, do you want to fold this in?

Jan
Julien Grall March 14, 2018, 3:22 p.m. | #14
Hi Jan,

On 03/12/2018 06:36 AM, Jan Beulich wrote:
>>>> Wei Liu <wei.liu2@citrix.com> 03/09/18 6:30 PM >>>
>> On Mon, Mar 05, 2018 at 07:38:36AM -0700, Jan Beulich wrote:
>>>>>> On 05.03.18 at 15:11, <julien.grall@arm.com> wrote:
>>>> On 05/03/18 14:00, Jan Beulich wrote:
>>>>>>>> On 05.03.18 at 14:43, <julien.grall@arm.com> wrote:
>>>>>> Anyway, I don't have much knowledge on the x86 to make the modification
>>>>>> that you suggested. So I am going to revert to _mfn(0) for x86.
>>>>>
>>>>> I'd prefer if you didn't, but well, it'll be one of us to clean it up
>>>>> then.
>>>> I can keep as INVALID_MFN. But then either you or Andrew (or anyone x86
>>>> folks) would have to provide the patch to skip incrementing invalid MFN
>>>> (if I understood correctly your request).
>>>
>>> Sigh - this should go together imo. While wrongly incrementing from
>>> zero was bad, wrongly wrapping from INVALID_MFN makes things
>>> worse.
>>
>> Try this patch?
> 
> Looks fine; Julien, do you want to fold this in?

Rather than folding this in,  I am planning to add this patch at the 
beginning of the series.

Cheers,

> 
> Jan
>

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9b77ab5f33..97dcdd5d50 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1072,10 +1072,9 @@  int map_pages_to_xen(unsigned long virt,
     return create_xen_entries(INSERT, virt, _mfn(mfn), nr_mfns, flags);
 }
 
-int populate_pt_range(unsigned long virt, unsigned long mfn,
-                      unsigned long nr_mfns)
+int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 {
-    return create_xen_entries(RESERVE, virt, _mfn(mfn), nr_mfns, 0);
+    return create_xen_entries(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
 }
 
 int destroy_xen_mappings(unsigned long v, unsigned long e)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index eba1afaa31..ecbfc95ae2 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5012,10 +5012,9 @@  int map_pages_to_xen(
     return 0;
 }
 
-int populate_pt_range(unsigned long virt, unsigned long mfn,
-                      unsigned long nr_mfns)
+int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
 {
-    return map_pages_to_xen(virt, mfn, nr_mfns, MAP_SMALL_PAGES);
+    return map_pages_to_xen(virt, mfn_x(INVALID_MFN), nr_mfns, MAP_SMALL_PAGES);
 }
 
 /*
diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 0b23f8fb97..11785ffb0a 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -42,7 +42,7 @@  void __init vm_init_type(enum vmap_region type, void *start, void *end)
     bitmap_fill(vm_bitmap(type), vm_low[type]);
 
     /* Populate page tables for the bitmap if necessary. */
-    populate_pt_range(va, 0, vm_low[type] - nr);
+    populate_pt_range(va, vm_low[type] - nr);
 }
 
 static void *vm_alloc(unsigned int nr, unsigned int align,
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 0e0e5112c6..f2c6738ad2 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -175,8 +175,7 @@  int destroy_xen_mappings(unsigned long v, unsigned long e);
  * Create only non-leaf page table entries for the
  * page range in Xen virtual address space.
  */
-int populate_pt_range(unsigned long virt, unsigned long mfn,
-                      unsigned long nr_mfns);
+int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
 /* Claim handling */
 unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
 int domain_set_outstanding_pages(struct domain *d, unsigned long pages);