Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
Hi Stefano,
On 05/04/2022 21:46, Stefano Stabellini wrote:
On Sat, 2 Apr 2022, Julien Grall wrote:
On 02/04/2022 00:35, Stefano Stabellini wrote:
+/* Return the level where mapping should be done */
+static int xen_pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned
long nr,
+unsigned int flags)
+{
+unsigned int level;
+unsigned long mask;
Shouldn't mask be 64-bit on aarch32?
The 3 variables we will use (mfn, vfn, nr) are unsigned long. So it is fine to
define the mask as unsigned long.
Good point
+}
+
static DEFINE_SPINLOCK(xen_pt_lock);
static int xen_pt_update(unsigned long virt,
mfn_t mfn,
- unsigned long nr_mfns,
+ const unsigned long nr_mfns,
Why const? nr_mfns is an unsigned long so it is passed as value: it
couldn't change the caller's parameter anyway. Just curious.
Because nr_mfns is used to flush the TLBs. In the original I made the mistake
to decrement the variable and only discovered later on when the TLB contained
the wrong entry.
Such bug tends to be very subtle and it is hard to find the root cause. So
better mark the variable const to avoid any surprise.
The short version of what I wrote is in the commit message. I can write a
small comment in the code if you want.
No, that's fine. Thanks for the explanation.
I thought about it and decided to add a comment. This will avoid someone
to remove the 'const'.
Cheers,
--
Julien Grall
Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
On Sat, 2 Apr 2022, Julien Grall wrote:
> On 02/04/2022 00:35, Stefano Stabellini wrote:
> > > +/* Return the level where mapping should be done */
> > > +static int xen_pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned
> > > long nr,
> > > +unsigned int flags)
> > > +{
> > > +unsigned int level;
> > > +unsigned long mask;
> >
> > Shouldn't mask be 64-bit on aarch32?
>
> The 3 variables we will use (mfn, vfn, nr) are unsigned long. So it is fine to
> define the mask as unsigned long.
Good point
> > > +}
> > > +
> > > static DEFINE_SPINLOCK(xen_pt_lock);
> > > static int xen_pt_update(unsigned long virt,
> > >mfn_t mfn,
> > > - unsigned long nr_mfns,
> > > + const unsigned long nr_mfns,
> >
> > Why const? nr_mfns is an unsigned long so it is passed as value: it
> > couldn't change the caller's parameter anyway. Just curious.
>
> Because nr_mfns is used to flush the TLBs. In the original I made the mistake
> to decrement the variable and only discovered later on when the TLB contained
> the wrong entry.
>
> Such bug tends to be very subtle and it is hard to find the root cause. So
> better mark the variable const to avoid any surprise.
>
> The short version of what I wrote is in the commit message. I can write a
> small comment in the code if you want.
No, that's fine. Thanks for the explanation.
> > >unsigned int flags)
> > > {
> > > int rc = 0;
> > > -unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> > > +unsigned long vfn = virt >> PAGE_SHIFT;
> > > +unsigned long left = nr_mfns;
> > > /*
> > >* For arm32, page-tables are different on each CPUs. Yet, they
> > > share
> > > @@ -1268,14 +1330,24 @@ static int xen_pt_update(unsigned long virt,
> > > spin_lock(_pt_lock);
> > > -for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > +while ( left )
> > > {
> > > -rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > +unsigned int order, level;
> > > +
> > > +level = xen_pt_mapping_level(vfn, mfn, left, flags);
> > > +order = XEN_PT_LEVEL_ORDER(level);
> > > +
> > > +ASSERT(left >= BIT(order, UL));
> > > +
> > > +rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level,
> > > flags);
> >
> > NIT: I know we don't have vfn_to_vaddr at the moment and there is no
> > widespread usage of vfn in Xen anyway, but it looks off to use
> > pfn_to_paddr on a vfn parameter. Maybe open-code pfn_to_paddr instead?
> > Or introduce vfn_to_vaddr locally in this file?
>
> To avoid inconsistency with mfn_to_maddr() and gfn_to_gaddr(), I don't want ot
> introduce vfn_to_vaddr() withtout the typesafe part. I think this is a bit
> over the top for now.
>
> So I will open-code pfn_to_paddr().
Sounds good
Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
Hi,
On 02/04/2022 00:35, Stefano Stabellini wrote:
+/* Return the level where mapping should be done */
+static int xen_pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr,
+unsigned int flags)
+{
+unsigned int level;
+unsigned long mask;
Shouldn't mask be 64-bit on aarch32?
The 3 variables we will use (mfn, vfn, nr) are unsigned long. So it is
fine to define the mask as unsigned long.
+}
+
static DEFINE_SPINLOCK(xen_pt_lock);
static int xen_pt_update(unsigned long virt,
mfn_t mfn,
- unsigned long nr_mfns,
+ const unsigned long nr_mfns,
Why const? nr_mfns is an unsigned long so it is passed as value: it
couldn't change the caller's parameter anyway. Just curious.
Because nr_mfns is used to flush the TLBs. In the original I made the
mistake to decrement the variable and only discovered later on when the
TLB contained the wrong entry.
Such bug tends to be very subtle and it is hard to find the root cause.
So better mark the variable const to avoid any surprise.
The short version of what I wrote is in the commit message. I can write
a small comment in the code if you want.
unsigned int flags)
{
int rc = 0;
-unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
+unsigned long vfn = virt >> PAGE_SHIFT;
+unsigned long left = nr_mfns;
/*
* For arm32, page-tables are different on each CPUs. Yet, they share
@@ -1268,14 +1330,24 @@ static int xen_pt_update(unsigned long virt,
spin_lock(_pt_lock);
-for ( ; addr < addr_end; addr += PAGE_SIZE )
+while ( left )
{
-rc = xen_pt_update_entry(root, addr, mfn, flags);
+unsigned int order, level;
+
+level = xen_pt_mapping_level(vfn, mfn, left, flags);
+order = XEN_PT_LEVEL_ORDER(level);
+
+ASSERT(left >= BIT(order, UL));
+
+rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level, flags);
NIT: I know we don't have vfn_to_vaddr at the moment and there is no
widespread usage of vfn in Xen anyway, but it looks off to use
pfn_to_paddr on a vfn parameter. Maybe open-code pfn_to_paddr instead?
Or introduce vfn_to_vaddr locally in this file?
To avoid inconsistency with mfn_to_maddr() and gfn_to_gaddr(), I don't
want ot introduce vfn_to_vaddr() withtout the typesafe part. I think
this is a bit over the top for now.
So I will open-code pfn_to_paddr().
Cheers,
--
Julien Grall
Re: [PATCH v3 04/19] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> At the moment, xen_pt_update_entry() only supports mapping at level 3
> (i.e 4KB mapping). While this is fine for most of the runtime helper,
> the boot code will require to use superpage mapping.
>
> We don't want to allow superpage mapping by default as some of the
> callers may expect small mappings (i.e populate_pt_range()) or even
> expect to unmap only a part of a superpage.
>
> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
> allow the caller to enable superpage mapping.
>
> As the code doesn't support all the combinations, xen_pt_check_entry()
> is extended to take into account the cases we don't support when
> using block mapping:
> - Replacing a table with a mapping. This may happen if region was
> first mapped with 4KB mapping and then later on replaced with a 2MB
> (or 1GB mapping).
> - Removing/modifying a table. This may happen if a caller try to
> remove a region with _PAGE_BLOCK set when it was created without it.
>
> Note that the current restriction means that the caller must ensure that
> _PAGE_BLOCK is consistently set/cleared across all the updates on a
> given virtual region. This ought to be fine with the expected use-cases.
>
> More rework will be necessary if we wanted to remove the restrictions.
>
> Note that nr_mfns is now marked const as it is used for flushing the
> TLBs and we don't want it to be modified.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Julien Grall
>
> ---
> Changes in v3:
> - Fix clash after prefixing the PT macros with XEN_PT_
> - Fix typoes in the commit message
> - Support superpage mappings even if nr is not suitably aligned
> - Move the logic to find the level in a separate function
>
> Changes in v2:
> - Pass the target level rather than the order to
> xen_pt_update_entry()
> - Update some comments
> - Open-code paddr_to_pfn()
> - Add my AWS signed-off-by
> ---
> xen/arch/arm/include/asm/page.h | 4 ++
> xen/arch/arm/mm.c | 108 ++--
> 2 files changed, 94 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.h
> index c6f9fb0d4e0c..07998df47bac 100644
> --- a/xen/arch/arm/include/asm/page.h
> +++ b/xen/arch/arm/include/asm/page.h
> @@ -69,6 +69,7 @@
> * [3:4] Permission flags
> * [5] Page present
> * [6] Only populate page tables
> + * [7] Superpage mappings is allowed
> */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
>
> @@ -82,6 +83,9 @@
> #define _PAGE_PRESENT(1U << 5)
> #define _PAGE_POPULATE (1U << 6)
>
> +#define _PAGE_BLOCK_BIT 7
> +#define _PAGE_BLOCK (1U << _PAGE_BLOCK_BIT)
> +
> /*
> * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
> * meant to be used outside of this header.
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 515d0906f85b..3af69b396bd1 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1063,9 +1063,10 @@ static int xen_pt_next_level(bool read_only, unsigned
> int level,
> }
>
> /* Sanity check of the entry */
> -static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
> + unsigned int flags)
> {
> -/* Sanity check when modifying a page. */
> +/* Sanity check when modifying an entry. */
> if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
> {
> /* We don't allow modifying an invalid entry. */
> @@ -1075,6 +1076,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int flags)
> return false;
> }
>
> +/* We don't allow modifying a table entry */
> +if ( !lpae_is_mapping(entry, level) )
> +{
> +mm_printk("Modifying a table entry is not allowed.\n");
> +return false;
> +}
> +
> /* We don't allow changing memory attributes. */
> if ( entry.pt.ai != PAGE_AI_MASK(flags) )
> {
> @@ -1090,7 +1098,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn,
> unsigned int flags)
> return false;
> }
> }
> -/* Sanity check when inserting a page */
> +/* Sanity check when inserting a mapping */
> else if ( flags & _PAGE_PRESENT )
> {
> /* We should be here with a valid MFN. */
> @@ -1099,18 +1107,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t
> mfn, unsigned int flags)
> /* We don't allow replacing any valid entry. */
> if ( lpae_is_valid(entry) )
> {
> -mm_printk("Changing MFN for a valid entry is not allowed
> (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> - mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +if ( lpae_is_mapping(entry, level) )
> +
