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

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

[Roger Pau Monne]

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(-)

-- 
2.46.0

[PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Roger Pau Monne]

Split the code that detects whether the physical and linear address of a
mapping request are suitable to be used in an L3 or L2 slot.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/page.h |  6 ++++++
 xen/arch/x86/mm.c               | 11 +++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h
index e01af28916b0..6970916d61d5 100644
--- a/xen/arch/x86/include/asm/page.h
+++ b/xen/arch/x86/include/asm/page.h
@@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
 #define l4_table_offset(a)         \
     (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
 
+/* Check if an address is aligned for a given slot level. */
+#define SLOT_IS_ALIGNED(v, m, s) \
+    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
+#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
+#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
+
 /* Convert a pointer to a page-table entry into pagetable slot index. */
 #define pgentry_ptr_to_slot(_p)    \
     (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d537a799bced..8f7c397a82d4 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5249,9 +5249,7 @@ int map_pages_to_xen(
         L3T_LOCK(current_l3page);
         ol3e = *pl3e;
 
-        if ( cpu_has_page1gb &&
-             !(((virt >> PAGE_SHIFT) | mfn_x(mfn)) &
-               ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)) &&
+        if ( cpu_has_page1gb && IS_L3E_ALIGNED(virt, mfn) &&
              nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
              !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
         {
@@ -5370,8 +5368,7 @@ int map_pages_to_xen(
         if ( !pl2e )
             goto out;
 
-        if ( ((((virt >> PAGE_SHIFT) | mfn_x(mfn)) &
-               ((1u << PAGETABLE_ORDER) - 1)) == 0) &&
+        if ( IS_L2E_ALIGNED(virt, mfn) &&
              (nr_mfns >= (1u << PAGETABLE_ORDER)) &&
              !(flags & (_PAGE_PAT|MAP_SMALL_PAGES)) )
         {
@@ -5541,9 +5538,7 @@ int map_pages_to_xen(
  check_l3:
         if ( cpu_has_page1gb &&
              (flags == PAGE_HYPERVISOR) &&
-             ((nr_mfns == 0) ||
-              !(((virt >> PAGE_SHIFT) | mfn_x(mfn)) &
-                ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1))) )
+             ((nr_mfns == 0) || IS_L3E_ALIGNED(virt, mfn)) )
         {
             unsigned long base_mfn;
             const l2_pgentry_t *l2t;
-- 
2.46.0

Re: [PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Jan Beulich]

On 06.11.2024 13:29, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/page.h
> +++ b/xen/arch/x86/include/asm/page.h
> @@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>  #define l4_table_offset(a)         \
>      (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
>  
> +/* Check if an address is aligned for a given slot level. */
> +#define SLOT_IS_ALIGNED(v, m, s) \
> +    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)

The check involving an address and an MFN, I think the comment would better
also reflect this. "Check if a (va,mfn) tuple is suitably aligned to be
mapped by a large page at a given page table level"?

As to the name of this helper macro - "SLOT" can mean about anything when
not further qualified. If the macro was local to ...

> +#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
> +#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
> +
>  /* Convert a pointer to a page-table entry into pagetable slot index. */
>  #define pgentry_ptr_to_slot(_p)    \
>      (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c

... this (sole) file using the derived ones, that might be acceptable. If
it's to remain in page.h, how about e.g. IS_LnE_ALIGNED()?

I further wonder whether it wouldn't be neater if just the level was passed
into the helper. Doing so wouldn't even require token concatenation (which
iirc both you and Andrew don't like in situations like this one), as the
mask can be calculated from just level and PAGETABLE_ORDER. At which point
it may even make sense to leave out the wrapper macros.

Jan

Re: [PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Roger Pau Monné]

On Thu, Nov 07, 2024 at 11:42:11AM +0100, Jan Beulich wrote:
> On 06.11.2024 13:29, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/page.h
> > +++ b/xen/arch/x86/include/asm/page.h
> > @@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
> >  #define l4_table_offset(a)         \
> >      (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
> >  
> > +/* Check if an address is aligned for a given slot level. */
> > +#define SLOT_IS_ALIGNED(v, m, s) \
> > +    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
> 
> The check involving an address and an MFN, I think the comment would better
> also reflect this. "Check if a (va,mfn) tuple is suitably aligned to be
> mapped by a large page at a given page table level"?
> 
> As to the name of this helper macro - "SLOT" can mean about anything when
> not further qualified. If the macro was local to ...
> 
> > +#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
> > +#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
> > +
> >  /* Convert a pointer to a page-table entry into pagetable slot index. */
> >  #define pgentry_ptr_to_slot(_p)    \
> >      (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> 
> ... this (sole) file using the derived ones, that might be acceptable. If
> it's to remain in page.h, how about e.g. IS_LnE_ALIGNED()?

Since you expressed further concerns in the next patch, I will move it
to being local to mm.c.  I don't have any other use-case, but assumed
the macros are generic enough to be useful in other contexts.

> I further wonder whether it wouldn't be neater if just the level was passed
> into the helper. Doing so wouldn't even require token concatenation (which
> iirc both you and Andrew don't like in situations like this one), as the
> mask can be calculated from just level and PAGETABLE_ORDER. At which point
> it may even make sense to leave out the wrapper macros.

I can see what I can do.

Thanks, Roger.

Re: [PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Roger Pau Monné]

On Thu, Nov 07, 2024 at 05:07:34PM +0100, Roger Pau Monné wrote:
> On Thu, Nov 07, 2024 at 11:42:11AM +0100, Jan Beulich wrote:
> > On 06.11.2024 13:29, Roger Pau Monne wrote:
> > > --- a/xen/arch/x86/include/asm/page.h
> > > +++ b/xen/arch/x86/include/asm/page.h
> > > @@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
> > >  #define l4_table_offset(a)         \
> > >      (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
> > >  
> > > +/* Check if an address is aligned for a given slot level. */
> > > +#define SLOT_IS_ALIGNED(v, m, s) \
> > > +    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
> > 
> > The check involving an address and an MFN, I think the comment would better
> > also reflect this. "Check if a (va,mfn) tuple is suitably aligned to be
> > mapped by a large page at a given page table level"?
> > 
> > As to the name of this helper macro - "SLOT" can mean about anything when
> > not further qualified. If the macro was local to ...
> > 
> > > +#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
> > > +#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
> > > +
> > >  /* Convert a pointer to a page-table entry into pagetable slot index. */
> > >  #define pgentry_ptr_to_slot(_p)    \
> > >      (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > 
> > ... this (sole) file using the derived ones, that might be acceptable. If
> > it's to remain in page.h, how about e.g. IS_LnE_ALIGNED()?
> 
> Since you expressed further concerns in the next patch, I will move it
> to being local to mm.c.  I don't have any other use-case, but assumed
> the macros are generic enough to be useful in other contexts.
> 
> > I further wonder whether it wouldn't be neater if just the level was passed
> > into the helper. Doing so wouldn't even require token concatenation (which
> > iirc both you and Andrew don't like in situations like this one), as the
> > mask can be calculated from just level and PAGETABLE_ORDER. At which point
> > it may even make sense to leave out the wrapper macros.
> 
> I can see what I can do.

Would something like:

#define IS_LnE_ALIGNED(v, m, n) \
    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << (PAGETABLE_ORDER * (n - 1))) - 1)

Defined only in the context of map_pages_to_xen() be OK with you?

I'm unsure whether it would be better if I still provided the
IS_L{2,3}E_ALIGNED() macros based on that, as IMO those macros made
the conditionals clearer to read.

Thanks, Roger.

Re: [PATCH v2 1/4] x86/mm: introduce helpers to detect super page alignment

[Jan Beulich]

On 07.11.2024 18:19, Roger Pau Monné wrote:
> On Thu, Nov 07, 2024 at 05:07:34PM +0100, Roger Pau Monné wrote:
>> On Thu, Nov 07, 2024 at 11:42:11AM +0100, Jan Beulich wrote:
>>> On 06.11.2024 13:29, Roger Pau Monne wrote:
>>>> --- a/xen/arch/x86/include/asm/page.h
>>>> +++ b/xen/arch/x86/include/asm/page.h
>>>> @@ -200,6 +200,12 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>>>>  #define l4_table_offset(a)         \
>>>>      (((a) >> L4_PAGETABLE_SHIFT) & (L4_PAGETABLE_ENTRIES - 1))
>>>>  
>>>> +/* Check if an address is aligned for a given slot level. */
>>>> +#define SLOT_IS_ALIGNED(v, m, s) \
>>>> +    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
>>>
>>> The check involving an address and an MFN, I think the comment would better
>>> also reflect this. "Check if a (va,mfn) tuple is suitably aligned to be
>>> mapped by a large page at a given page table level"?
>>>
>>> As to the name of this helper macro - "SLOT" can mean about anything when
>>> not further qualified. If the macro was local to ...
>>>
>>>> +#define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
>>>> +#define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
>>>> +
>>>>  /* Convert a pointer to a page-table entry into pagetable slot index. */
>>>>  #define pgentry_ptr_to_slot(_p)    \
>>>>      (((unsigned long)(_p) & ~PAGE_MASK) / sizeof(*(_p)))
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>
>>> ... this (sole) file using the derived ones, that might be acceptable. If
>>> it's to remain in page.h, how about e.g. IS_LnE_ALIGNED()?
>>
>> Since you expressed further concerns in the next patch, I will move it
>> to being local to mm.c.  I don't have any other use-case, but assumed
>> the macros are generic enough to be useful in other contexts.
>>
>>> I further wonder whether it wouldn't be neater if just the level was passed
>>> into the helper. Doing so wouldn't even require token concatenation (which
>>> iirc both you and Andrew don't like in situations like this one), as the
>>> mask can be calculated from just level and PAGETABLE_ORDER. At which point
>>> it may even make sense to leave out the wrapper macros.
>>
>> I can see what I can do.
> 
> Would something like:
> 
> #define IS_LnE_ALIGNED(v, m, n) \
>     IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << (PAGETABLE_ORDER * (n - 1))) - 1)
> 
> Defined only in the context of map_pages_to_xen() be OK with you?

Yes.

> I'm unsure whether it would be better if I still provided the
> IS_L{2,3}E_ALIGNED() macros based on that, as IMO those macros made
> the conditionals clearer to read.

Not sure without actually seeing it in place. Without those wrapper macros,
having n be the first macro parameter may help reduce the visual difference
between both variants. Yet if you clearly feel better with the wrappers,
I'm not going to insist on omitting them.

Jan

[PATCH v2 2/4] x86/mm: special case super page alignment detection for INVALID_MFN

[Roger Pau Monne]

INVALID_MFN is ~0, so by it having all bits as 1s it doesn't fulfill the
super-page address alignment checks for L3 and L2 entries.  Special case
INVALID_MFN so it's considered to be aligned for all slots.

This fixes a regression introduced by 0b6b51a69f4d, where the switch from 0 to
INVALID_MFN caused all super-pages to be shattered when attempting to remove
mappings by passing INVALID_MFN instead of 0.

Fixes: 0b6b51a69f4d ('xen/mm: Switch map_pages_to_xen to use MFN typesafe')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/page.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h
index 6970916d61d5..2fa4061dc77a 100644
--- a/xen/arch/x86/include/asm/page.h
+++ b/xen/arch/x86/include/asm/page.h
@@ -202,7 +202,8 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
 
 /* Check if an address is aligned for a given slot level. */
 #define SLOT_IS_ALIGNED(v, m, s) \
-    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
+    IS_ALIGNED(PFN_DOWN(v) | (mfn_eq(m, INVALID_MFN) ? 0 : mfn_x(m)), \
+               (1UL << ((s) - PAGE_SHIFT)) - 1)
 #define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
 #define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
 
-- 
2.46.0

Re: [PATCH v2 2/4] x86/mm: special case super page alignment detection for INVALID_MFN

[Jan Beulich]

On 06.11.2024 13:29, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/page.h
> +++ b/xen/arch/x86/include/asm/page.h
> @@ -202,7 +202,8 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>  
>  /* Check if an address is aligned for a given slot level. */
>  #define SLOT_IS_ALIGNED(v, m, s) \
> -    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
> +    IS_ALIGNED(PFN_DOWN(v) | (mfn_eq(m, INVALID_MFN) ? 0 : mfn_x(m)), \
> +               (1UL << ((s) - PAGE_SHIFT)) - 1)
>  #define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
>  #define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)

With this adjustment it feels yet more important for these macros to
become local ones in x86/mm.c. This special property may not be what one
wants in the general case. And m is now also evaluated twice (really:
once or twice), which a "random" user of the macro may not like.

I'm further uncertain now that this is the way to go to address the
original issue. Notably for the 1G-mapping case it may be better to go
from the incoming flags having _PAGE_PRESENT clear. After all we can
always create non-present "large" PTEs. E.g.

        if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
             IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
             nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
             !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )

Thoughts?

Jan

Re: [PATCH v2 2/4] x86/mm: special case super page alignment detection for INVALID_MFN

[Roger Pau Monné]

On Thu, Nov 07, 2024 at 12:06:41PM +0100, Jan Beulich wrote:
> On 06.11.2024 13:29, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/include/asm/page.h
> > +++ b/xen/arch/x86/include/asm/page.h
> > @@ -202,7 +202,8 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
> >  
> >  /* Check if an address is aligned for a given slot level. */
> >  #define SLOT_IS_ALIGNED(v, m, s) \
> > -    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
> > +    IS_ALIGNED(PFN_DOWN(v) | (mfn_eq(m, INVALID_MFN) ? 0 : mfn_x(m)), \
> > +               (1UL << ((s) - PAGE_SHIFT)) - 1)
> >  #define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
> >  #define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
> 
> With this adjustment it feels yet more important for these macros to
> become local ones in x86/mm.c. This special property may not be what one
> wants in the general case. And m is now also evaluated twice (really:
> once or twice), which a "random" user of the macro may not like.
> 
> I'm further uncertain now that this is the way to go to address the
> original issue. Notably for the 1G-mapping case it may be better to go
> from the incoming flags having _PAGE_PRESENT clear. After all we can
> always create non-present "large" PTEs. E.g.

Hm, I don't think we want to do that in map_pages_to_xen() as part of
this change.  Doing so would possibly imply the freeing of
intermediate page-tables when Xen previously didn't free them.  If the
CPU didn't support 1GB mappings we would always keep the L2, even if
fully empty.  With your proposed change we would now free such L2.

I'm not saying it's a wrong change, but just didn't want to put this
extra change of behavior together with a bugfix for an existing issue.

> 
>         if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
>              IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
>              nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
>              !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
> 
> Thoughts?

I was doing it based on mfn because that's how it worked previously
when 0 was passed instead of INVALID_MFN, and because I think it was
cleaner to hide the evaluation inside of IS_LnE_ALIGNED() instead of
open-coding it for every call to IS_LnE_ALIGNED().

If we want to do it based on flags it would be best if those are
passed to IS_LnE_ALIGNED(), but again, might be best to do it in a
followup patch and not part of this bugfix.  I fear it could have
unpredicted consequences.

Thanks, Roger.

Re: [PATCH v2 2/4] x86/mm: special case super page alignment detection for INVALID_MFN

[Jan Beulich]

On 07.11.2024 16:52, Roger Pau Monné wrote:
> On Thu, Nov 07, 2024 at 12:06:41PM +0100, Jan Beulich wrote:
>> On 06.11.2024 13:29, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/include/asm/page.h
>>> +++ b/xen/arch/x86/include/asm/page.h
>>> @@ -202,7 +202,8 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>>>  
>>>  /* Check if an address is aligned for a given slot level. */
>>>  #define SLOT_IS_ALIGNED(v, m, s) \
>>> -    IS_ALIGNED(PFN_DOWN(v) | mfn_x(m), (1UL << ((s) - PAGE_SHIFT)) - 1)
>>> +    IS_ALIGNED(PFN_DOWN(v) | (mfn_eq(m, INVALID_MFN) ? 0 : mfn_x(m)), \
>>> +               (1UL << ((s) - PAGE_SHIFT)) - 1)
>>>  #define IS_L3E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L3_PAGETABLE_SHIFT)
>>>  #define IS_L2E_ALIGNED(v, m) SLOT_IS_ALIGNED(v, m, L2_PAGETABLE_SHIFT)
>>
>> With this adjustment it feels yet more important for these macros to
>> become local ones in x86/mm.c. This special property may not be what one
>> wants in the general case. And m is now also evaluated twice (really:
>> once or twice), which a "random" user of the macro may not like.
>>
>> I'm further uncertain now that this is the way to go to address the
>> original issue. Notably for the 1G-mapping case it may be better to go
>> from the incoming flags having _PAGE_PRESENT clear. After all we can
>> always create non-present "large" PTEs. E.g.
> 
> Hm, I don't think we want to do that in map_pages_to_xen() as part of
> this change.  Doing so would possibly imply the freeing of
> intermediate page-tables when Xen previously didn't free them.  If the
> CPU didn't support 1GB mappings we would always keep the L2, even if
> fully empty.  With your proposed change we would now free such L2.
> 
> I'm not saying it's a wrong change, but just didn't want to put this
> extra change of behavior together with a bugfix for an existing issue.

I can understand your concern here; perhaps indeed best to keep that
adjustment separate.

>>         if ( (cpu_has_page1gb || !(flags & _PAGE_PRESENT)) &&
>>              IS_L3E_ALIGNED(virt, flags & _PAGE_PRESENT ? mfn : _mfn(0)) &&
>>              nr_mfns >= (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) &&
>>              !(flags & (_PAGE_PAT | MAP_SMALL_PAGES)) )
>>
>> Thoughts?
> 
> I was doing it based on mfn because that's how it worked previously
> when 0 was passed instead of INVALID_MFN, and because I think it was
> cleaner to hide the evaluation inside of IS_LnE_ALIGNED() instead of
> open-coding it for every call to IS_LnE_ALIGNED().
> 
> If we want to do it based on flags it would be best if those are
> passed to IS_LnE_ALIGNED(), but again, might be best to do it in a
> followup patch and not part of this bugfix.  I fear it could have
> unpredicted consequences.

Here, however, I view the flags-based approach as simply a different
(and imo more correct) way of addressing the issue at hand. The special
casing of MFN 0 had always been somewhat bogus imo, just that in the
old days we didn't even have a proper INVALID_MFN.

Jan

[PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Roger Pau Monne]

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;
         return NULL;
     }
-- 
2.46.0

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Jan Beulich]

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

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Roger Pau Monné]

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.

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Jan Beulich]

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

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Andrew Cooper]

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

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Jan Beulich]

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

Re: [PATCH v2 3/4] x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()

[Roger Pau Monné]

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.

[PATCH v2 4/4] x86/mm: ensure L2 is always freed if empty

[Roger Pau Monne]

The current logic in modify_xen_mappings() allows for fully empty L2 tables to
not be freed and unhooked from the parent L3 if the last L2 slot is not
populated.

Ensure that even when an L2 slot is empty the logic to check whether the whole
L2 can be removed is not skipped.

Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 8f7c397a82d4..05d3ba095627 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5712,7 +5712,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 
             v += 1UL << L2_PAGETABLE_SHIFT;
             v &= ~((1UL << L2_PAGETABLE_SHIFT) - 1);
-            continue;
+            goto check_l3;
         }
 
         if ( l2e_get_flags(*pl2e) & _PAGE_PSE )
-- 
2.46.0

Re: [PATCH v2 0/4] x86/mm: miscellaneous fixes

[Jan Beulich]

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

Re: [PATCH v2 0/4] x86/mm: miscellaneous fixes

[Roger Pau Monné]

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.

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

[Roger Pau Monne]

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: skip super-page alignment checks for non-present entries
  x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()
  x86/mm: ensure L2 is always freed if empty

 xen/arch/x86/mm.c    | 30 ++++++++++++++++++++++--------
 xen/arch/x86/setup.c |  4 +++-
 2 files changed, 25 insertions(+), 9 deletions(-)

-- 
2.46.0