| 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 | expand |
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.
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,
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.
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,
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.
>>> 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
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,
>>> 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
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,
>>> 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
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) &&
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,
>>> 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
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 >
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);
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(-)