| Message ID | 20241106122927.26461-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/mm: miscellaneous fixes | expand |
On 06.11.2024 13:29, Roger Pau Monne wrote: > bootstrap_map_addr() top level comment states that it doesn't indented to > remove the L2 tables, as the same L2 will be re-used to create further 2MB > mappings. With that I assume you refer to the 2nd sentence in the comment immediately ahead of the function. According to my reading, it may imply what you say, but it certainly doesn't "state" this. Imo the problem was latent already before, if BOOTSTRAP_MAP_{BASE,LIMIT} were changed to cover at least one full L3E range. Which isn't to say that ... > It's incorrect for the function to use destroy_xen_mappings() which > will free empty L2 tables. > > Fix this by using map_pages_to_xen(), which does zap the page-table entries, > but does not free page-table structures even when empty. > > Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') > Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> > --- > The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly > when empty, it would have become obvious that bootstrap_map_addr() shouldn't be > using it if it wants to keep the L2. 4376c05c3113 should have switched > bootstrap_map_addr() to not use destroy_xen_mappings(). ... I mind the commit you name to be blamed. It was clearly something I missed back then. With the 1st sentence of the description re-worded some: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Thu, Nov 07, 2024 at 12:23:51PM +0100, Jan Beulich wrote: > On 06.11.2024 13:29, Roger Pau Monne wrote: > > bootstrap_map_addr() top level comment states that it doesn't indented to > > remove the L2 tables, as the same L2 will be re-used to create further 2MB > > mappings. > > With that I assume you refer to the 2nd sentence in the comment immediately > ahead of the function. According to my reading, it may imply what you say, > but it certainly doesn't "state" this. Imo the problem was latent already > before, if BOOTSTRAP_MAP_{BASE,LIMIT} were changed to cover at least one > full L3E range. Which isn't to say that ... Hm, maybe I've implied too much from that comment. What about replacing the first paragraph with: "bootstrap_map_addr() needs to be careful to not remove existing page-table structures when tearing down mappings, as such pagetable structures might be needed to fulfill subsequent mappings requests. The comment ahead of the function already notes that pagetable memory shouldn't be allocated." > > It's incorrect for the function to use destroy_xen_mappings() which > > will free empty L2 tables. > > > > Fix this by using map_pages_to_xen(), which does zap the page-table entries, > > but does not free page-table structures even when empty. > > > > Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') > > Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> > > --- > > The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly > > when empty, it would have become obvious that bootstrap_map_addr() shouldn't be > > using it if it wants to keep the L2. 4376c05c3113 should have switched > > bootstrap_map_addr() to not use destroy_xen_mappings(). > > ... I mind the commit you name to be blamed. It was clearly something I > missed back then. > > With the 1st sentence of the description re-worded some: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, Roger.
On 07.11.2024 12:54, Roger Pau Monné wrote: > On Thu, Nov 07, 2024 at 12:23:51PM +0100, Jan Beulich wrote: >> On 06.11.2024 13:29, Roger Pau Monne wrote: >>> bootstrap_map_addr() top level comment states that it doesn't indented to >>> remove the L2 tables, as the same L2 will be re-used to create further 2MB >>> mappings. >> >> With that I assume you refer to the 2nd sentence in the comment immediately >> ahead of the function. According to my reading, it may imply what you say, >> but it certainly doesn't "state" this. Imo the problem was latent already >> before, if BOOTSTRAP_MAP_{BASE,LIMIT} were changed to cover at least one >> full L3E range. Which isn't to say that ... > > Hm, maybe I've implied too much from that comment. What about > replacing the first paragraph with: > > "bootstrap_map_addr() needs to be careful to not remove existing > page-table structures when tearing down mappings, as such pagetable > structures might be needed to fulfill subsequent mappings requests. > The comment ahead of the function already notes that pagetable memory > shouldn't be allocated." SGTM. Thanks, Jan
On 06/11/2024 12:29 pm, Roger Pau Monne wrote: > bootstrap_map_addr() top level comment states that it doesn't indented to > remove the L2 tables, as the same L2 will be re-used to create further 2MB > mappings. It's incorrect for the function to use destroy_xen_mappings() which > will free empty L2 tables. > > Fix this by using map_pages_to_xen(), which does zap the page-table entries, > but does not free page-table structures even when empty. > > Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') > Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> > --- > The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly > when empty, it would have become obvious that bootstrap_map_addr() shouldn't be > using it if it wants to keep the L2. 4376c05c3113 should have switched > bootstrap_map_addr() to not use destroy_xen_mappings(). > --- > xen/arch/x86/setup.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 177f4024abca..815b8651ba79 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -456,7 +456,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end) > > if ( !end ) > { > - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); > + map_pages_to_xen(BOOTSTRAP_MAP_BASE, INVALID_MFN, > + PFN_DOWN(map_cur - BOOTSTRAP_MAP_BASE), > + _PAGE_NONE); > map_cur = BOOTSTRAP_MAP_BASE; One option to consider is this. diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index eac8488c4ca5..b317a4d12f55 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -461,8 +461,13 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end) if ( !end ) { - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); - map_cur = BOOTSTRAP_MAP_BASE; + if ( map_cur > BOOTSTRAP_MAP_BASE ) + { + memset(&l2_bootmap[l2_table_offset(BOOTSTRAP_MAP_BASE)], + 0, (map_cur - BOOTSTRAP_MAP_BASE) >> L2_PAGETABLE_SHIFT); + flush_tlb_local(); + map_cur = BOOTSTRAP_MAP_BASE; + } return NULL; } We know for certain there's nothing to free, and and this far less logic than either destroy_xen_mappings() or map_pages_to_xen(). ~Andrew
On 08.11.2024 10:41, Andrew Cooper wrote: > On 06/11/2024 12:29 pm, Roger Pau Monne wrote: >> bootstrap_map_addr() top level comment states that it doesn't indented to >> remove the L2 tables, as the same L2 will be re-used to create further 2MB >> mappings. It's incorrect for the function to use destroy_xen_mappings() which >> will free empty L2 tables. >> >> Fix this by using map_pages_to_xen(), which does zap the page-table entries, >> but does not free page-table structures even when empty. >> >> Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') >> Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> >> --- >> The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly >> when empty, it would have become obvious that bootstrap_map_addr() shouldn't be >> using it if it wants to keep the L2. 4376c05c3113 should have switched >> bootstrap_map_addr() to not use destroy_xen_mappings(). >> --- >> xen/arch/x86/setup.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 177f4024abca..815b8651ba79 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -456,7 +456,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end) >> >> if ( !end ) >> { >> - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); >> + map_pages_to_xen(BOOTSTRAP_MAP_BASE, INVALID_MFN, >> + PFN_DOWN(map_cur - BOOTSTRAP_MAP_BASE), >> + _PAGE_NONE); >> map_cur = BOOTSTRAP_MAP_BASE; > > One option to consider is this. > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index eac8488c4ca5..b317a4d12f55 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -461,8 +461,13 @@ static void *__init bootstrap_map_addr(paddr_t > start, paddr_t end) > > if ( !end ) > { > - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); > - map_cur = BOOTSTRAP_MAP_BASE; > + if ( map_cur > BOOTSTRAP_MAP_BASE ) > + { > + memset(&l2_bootmap[l2_table_offset(BOOTSTRAP_MAP_BASE)], > + 0, (map_cur - BOOTSTRAP_MAP_BASE) >> > L2_PAGETABLE_SHIFT); > + flush_tlb_local(); > + map_cur = BOOTSTRAP_MAP_BASE; > + } > return NULL; > } > > We know for certain there's nothing to free, and and this far less logic > than either destroy_xen_mappings() or map_pages_to_xen(). Yet then such open-coding can badly bite us at other times. Jan
On Fri, Nov 08, 2024 at 09:41:35AM +0000, Andrew Cooper wrote: > On 06/11/2024 12:29 pm, Roger Pau Monne wrote: > > bootstrap_map_addr() top level comment states that it doesn't indented to > > remove the L2 tables, as the same L2 will be re-used to create further 2MB > > mappings. It's incorrect for the function to use destroy_xen_mappings() which > > will free empty L2 tables. > > > > Fix this by using map_pages_to_xen(), which does zap the page-table entries, > > but does not free page-table structures even when empty. > > > > Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') > > Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> > > --- > > The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly > > when empty, it would have become obvious that bootstrap_map_addr() shouldn't be > > using it if it wants to keep the L2. 4376c05c3113 should have switched > > bootstrap_map_addr() to not use destroy_xen_mappings(). > > --- > > xen/arch/x86/setup.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > > index 177f4024abca..815b8651ba79 100644 > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -456,7 +456,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end) > > > > if ( !end ) > > { > > - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); > > + map_pages_to_xen(BOOTSTRAP_MAP_BASE, INVALID_MFN, > > + PFN_DOWN(map_cur - BOOTSTRAP_MAP_BASE), > > + _PAGE_NONE); > > map_cur = BOOTSTRAP_MAP_BASE; > > One option to consider is this. > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index eac8488c4ca5..b317a4d12f55 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -461,8 +461,13 @@ static void *__init bootstrap_map_addr(paddr_t > start, paddr_t end) > > if ( !end ) > { > - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); > - map_cur = BOOTSTRAP_MAP_BASE; > + if ( map_cur > BOOTSTRAP_MAP_BASE ) > + { > + memset(&l2_bootmap[l2_table_offset(BOOTSTRAP_MAP_BASE)], > + 0, (map_cur - BOOTSTRAP_MAP_BASE) >> > L2_PAGETABLE_SHIFT); > + flush_tlb_local(); > + map_cur = BOOTSTRAP_MAP_BASE; > + } > return NULL; > } > > We know for certain there's nothing to free, and and this far less logic > than either destroy_xen_mappings() or map_pages_to_xen(). Should we then also consider using l2_bootmap directly when creating the mappings? So that we have symmetry between the map and unmap logic. I think it might be better do to this change as a followup patch, as I would like to change both the map and unmap paths at the same time. Thanks, Roger.
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 177f4024abca..815b8651ba79 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -456,7 +456,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end) if ( !end ) { - destroy_xen_mappings(BOOTSTRAP_MAP_BASE, BOOTSTRAP_MAP_LIMIT); + map_pages_to_xen(BOOTSTRAP_MAP_BASE, INVALID_MFN, + PFN_DOWN(map_cur - BOOTSTRAP_MAP_BASE), + _PAGE_NONE); map_cur = BOOTSTRAP_MAP_BASE; return NULL; }
bootstrap_map_addr() top level comment states that it doesn't indented to remove the L2 tables, as the same L2 will be re-used to create further 2MB mappings. It's incorrect for the function to use destroy_xen_mappings() which will free empty L2 tables. Fix this by using map_pages_to_xen(), which does zap the page-table entries, but does not free page-table structures even when empty. Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> --- The fixes tag reflects the fact that if 4376c05c3113 freed the L2 correctly when empty, it would have become obvious that bootstrap_map_addr() shouldn't be using it if it wants to keep the L2. 4376c05c3113 should have switched bootstrap_map_addr() to not use destroy_xen_mappings(). --- xen/arch/x86/setup.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)