[v2,0/4] x86/mm: miscellaneous fixes

Message

Roger Pau Monné Nov. 6, 2024, 12:29 p.m. UTC

Hello,

The attempt to fix destroy_xen_mappings() so that L2 tables are
consistently freed uncovered some errors in the memory management code.
The following series attempts to fix them.

All patches except for 4/4 are new in v2, and 4/4 has no change from v1,
hence kept Jan's Reviewed-by tag in 4/4.

Thanks, Roger.

Roger Pau Monne (4):
  x86/mm: introduce helpers to detect super page alignment
  x86/mm: special case super page alignment detection for INVALID_MFN
  x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()
  x86/mm: ensure L2 is always freed if empty

 xen/arch/x86/include/asm/page.h |  7 +++++++
 xen/arch/x86/mm.c               | 13 ++++---------
 xen/arch/x86/setup.c            |  4 +++-
 3 files changed, 14 insertions(+), 10 deletions(-)

Comments

Jan Beulich Nov. 7, 2024, 11:28 a.m. UTC | #1

On 06.11.2024 13:29, Roger Pau Monne wrote:
> The attempt to fix destroy_xen_mappings() so that L2 tables are
> consistently freed uncovered some errors in the memory management code.
> The following series attempts to fix them.
> 
> All patches except for 4/4 are new in v2, and 4/4 has no change from v1,
> hence kept Jan's Reviewed-by tag in 4/4.

Just to mention it: When a patch was buggy, and perhaps even more so when
it actually needed reverting, I think all R-b (and likely even all A-b)
should be dropped. Clearly the reviewer, too, missed an important point.

That said, the tag is fine to keep in this specific case; I would merely
have re-instated it after looking through the prereq changes.

> Roger Pau Monne (4):
>   x86/mm: introduce helpers to detect super page alignment
>   x86/mm: special case super page alignment detection for INVALID_MFN
>   x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

Btw - it looks like patch 3 (with the possibly adjusted description) is
independent of patch 1 and 2. If you can confirm that and if we can agree
on replacement wording for its description, it could go in before you
send out a v3.

Jan

Roger Pau Monné Nov. 7, 2024, 11:48 a.m. UTC | #2

On Thu, Nov 07, 2024 at 12:28:11PM +0100, Jan Beulich wrote:
> On 06.11.2024 13:29, Roger Pau Monne wrote:
> > The attempt to fix destroy_xen_mappings() so that L2 tables are
> > consistently freed uncovered some errors in the memory management code.
> > The following series attempts to fix them.
> > 
> > All patches except for 4/4 are new in v2, and 4/4 has no change from v1,
> > hence kept Jan's Reviewed-by tag in 4/4.
> 
> Just to mention it: When a patch was buggy, and perhaps even more so when
> it actually needed reverting, I think all R-b (and likely even all A-b)
> should be dropped. Clearly the reviewer, too, missed an important point.

My consideration for keeping you RB was that the patch wasn't buggy
itself, but the bug fixed by the patch uncovered bugs in other areas
of the code.

Hence it's my understanding the patch was and still is correct, but
given the timeframe in which the breackage was discovered (the evening
before a public holiday leading to a weekend) I feel it was safer to
revert than to either rush a fix or lave the tree broken until next
Monday.

> That said, the tag is fine to keep in this specific case; I would merely
> have re-instated it after looking through the prereq changes.
> 
> > Roger Pau Monne (4):
> >   x86/mm: introduce helpers to detect super page alignment
> >   x86/mm: special case super page alignment detection for INVALID_MFN
> >   x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()
> 
> Btw - it looks like patch 3 (with the possibly adjusted description) is
> independent of patch 1 and 2. If you can confirm that and if we can agree
> on replacement wording for its description, it could go in before you
> send out a v3.

No, I'm afraid it can't go in ahead of 1 and 2, as with the current
logic in map_pages_to_xen() using it in bootstrap_map_addr() will lead
to allocation requests, and thus hitting a BUG_ON().  Calling
map_pages_to_xen() with INVALID_MFN currently causes super-page
shattering if present.

Thanks, Roger.