* [Xen-devel] [PATCH MM-PART3 v2 00/12] xen/arm: Provide a generic function to update Xen PT
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii_Anisov
Hi all,
This is the third part of the boot/memory rework for Xen on Arm. At the
moment, the update to Xen PT is scattered all around mm.c. This makes
difficult to rework Xen memory layout or even ensuring we are following the
Arm Arm properly (and we are not so far!).
This part contains code to provide a generic function to update Xen PT.
While I could have started from scratch, I decided to base the new function
on create_xen_entries() (now renamed xen_pt_update()). This makes slightly
easier to follow the changes.
In this series, the new generic function will only support 3rd-level update
and cannot be used in early boot (i.e because xenheap is not initialized).
This will be extended in follow-up series to allow more use within mm.c.
There are probably some optimization possible around the TLBs flush. I haven't
looked at it so far.
The last two patches of this series is to show how existing callers can be
converted. There are more conversion to come in follow-up series.
This series is based on the first two parts sent separately (see [1] and [2]).
For convenience, I provided a branch with all the patches applied based on
staging:
git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part3/v2
Cheers,
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01109.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01149.html
Julien Grall (12):
xen/arm: lpae: Add a macro to generate offsets from an address
xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
xen/arm: mm: Only increment mfn when valid in xen_pt_update
xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
xen/arm: mm: Sanity check any update of Xen page tables
xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
xen/arm: mm: Remove enum xenmap_operation
xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
xen/arm: mm: Rework Xen page-tables walk during update
xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
xen/arm: mm: Remove set_pte_flags_on_range()
xen/arch/arm/mm.c | 421 ++++++++++++++++++++++++++++++---------------
xen/arch/arm/p2m.c | 23 +--
xen/include/asm-arm/lpae.h | 9 +
xen/include/asm-arm/page.h | 9 +-
4 files changed, 305 insertions(+), 157 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* [PATCH MM-PART3 v2 01/12] xen/arm: lpae: Add a macro to generate offsets from an address
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
There are few places requiring to generate offsets from an address.
Rather than open-coding everywhere, we can introduce a macro to do the
job for us.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/p2m.c | 23 +++--------------------
xen/include/asm-arm/lpae.h | 9 +++++++++
2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 92c2413f20..06fa342a8f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -368,12 +368,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
p2m_type_t _t;
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, addr);
ASSERT(p2m_is_locked(p2m));
BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
@@ -888,7 +883,6 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
p2m_type_t t,
p2m_access_t a)
{
- paddr_t addr = gfn_to_gaddr(sgfn);
unsigned int level = 0;
unsigned int target = 3 - (page_order / LPAE_SHIFT);
lpae_t *entry, *table, orig_pte;
@@ -897,12 +891,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, gfn_to_gaddr(sgfn));
ASSERT(p2m_is_write_locked(p2m));
@@ -1199,15 +1188,9 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
unsigned int level = 0;
bool resolved = false;
lpae_t entry, *table;
- paddr_t addr = gfn_to_gaddr(gfn);
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, gfn_to_gaddr(gfn));
p2m_write_lock(p2m);
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 545b0c8f24..c22780f8f3 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -218,6 +218,15 @@ TABLE_OFFSET_HELPERS(64);
#undef TABLE_OFFSET
#undef TABLE_OFFSET_HELPERS
+/* Generate an array @var containing the offset for each level from @addr */
+#define DECLARE_OFFSETS(var, addr) \
+ const unsigned int var[4] = { \
+ zeroeth_table_offset(addr), \
+ first_table_offset(addr), \
+ second_table_offset(addr), \
+ third_table_offset(addr) \
+ }
+
#endif /* __ASSEMBLY__ */
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 01/12] xen/arm: lpae: Add a macro to generate offsets from an address
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
There are few places requiring to generate offsets from an address.
Rather than open-coding everywhere, we can introduce a macro to do the
job for us.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/p2m.c | 23 +++--------------------
xen/include/asm-arm/lpae.h | 9 +++++++++
2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 92c2413f20..06fa342a8f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -368,12 +368,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
p2m_type_t _t;
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, addr);
ASSERT(p2m_is_locked(p2m));
BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
@@ -888,7 +883,6 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
p2m_type_t t,
p2m_access_t a)
{
- paddr_t addr = gfn_to_gaddr(sgfn);
unsigned int level = 0;
unsigned int target = 3 - (page_order / LPAE_SHIFT);
lpae_t *entry, *table, orig_pte;
@@ -897,12 +891,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, gfn_to_gaddr(sgfn));
ASSERT(p2m_is_write_locked(p2m));
@@ -1199,15 +1188,9 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
unsigned int level = 0;
bool resolved = false;
lpae_t entry, *table;
- paddr_t addr = gfn_to_gaddr(gfn);
/* Convenience aliases */
- const unsigned int offsets[4] = {
- zeroeth_table_offset(addr),
- first_table_offset(addr),
- second_table_offset(addr),
- third_table_offset(addr)
- };
+ DECLARE_OFFSETS(offsets, gfn_to_gaddr(gfn));
p2m_write_lock(p2m);
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 545b0c8f24..c22780f8f3 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -218,6 +218,15 @@ TABLE_OFFSET_HELPERS(64);
#undef TABLE_OFFSET
#undef TABLE_OFFSET_HELPERS
+/* Generate an array @var containing the offset for each level from @addr */
+#define DECLARE_OFFSETS(var, addr) \
+ const unsigned int var[4] = { \
+ zeroeth_table_offset(addr), \
+ first_table_offset(addr), \
+ second_table_offset(addr), \
+ third_table_offset(addr) \
+ }
+
#endif /* __ASSEMBLY__ */
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 01/12] xen/arm: lpae: Add a macro to generate offsets from an address
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 18:21 ` Stefano Stabellini
2019-06-11 18:27 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 18:21 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> There are few places requiring to generate offsets from an address.
> Rather than open-coding everywhere, we can introduce a macro to do the
> job for us.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/p2m.c | 23 +++--------------------
> xen/include/asm-arm/lpae.h | 9 +++++++++
> 2 files changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 92c2413f20..06fa342a8f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -368,12 +368,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> p2m_type_t _t;
>
> /* Convenience aliases */
I don't know if you want to keep this comment, we could get rid of it.
Either way:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> - const unsigned int offsets[4] = {
> - zeroeth_table_offset(addr),
> - first_table_offset(addr),
> - second_table_offset(addr),
> - third_table_offset(addr)
> - };
> + DECLARE_OFFSETS(offsets, addr);
>
> ASSERT(p2m_is_locked(p2m));
> BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> @@ -888,7 +883,6 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
> p2m_type_t t,
> p2m_access_t a)
> {
> - paddr_t addr = gfn_to_gaddr(sgfn);
> unsigned int level = 0;
> unsigned int target = 3 - (page_order / LPAE_SHIFT);
> lpae_t *entry, *table, orig_pte;
> @@ -897,12 +891,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
> bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
>
> /* Convenience aliases */
> - const unsigned int offsets[4] = {
> - zeroeth_table_offset(addr),
> - first_table_offset(addr),
> - second_table_offset(addr),
> - third_table_offset(addr)
> - };
> + DECLARE_OFFSETS(offsets, gfn_to_gaddr(sgfn));
>
> ASSERT(p2m_is_write_locked(p2m));
>
> @@ -1199,15 +1188,9 @@ bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
> unsigned int level = 0;
> bool resolved = false;
> lpae_t entry, *table;
> - paddr_t addr = gfn_to_gaddr(gfn);
>
> /* Convenience aliases */
> - const unsigned int offsets[4] = {
> - zeroeth_table_offset(addr),
> - first_table_offset(addr),
> - second_table_offset(addr),
> - third_table_offset(addr)
> - };
> + DECLARE_OFFSETS(offsets, gfn_to_gaddr(gfn));
>
> p2m_write_lock(p2m);
>
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 545b0c8f24..c22780f8f3 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -218,6 +218,15 @@ TABLE_OFFSET_HELPERS(64);
> #undef TABLE_OFFSET
> #undef TABLE_OFFSET_HELPERS
>
> +/* Generate an array @var containing the offset for each level from @addr */
> +#define DECLARE_OFFSETS(var, addr) \
> + const unsigned int var[4] = { \
> + zeroeth_table_offset(addr), \
> + first_table_offset(addr), \
> + second_table_offset(addr), \
> + third_table_offset(addr) \
> + }
> +
> #endif /* __ASSEMBLY__ */
>
> /*
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 01/12] xen/arm: lpae: Add a macro to generate offsets from an address
2019-06-11 18:21 ` Stefano Stabellini
@ 2019-06-11 18:27 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-11 18:27 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 11/06/2019 19:21, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> There are few places requiring to generate offsets from an address.
>> Rather than open-coding everywhere, we can introduce a macro to do the
>> job for us.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>> Changes in v2:
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/p2m.c | 23 +++--------------------
>> xen/include/asm-arm/lpae.h | 9 +++++++++
>> 2 files changed, 12 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 92c2413f20..06fa342a8f 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -368,12 +368,7 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
>> p2m_type_t _t;
>>
>> /* Convenience aliases */
>
> I don't know if you want to keep this comment, we could get rid of it.
It is probably useless now. I will drop it along with the newline on top.
> Either way:
>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Thank you!
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 02/12] xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
create_xen_entries() is doing more than creating entries. It can also
modify and remove entries.
Rename the function to make clear what the function is actually doing.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b408de7c75..36e22fc9ad 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -970,11 +970,11 @@ enum xenmap_operation {
static DEFINE_SPINLOCK(xen_pt_lock);
-static int create_xen_entries(enum xenmap_operation op,
- unsigned long virt,
- mfn_t mfn,
- unsigned long nr_mfns,
- unsigned int flags)
+static int xen_pt_update(enum xenmap_operation op,
+ unsigned long virt,
+ mfn_t mfn,
+ unsigned long nr_mfns,
+ unsigned int flags)
{
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
@@ -1067,25 +1067,24 @@ int map_pages_to_xen(unsigned long virt,
unsigned long nr_mfns,
unsigned int flags)
{
- return create_xen_entries(INSERT, virt, mfn, nr_mfns, flags);
+ return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
}
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return create_xen_entries(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
+ return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
{
ASSERT(v <= e);
- return create_xen_entries(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+ return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
}
int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
{
ASSERT(s <= e);
- return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
- flags);
+ return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 02/12] xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
create_xen_entries() is doing more than creating entries. It can also
modify and remove entries.
Rename the function to make clear what the function is actually doing.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b408de7c75..36e22fc9ad 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -970,11 +970,11 @@ enum xenmap_operation {
static DEFINE_SPINLOCK(xen_pt_lock);
-static int create_xen_entries(enum xenmap_operation op,
- unsigned long virt,
- mfn_t mfn,
- unsigned long nr_mfns,
- unsigned int flags)
+static int xen_pt_update(enum xenmap_operation op,
+ unsigned long virt,
+ mfn_t mfn,
+ unsigned long nr_mfns,
+ unsigned int flags)
{
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
@@ -1067,25 +1067,24 @@ int map_pages_to_xen(unsigned long virt,
unsigned long nr_mfns,
unsigned int flags)
{
- return create_xen_entries(INSERT, virt, mfn, nr_mfns, flags);
+ return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
}
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return create_xen_entries(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
+ return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
{
ASSERT(v <= e);
- return create_xen_entries(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+ return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
}
int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
{
ASSERT(s <= e);
- return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
- flags);
+ return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 02/12] xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 18:23 ` Stefano Stabellini
-1 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 18:23 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> create_xen_entries() is doing more than creating entries. It can also
> modify and remove entries.
>
> Rename the function to make clear what the function is actually doing.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b408de7c75..36e22fc9ad 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -970,11 +970,11 @@ enum xenmap_operation {
>
> static DEFINE_SPINLOCK(xen_pt_lock);
>
> -static int create_xen_entries(enum xenmap_operation op,
> - unsigned long virt,
> - mfn_t mfn,
> - unsigned long nr_mfns,
> - unsigned int flags)
> +static int xen_pt_update(enum xenmap_operation op,
> + unsigned long virt,
> + mfn_t mfn,
> + unsigned long nr_mfns,
> + unsigned int flags)
> {
> int rc = 0;
> unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> @@ -1067,25 +1067,24 @@ int map_pages_to_xen(unsigned long virt,
> unsigned long nr_mfns,
> unsigned int flags)
> {
> - return create_xen_entries(INSERT, virt, mfn, nr_mfns, flags);
> + return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
> }
>
> int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
> {
> - return create_xen_entries(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
> + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
> }
>
> int destroy_xen_mappings(unsigned long v, unsigned long e)
> {
> ASSERT(v <= e);
> - return create_xen_entries(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> + return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> }
>
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> ASSERT(s <= e);
> - return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
> - flags);
> + return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> }
>
> enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 03/12] xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
In preparation of rework of the Xen PT, the logic to update an entry
in moved out in a separate function.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 140 +++++++++++++++++++++++++++++-------------------------
1 file changed, 74 insertions(+), 66 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 36e22fc9ad..f956aa6399 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -968,6 +968,76 @@ enum xenmap_operation {
RESERVE
};
+static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
+ mfn_t mfn, unsigned int flags)
+{
+ lpae_t pte, *entry;
+ lpae_t *third = NULL;
+
+ entry = &xen_second[second_linear_offset(addr)];
+ if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
+ {
+ int rc = create_xen_table(entry);
+ if ( rc < 0 ) {
+ printk("%s: L2 failed\n", __func__);
+ return rc;
+ }
+ }
+
+ BUG_ON(!lpae_is_valid(*entry));
+
+ third = mfn_to_virt(lpae_get_mfn(*entry));
+ entry = &third[third_table_offset(addr)];
+
+ switch ( op ) {
+ case INSERT:
+ case RESERVE:
+ if ( lpae_is_valid(*entry) )
+ {
+ printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
+ __func__, addr, mfn_x(mfn));
+ return -EINVAL;
+ }
+ if ( op == RESERVE )
+ break;
+ pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
+ BUG_ON(!pte.pt.ro && !pte.pt.xn);
+ pte.pt.table = 1;
+ write_pte(entry, pte);
+ break;
+ case MODIFY:
+ case REMOVE:
+ if ( !lpae_is_valid(*entry) )
+ {
+ printk("%s: trying to %s a non-existing mapping addr=%lx\n",
+ __func__, op == REMOVE ? "remove" : "modify", addr);
+ return -EINVAL;
+ }
+ if ( op == REMOVE )
+ pte.bits = 0;
+ else
+ {
+ pte = *entry;
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
+ if ( !pte.pt.ro && !pte.pt.xn )
+ {
+ printk("%s: Incorrect combination for addr=%lx\n",
+ __func__, addr);
+ return -EINVAL;
+ }
+ }
+ write_pte(entry, pte);
+ break;
+ default:
+ BUG();
+ }
+
+ return 0;
+}
+
static DEFINE_SPINLOCK(xen_pt_lock);
static int xen_pt_update(enum xenmap_operation op,
@@ -978,78 +1048,16 @@ static int xen_pt_update(enum xenmap_operation op,
{
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
- lpae_t pte, *entry;
- lpae_t *third = NULL;
spin_lock(&xen_pt_lock);
for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
{
- entry = &xen_second[second_linear_offset(addr)];
- if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
- {
- rc = create_xen_table(entry);
- if ( rc < 0 ) {
- printk("%s: L2 failed\n", __func__);
- goto out;
- }
- }
-
- BUG_ON(!lpae_is_valid(*entry));
-
- third = mfn_to_virt(lpae_get_mfn(*entry));
- entry = &third[third_table_offset(addr)];
-
- switch ( op ) {
- case INSERT:
- case RESERVE:
- if ( lpae_is_valid(*entry) )
- {
- printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
- __func__, addr, mfn_x(mfn));
- rc = -EINVAL;
- goto out;
- }
- if ( op == RESERVE )
- break;
- pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- BUG_ON(!pte.pt.ro && !pte.pt.xn);
- pte.pt.table = 1;
- write_pte(entry, pte);
- break;
- case MODIFY:
- case REMOVE:
- if ( !lpae_is_valid(*entry) )
- {
- printk("%s: trying to %s a non-existing mapping addr=%lx\n",
- __func__, op == REMOVE ? "remove" : "modify", addr);
- rc = -EINVAL;
- goto out;
- }
- if ( op == REMOVE )
- pte.bits = 0;
- else
- {
- pte = *entry;
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- if ( !pte.pt.ro && !pte.pt.xn )
- {
- printk("%s: Incorrect combination for addr=%lx\n",
- __func__, addr);
- rc = -EINVAL;
- goto out;
- }
- }
- write_pte(entry, pte);
- break;
- default:
- BUG();
- }
+ rc = xen_pt_update_entry(op, addr, mfn, flags);
+ if ( rc )
+ break;
}
-out:
+
/*
* Flush the TLBs even in case of failure because we may have
* partially modified the PT. This will prevent any unexpected
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 03/12] xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
In preparation of rework of the Xen PT, the logic to update an entry
in moved out in a separate function.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 140 +++++++++++++++++++++++++++++-------------------------
1 file changed, 74 insertions(+), 66 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 36e22fc9ad..f956aa6399 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -968,6 +968,76 @@ enum xenmap_operation {
RESERVE
};
+static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
+ mfn_t mfn, unsigned int flags)
+{
+ lpae_t pte, *entry;
+ lpae_t *third = NULL;
+
+ entry = &xen_second[second_linear_offset(addr)];
+ if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
+ {
+ int rc = create_xen_table(entry);
+ if ( rc < 0 ) {
+ printk("%s: L2 failed\n", __func__);
+ return rc;
+ }
+ }
+
+ BUG_ON(!lpae_is_valid(*entry));
+
+ third = mfn_to_virt(lpae_get_mfn(*entry));
+ entry = &third[third_table_offset(addr)];
+
+ switch ( op ) {
+ case INSERT:
+ case RESERVE:
+ if ( lpae_is_valid(*entry) )
+ {
+ printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
+ __func__, addr, mfn_x(mfn));
+ return -EINVAL;
+ }
+ if ( op == RESERVE )
+ break;
+ pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
+ BUG_ON(!pte.pt.ro && !pte.pt.xn);
+ pte.pt.table = 1;
+ write_pte(entry, pte);
+ break;
+ case MODIFY:
+ case REMOVE:
+ if ( !lpae_is_valid(*entry) )
+ {
+ printk("%s: trying to %s a non-existing mapping addr=%lx\n",
+ __func__, op == REMOVE ? "remove" : "modify", addr);
+ return -EINVAL;
+ }
+ if ( op == REMOVE )
+ pte.bits = 0;
+ else
+ {
+ pte = *entry;
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
+ if ( !pte.pt.ro && !pte.pt.xn )
+ {
+ printk("%s: Incorrect combination for addr=%lx\n",
+ __func__, addr);
+ return -EINVAL;
+ }
+ }
+ write_pte(entry, pte);
+ break;
+ default:
+ BUG();
+ }
+
+ return 0;
+}
+
static DEFINE_SPINLOCK(xen_pt_lock);
static int xen_pt_update(enum xenmap_operation op,
@@ -978,78 +1048,16 @@ static int xen_pt_update(enum xenmap_operation op,
{
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
- lpae_t pte, *entry;
- lpae_t *third = NULL;
spin_lock(&xen_pt_lock);
for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
{
- entry = &xen_second[second_linear_offset(addr)];
- if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
- {
- rc = create_xen_table(entry);
- if ( rc < 0 ) {
- printk("%s: L2 failed\n", __func__);
- goto out;
- }
- }
-
- BUG_ON(!lpae_is_valid(*entry));
-
- third = mfn_to_virt(lpae_get_mfn(*entry));
- entry = &third[third_table_offset(addr)];
-
- switch ( op ) {
- case INSERT:
- case RESERVE:
- if ( lpae_is_valid(*entry) )
- {
- printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
- __func__, addr, mfn_x(mfn));
- rc = -EINVAL;
- goto out;
- }
- if ( op == RESERVE )
- break;
- pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- BUG_ON(!pte.pt.ro && !pte.pt.xn);
- pte.pt.table = 1;
- write_pte(entry, pte);
- break;
- case MODIFY:
- case REMOVE:
- if ( !lpae_is_valid(*entry) )
- {
- printk("%s: trying to %s a non-existing mapping addr=%lx\n",
- __func__, op == REMOVE ? "remove" : "modify", addr);
- rc = -EINVAL;
- goto out;
- }
- if ( op == REMOVE )
- pte.bits = 0;
- else
- {
- pte = *entry;
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- if ( !pte.pt.ro && !pte.pt.xn )
- {
- printk("%s: Incorrect combination for addr=%lx\n",
- __func__, addr);
- rc = -EINVAL;
- goto out;
- }
- }
- write_pte(entry, pte);
- break;
- default:
- BUG();
- }
+ rc = xen_pt_update_entry(op, addr, mfn, flags);
+ if ( rc )
+ break;
}
-out:
+
/*
* Flush the TLBs even in case of failure because we may have
* partially modified the PT. This will prevent any unexpected
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 03/12] xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 18:29 ` Stefano Stabellini
-1 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 18:29 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> In preparation of rework of the Xen PT, the logic to update an entry
> in moved out in a separate function.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 140 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 74 insertions(+), 66 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 36e22fc9ad..f956aa6399 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -968,6 +968,76 @@ enum xenmap_operation {
> RESERVE
> };
>
> +static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> + mfn_t mfn, unsigned int flags)
> +{
> + lpae_t pte, *entry;
> + lpae_t *third = NULL;
> +
> + entry = &xen_second[second_linear_offset(addr)];
> + if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> + {
> + int rc = create_xen_table(entry);
> + if ( rc < 0 ) {
> + printk("%s: L2 failed\n", __func__);
> + return rc;
> + }
> + }
> +
> + BUG_ON(!lpae_is_valid(*entry));
> +
> + third = mfn_to_virt(lpae_get_mfn(*entry));
> + entry = &third[third_table_offset(addr)];
> +
> + switch ( op ) {
> + case INSERT:
> + case RESERVE:
> + if ( lpae_is_valid(*entry) )
> + {
> + printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
> + __func__, addr, mfn_x(mfn));
> + return -EINVAL;
> + }
> + if ( op == RESERVE )
> + break;
> + pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> + pte.pt.ro = PAGE_RO_MASK(flags);
> + pte.pt.xn = PAGE_XN_MASK(flags);
> + BUG_ON(!pte.pt.ro && !pte.pt.xn);
> + pte.pt.table = 1;
> + write_pte(entry, pte);
> + break;
> + case MODIFY:
> + case REMOVE:
> + if ( !lpae_is_valid(*entry) )
> + {
> + printk("%s: trying to %s a non-existing mapping addr=%lx\n",
> + __func__, op == REMOVE ? "remove" : "modify", addr);
> + return -EINVAL;
> + }
> + if ( op == REMOVE )
> + pte.bits = 0;
> + else
> + {
> + pte = *entry;
> + pte.pt.ro = PAGE_RO_MASK(flags);
> + pte.pt.xn = PAGE_XN_MASK(flags);
> + if ( !pte.pt.ro && !pte.pt.xn )
> + {
> + printk("%s: Incorrect combination for addr=%lx\n",
> + __func__, addr);
> + return -EINVAL;
> + }
> + }
> + write_pte(entry, pte);
> + break;
> + default:
> + BUG();
> + }
> +
> + return 0;
> +}
> +
> static DEFINE_SPINLOCK(xen_pt_lock);
>
> static int xen_pt_update(enum xenmap_operation op,
> @@ -978,78 +1048,16 @@ static int xen_pt_update(enum xenmap_operation op,
> {
> int rc = 0;
> unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> - lpae_t pte, *entry;
> - lpae_t *third = NULL;
>
> spin_lock(&xen_pt_lock);
>
> for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
> {
> - entry = &xen_second[second_linear_offset(addr)];
> - if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> - {
> - rc = create_xen_table(entry);
> - if ( rc < 0 ) {
> - printk("%s: L2 failed\n", __func__);
> - goto out;
> - }
> - }
> -
> - BUG_ON(!lpae_is_valid(*entry));
> -
> - third = mfn_to_virt(lpae_get_mfn(*entry));
> - entry = &third[third_table_offset(addr)];
> -
> - switch ( op ) {
> - case INSERT:
> - case RESERVE:
> - if ( lpae_is_valid(*entry) )
> - {
> - printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
> - __func__, addr, mfn_x(mfn));
> - rc = -EINVAL;
> - goto out;
> - }
> - if ( op == RESERVE )
> - break;
> - pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> - pte.pt.ro = PAGE_RO_MASK(flags);
> - pte.pt.xn = PAGE_XN_MASK(flags);
> - BUG_ON(!pte.pt.ro && !pte.pt.xn);
> - pte.pt.table = 1;
> - write_pte(entry, pte);
> - break;
> - case MODIFY:
> - case REMOVE:
> - if ( !lpae_is_valid(*entry) )
> - {
> - printk("%s: trying to %s a non-existing mapping addr=%lx\n",
> - __func__, op == REMOVE ? "remove" : "modify", addr);
> - rc = -EINVAL;
> - goto out;
> - }
> - if ( op == REMOVE )
> - pte.bits = 0;
> - else
> - {
> - pte = *entry;
> - pte.pt.ro = PAGE_RO_MASK(flags);
> - pte.pt.xn = PAGE_XN_MASK(flags);
> - if ( !pte.pt.ro && !pte.pt.xn )
> - {
> - printk("%s: Incorrect combination for addr=%lx\n",
> - __func__, addr);
> - rc = -EINVAL;
> - goto out;
> - }
> - }
> - write_pte(entry, pte);
> - break;
> - default:
> - BUG();
> - }
> + rc = xen_pt_update_entry(op, addr, mfn, flags);
> + if ( rc )
> + break;
> }
> -out:
> +
> /*
> * Flush the TLBs even in case of failure because we may have
> * partially modified the PT. This will prevent any unexpected
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, the MFN will be incremented even if it is invalid. This will
result to have a valid MFN after the first iteration.
While this is not a major problem today, this will be in the future if
the code expect an invalid MFN at every iteration.
Such behavior is prevented by avoiding to increment an invalid MFN.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Move the patch earlier on in the series
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f956aa6399..9de2a1150f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
spin_lock(&xen_pt_lock);
- for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
+ for( ; addr < addr_end; addr += PAGE_SIZE )
{
rc = xen_pt_update_entry(op, addr, mfn, flags);
if ( rc )
break;
+
+ if ( !mfn_eq(mfn, INVALID_MFN) )
+ mfn = mfn_add(mfn, 1);
}
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, the MFN will be incremented even if it is invalid. This will
result to have a valid MFN after the first iteration.
While this is not a major problem today, this will be in the future if
the code expect an invalid MFN at every iteration.
Such behavior is prevented by avoiding to increment an invalid MFN.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Move the patch earlier on in the series
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f956aa6399..9de2a1150f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
spin_lock(&xen_pt_lock);
- for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
+ for( ; addr < addr_end; addr += PAGE_SIZE )
{
rc = xen_pt_update_entry(op, addr, mfn, flags);
if ( rc )
break;
+
+ if ( !mfn_eq(mfn, INVALID_MFN) )
+ mfn = mfn_add(mfn, 1);
}
/*
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 18:37 ` Stefano Stabellini
2019-06-11 19:56 ` [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update) Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 18:37 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> Currently, the MFN will be incremented even if it is invalid. This will
> result to have a valid MFN after the first iteration.
>
> While this is not a major problem today, this will be in the future if
> the code expect an invalid MFN at every iteration.
>
> Such behavior is prevented by avoiding to increment an invalid MFN.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Move the patch earlier on in the series
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f956aa6399..9de2a1150f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>
> spin_lock(&xen_pt_lock);
>
> - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
> + for( ; addr < addr_end; addr += PAGE_SIZE )
> {
> rc = xen_pt_update_entry(op, addr, mfn, flags);
> if ( rc )
> break;
> +
> + if ( !mfn_eq(mfn, INVALID_MFN) )
> + mfn = mfn_add(mfn, 1);
> }
This is OK but got me thinking: should we be updating the mfn in mfn_add
if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
the static inline mfn_t mfn_add function. What do you think? I don't
think there are any valid scenarios where we want to increment
INVALID_MFN...
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)
2019-06-11 18:37 ` Stefano Stabellini
@ 2019-06-11 19:56 ` Julien Grall
2019-06-11 20:24 ` Andrew Cooper
2019-06-12 7:53 ` Jan Beulich
0 siblings, 2 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-11 19:56 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrii Anisov, Andrew Cooper, Oleksandr_Tyshchenko, Jan Beulich,
xen-devel, nd
Hi,
On 11/06/2019 19:37, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> Currently, the MFN will be incremented even if it is invalid. This will
>> result to have a valid MFN after the first iteration.
>>
>> While this is not a major problem today, this will be in the future if
>> the code expect an invalid MFN at every iteration.
>>
>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>> Changes in v2:
>> - Move the patch earlier on in the series
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/mm.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index f956aa6399..9de2a1150f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>
>> spin_lock(&xen_pt_lock);
>>
>> - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>> + for( ; addr < addr_end; addr += PAGE_SIZE )
>> {
>> rc = xen_pt_update_entry(op, addr, mfn, flags);
>> if ( rc )
>> break;
>> +
>> + if ( !mfn_eq(mfn, INVALID_MFN) )
>> + mfn = mfn_add(mfn, 1);
>> }
>
> This is OK but got me thinking: should we be updating the mfn in mfn_add
> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
> the static inline mfn_t mfn_add function. What do you think? I don't
> think there are any valid scenarios where we want to increment
> INVALID_MFN...
My first thought is mfn_add(...) may be used in place where we know the
mfn is not INVALID_MFN. So we would add extra check when it may not be
necessary. Although, I am not sure if it is important.
I have added Andrew & Jan to get any opinions.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)
2019-06-11 19:56 ` [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update) Julien Grall
@ 2019-06-11 20:24 ` Andrew Cooper
2019-06-12 12:47 ` Julien Grall
2019-06-12 7:53 ` Jan Beulich
1 sibling, 1 reply; 64+ messages in thread
From: Andrew Cooper @ 2019-06-11 20:24 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini
Cc: xen-devel, nd, Andrii Anisov, Jan Beulich, Oleksandr_Tyshchenko
On 11/06/2019 20:56, Julien Grall wrote:
> Hi,
>
> On 11/06/2019 19:37, Stefano Stabellini wrote:
>> On Tue, 14 May 2019, Julien Grall wrote:
>>> Currently, the MFN will be incremented even if it is invalid. This will
>>> result to have a valid MFN after the first iteration.
>>>
>>> While this is not a major problem today, this will be in the future if
>>> the code expect an invalid MFN at every iteration.
>>>
>>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> ---
>>> Changes in v2:
>>> - Move the patch earlier on in the series
>>> - Add Andrii's reviewed-by
>>> ---
>>> xen/arch/arm/mm.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index f956aa6399..9de2a1150f 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>>
>>> spin_lock(&xen_pt_lock);
>>>
>>> - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>> + for( ; addr < addr_end; addr += PAGE_SIZE )
>>> {
>>> rc = xen_pt_update_entry(op, addr, mfn, flags);
>>> if ( rc )
>>> break;
>>> +
>>> + if ( !mfn_eq(mfn, INVALID_MFN) )
>>> + mfn = mfn_add(mfn, 1);
>>> }
>> This is OK but got me thinking: should we be updating the mfn in mfn_add
>> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
>> the static inline mfn_t mfn_add function. What do you think? I don't
>> think there are any valid scenarios where we want to increment
>> INVALID_MFN...
> My first thought is mfn_add(...) may be used in place where we know the
> mfn is not INVALID_MFN. So we would add extra check when it may not be
> necessary. Although, I am not sure if it is important.
>
> I have added Andrew & Jan to get any opinions.
mfn_add(foo, bar) is shorthand for foo += bar, and should remain as such.
It exists only because we can't overload operators in C, and want
something slightly more readable than _mfn(mfn_x(foo) + bar)
Behind the scenes, the compiler will turn it back into a single add
instruction.
The saturating behaviour here is specific to the pagetable opereations
where passing INVALID_MFN is an alias for unmap, and is therefore not
useful in the majority of the users of mfn_add(), and certainly not
something we should have a hidden branch for in the middle of many tight
loops.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)
2019-06-11 20:24 ` Andrew Cooper
@ 2019-06-12 12:47 ` Julien Grall
2019-06-12 15:57 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-12 12:47 UTC (permalink / raw)
To: Andrew Cooper, Stefano Stabellini
Cc: xen-devel, nd, Andrii Anisov, Jan Beulich, Oleksandr_Tyshchenko
Hi,
On 11/06/2019 21:24, Andrew Cooper wrote:
> On 11/06/2019 20:56, Julien Grall wrote:
>> On 11/06/2019 19:37, Stefano Stabellini wrote:
>>> On Tue, 14 May 2019, Julien Grall wrote:
>>>> Currently, the MFN will be incremented even if it is invalid. This will
>>>> result to have a valid MFN after the first iteration.
>>>>
>>>> While this is not a major problem today, this will be in the future if
>>>> the code expect an invalid MFN at every iteration.
>>>>
>>>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>>
>>>> ---
>>>> Changes in v2:
>>>> - Move the patch earlier on in the series
>>>> - Add Andrii's reviewed-by
>>>> ---
>>>> xen/arch/arm/mm.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>>> index f956aa6399..9de2a1150f 100644
>>>> --- a/xen/arch/arm/mm.c
>>>> +++ b/xen/arch/arm/mm.c
>>>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>>>
>>>> spin_lock(&xen_pt_lock);
>>>>
>>>> - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>>> + for( ; addr < addr_end; addr += PAGE_SIZE )
>>>> {
>>>> rc = xen_pt_update_entry(op, addr, mfn, flags);
>>>> if ( rc )
>>>> break;
>>>> +
>>>> + if ( !mfn_eq(mfn, INVALID_MFN) )
>>>> + mfn = mfn_add(mfn, 1);
>>>> }
>>> This is OK but got me thinking: should we be updating the mfn in mfn_add
>>> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
>>> the static inline mfn_t mfn_add function. What do you think? I don't
>>> think there are any valid scenarios where we want to increment
>>> INVALID_MFN...
>> My first thought is mfn_add(...) may be used in place where we know the
>> mfn is not INVALID_MFN. So we would add extra check when it may not be
>> necessary. Although, I am not sure if it is important.
>>
>> I have added Andrew & Jan to get any opinions.
>
> mfn_add(foo, bar) is shorthand for foo += bar, and should remain as such.
>
> It exists only because we can't overload operators in C, and want
> something slightly more readable than _mfn(mfn_x(foo) + bar)
>
> Behind the scenes, the compiler will turn it back into a single add
> instruction.
>
> The saturating behaviour here is specific to the pagetable opereations
> where passing INVALID_MFN is an alias for unmap, and is therefore not
> useful in the majority of the users of mfn_add(), and certainly not
> something we should have a hidden branch for in the middle of many tight
> loops.
Thank you for the input! I will keep mfn_add() as it is.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)
2019-06-12 12:47 ` Julien Grall
@ 2019-06-12 15:57 ` Stefano Stabellini
0 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 15:57 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Andrii Anisov, Andrew Cooper,
Oleksandr_Tyshchenko, Jan Beulich, xen-devel, nd
On Wed, 12 Jun 2019, Julien Grall wrote:
> Hi,
>
> On 11/06/2019 21:24, Andrew Cooper wrote:
> > On 11/06/2019 20:56, Julien Grall wrote:
> > > On 11/06/2019 19:37, Stefano Stabellini wrote:
> > > > On Tue, 14 May 2019, Julien Grall wrote:
> > > > > Currently, the MFN will be incremented even if it is invalid. This
> > > > > will
> > > > > result to have a valid MFN after the first iteration.
> > > > >
> > > > > While this is not a major problem today, this will be in the future if
> > > > > the code expect an invalid MFN at every iteration.
> > > > >
> > > > > Such behavior is prevented by avoiding to increment an invalid MFN.
> > > > >
> > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > > > >
> > > > > ---
> > > > > Changes in v2:
> > > > > - Move the patch earlier on in the series
> > > > > - Add Andrii's reviewed-by
> > > > > ---
> > > > > xen/arch/arm/mm.c | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > > > index f956aa6399..9de2a1150f 100644
> > > > > --- a/xen/arch/arm/mm.c
> > > > > +++ b/xen/arch/arm/mm.c
> > > > > @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation
> > > > > op,
> > > > > spin_lock(&xen_pt_lock);
> > > > > - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn,
> > > > > 1))
> > > > > + for( ; addr < addr_end; addr += PAGE_SIZE )
> > > > > {
> > > > > rc = xen_pt_update_entry(op, addr, mfn, flags);
> > > > > if ( rc )
> > > > > break;
> > > > > +
> > > > > + if ( !mfn_eq(mfn, INVALID_MFN) )
> > > > > + mfn = mfn_add(mfn, 1);
> > > > > }
> > > > This is OK but got me thinking: should we be updating the mfn in mfn_add
> > > > if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
> > > > the static inline mfn_t mfn_add function. What do you think? I don't
> > > > think there are any valid scenarios where we want to increment
> > > > INVALID_MFN...
> > > My first thought is mfn_add(...) may be used in place where we know the
> > > mfn is not INVALID_MFN. So we would add extra check when it may not be
> > > necessary. Although, I am not sure if it is important.
> > >
> > > I have added Andrew & Jan to get any opinions.
> >
> > mfn_add(foo, bar) is shorthand for foo += bar, and should remain as such.
> >
> > It exists only because we can't overload operators in C, and want
> > something slightly more readable than _mfn(mfn_x(foo) + bar)
> >
> > Behind the scenes, the compiler will turn it back into a single add
> > instruction.
> >
> > The saturating behaviour here is specific to the pagetable opereations
> > where passing INVALID_MFN is an alias for unmap, and is therefore not
> > useful in the majority of the users of mfn_add(), and certainly not
> > something we should have a hidden branch for in the middle of many tight
> > loops.
>
> Thank you for the input! I will keep mfn_add() as it is.
Add my acked-by to the original patch.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)
2019-06-11 19:56 ` [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update) Julien Grall
2019-06-11 20:24 ` Andrew Cooper
@ 2019-06-12 7:53 ` Jan Beulich
1 sibling, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2019-06-12 7:53 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini
Cc: Andrew Cooper, nd, andrii_anisov, xen-devel, oleksandr_tyshchenko
>>> On 11.06.19 at 21:56, <Julien.Grall@arm.com> wrote:
> Hi,
>
> On 11/06/2019 19:37, Stefano Stabellini wrote:
>> On Tue, 14 May 2019, Julien Grall wrote:
>>> Currently, the MFN will be incremented even if it is invalid. This will
>>> result to have a valid MFN after the first iteration.
>>>
>>> While this is not a major problem today, this will be in the future if
>>> the code expect an invalid MFN at every iteration.
>>>
>>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> ---
>>> Changes in v2:
>>> - Move the patch earlier on in the series
>>> - Add Andrii's reviewed-by
>>> ---
>>> xen/arch/arm/mm.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index f956aa6399..9de2a1150f 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>>
>>> spin_lock(&xen_pt_lock);
>>>
>>> - for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>> + for( ; addr < addr_end; addr += PAGE_SIZE )
>>> {
>>> rc = xen_pt_update_entry(op, addr, mfn, flags);
>>> if ( rc )
>>> break;
>>> +
>>> + if ( !mfn_eq(mfn, INVALID_MFN) )
>>> + mfn = mfn_add(mfn, 1);
>>> }
>>
>> This is OK but got me thinking: should we be updating the mfn in mfn_add
>> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
>> the static inline mfn_t mfn_add function. What do you think? I don't
>> think there are any valid scenarios where we want to increment
>> INVALID_MFN...
>
> My first thought is mfn_add(...) may be used in place where we know the
> mfn is not INVALID_MFN. So we would add extra check when it may not be
> necessary. Although, I am not sure if it is important.
>
> I have added Andrew & Jan to get any opinions.
FWIW - I agree with Andrew.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 05/12] xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
At the moment, the flags are not enough to describe what kind of update
will done on the VA range. They need to be used in conjunction with the
enum xenmap_operation.
It would be more convenient to have all the information for the update
in a single place.
Two new flags are added to remove the relience on xenmap_operation:
- _PAGE_PRESENT: Indicate whether we are adding/removing the mapping
- _PAGE_POPULATE: Indicate whether we only populate page-tables
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 2 +-
xen/include/asm-arm/page.h | 9 +++++++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9de2a1150f..2192dede55 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt,
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
+ return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 2bcdb0f1a5..caf2fac1ff 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -76,6 +76,8 @@
*
* [0:2] Memory Attribute Index
* [3:4] Permission flags
+ * [5] Present bit
+ * [6] Populate page table
*/
#define PAGE_AI_MASK(x) ((x) & 0x7U)
@@ -86,12 +88,15 @@
#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
+#define _PAGE_PRESENT (1U << 5)
+#define _PAGE_POPULATE (1U << 6)
+
/*
* _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
* meant to be used outside of this header.
*/
-#define _PAGE_DEVICE _PAGE_XN
-#define _PAGE_NORMAL MT_NORMAL
+#define _PAGE_DEVICE (_PAGE_XN|_PAGE_PRESENT)
+#define _PAGE_NORMAL (MT_NORMAL|_PAGE_PRESENT)
#define PAGE_HYPERVISOR_RO (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
#define PAGE_HYPERVISOR_RX (_PAGE_NORMAL|_PAGE_RO)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 05/12] xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
At the moment, the flags are not enough to describe what kind of update
will done on the VA range. They need to be used in conjunction with the
enum xenmap_operation.
It would be more convenient to have all the information for the update
in a single place.
Two new flags are added to remove the relience on xenmap_operation:
- _PAGE_PRESENT: Indicate whether we are adding/removing the mapping
- _PAGE_POPULATE: Indicate whether we only populate page-tables
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 2 +-
xen/include/asm-arm/page.h | 9 +++++++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9de2a1150f..2192dede55 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt,
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
+ return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 2bcdb0f1a5..caf2fac1ff 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -76,6 +76,8 @@
*
* [0:2] Memory Attribute Index
* [3:4] Permission flags
+ * [5] Present bit
+ * [6] Populate page table
*/
#define PAGE_AI_MASK(x) ((x) & 0x7U)
@@ -86,12 +88,15 @@
#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
+#define _PAGE_PRESENT (1U << 5)
+#define _PAGE_POPULATE (1U << 6)
+
/*
* _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
* meant to be used outside of this header.
*/
-#define _PAGE_DEVICE _PAGE_XN
-#define _PAGE_NORMAL MT_NORMAL
+#define _PAGE_DEVICE (_PAGE_XN|_PAGE_PRESENT)
+#define _PAGE_NORMAL (MT_NORMAL|_PAGE_PRESENT)
#define PAGE_HYPERVISOR_RO (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
#define PAGE_HYPERVISOR_RX (_PAGE_NORMAL|_PAGE_RO)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 05/12] xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 22:35 ` Stefano Stabellini
2019-06-12 13:00 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 22:35 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> At the moment, the flags are not enough to describe what kind of update
> will done on the VA range. They need to be used in conjunction with the
> enum xenmap_operation.
>
> It would be more convenient to have all the information for the update
> in a single place.
>
> Two new flags are added to remove the relience on xenmap_operation:
> - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping
> - _PAGE_POPULATE: Indicate whether we only populate page-tables
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
Looking ahead in this series, I know that this is done so that later on
you can remove enum xenmap_operation. But what is the end goal? Why do
we want to remove enum xenmap_operation? I guess it is to make the code
more reusable?
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 2 +-
> xen/include/asm-arm/page.h | 9 +++++++--
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 9de2a1150f..2192dede55 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt,
>
> int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
> {
> - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
> + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> }
>
> int destroy_xen_mappings(unsigned long v, unsigned long e)
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 2bcdb0f1a5..caf2fac1ff 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -76,6 +76,8 @@
> *
> * [0:2] Memory Attribute Index
> * [3:4] Permission flags
> + * [5] Present bit
> + * [6] Populate page table
[5] is pretty clear. As a nit, I would probably write:
[5] Page Present
because there is no need to say "bit", the [5] means it is a bit.
Otherwise, something like the following:
[5] Present in memory
but it's unimportant.
It's [6] that we might want to explain a bit better: if we say "Populate
page table" then every time we want the Xen pagetable to be populated we
would need to pass _PAGE_POPULATE, even when the page is present in
memory. Do you see what I mean? I am not entirely sure how to make it
clearer. Maybe:
[6] Only populate page tables
"Only" being the key word.
> */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
>
> @@ -86,12 +88,15 @@
> #define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> #define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>
> +#define _PAGE_PRESENT (1U << 5)
> +#define _PAGE_POPULATE (1U << 6)
> +
> /*
> * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
> * meant to be used outside of this header.
> */
> -#define _PAGE_DEVICE _PAGE_XN
> -#define _PAGE_NORMAL MT_NORMAL
> +#define _PAGE_DEVICE (_PAGE_XN|_PAGE_PRESENT)
> +#define _PAGE_NORMAL (MT_NORMAL|_PAGE_PRESENT)
>
> #define PAGE_HYPERVISOR_RO (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
> #define PAGE_HYPERVISOR_RX (_PAGE_NORMAL|_PAGE_RO)
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 05/12] xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
2019-06-11 22:35 ` Stefano Stabellini
@ 2019-06-12 13:00 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-12 13:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 11/06/2019 23:35, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> At the moment, the flags are not enough to describe what kind of update
>> will done on the VA range. They need to be used in conjunction with the
>> enum xenmap_operation.
>>
>> It would be more convenient to have all the information for the update
>> in a single place.
>>
>> Two new flags are added to remove the relience on xenmap_operation:
>> - _PAGE_PRESENT: Indicate whether we are adding/removing the mapping
>> - _PAGE_POPULATE: Indicate whether we only populate page-tables
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> Looking ahead in this series, I know that this is done so that later on
> you can remove enum xenmap_operation. But what is the end goal? Why do
> we want to remove enum xenmap_operation? I guess it is to make the code
> more reusable?
The end goal is to streamline as much as possible to page-table update. I wanted
to have all the information in flags because it is much easier to reason with
one variable over two variables.
Furthermore, x86 code allows map_pages_to_xen(...) to destroy mappings but not
the underlying page-tables. This is used for instance for the vunmap to avoid
re-creating the page-tables afterwards. I have been thinking to introduce
similar things on Arm.
Keeping the xenmap_operation would make it awkward to support it because you
would have to translate the flags to the actual operations.
>> ---
>> Changes in v2:
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/mm.c | 2 +-
>> xen/include/asm-arm/page.h | 9 +++++++--
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 9de2a1150f..2192dede55 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1083,7 +1083,7 @@ int map_pages_to_xen(unsigned long virt,
>>
>> int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>> {
>> - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, 0);
>> + return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
>> }
>>
>> int destroy_xen_mappings(unsigned long v, unsigned long e)
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 2bcdb0f1a5..caf2fac1ff 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -76,6 +76,8 @@
>> *
>> * [0:2] Memory Attribute Index
>> * [3:4] Permission flags
>> + * [5] Present bit
>> + * [6] Populate page table
>
> [5] is pretty clear. As a nit, I would probably write:
>
> [5] Page Present
Better alternative, I will update the comment.
>
> because there is no need to say "bit", the [5] means it is a bit.
> Otherwise, something like the following:
>
> [5] Present in memory
>
> but it's unimportant.
>
> It's [6] that we might want to explain a bit better: if we say "Populate
> page table" then every time we want the Xen pagetable to be populated we
> would need to pass _PAGE_POPULATE, even when the page is present in
> memory. Do you see what I mean? I am not entirely sure how to make it
> clearer. Maybe:
To be honest, I was not entirely happy with the current comment. But I also
wasn't able to find a better one :).
>
> [6] Only populate page tables
I am happy with this alternative. I will update the comment.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
The code handling Xen PT update has quite a few restrictions on what it
can do. This is not a bad thing as it keeps the code simple.
There are already a few checks scattered in current page table handling.
However they are not sufficient as they could still allow to
modify/remove entry with contiguous bit set.
The checks are divided in two sets:
- per entry check: They are gathered in a new function that will
check whether an update is valid based on the flags passed and the
current value of an entry.
- global check: They are sanity check on xen_pt_update() parameters.
Additionally to contiguous check, we also now check that the caller is
not trying to modify the memory attributes of an entry.
Lastly, it was probably a bit over the top to forbid removing an
invalid mapping. This could just be ignored. The new behavior will be
helpful in future changes.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Correctly detect the removal of a page
- Fix ASSERT on flags in the else case
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 115 +++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 97 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2192dede55..45a6f9287f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
#undef mfn_to_virt
#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
+#ifdef NDEBUG
+static inline void
+__attribute__ ((__format__ (__printf__, 1, 2)))
+mm_printk(const char *fmt, ...) {}
+#else
+#define mm_printk(fmt, args...) \
+ do \
+ { \
+ dprintk(XENLOG_ERR, fmt, ## args); \
+ WARN(); \
+ } while (0);
+#endif
+
#define DEFINE_PAGE_TABLES(name, nr) \
lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
@@ -968,12 +981,74 @@ enum xenmap_operation {
RESERVE
};
+/* Sanity check of the entry */
+static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
+{
+ /* Sanity check when modifying a page. */
+ if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
+ {
+ /* We don't allow changing memory attributes. */
+ if ( entry.pt.ai != PAGE_AI_MASK(flags) )
+ {
+ mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
+ entry.pt.ai, PAGE_AI_MASK(flags));
+ return false;
+ }
+
+ /* We don't allow modifying entry with contiguous bit set. */
+ if ( entry.pt.contig )
+ {
+ mm_printk("Modifying entry with contiguous bit set is not allowed.\n");
+ return false;
+ }
+ }
+ /* Sanity check when inserting a page */
+ else if ( flags & _PAGE_PRESENT )
+ {
+ /* We should be here with a valid MFN. */
+ ASSERT(!mfn_eq(mfn, INVALID_MFN));
+
+ /* 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));
+ return false;
+ }
+ }
+ /* Sanity check when removing a page. */
+ else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
+ {
+ /* We should be here with an invalid MFN. */
+ ASSERT(mfn_eq(mfn, INVALID_MFN));
+
+ /* We don't allow removing page with contiguous bit set. */
+ if ( entry.pt.contig )
+ {
+ mm_printk("Removing entry with contiguous bit set is not allowed.\n");
+ return false;
+ }
+ }
+ /* Sanity check when populating the page-table. No check so far. */
+ else
+ {
+ ASSERT(flags & _PAGE_POPULATE);
+ /* We should be here with an invalid MFN */
+ ASSERT(mfn_eq(mfn, INVALID_MFN));
+ }
+
+ return true;
+}
+
static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
mfn_t mfn, unsigned int flags)
{
lpae_t pte, *entry;
lpae_t *third = NULL;
+ /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
+ ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
+
entry = &xen_second[second_linear_offset(addr)];
if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
{
@@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
third = mfn_to_virt(lpae_get_mfn(*entry));
entry = &third[third_table_offset(addr)];
+ if ( !xen_pt_check_entry(*entry, mfn, flags) )
+ return -EINVAL;
+
switch ( op ) {
case INSERT:
case RESERVE:
- if ( lpae_is_valid(*entry) )
- {
- printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
- __func__, addr, mfn_x(mfn));
- return -EINVAL;
- }
if ( op == RESERVE )
break;
pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
@@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
break;
case MODIFY:
case REMOVE:
- if ( !lpae_is_valid(*entry) )
- {
- printk("%s: trying to %s a non-existing mapping addr=%lx\n",
- __func__, op == REMOVE ? "remove" : "modify", addr);
- return -EINVAL;
- }
if ( op == REMOVE )
pte.bits = 0;
else
@@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
pte = *entry;
pte.pt.ro = PAGE_RO_MASK(flags);
pte.pt.xn = PAGE_XN_MASK(flags);
- if ( !pte.pt.ro && !pte.pt.xn )
- {
- printk("%s: Incorrect combination for addr=%lx\n",
- __func__, addr);
- return -EINVAL;
- }
}
write_pte(entry, pte);
break;
@@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
+ /*
+ * The hardware was configured to forbid mapping both writeable and
+ * executable.
+ * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
+ * prevent any update if this happen.
+ */
+ if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
+ !PAGE_XN_MASK(flags) )
+ {
+ mm_printk("Mappings should not be both Writeable and Executable.\n");
+ return -EINVAL;
+ }
+
+ if ( !IS_ALIGNED(virt, PAGE_SIZE) )
+ {
+ mm_printk("The virtual address is not aligned to the page-size.\n");
+ return -EINVAL;
+ }
+
spin_lock(&xen_pt_lock);
for( ; addr < addr_end; addr += PAGE_SIZE )
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
The code handling Xen PT update has quite a few restrictions on what it
can do. This is not a bad thing as it keeps the code simple.
There are already a few checks scattered in current page table handling.
However they are not sufficient as they could still allow to
modify/remove entry with contiguous bit set.
The checks are divided in two sets:
- per entry check: They are gathered in a new function that will
check whether an update is valid based on the flags passed and the
current value of an entry.
- global check: They are sanity check on xen_pt_update() parameters.
Additionally to contiguous check, we also now check that the caller is
not trying to modify the memory attributes of an entry.
Lastly, it was probably a bit over the top to forbid removing an
invalid mapping. This could just be ignored. The new behavior will be
helpful in future changes.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Correctly detect the removal of a page
- Fix ASSERT on flags in the else case
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 115 +++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 97 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2192dede55..45a6f9287f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
#undef mfn_to_virt
#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
+#ifdef NDEBUG
+static inline void
+__attribute__ ((__format__ (__printf__, 1, 2)))
+mm_printk(const char *fmt, ...) {}
+#else
+#define mm_printk(fmt, args...) \
+ do \
+ { \
+ dprintk(XENLOG_ERR, fmt, ## args); \
+ WARN(); \
+ } while (0);
+#endif
+
#define DEFINE_PAGE_TABLES(name, nr) \
lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
@@ -968,12 +981,74 @@ enum xenmap_operation {
RESERVE
};
+/* Sanity check of the entry */
+static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
+{
+ /* Sanity check when modifying a page. */
+ if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
+ {
+ /* We don't allow changing memory attributes. */
+ if ( entry.pt.ai != PAGE_AI_MASK(flags) )
+ {
+ mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
+ entry.pt.ai, PAGE_AI_MASK(flags));
+ return false;
+ }
+
+ /* We don't allow modifying entry with contiguous bit set. */
+ if ( entry.pt.contig )
+ {
+ mm_printk("Modifying entry with contiguous bit set is not allowed.\n");
+ return false;
+ }
+ }
+ /* Sanity check when inserting a page */
+ else if ( flags & _PAGE_PRESENT )
+ {
+ /* We should be here with a valid MFN. */
+ ASSERT(!mfn_eq(mfn, INVALID_MFN));
+
+ /* 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));
+ return false;
+ }
+ }
+ /* Sanity check when removing a page. */
+ else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
+ {
+ /* We should be here with an invalid MFN. */
+ ASSERT(mfn_eq(mfn, INVALID_MFN));
+
+ /* We don't allow removing page with contiguous bit set. */
+ if ( entry.pt.contig )
+ {
+ mm_printk("Removing entry with contiguous bit set is not allowed.\n");
+ return false;
+ }
+ }
+ /* Sanity check when populating the page-table. No check so far. */
+ else
+ {
+ ASSERT(flags & _PAGE_POPULATE);
+ /* We should be here with an invalid MFN */
+ ASSERT(mfn_eq(mfn, INVALID_MFN));
+ }
+
+ return true;
+}
+
static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
mfn_t mfn, unsigned int flags)
{
lpae_t pte, *entry;
lpae_t *third = NULL;
+ /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
+ ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
+
entry = &xen_second[second_linear_offset(addr)];
if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
{
@@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
third = mfn_to_virt(lpae_get_mfn(*entry));
entry = &third[third_table_offset(addr)];
+ if ( !xen_pt_check_entry(*entry, mfn, flags) )
+ return -EINVAL;
+
switch ( op ) {
case INSERT:
case RESERVE:
- if ( lpae_is_valid(*entry) )
- {
- printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
- __func__, addr, mfn_x(mfn));
- return -EINVAL;
- }
if ( op == RESERVE )
break;
pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
@@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
break;
case MODIFY:
case REMOVE:
- if ( !lpae_is_valid(*entry) )
- {
- printk("%s: trying to %s a non-existing mapping addr=%lx\n",
- __func__, op == REMOVE ? "remove" : "modify", addr);
- return -EINVAL;
- }
if ( op == REMOVE )
pte.bits = 0;
else
@@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
pte = *entry;
pte.pt.ro = PAGE_RO_MASK(flags);
pte.pt.xn = PAGE_XN_MASK(flags);
- if ( !pte.pt.ro && !pte.pt.xn )
- {
- printk("%s: Incorrect combination for addr=%lx\n",
- __func__, addr);
- return -EINVAL;
- }
}
write_pte(entry, pte);
break;
@@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
int rc = 0;
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
+ /*
+ * The hardware was configured to forbid mapping both writeable and
+ * executable.
+ * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
+ * prevent any update if this happen.
+ */
+ if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
+ !PAGE_XN_MASK(flags) )
+ {
+ mm_printk("Mappings should not be both Writeable and Executable.\n");
+ return -EINVAL;
+ }
+
+ if ( !IS_ALIGNED(virt, PAGE_SIZE) )
+ {
+ mm_printk("The virtual address is not aligned to the page-size.\n");
+ return -EINVAL;
+ }
+
spin_lock(&xen_pt_lock);
for( ; addr < addr_end; addr += PAGE_SIZE )
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 0:10 ` Stefano Stabellini
2019-06-12 14:48 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 0:10 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> The code handling Xen PT update has quite a few restrictions on what it
> can do. This is not a bad thing as it keeps the code simple.
>
> There are already a few checks scattered in current page table handling.
> However they are not sufficient as they could still allow to
> modify/remove entry with contiguous bit set.
>
> The checks are divided in two sets:
> - per entry check: They are gathered in a new function that will
> check whether an update is valid based on the flags passed and the
> current value of an entry.
> - global check: They are sanity check on xen_pt_update() parameters.
>
> Additionally to contiguous check, we also now check that the caller is
> not trying to modify the memory attributes of an entry.
>
> Lastly, it was probably a bit over the top to forbid removing an
> invalid mapping. This could just be ignored. The new behavior will be
> helpful in future changes.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Correctly detect the removal of a page
> - Fix ASSERT on flags in the else case
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 115 +++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 97 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 2192dede55..45a6f9287f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
> #undef mfn_to_virt
> #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>
> +#ifdef NDEBUG
> +static inline void
> +__attribute__ ((__format__ (__printf__, 1, 2)))
> +mm_printk(const char *fmt, ...) {}
> +#else
> +#define mm_printk(fmt, args...) \
> + do \
> + { \
> + dprintk(XENLOG_ERR, fmt, ## args); \
> + WARN(); \
> + } while (0);
> +#endif
> +
> #define DEFINE_PAGE_TABLES(name, nr) \
> lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>
> @@ -968,12 +981,74 @@ enum xenmap_operation {
> RESERVE
> };
>
> +/* Sanity check of the entry */
> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> +{
> + /* Sanity check when modifying a page. */
> + if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
> + {
I understand we could skip the valid check on REMOVE, but should we skip
it on MODIFY too? Is that also going to be helpful in future changes?
> + /* We don't allow changing memory attributes. */
> + if ( entry.pt.ai != PAGE_AI_MASK(flags) )
> + {
> + mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
> + entry.pt.ai, PAGE_AI_MASK(flags));
> + return false;
> + }
> +
> + /* We don't allow modifying entry with contiguous bit set. */
> + if ( entry.pt.contig )
> + {
> + mm_printk("Modifying entry with contiguous bit set is not allowed.\n");
> + return false;
> + }
> + }
> + /* Sanity check when inserting a page */
> + else if ( flags & _PAGE_PRESENT )
> + {
> + /* We should be here with a valid MFN. */
> + ASSERT(!mfn_eq(mfn, INVALID_MFN));
> +
> + /* 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));
> + return false;
> + }
> + }
> + /* Sanity check when removing a page. */
> + else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
> + {
> + /* We should be here with an invalid MFN. */
> + ASSERT(mfn_eq(mfn, INVALID_MFN));
> +
> + /* We don't allow removing page with contiguous bit set. */
> + if ( entry.pt.contig )
> + {
> + mm_printk("Removing entry with contiguous bit set is not allowed.\n");
> + return false;
> + }
> + }
> + /* Sanity check when populating the page-table. No check so far. */
> + else
> + {
> + ASSERT(flags & _PAGE_POPULATE);
> + /* We should be here with an invalid MFN */
> + ASSERT(mfn_eq(mfn, INVALID_MFN));
> + }
> +
> + return true;
> +}
> +
> static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> mfn_t mfn, unsigned int flags)
> {
> lpae_t pte, *entry;
> lpae_t *third = NULL;
>
> + /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
> + ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
over 80 chars?
> entry = &xen_second[second_linear_offset(addr)];
> if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> {
> @@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> third = mfn_to_virt(lpae_get_mfn(*entry));
> entry = &third[third_table_offset(addr)];
>
> + if ( !xen_pt_check_entry(*entry, mfn, flags) )
> + return -EINVAL;
> +
> switch ( op ) {
> case INSERT:
> case RESERVE:
> - if ( lpae_is_valid(*entry) )
> - {
> - printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
> - __func__, addr, mfn_x(mfn));
> - return -EINVAL;
> - }
> if ( op == RESERVE )
> break;
> pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> @@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> break;
> case MODIFY:
> case REMOVE:
> - if ( !lpae_is_valid(*entry) )
> - {
> - printk("%s: trying to %s a non-existing mapping addr=%lx\n",
> - __func__, op == REMOVE ? "remove" : "modify", addr);
> - return -EINVAL;
> - }
> if ( op == REMOVE )
> pte.bits = 0;
> else
> @@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> pte = *entry;
> pte.pt.ro = PAGE_RO_MASK(flags);
> pte.pt.xn = PAGE_XN_MASK(flags);
> - if ( !pte.pt.ro && !pte.pt.xn )
> - {
> - printk("%s: Incorrect combination for addr=%lx\n",
> - __func__, addr);
> - return -EINVAL;
> - }
> }
> write_pte(entry, pte);
> break;
> @@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
> int rc = 0;
> unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
>
> + /*
> + * The hardware was configured to forbid mapping both writeable and
> + * executable.
> + * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
> + * prevent any update if this happen.
> + */
> + if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
> + !PAGE_XN_MASK(flags) )
> + {
> + mm_printk("Mappings should not be both Writeable and Executable.\n");
> + return -EINVAL;
> + }
I am thinking this is serious enough that we might want to always print
this warning when this error happens. At the same time it is awkward to
have all the other messages using mm_printk and only this one being
different. So I'll live it to you, it is also OK at this.
> + if ( !IS_ALIGNED(virt, PAGE_SIZE) )
> + {
> + mm_printk("The virtual address is not aligned to the page-size.\n");
> + return -EINVAL;
> + }
> +
> spin_lock(&xen_pt_lock);
>
> for( ; addr < addr_end; addr += PAGE_SIZE )
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
2019-06-12 0:10 ` Stefano Stabellini
@ 2019-06-12 14:48 ` Julien Grall
2019-06-12 15:54 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-12 14:48 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 12/06/2019 01:10, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> The code handling Xen PT update has quite a few restrictions on what it
>> can do. This is not a bad thing as it keeps the code simple.
>>
>> There are already a few checks scattered in current page table handling.
>> However they are not sufficient as they could still allow to
>> modify/remove entry with contiguous bit set.
>>
>> The checks are divided in two sets:
>> - per entry check: They are gathered in a new function that will
>> check whether an update is valid based on the flags passed and the
>> current value of an entry.
>> - global check: They are sanity check on xen_pt_update() parameters.
>>
>> Additionally to contiguous check, we also now check that the caller is
>> not trying to modify the memory attributes of an entry.
>>
>> Lastly, it was probably a bit over the top to forbid removing an
>> invalid mapping. This could just be ignored. The new behavior will be
>> helpful in future changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>> Changes in v2:
>> - Correctly detect the removal of a page
>> - Fix ASSERT on flags in the else case
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/mm.c | 115 +++++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 97 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 2192dede55..45a6f9287f 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
>> #undef mfn_to_virt
>> #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>>
>> +#ifdef NDEBUG
>> +static inline void
>> +__attribute__ ((__format__ (__printf__, 1, 2)))
>> +mm_printk(const char *fmt, ...) {}
>> +#else
>> +#define mm_printk(fmt, args...) \
>> + do \
>> + { \
>> + dprintk(XENLOG_ERR, fmt, ## args); \
>> + WARN(); \
>> + } while (0);
>> +#endif
>> +
>> #define DEFINE_PAGE_TABLES(name, nr) \
>> lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>>
>> @@ -968,12 +981,74 @@ enum xenmap_operation {
>> RESERVE
>> };
>>
>> +/* Sanity check of the entry */
>> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>> +{
>> + /* Sanity check when modifying a page. */
>> + if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
>> + {
>
> I understand we could skip the valid check on REMOVE, but should we skip
> it on MODIFY too? Is that also going to be helpful in future changes?
Hmmm, I can't exactly remember why I didn't check the valid bit here.
I did it for REMOVE as for the early FDT mapping it is more convenient to remove
the full possible range over keeping track of the exact start/size.
I would assume the same would hold for MODIFY, but I don't have a concrete
example here. I am happy to add the valid check and defer the decision to remove
it if it is deem to be useful in the future.
>
>
>> + /* We don't allow changing memory attributes. */
>> + if ( entry.pt.ai != PAGE_AI_MASK(flags) )
>> + {
>> + mm_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
>> + entry.pt.ai, PAGE_AI_MASK(flags));
>> + return false;
>> + }
>> +
>> + /* We don't allow modifying entry with contiguous bit set. */
>> + if ( entry.pt.contig )
>> + {
>> + mm_printk("Modifying entry with contiguous bit set is not allowed.\n");
>> + return false;
>> + }
>> + }
>> + /* Sanity check when inserting a page */
>> + else if ( flags & _PAGE_PRESENT )
>> + {
>> + /* We should be here with a valid MFN. */
>> + ASSERT(!mfn_eq(mfn, INVALID_MFN));
>> +
>> + /* 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));
>> + return false;
>> + }
>> + }
>> + /* Sanity check when removing a page. */
>> + else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
>> + {
>> + /* We should be here with an invalid MFN. */
>> + ASSERT(mfn_eq(mfn, INVALID_MFN));
>> +
>> + /* We don't allow removing page with contiguous bit set. */
>> + if ( entry.pt.contig )
>> + {
>> + mm_printk("Removing entry with contiguous bit set is not allowed.\n");
>> + return false;
>> + }
>> + }
>> + /* Sanity check when populating the page-table. No check so far. */
>> + else
>> + {
>> + ASSERT(flags & _PAGE_POPULATE);
>> + /* We should be here with an invalid MFN */
>> + ASSERT(mfn_eq(mfn, INVALID_MFN));
>> + }
>> +
>> + return true;
>> +}
>> +
>> static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>> mfn_t mfn, unsigned int flags)
>> {
>> lpae_t pte, *entry;
>> lpae_t *third = NULL;
>>
>> + /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
>> + ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
>
> over 80 chars?
It is 87 chars, I was hoping you didn't notice it :). The line splitting result
to nasty code. Alternatively, I could introduce a define for
_PAGE_POPULATE|_PAGE_PRESENT, maybe EXCLUSIVE_FLAGS?
Any preference?
>
>
>> entry = &xen_second[second_linear_offset(addr)];
>> if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
>> {
>> @@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>> third = mfn_to_virt(lpae_get_mfn(*entry));
>> entry = &third[third_table_offset(addr)];
>>
>> + if ( !xen_pt_check_entry(*entry, mfn, flags) )
>> + return -EINVAL;
>> +
>> switch ( op ) {
>> case INSERT:
>> case RESERVE:
>> - if ( lpae_is_valid(*entry) )
>> - {
>> - printk("%s: trying to replace an existing mapping addr=%lx mfn=%"PRI_mfn"\n",
>> - __func__, addr, mfn_x(mfn));
>> - return -EINVAL;
>> - }
>> if ( op == RESERVE )
>> break;
>> pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>> @@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>> break;
>> case MODIFY:
>> case REMOVE:
>> - if ( !lpae_is_valid(*entry) )
>> - {
>> - printk("%s: trying to %s a non-existing mapping addr=%lx\n",
>> - __func__, op == REMOVE ? "remove" : "modify", addr);
>> - return -EINVAL;
>> - }
>> if ( op == REMOVE )
>> pte.bits = 0;
>> else
>> @@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>> pte = *entry;
>> pte.pt.ro = PAGE_RO_MASK(flags);
>> pte.pt.xn = PAGE_XN_MASK(flags);
>> - if ( !pte.pt.ro && !pte.pt.xn )
>> - {
>> - printk("%s: Incorrect combination for addr=%lx\n",
>> - __func__, addr);
>> - return -EINVAL;
>> - }
>> }
>> write_pte(entry, pte);
>> break;
>> @@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
>> int rc = 0;
>> unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
>>
>> + /*
>> + * The hardware was configured to forbid mapping both writeable and
>> + * executable.
>> + * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
>> + * prevent any update if this happen.
>> + */
>> + if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
>> + !PAGE_XN_MASK(flags) )
>> + {
>> + mm_printk("Mappings should not be both Writeable and Executable.\n");
>> + return -EINVAL;
>> + }
>
> I am thinking this is serious enough that we might want to always print
> this warning when this error happens. At the same time it is awkward to
> have all the other messages using mm_printk and only this one being
> different. So I'll live it to you, it is also OK at this.
Any error here means the caller didn't do sanity check (if input is external) or
pass the wrong parameters. I purposefully chose mm_printk over a normal printk
because this could potentially lead to a DoS if accessible from outside of Xen.
If the error happen, then there are an high chance with DEBUG (or hacking
mm_printk to be used in non-debug build) will make it appear as well.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
2019-06-12 14:48 ` Julien Grall
@ 2019-06-12 15:54 ` Stefano Stabellini
2019-06-12 15:58 ` Julien Grall
0 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 15:54 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Wed, 12 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 12/06/2019 01:10, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > The code handling Xen PT update has quite a few restrictions on what it
> > > can do. This is not a bad thing as it keeps the code simple.
> > >
> > > There are already a few checks scattered in current page table handling.
> > > However they are not sufficient as they could still allow to
> > > modify/remove entry with contiguous bit set.
> > >
> > > The checks are divided in two sets:
> > > - per entry check: They are gathered in a new function that will
> > > check whether an update is valid based on the flags passed and the
> > > current value of an entry.
> > > - global check: They are sanity check on xen_pt_update() parameters.
> > >
> > > Additionally to contiguous check, we also now check that the caller is
> > > not trying to modify the memory attributes of an entry.
> > >
> > > Lastly, it was probably a bit over the top to forbid removing an
> > > invalid mapping. This could just be ignored. The new behavior will be
> > > helpful in future changes.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > >
> > > ---
> > > Changes in v2:
> > > - Correctly detect the removal of a page
> > > - Fix ASSERT on flags in the else case
> > > - Add Andrii's reviewed-by
> > > ---
> > > xen/arch/arm/mm.c | 115
> > > +++++++++++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 97 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 2192dede55..45a6f9287f 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -50,6 +50,19 @@ struct domain *dom_xen, *dom_io, *dom_cow;
> > > #undef mfn_to_virt
> > > #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
> > > +#ifdef NDEBUG
> > > +static inline void
> > > +__attribute__ ((__format__ (__printf__, 1, 2)))
> > > +mm_printk(const char *fmt, ...) {}
> > > +#else
> > > +#define mm_printk(fmt, args...) \
> > > + do \
> > > + { \
> > > + dprintk(XENLOG_ERR, fmt, ## args); \
> > > + WARN(); \
> > > + } while (0);
> > > +#endif
> > > +
> > > #define DEFINE_PAGE_TABLES(name, nr) \
> > > lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
> > > @@ -968,12 +981,74 @@ enum xenmap_operation {
> > > RESERVE
> > > };
> > > +/* Sanity check of the entry */
> > > +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int
> > > flags)
> > > +{
> > > + /* Sanity check when modifying a page. */
> > > + if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
> > > + {
> >
> > I understand we could skip the valid check on REMOVE, but should we skip
> > it on MODIFY too? Is that also going to be helpful in future changes?
>
> Hmmm, I can't exactly remember why I didn't check the valid bit here.
>
> I did it for REMOVE as for the early FDT mapping it is more convenient to
> remove the full possible range over keeping track of the exact start/size.
>
> I would assume the same would hold for MODIFY, but I don't have a concrete
> example here. I am happy to add the valid check and defer the decision to
> remove it if it is deem to be useful in the future.
Yes, it would be better
> >
> > > + /* We don't allow changing memory attributes. */
> > > + if ( entry.pt.ai != PAGE_AI_MASK(flags) )
> > > + {
> > > + mm_printk("Modifying memory attributes is not allowed (0x%x
> > > -> 0x%x).\n",
> > > + entry.pt.ai, PAGE_AI_MASK(flags));
> > > + return false;
> > > + }
> > > +
> > > + /* We don't allow modifying entry with contiguous bit set. */
> > > + if ( entry.pt.contig )
> > > + {
> > > + mm_printk("Modifying entry with contiguous bit set is not
> > > allowed.\n");
> > > + return false;
> > > + }
> > > + }
> > > + /* Sanity check when inserting a page */
> > > + else if ( flags & _PAGE_PRESENT )
> > > + {
> > > + /* We should be here with a valid MFN. */
> > > + ASSERT(!mfn_eq(mfn, INVALID_MFN));
> > > +
> > > + /* 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));
> > > + return false;
> > > + }
> > > + }
> > > + /* Sanity check when removing a page. */
> > > + else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
> > > + {
> > > + /* We should be here with an invalid MFN. */
> > > + ASSERT(mfn_eq(mfn, INVALID_MFN));
> > > +
> > > + /* We don't allow removing page with contiguous bit set. */
> > > + if ( entry.pt.contig )
> > > + {
> > > + mm_printk("Removing entry with contiguous bit set is not
> > > allowed.\n");
> > > + return false;
> > > + }
> > > + }
> > > + /* Sanity check when populating the page-table. No check so far. */
> > > + else
> > > + {
> > > + ASSERT(flags & _PAGE_POPULATE);
> > > + /* We should be here with an invalid MFN */
> > > + ASSERT(mfn_eq(mfn, INVALID_MFN));
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > static int xen_pt_update_entry(enum xenmap_operation op, unsigned long
> > > addr,
> > > mfn_t mfn, unsigned int flags)
> > > {
> > > lpae_t pte, *entry;
> > > lpae_t *third = NULL;
> > > + /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together.
> > > */
> > > + ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) !=
> > > (_PAGE_POPULATE|_PAGE_PRESENT));
> >
> > over 80 chars?
>
> It is 87 chars, I was hoping you didn't notice it :). The line splitting
> result to nasty code. Alternatively, I could introduce a define for
> _PAGE_POPULATE|_PAGE_PRESENT, maybe EXCLUSIVE_FLAGS?
>
> Any preference?
I don't care so much about 80 chars limit.
Anything but another macro :-)
> > > entry = &xen_second[second_linear_offset(addr)];
> > > if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> > > {
> > > @@ -989,15 +1064,12 @@ static int xen_pt_update_entry(enum
> > > xenmap_operation op, unsigned long addr,
> > > third = mfn_to_virt(lpae_get_mfn(*entry));
> > > entry = &third[third_table_offset(addr)];
> > > + if ( !xen_pt_check_entry(*entry, mfn, flags) )
> > > + return -EINVAL;
> > > +
> > > switch ( op ) {
> > > case INSERT:
> > > case RESERVE:
> > > - if ( lpae_is_valid(*entry) )
> > > - {
> > > - printk("%s: trying to replace an existing mapping
> > > addr=%lx mfn=%"PRI_mfn"\n",
> > > - __func__, addr, mfn_x(mfn));
> > > - return -EINVAL;
> > > - }
> > > if ( op == RESERVE )
> > > break;
> > > pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> > > @@ -1009,12 +1081,6 @@ static int xen_pt_update_entry(enum
> > > xenmap_operation op, unsigned long addr,
> > > break;
> > > case MODIFY:
> > > case REMOVE:
> > > - if ( !lpae_is_valid(*entry) )
> > > - {
> > > - printk("%s: trying to %s a non-existing mapping
> > > addr=%lx\n",
> > > - __func__, op == REMOVE ? "remove" : "modify",
> > > addr);
> > > - return -EINVAL;
> > > - }
> > > if ( op == REMOVE )
> > > pte.bits = 0;
> > > else
> > > @@ -1022,12 +1088,6 @@ static int xen_pt_update_entry(enum
> > > xenmap_operation op, unsigned long addr,
> > > pte = *entry;
> > > pte.pt.ro = PAGE_RO_MASK(flags);
> > > pte.pt.xn = PAGE_XN_MASK(flags);
> > > - if ( !pte.pt.ro && !pte.pt.xn )
> > > - {
> > > - printk("%s: Incorrect combination for addr=%lx\n",
> > > - __func__, addr);
> > > - return -EINVAL;
> > > - }
> > > }
> > > write_pte(entry, pte);
> > > break;
> > > @@ -1049,6 +1109,25 @@ static int xen_pt_update(enum xenmap_operation op,
> > > int rc = 0;
> > > unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> > > + /*
> > > + * The hardware was configured to forbid mapping both writeable and
> > > + * executable.
> > > + * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
> > > + * prevent any update if this happen.
> > > + */
> > > + if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
> > > + !PAGE_XN_MASK(flags) )
> > > + {
> > > + mm_printk("Mappings should not be both Writeable and
> > > Executable.\n");
> > > + return -EINVAL;
> > > + }
> >
> > I am thinking this is serious enough that we might want to always print
> > this warning when this error happens. At the same time it is awkward to
> > have all the other messages using mm_printk and only this one being
> > different. So I'll live it to you, it is also OK at this.
>
> Any error here means the caller didn't do sanity check (if input is external)
> or pass the wrong parameters. I purposefully chose mm_printk over a normal
> printk because this could potentially lead to a DoS if accessible from outside
> of Xen.
>
> If the error happen, then there are an high chance with DEBUG (or hacking
> mm_printk to be used in non-debug build) will make it appear as well.
OK
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 06/12] xen/arm: mm: Sanity check any update of Xen page tables
2019-06-12 15:54 ` Stefano Stabellini
@ 2019-06-12 15:58 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-12 15:58 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi,
On 12/06/2019 16:54, Stefano Stabellini wrote:
> On Wed, 12 Jun 2019, Julien Grall wrote:
>> On 12/06/2019 01:10, Stefano Stabellini wrote:
>>> On Tue, 14 May 2019, Julien Grall wrote:
>>> I understand we could skip the valid check on REMOVE, but should we skip
>>> it on MODIFY too? Is that also going to be helpful in future changes?
>>
>> Hmmm, I can't exactly remember why I didn't check the valid bit here.
>>
>> I did it for REMOVE as for the early FDT mapping it is more convenient to
>> remove the full possible range over keeping track of the exact start/size.
>>
>> I would assume the same would hold for MODIFY, but I don't have a concrete
>> example here. I am happy to add the valid check and defer the decision to
>> remove it if it is deem to be useful in the future.
>
> Yes, it would be better
I will update it in the next version.
[...]
>>>> static int xen_pt_update_entry(enum xenmap_operation op, unsigned long
>>>> addr,
>>>> mfn_t mfn, unsigned int flags)
>>>> {
>>>> lpae_t pte, *entry;
>>>> lpae_t *third = NULL;
>>>> + /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together.
>>>> */
>>>> + ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) !=
>>>> (_PAGE_POPULATE|_PAGE_PRESENT));
>>>
>>> over 80 chars?
>>
>> It is 87 chars, I was hoping you didn't notice it :). The line splitting
>> result to nasty code. Alternatively, I could introduce a define for
>> _PAGE_POPULATE|_PAGE_PRESENT, maybe EXCLUSIVE_FLAGS?
>>
>> Any preference?
>
> I don't care so much about 80 chars limit.
> Anything but another macro :-)
Ok I will keep the 80 lines then!
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 07/12] xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
With the newly introduced flags, it is now possible to know how the page
will be updated through the flags.
All the use of xenmap_operation are now replaced with the flags. At the
same time, validity check are now removed as they are gathered in
xen_pt_check_entry().
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Fix typo in the commit message
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 47 +++++++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 45a6f9287f..86e1faeeb5 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1067,34 +1067,33 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
if ( !xen_pt_check_entry(*entry, mfn, flags) )
return -EINVAL;
- switch ( op ) {
- case INSERT:
- case RESERVE:
- if ( op == RESERVE )
- break;
+ /* If we are only populating page-table, then we are done. */
+ if ( flags & _PAGE_POPULATE )
+ return 0;
+
+ /* We are removing the page */
+ if ( !(flags & _PAGE_PRESENT) )
+ memset(&pte, 0x00, sizeof(pte));
+ else
+ {
+ /* We are inserting a mapping => Create new pte. */
+ if ( !mfn_eq(mfn, INVALID_MFN) )
+ {
pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- BUG_ON(!pte.pt.ro && !pte.pt.xn);
+
+ /* Third level entries set pte.pt.table = 1 */
pte.pt.table = 1;
- write_pte(entry, pte);
- break;
- case MODIFY:
- case REMOVE:
- if ( op == REMOVE )
- pte.bits = 0;
- else
- {
- pte = *entry;
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- }
- write_pte(entry, pte);
- break;
- default:
- BUG();
+ }
+ else /* We are updating the permission => Copy the current pte. */
+ pte = *entry;
+
+ /* Set permission */
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
}
+ write_pte(entry, pte);
+
return 0;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 07/12] xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
With the newly introduced flags, it is now possible to know how the page
will be updated through the flags.
All the use of xenmap_operation are now replaced with the flags. At the
same time, validity check are now removed as they are gathered in
xen_pt_check_entry().
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Fix typo in the commit message
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 47 +++++++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 45a6f9287f..86e1faeeb5 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1067,34 +1067,33 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
if ( !xen_pt_check_entry(*entry, mfn, flags) )
return -EINVAL;
- switch ( op ) {
- case INSERT:
- case RESERVE:
- if ( op == RESERVE )
- break;
+ /* If we are only populating page-table, then we are done. */
+ if ( flags & _PAGE_POPULATE )
+ return 0;
+
+ /* We are removing the page */
+ if ( !(flags & _PAGE_PRESENT) )
+ memset(&pte, 0x00, sizeof(pte));
+ else
+ {
+ /* We are inserting a mapping => Create new pte. */
+ if ( !mfn_eq(mfn, INVALID_MFN) )
+ {
pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- BUG_ON(!pte.pt.ro && !pte.pt.xn);
+
+ /* Third level entries set pte.pt.table = 1 */
pte.pt.table = 1;
- write_pte(entry, pte);
- break;
- case MODIFY:
- case REMOVE:
- if ( op == REMOVE )
- pte.bits = 0;
- else
- {
- pte = *entry;
- pte.pt.ro = PAGE_RO_MASK(flags);
- pte.pt.xn = PAGE_XN_MASK(flags);
- }
- write_pte(entry, pte);
- break;
- default:
- BUG();
+ }
+ else /* We are updating the permission => Copy the current pte. */
+ pte = *entry;
+
+ /* Set permission */
+ pte.pt.ro = PAGE_RO_MASK(flags);
+ pte.pt.xn = PAGE_XN_MASK(flags);
}
+ write_pte(entry, pte);
+
return 0;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 07/12] xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 22:22 ` Stefano Stabellini
-1 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 22:22 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> With the newly introduced flags, it is now possible to know how the page
> will be updated through the flags.
>
> All the use of xenmap_operation are now replaced with the flags. At the
> same time, validity check are now removed as they are gathered in
> xen_pt_check_entry().
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> Changes in v2:
> - Fix typo in the commit message
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 47 +++++++++++++++++++++++------------------------
> 1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 45a6f9287f..86e1faeeb5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1067,34 +1067,33 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> if ( !xen_pt_check_entry(*entry, mfn, flags) )
> return -EINVAL;
>
> - switch ( op ) {
> - case INSERT:
> - case RESERVE:
> - if ( op == RESERVE )
> - break;
> + /* If we are only populating page-table, then we are done. */
> + if ( flags & _PAGE_POPULATE )
> + return 0;
> +
> + /* We are removing the page */
> + if ( !(flags & _PAGE_PRESENT) )
> + memset(&pte, 0x00, sizeof(pte));
> + else
> + {
> + /* We are inserting a mapping => Create new pte. */
> + if ( !mfn_eq(mfn, INVALID_MFN) )
> + {
> pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> - pte.pt.ro = PAGE_RO_MASK(flags);
> - pte.pt.xn = PAGE_XN_MASK(flags);
> - BUG_ON(!pte.pt.ro && !pte.pt.xn);
> +
> + /* Third level entries set pte.pt.table = 1 */
> pte.pt.table = 1;
> - write_pte(entry, pte);
> - break;
> - case MODIFY:
> - case REMOVE:
> - if ( op == REMOVE )
> - pte.bits = 0;
> - else
> - {
> - pte = *entry;
> - pte.pt.ro = PAGE_RO_MASK(flags);
> - pte.pt.xn = PAGE_XN_MASK(flags);
> - }
> - write_pte(entry, pte);
> - break;
> - default:
> - BUG();
> + }
> + else /* We are updating the permission => Copy the current pte. */
> + pte = *entry;
> +
> + /* Set permission */
> + pte.pt.ro = PAGE_RO_MASK(flags);
> + pte.pt.xn = PAGE_XN_MASK(flags);
> }
>
> + write_pte(entry, pte);
> +
> return 0;
> }
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 08/12] xen/arm: mm: Remove enum xenmap_operation
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
The enum xenmap_operation is not used anymore. So remove it.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 86e1faeeb5..651e296041 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -974,13 +974,6 @@ static int create_xen_table(lpae_t *entry)
return 0;
}
-enum xenmap_operation {
- INSERT,
- REMOVE,
- MODIFY,
- RESERVE
-};
-
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1040,8 +1033,8 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
return true;
}
-static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
- mfn_t mfn, unsigned int flags)
+static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
+ unsigned int flags)
{
lpae_t pte, *entry;
lpae_t *third = NULL;
@@ -1099,8 +1092,7 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
static DEFINE_SPINLOCK(xen_pt_lock);
-static int xen_pt_update(enum xenmap_operation op,
- unsigned long virt,
+static int xen_pt_update(unsigned long virt,
mfn_t mfn,
unsigned long nr_mfns,
unsigned int flags)
@@ -1131,7 +1123,7 @@ static int xen_pt_update(enum xenmap_operation op,
for( ; addr < addr_end; addr += PAGE_SIZE )
{
- rc = xen_pt_update_entry(op, addr, mfn, flags);
+ rc = xen_pt_update_entry(addr, mfn, flags);
if ( rc )
break;
@@ -1156,24 +1148,24 @@ int map_pages_to_xen(unsigned long virt,
unsigned long nr_mfns,
unsigned int flags)
{
- return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
+ return xen_pt_update(virt, mfn, nr_mfns, flags);
}
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
+ return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
{
ASSERT(v <= e);
- return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+ return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
}
int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
{
ASSERT(s <= e);
- return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
+ return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 08/12] xen/arm: mm: Remove enum xenmap_operation
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
The enum xenmap_operation is not used anymore. So remove it.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 86e1faeeb5..651e296041 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -974,13 +974,6 @@ static int create_xen_table(lpae_t *entry)
return 0;
}
-enum xenmap_operation {
- INSERT,
- REMOVE,
- MODIFY,
- RESERVE
-};
-
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1040,8 +1033,8 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
return true;
}
-static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
- mfn_t mfn, unsigned int flags)
+static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
+ unsigned int flags)
{
lpae_t pte, *entry;
lpae_t *third = NULL;
@@ -1099,8 +1092,7 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
static DEFINE_SPINLOCK(xen_pt_lock);
-static int xen_pt_update(enum xenmap_operation op,
- unsigned long virt,
+static int xen_pt_update(unsigned long virt,
mfn_t mfn,
unsigned long nr_mfns,
unsigned int flags)
@@ -1131,7 +1123,7 @@ static int xen_pt_update(enum xenmap_operation op,
for( ; addr < addr_end; addr += PAGE_SIZE )
{
- rc = xen_pt_update_entry(op, addr, mfn, flags);
+ rc = xen_pt_update_entry(addr, mfn, flags);
if ( rc )
break;
@@ -1156,24 +1148,24 @@ int map_pages_to_xen(unsigned long virt,
unsigned long nr_mfns,
unsigned int flags)
{
- return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
+ return xen_pt_update(virt, mfn, nr_mfns, flags);
}
int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
{
- return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
+ return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
}
int destroy_xen_mappings(unsigned long v, unsigned long e)
{
ASSERT(v <= e);
- return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+ return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
}
int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
{
ASSERT(s <= e);
- return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
+ return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 08/12] xen/arm: mm: Remove enum xenmap_operation
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-11 22:38 ` Stefano Stabellini
-1 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-11 22:38 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> The enum xenmap_operation is not used anymore. So remove it.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 86e1faeeb5..651e296041 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -974,13 +974,6 @@ static int create_xen_table(lpae_t *entry)
> return 0;
> }
>
> -enum xenmap_operation {
> - INSERT,
> - REMOVE,
> - MODIFY,
> - RESERVE
> -};
> -
> /* Sanity check of the entry */
> static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> {
> @@ -1040,8 +1033,8 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> return true;
> }
>
> -static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
> - mfn_t mfn, unsigned int flags)
> +static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
> + unsigned int flags)
> {
> lpae_t pte, *entry;
> lpae_t *third = NULL;
> @@ -1099,8 +1092,7 @@ static int xen_pt_update_entry(enum xenmap_operation op, unsigned long addr,
>
> static DEFINE_SPINLOCK(xen_pt_lock);
>
> -static int xen_pt_update(enum xenmap_operation op,
> - unsigned long virt,
> +static int xen_pt_update(unsigned long virt,
> mfn_t mfn,
> unsigned long nr_mfns,
> unsigned int flags)
> @@ -1131,7 +1123,7 @@ static int xen_pt_update(enum xenmap_operation op,
>
> for( ; addr < addr_end; addr += PAGE_SIZE )
> {
> - rc = xen_pt_update_entry(op, addr, mfn, flags);
> + rc = xen_pt_update_entry(addr, mfn, flags);
> if ( rc )
> break;
>
> @@ -1156,24 +1148,24 @@ int map_pages_to_xen(unsigned long virt,
> unsigned long nr_mfns,
> unsigned int flags)
> {
> - return xen_pt_update(INSERT, virt, mfn, nr_mfns, flags);
> + return xen_pt_update(virt, mfn, nr_mfns, flags);
> }
>
> int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
> {
> - return xen_pt_update(RESERVE, virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> + return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> }
>
> int destroy_xen_mappings(unsigned long v, unsigned long e)
> {
> ASSERT(v <= e);
> - return xen_pt_update(REMOVE, v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> + return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> }
>
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> ASSERT(s <= e);
> - return xen_pt_update(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> + return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> }
>
> enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 09/12] xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, the virtual address of the 3rd level page-tables is obtained
using mfn_to_virt().
On Arm32, mfn_to_virt can only work on xenheap page. While in practice
all the page-tables updated will reside in xenheap, in practive the
page-tables covering Xen memory (e.g xen_mapping) is part of Xen binary.
Furthermore, a follow-up change will update xen_pt_update_entry() to
walk all the levels and therefore be more generic. Some of the
page-tables will also part of Xen memory and therefore will not be
reachable using mfn_to_virt().
The easiest way to reach those pages is to use {, un}map_domain_page().
While on arm32 this means an extra mapping in the normal cases, this is not
very important as xen page-tables are not updated often.
In order to allow future change in the way Xen page-tables are mapped,
two new helpers are introduced to map/unmap the page-tables.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 651e296041..f5979f549b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -974,6 +974,16 @@ static int create_xen_table(lpae_t *entry)
return 0;
}
+static lpae_t *xen_map_table(mfn_t mfn)
+{
+ return map_domain_page(mfn);
+}
+
+static void xen_unmap_table(const lpae_t *table)
+{
+ unmap_domain_page(table);
+}
+
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1036,6 +1046,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
unsigned int flags)
{
+ int rc;
lpae_t pte, *entry;
lpae_t *third = NULL;
@@ -1054,15 +1065,17 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
BUG_ON(!lpae_is_valid(*entry));
- third = mfn_to_virt(lpae_get_mfn(*entry));
+ third = xen_map_table(lpae_get_mfn(*entry));
entry = &third[third_table_offset(addr)];
+ rc = -EINVAL;
if ( !xen_pt_check_entry(*entry, mfn, flags) )
- return -EINVAL;
+ goto out;
/* If we are only populating page-table, then we are done. */
+ rc = 0;
if ( flags & _PAGE_POPULATE )
- return 0;
+ goto out;
/* We are removing the page */
if ( !(flags & _PAGE_PRESENT) )
@@ -1087,7 +1100,12 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
write_pte(entry, pte);
- return 0;
+ rc = 0;
+
+out:
+ xen_unmap_table(third);
+
+ return rc;
}
static DEFINE_SPINLOCK(xen_pt_lock);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 09/12] xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, the virtual address of the 3rd level page-tables is obtained
using mfn_to_virt().
On Arm32, mfn_to_virt can only work on xenheap page. While in practice
all the page-tables updated will reside in xenheap, in practive the
page-tables covering Xen memory (e.g xen_mapping) is part of Xen binary.
Furthermore, a follow-up change will update xen_pt_update_entry() to
walk all the levels and therefore be more generic. Some of the
page-tables will also part of Xen memory and therefore will not be
reachable using mfn_to_virt().
The easiest way to reach those pages is to use {, un}map_domain_page().
While on arm32 this means an extra mapping in the normal cases, this is not
very important as xen page-tables are not updated often.
In order to allow future change in the way Xen page-tables are mapped,
two new helpers are introduced to map/unmap the page-tables.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 651e296041..f5979f549b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -974,6 +974,16 @@ static int create_xen_table(lpae_t *entry)
return 0;
}
+static lpae_t *xen_map_table(mfn_t mfn)
+{
+ return map_domain_page(mfn);
+}
+
+static void xen_unmap_table(const lpae_t *table)
+{
+ unmap_domain_page(table);
+}
+
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1036,6 +1046,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
unsigned int flags)
{
+ int rc;
lpae_t pte, *entry;
lpae_t *third = NULL;
@@ -1054,15 +1065,17 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
BUG_ON(!lpae_is_valid(*entry));
- third = mfn_to_virt(lpae_get_mfn(*entry));
+ third = xen_map_table(lpae_get_mfn(*entry));
entry = &third[third_table_offset(addr)];
+ rc = -EINVAL;
if ( !xen_pt_check_entry(*entry, mfn, flags) )
- return -EINVAL;
+ goto out;
/* If we are only populating page-table, then we are done. */
+ rc = 0;
if ( flags & _PAGE_POPULATE )
- return 0;
+ goto out;
/* We are removing the page */
if ( !(flags & _PAGE_PRESENT) )
@@ -1087,7 +1100,12 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
write_pte(entry, pte);
- return 0;
+ rc = 0;
+
+out:
+ xen_unmap_table(third);
+
+ return rc;
}
static DEFINE_SPINLOCK(xen_pt_lock);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 09/12] xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 22:25 ` Stefano Stabellini
2019-06-13 8:07 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 22:25 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> Currently, the virtual address of the 3rd level page-tables is obtained
> using mfn_to_virt().
>
> On Arm32, mfn_to_virt can only work on xenheap page. While in practice
> all the page-tables updated will reside in xenheap, in practive the
^ in theory ?
> page-tables covering Xen memory (e.g xen_mapping) is part of Xen binary.
>
> Furthermore, a follow-up change will update xen_pt_update_entry() to
> walk all the levels and therefore be more generic. Some of the
> page-tables will also part of Xen memory and therefore will not be
> reachable using mfn_to_virt().
>
> The easiest way to reach those pages is to use {, un}map_domain_page().
> While on arm32 this means an extra mapping in the normal cases, this is not
> very important as xen page-tables are not updated often.
>
> In order to allow future change in the way Xen page-tables are mapped,
> two new helpers are introduced to map/unmap the page-tables.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
aside from the typo above:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 651e296041..f5979f549b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -974,6 +974,16 @@ static int create_xen_table(lpae_t *entry)
> return 0;
> }
>
> +static lpae_t *xen_map_table(mfn_t mfn)
> +{
> + return map_domain_page(mfn);
> +}
> +
> +static void xen_unmap_table(const lpae_t *table)
> +{
> + unmap_domain_page(table);
> +}
> +
> /* Sanity check of the entry */
> static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> {
> @@ -1036,6 +1046,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
> unsigned int flags)
> {
> + int rc;
> lpae_t pte, *entry;
> lpae_t *third = NULL;
>
> @@ -1054,15 +1065,17 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
>
> BUG_ON(!lpae_is_valid(*entry));
>
> - third = mfn_to_virt(lpae_get_mfn(*entry));
> + third = xen_map_table(lpae_get_mfn(*entry));
> entry = &third[third_table_offset(addr)];
>
> + rc = -EINVAL;
> if ( !xen_pt_check_entry(*entry, mfn, flags) )
> - return -EINVAL;
> + goto out;
>
> /* If we are only populating page-table, then we are done. */
> + rc = 0;
> if ( flags & _PAGE_POPULATE )
> - return 0;
> + goto out;
>
> /* We are removing the page */
> if ( !(flags & _PAGE_PRESENT) )
> @@ -1087,7 +1100,12 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
>
> write_pte(entry, pte);
>
> - return 0;
> + rc = 0;
> +
> +out:
> + xen_unmap_table(third);
> +
> + return rc;
> }
>
> static DEFINE_SPINLOCK(xen_pt_lock);
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 09/12] xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
2019-06-12 22:25 ` Stefano Stabellini
@ 2019-06-13 8:07 ` Julien Grall
2019-06-13 17:55 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 8:07 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 6/12/19 11:25 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> Currently, the virtual address of the 3rd level page-tables is obtained
>> using mfn_to_virt().
>>
>> On Arm32, mfn_to_virt can only work on xenheap page. While in practice
>> all the page-tables updated will reside in xenheap, in practive the
> ^ in theory ?
The first one should to be "theory" and the second "practice". Because
some of the bootstrap page-tables (e.g xen_fixmap/xen_mapping) are part
of Xen binary.
>
>
>> page-tables covering Xen memory (e.g xen_mapping) is part of Xen binary.
>>
>> Furthermore, a follow-up change will update xen_pt_update_entry() to
>> walk all the levels and therefore be more generic. Some of the
>> page-tables will also part of Xen memory and therefore will not be
>> reachable using mfn_to_virt().
>>
>> The easiest way to reach those pages is to use {, un}map_domain_page().
>> While on arm32 this means an extra mapping in the normal cases, this is not
>> very important as xen page-tables are not updated often.
>>
>> In order to allow future change in the way Xen page-tables are mapped,
>> two new helpers are introduced to map/unmap the page-tables.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> aside from the typo above:
Let me know if my suggestion makes sense above.
>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Thank you.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 09/12] xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
2019-06-13 8:07 ` Julien Grall
@ 2019-06-13 17:55 ` Stefano Stabellini
0 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 17:55 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 6/12/19 11:25 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > Currently, the virtual address of the 3rd level page-tables is obtained
> > > using mfn_to_virt().
> > >
> > > On Arm32, mfn_to_virt can only work on xenheap page. While in practice
> > > all the page-tables updated will reside in xenheap, in practive the
> > ^ in theory ?
>
> The first one should to be "theory" and the second "practice". Because some of
> the bootstrap page-tables (e.g xen_fixmap/xen_mapping) are part of Xen binary.
>
> >
> >
> > > page-tables covering Xen memory (e.g xen_mapping) is part of Xen binary.
> > >
> > > Furthermore, a follow-up change will update xen_pt_update_entry() to
> > > walk all the levels and therefore be more generic. Some of the
> > > page-tables will also part of Xen memory and therefore will not be
> > > reachable using mfn_to_virt().
> > >
> > > The easiest way to reach those pages is to use {, un}map_domain_page().
> > > While on arm32 this means an extra mapping in the normal cases, this is
> > > not
> > > very important as xen page-tables are not updated often.
> > >
> > > In order to allow future change in the way Xen page-tables are mapped,
> > > two new helpers are introduced to map/unmap the page-tables.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> >
> > aside from the typo above:
>
> Let me know if my suggestion makes sense above.
Yes, fine
> > Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>
> Thank you.
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, xen_pt_update_entry() is only able to update the region covered
by xen_second (i.e 0 to 0x7fffffff).
Because of the restriction we end to have multiple functions in mm.c
modifying the page-tables differently.
Furthermore, we never walked the page-tables fully. This means that any
change in the layout may requires major rewrite of the page-tables code.
Lastly, we have been quite lucky that no one ever tried to pass an address
outside this range because it would have blown-up.
xen_pt_update_entry() is reworked to walk over the page-tables every
time. The logic has been borrowed from arch/arm/p2m.c and contain some
limitations for the time being:
- Superpage cannot be shattered
- Only level 3 (i.e 4KB) can be done
Note that the parameter 'addr' has been renamed to 'virt' to make clear
we are dealing with a virtual address.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 106 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f5979f549b..9a40754f44 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
unmap_domain_page(table);
}
+#define XEN_TABLE_MAP_FAILED 0
+#define XEN_TABLE_SUPER_PAGE 1
+#define XEN_TABLE_NORMAL_PAGE 2
+
+/*
+ * Take the currently mapped table, find the corresponding entry,
+ * and map the next table, if available.
+ *
+ * The read_only parameters indicates whether intermediate tables should
+ * be allocated when not present.
+ *
+ * Return values:
+ * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
+ * was empty, or allocating a new page failed.
+ * XEN_TABLE_NORMAL_PAGE: next level mapped normally
+ * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
+ */
+static int xen_pt_next_level(bool read_only, unsigned int level,
+ lpae_t **table, unsigned int offset)
+{
+ lpae_t *entry;
+ int ret;
+
+ entry = *table + offset;
+
+ if ( !lpae_is_valid(*entry) )
+ {
+ if ( read_only )
+ return XEN_TABLE_MAP_FAILED;
+
+ ret = create_xen_table(entry);
+ if ( ret )
+ return XEN_TABLE_MAP_FAILED;
+ }
+
+ ASSERT(lpae_is_valid(*entry));
+
+ /* The function xen_pt_next_level is never called at the 3rd level */
+ if ( lpae_is_mapping(*entry, level) )
+ return XEN_TABLE_SUPER_PAGE;
+
+ xen_unmap_table(*table);
+ *table = xen_map_table(lpae_get_mfn(*entry));
+
+ return XEN_TABLE_NORMAL_PAGE;
+}
+
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1043,30 +1090,65 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
return true;
}
-static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
- unsigned int flags)
+static int xen_pt_update_entry(mfn_t root, unsigned long virt,
+ mfn_t mfn, unsigned int flags)
{
int rc;
+ unsigned int level;
+ /* We only support 4KB mapping (i.e level 3) for now */
+ unsigned int target = 3;
+ lpae_t *table;
+ /*
+ * The intermediate page tables are read-only when the MFN is not valid
+ * and we are not populating page table.
+ * This means we either modify permissions or remove an entry.
+ */
+ bool read_only = mfn_eq(mfn, INVALID_MFN) && !(flags & _PAGE_POPULATE);
lpae_t pte, *entry;
- lpae_t *third = NULL;
+
+ /* convenience aliases */
+ DECLARE_OFFSETS(offsets, (paddr_t)virt);
/* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
- entry = &xen_second[second_linear_offset(addr)];
- if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
+ table = xen_map_table(root);
+ for ( level = HYP_PT_ROOT_LEVEL; level < target; level++ )
{
- int rc = create_xen_table(entry);
- if ( rc < 0 ) {
- printk("%s: L2 failed\n", __func__);
- return rc;
+ rc = xen_pt_next_level(read_only, level, &table, offsets[level]);
+ if ( rc == XEN_TABLE_MAP_FAILED )
+ {
+ /*
+ * We are here because xen_pt_next_level has failed to map
+ * the intermediate page table (e.g the table does not exist
+ * and the pt is read-only). It is a valid case when
+ * removing a mapping as it may not exist in the page table.
+ * In this case, just ignore it.
+ */
+ if ( flags & (_PAGE_PRESENT|_PAGE_POPULATE) )
+ {
+ mm_printk("%s: Unable to map level %u\n", __func__, level);
+ rc = -ENOENT;
+ goto out;
+ }
+ else
+ {
+ rc = 0;
+ goto out;
+ }
}
+ else if ( rc != XEN_TABLE_NORMAL_PAGE )
+ break;
}
- BUG_ON(!lpae_is_valid(*entry));
+ if ( level != target )
+ {
+ mm_printk("%s: Shattering superpage is not supported\n", __func__);
+ rc = -EOPNOTSUPP;
+ goto out;
+ }
- third = xen_map_table(lpae_get_mfn(*entry));
- entry = &third[third_table_offset(addr)];
+ entry = table + offsets[level];
rc = -EINVAL;
if ( !xen_pt_check_entry(*entry, mfn, flags) )
@@ -1103,7 +1185,7 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
rc = 0;
out:
- xen_unmap_table(third);
+ xen_unmap_table(table);
return rc;
}
@@ -1119,6 +1201,15 @@ static int xen_pt_update(unsigned long virt,
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
/*
+ * For arm32, page-tables are different on each CPUs. Yet, they share
+ * some common mappings. It is assumed that only common mappings
+ * will be modified with this function.
+ *
+ * XXX: Add a check.
+ */
+ const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
+
+ /*
* The hardware was configured to forbid mapping both writeable and
* executable.
* When modifying/creating mapping (i.e _PAGE_PRESENT is set),
@@ -1139,9 +1230,9 @@ static int xen_pt_update(unsigned long virt,
spin_lock(&xen_pt_lock);
- for( ; addr < addr_end; addr += PAGE_SIZE )
+ for ( ; addr < addr_end; addr += PAGE_SIZE )
{
- rc = xen_pt_update_entry(addr, mfn, flags);
+ rc = xen_pt_update_entry(root, addr, mfn, flags);
if ( rc )
break;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
Currently, xen_pt_update_entry() is only able to update the region covered
by xen_second (i.e 0 to 0x7fffffff).
Because of the restriction we end to have multiple functions in mm.c
modifying the page-tables differently.
Furthermore, we never walked the page-tables fully. This means that any
change in the layout may requires major rewrite of the page-tables code.
Lastly, we have been quite lucky that no one ever tried to pass an address
outside this range because it would have blown-up.
xen_pt_update_entry() is reworked to walk over the page-tables every
time. The logic has been borrowed from arch/arm/p2m.c and contain some
limitations for the time being:
- Superpage cannot be shattered
- Only level 3 (i.e 4KB) can be done
Note that the parameter 'addr' has been renamed to 'virt' to make clear
we are dealing with a virtual address.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 106 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f5979f549b..9a40754f44 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
unmap_domain_page(table);
}
+#define XEN_TABLE_MAP_FAILED 0
+#define XEN_TABLE_SUPER_PAGE 1
+#define XEN_TABLE_NORMAL_PAGE 2
+
+/*
+ * Take the currently mapped table, find the corresponding entry,
+ * and map the next table, if available.
+ *
+ * The read_only parameters indicates whether intermediate tables should
+ * be allocated when not present.
+ *
+ * Return values:
+ * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
+ * was empty, or allocating a new page failed.
+ * XEN_TABLE_NORMAL_PAGE: next level mapped normally
+ * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
+ */
+static int xen_pt_next_level(bool read_only, unsigned int level,
+ lpae_t **table, unsigned int offset)
+{
+ lpae_t *entry;
+ int ret;
+
+ entry = *table + offset;
+
+ if ( !lpae_is_valid(*entry) )
+ {
+ if ( read_only )
+ return XEN_TABLE_MAP_FAILED;
+
+ ret = create_xen_table(entry);
+ if ( ret )
+ return XEN_TABLE_MAP_FAILED;
+ }
+
+ ASSERT(lpae_is_valid(*entry));
+
+ /* The function xen_pt_next_level is never called at the 3rd level */
+ if ( lpae_is_mapping(*entry, level) )
+ return XEN_TABLE_SUPER_PAGE;
+
+ xen_unmap_table(*table);
+ *table = xen_map_table(lpae_get_mfn(*entry));
+
+ return XEN_TABLE_NORMAL_PAGE;
+}
+
/* Sanity check of the entry */
static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
{
@@ -1043,30 +1090,65 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
return true;
}
-static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
- unsigned int flags)
+static int xen_pt_update_entry(mfn_t root, unsigned long virt,
+ mfn_t mfn, unsigned int flags)
{
int rc;
+ unsigned int level;
+ /* We only support 4KB mapping (i.e level 3) for now */
+ unsigned int target = 3;
+ lpae_t *table;
+ /*
+ * The intermediate page tables are read-only when the MFN is not valid
+ * and we are not populating page table.
+ * This means we either modify permissions or remove an entry.
+ */
+ bool read_only = mfn_eq(mfn, INVALID_MFN) && !(flags & _PAGE_POPULATE);
lpae_t pte, *entry;
- lpae_t *third = NULL;
+
+ /* convenience aliases */
+ DECLARE_OFFSETS(offsets, (paddr_t)virt);
/* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
- entry = &xen_second[second_linear_offset(addr)];
- if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
+ table = xen_map_table(root);
+ for ( level = HYP_PT_ROOT_LEVEL; level < target; level++ )
{
- int rc = create_xen_table(entry);
- if ( rc < 0 ) {
- printk("%s: L2 failed\n", __func__);
- return rc;
+ rc = xen_pt_next_level(read_only, level, &table, offsets[level]);
+ if ( rc == XEN_TABLE_MAP_FAILED )
+ {
+ /*
+ * We are here because xen_pt_next_level has failed to map
+ * the intermediate page table (e.g the table does not exist
+ * and the pt is read-only). It is a valid case when
+ * removing a mapping as it may not exist in the page table.
+ * In this case, just ignore it.
+ */
+ if ( flags & (_PAGE_PRESENT|_PAGE_POPULATE) )
+ {
+ mm_printk("%s: Unable to map level %u\n", __func__, level);
+ rc = -ENOENT;
+ goto out;
+ }
+ else
+ {
+ rc = 0;
+ goto out;
+ }
}
+ else if ( rc != XEN_TABLE_NORMAL_PAGE )
+ break;
}
- BUG_ON(!lpae_is_valid(*entry));
+ if ( level != target )
+ {
+ mm_printk("%s: Shattering superpage is not supported\n", __func__);
+ rc = -EOPNOTSUPP;
+ goto out;
+ }
- third = xen_map_table(lpae_get_mfn(*entry));
- entry = &third[third_table_offset(addr)];
+ entry = table + offsets[level];
rc = -EINVAL;
if ( !xen_pt_check_entry(*entry, mfn, flags) )
@@ -1103,7 +1185,7 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
rc = 0;
out:
- xen_unmap_table(third);
+ xen_unmap_table(table);
return rc;
}
@@ -1119,6 +1201,15 @@ static int xen_pt_update(unsigned long virt,
unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
/*
+ * For arm32, page-tables are different on each CPUs. Yet, they share
+ * some common mappings. It is assumed that only common mappings
+ * will be modified with this function.
+ *
+ * XXX: Add a check.
+ */
+ const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
+
+ /*
* The hardware was configured to forbid mapping both writeable and
* executable.
* When modifying/creating mapping (i.e _PAGE_PRESENT is set),
@@ -1139,9 +1230,9 @@ static int xen_pt_update(unsigned long virt,
spin_lock(&xen_pt_lock);
- for( ; addr < addr_end; addr += PAGE_SIZE )
+ for ( ; addr < addr_end; addr += PAGE_SIZE )
{
- rc = xen_pt_update_entry(addr, mfn, flags);
+ rc = xen_pt_update_entry(root, addr, mfn, flags);
if ( rc )
break;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 22:52 ` Stefano Stabellini
2019-06-13 8:20 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 22:52 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> Currently, xen_pt_update_entry() is only able to update the region covered
> by xen_second (i.e 0 to 0x7fffffff).
>
> Because of the restriction we end to have multiple functions in mm.c
> modifying the page-tables differently.
>
> Furthermore, we never walked the page-tables fully. This means that any
> change in the layout may requires major rewrite of the page-tables code.
>
> Lastly, we have been quite lucky that no one ever tried to pass an address
> outside this range because it would have blown-up.
>
> xen_pt_update_entry() is reworked to walk over the page-tables every
> time. The logic has been borrowed from arch/arm/p2m.c and contain some
> limitations for the time being:
> - Superpage cannot be shattered
> - Only level 3 (i.e 4KB) can be done
>
> Note that the parameter 'addr' has been renamed to 'virt' to make clear
> we are dealing with a virtual address.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 106 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index f5979f549b..9a40754f44 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
> unmap_domain_page(table);
> }
>
> +#define XEN_TABLE_MAP_FAILED 0
> +#define XEN_TABLE_SUPER_PAGE 1
> +#define XEN_TABLE_NORMAL_PAGE 2
Minor NIT: do we want to have XEN_TABLE_MAP_FAILED be -1 to follow the
pattern that errors are < 0 ? Not important though.
> +/*
> + * Take the currently mapped table, find the corresponding entry,
> + * and map the next table, if available.
> + *
> + * The read_only parameters indicates whether intermediate tables should
> + * be allocated when not present.
I wonder if it would be a good idea to rename read_only to something
more obviously connected to the idea that tables get created. Maybe
create_missing? It would have to match the variable and comment added
below in xen_pt_update_entry. I don't have a strong opinion on this.
> + * Return values:
> + * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
> + * was empty, or allocating a new page failed.
> + * XEN_TABLE_NORMAL_PAGE: next level mapped normally
> + * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
> + */
> +static int xen_pt_next_level(bool read_only, unsigned int level,
> + lpae_t **table, unsigned int offset)
> +{
> + lpae_t *entry;
> + int ret;
> +
> + entry = *table + offset;
> +
> + if ( !lpae_is_valid(*entry) )
> + {
> + if ( read_only )
> + return XEN_TABLE_MAP_FAILED;
> +
> + ret = create_xen_table(entry);
> + if ( ret )
> + return XEN_TABLE_MAP_FAILED;
> + }
> +
> + ASSERT(lpae_is_valid(*entry));
Why the ASSERT just after the lpae_is_valid check above?
> + /* The function xen_pt_next_level is never called at the 3rd level */
> + if ( lpae_is_mapping(*entry, level) )
> + return XEN_TABLE_SUPER_PAGE;
> +
> + xen_unmap_table(*table);
> + *table = xen_map_table(lpae_get_mfn(*entry));
> +
> + return XEN_TABLE_NORMAL_PAGE;
> +}
> +
> /* Sanity check of the entry */
> static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> {
> @@ -1043,30 +1090,65 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> return true;
> }
>
> -static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
> - unsigned int flags)
> +static int xen_pt_update_entry(mfn_t root, unsigned long virt,
> + mfn_t mfn, unsigned int flags)
> {
> int rc;
> + unsigned int level;
> + /* We only support 4KB mapping (i.e level 3) for now */
> + unsigned int target = 3;
> + lpae_t *table;
> + /*
> + * The intermediate page tables are read-only when the MFN is not valid
> + * and we are not populating page table.
> + * This means we either modify permissions or remove an entry.
> + */
> + bool read_only = mfn_eq(mfn, INVALID_MFN) && !(flags & _PAGE_POPULATE);
> lpae_t pte, *entry;
> - lpae_t *third = NULL;
> +
> + /* convenience aliases */
> + DECLARE_OFFSETS(offsets, (paddr_t)virt);
>
> /* _PAGE_POPULATE and _PAGE_PRESENT should never be set together. */
> ASSERT((flags & (_PAGE_POPULATE|_PAGE_PRESENT)) != (_PAGE_POPULATE|_PAGE_PRESENT));
>
> - entry = &xen_second[second_linear_offset(addr)];
> - if ( !lpae_is_valid(*entry) || !lpae_is_table(*entry, 2) )
> + table = xen_map_table(root);
> + for ( level = HYP_PT_ROOT_LEVEL; level < target; level++ )
> {
> - int rc = create_xen_table(entry);
> - if ( rc < 0 ) {
> - printk("%s: L2 failed\n", __func__);
> - return rc;
> + rc = xen_pt_next_level(read_only, level, &table, offsets[level]);
> + if ( rc == XEN_TABLE_MAP_FAILED )
> + {
> + /*
> + * We are here because xen_pt_next_level has failed to map
> + * the intermediate page table (e.g the table does not exist
> + * and the pt is read-only). It is a valid case when
> + * removing a mapping as it may not exist in the page table.
> + * In this case, just ignore it.
> + */
> + if ( flags & (_PAGE_PRESENT|_PAGE_POPULATE) )
> + {
> + mm_printk("%s: Unable to map level %u\n", __func__, level);
> + rc = -ENOENT;
> + goto out;
> + }
> + else
> + {
> + rc = 0;
> + goto out;
> + }
> }
> + else if ( rc != XEN_TABLE_NORMAL_PAGE )
> + break;
> }
>
> - BUG_ON(!lpae_is_valid(*entry));
> + if ( level != target )
> + {
> + mm_printk("%s: Shattering superpage is not supported\n", __func__);
> + rc = -EOPNOTSUPP;
> + goto out;
> + }
>
> - third = xen_map_table(lpae_get_mfn(*entry));
> - entry = &third[third_table_offset(addr)];
> + entry = table + offsets[level];
>
> rc = -EINVAL;
> if ( !xen_pt_check_entry(*entry, mfn, flags) )
> @@ -1103,7 +1185,7 @@ static int xen_pt_update_entry(unsigned long addr, mfn_t mfn,
> rc = 0;
>
> out:
> - xen_unmap_table(third);
> + xen_unmap_table(table);
>
> return rc;
> }
> @@ -1119,6 +1201,15 @@ static int xen_pt_update(unsigned long virt,
> unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
>
> /*
> + * For arm32, page-tables are different on each CPUs. Yet, they share
> + * some common mappings. It is assumed that only common mappings
> + * will be modified with this function.
> + *
> + * XXX: Add a check.
> + */
> + const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
> +
> + /*
> * The hardware was configured to forbid mapping both writeable and
> * executable.
> * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
> @@ -1139,9 +1230,9 @@ static int xen_pt_update(unsigned long virt,
>
> spin_lock(&xen_pt_lock);
>
> - for( ; addr < addr_end; addr += PAGE_SIZE )
> + for ( ; addr < addr_end; addr += PAGE_SIZE )
> {
> - rc = xen_pt_update_entry(addr, mfn, flags);
> + rc = xen_pt_update_entry(root, addr, mfn, flags);
> if ( rc )
> break;
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
2019-06-12 22:52 ` Stefano Stabellini
@ 2019-06-13 8:20 ` Julien Grall
2019-06-13 17:59 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 8:20 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 6/12/19 11:52 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> Currently, xen_pt_update_entry() is only able to update the region covered
>> by xen_second (i.e 0 to 0x7fffffff).
>>
>> Because of the restriction we end to have multiple functions in mm.c
>> modifying the page-tables differently.
>>
>> Furthermore, we never walked the page-tables fully. This means that any
>> change in the layout may requires major rewrite of the page-tables code.
>>
>> Lastly, we have been quite lucky that no one ever tried to pass an address
>> outside this range because it would have blown-up.
>>
>> xen_pt_update_entry() is reworked to walk over the page-tables every
>> time. The logic has been borrowed from arch/arm/p2m.c and contain some
>> limitations for the time being:
>> - Superpage cannot be shattered
>> - Only level 3 (i.e 4KB) can be done
>>
>> Note that the parameter 'addr' has been renamed to 'virt' to make clear
>> we are dealing with a virtual address.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>> Changes in v2:
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/mm.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 106 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index f5979f549b..9a40754f44 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
>> unmap_domain_page(table);
>> }
>>
>> +#define XEN_TABLE_MAP_FAILED 0
>> +#define XEN_TABLE_SUPER_PAGE 1
>> +#define XEN_TABLE_NORMAL_PAGE 2
>
> Minor NIT: do we want to have XEN_TABLE_MAP_FAILED be -1 to follow the
> pattern that errors are < 0 ? Not important though.
The value of XEN_TABLE_* here does not matter, you can see it as an
open-coded enum. This was borrowed from arm/p2m.c (which was based on
x86/mm/p2m-pt.c).
For the time being, I would prefer to keep it as is because it makes
easier to spot the difference with the p2m code. I can consider
switching the two to enum afterwards.
>
>
>> +/*
>> + * Take the currently mapped table, find the corresponding entry,
>> + * and map the next table, if available.
>> + *
>> + * The read_only parameters indicates whether intermediate tables should
>> + * be allocated when not present.
>
> I wonder if it would be a good idea to rename read_only to something
> more obviously connected to the idea that tables get created. Maybe
> create_missing? It would have to match the variable and comment added
> below in xen_pt_update_entry. I don't have a strong opinion on this.
Same as above here, the comment is a replicate of p2m.c
>
>
>> + * Return values:
>> + * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
>> + * was empty, or allocating a new page failed.
>> + * XEN_TABLE_NORMAL_PAGE: next level mapped normally
>> + * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
>> + */
>> +static int xen_pt_next_level(bool read_only, unsigned int level,
>> + lpae_t **table, unsigned int offset)
>> +{
>> + lpae_t *entry;
>> + int ret;
>> +
>> + entry = *table + offset;
>> +
>> + if ( !lpae_is_valid(*entry) )
>> + {
>> + if ( read_only )
>> + return XEN_TABLE_MAP_FAILED;
>> +
>> + ret = create_xen_table(entry);
>> + if ( ret )
>> + return XEN_TABLE_MAP_FAILED;
>> + }
>> +
>> + ASSERT(lpae_is_valid(*entry));
>
> Why the ASSERT just after the lpae_is_valid check above?
When the entry is invalid, the new page table will be allocated and the
entry will be generated. The rest of the function will then be executed.
The ASSERT() here confirms the entry we have in hand is valid in all the
cases.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
2019-06-13 8:20 ` Julien Grall
@ 2019-06-13 17:59 ` Stefano Stabellini
2019-06-13 21:32 ` Julien Grall
0 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 17:59 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 6/12/19 11:52 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > Currently, xen_pt_update_entry() is only able to update the region covered
> > > by xen_second (i.e 0 to 0x7fffffff).
> > >
> > > Because of the restriction we end to have multiple functions in mm.c
> > > modifying the page-tables differently.
> > >
> > > Furthermore, we never walked the page-tables fully. This means that any
> > > change in the layout may requires major rewrite of the page-tables code.
> > >
> > > Lastly, we have been quite lucky that no one ever tried to pass an address
> > > outside this range because it would have blown-up.
> > >
> > > xen_pt_update_entry() is reworked to walk over the page-tables every
> > > time. The logic has been borrowed from arch/arm/p2m.c and contain some
> > > limitations for the time being:
> > > - Superpage cannot be shattered
> > > - Only level 3 (i.e 4KB) can be done
> > >
> > > Note that the parameter 'addr' has been renamed to 'virt' to make clear
> > > we are dealing with a virtual address.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > >
> > > ---
> > > Changes in v2:
> > > - Add Andrii's reviewed-by
> > > ---
> > > xen/arch/arm/mm.c | 121
> > > +++++++++++++++++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 106 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index f5979f549b..9a40754f44 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -984,6 +984,53 @@ static void xen_unmap_table(const lpae_t *table)
> > > unmap_domain_page(table);
> > > }
> > > +#define XEN_TABLE_MAP_FAILED 0
> > > +#define XEN_TABLE_SUPER_PAGE 1
> > > +#define XEN_TABLE_NORMAL_PAGE 2
> >
> > Minor NIT: do we want to have XEN_TABLE_MAP_FAILED be -1 to follow the
> > pattern that errors are < 0 ? Not important though.
>
> The value of XEN_TABLE_* here does not matter, you can see it as an open-coded
> enum. This was borrowed from arm/p2m.c (which was based on x86/mm/p2m-pt.c).
>
> For the time being, I would prefer to keep it as is because it makes easier to
> spot the difference with the p2m code. I can consider switching the two to
> enum afterwards.
OK
> >
> > > +/*
> > > + * Take the currently mapped table, find the corresponding entry,
> > > + * and map the next table, if available.
> > > + *
> > > + * The read_only parameters indicates whether intermediate tables should
> > > + * be allocated when not present.
> >
> > I wonder if it would be a good idea to rename read_only to something
> > more obviously connected to the idea that tables get created. Maybe
> > create_missing? It would have to match the variable and comment added
> > below in xen_pt_update_entry. I don't have a strong opinion on this.
>
> Same as above here, the comment is a replicate of p2m.c
OK
> > > + * Return values:
> > > + * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
> > > + * was empty, or allocating a new page failed.
> > > + * XEN_TABLE_NORMAL_PAGE: next level mapped normally
> > > + * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
> > > + */
> > > +static int xen_pt_next_level(bool read_only, unsigned int level,
> > > + lpae_t **table, unsigned int offset)
> > > +{
> > > + lpae_t *entry;
> > > + int ret;
> > > +
> > > + entry = *table + offset;
> > > +
> > > + if ( !lpae_is_valid(*entry) )
> > > + {
> > > + if ( read_only )
> > > + return XEN_TABLE_MAP_FAILED;
> > > +
> > > + ret = create_xen_table(entry);
> > > + if ( ret )
> > > + return XEN_TABLE_MAP_FAILED;
> > > + }
> > > +
> > > + ASSERT(lpae_is_valid(*entry));
> >
> > Why the ASSERT just after the lpae_is_valid check above?
>
> When the entry is invalid, the new page table will be allocated and the entry
> will be generated. The rest of the function will then be executed. The
> ASSERT() here confirms the entry we have in hand is valid in all the cases.
So it's to double-check that after getting into the `if' statement, the
entry becomes valid, which is kind of redundant due to the two errors
check above but it is still valid. OK.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
2019-06-13 17:59 ` Stefano Stabellini
@ 2019-06-13 21:32 ` Julien Grall
2019-06-13 22:57 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 21:32 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 13/06/2019 18:59, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote: >>>> + * Return values:
>>>> + * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
>>>> + * was empty, or allocating a new page failed.
>>>> + * XEN_TABLE_NORMAL_PAGE: next level mapped normally
>>>> + * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
>>>> + */
>>>> +static int xen_pt_next_level(bool read_only, unsigned int level,
>>>> + lpae_t **table, unsigned int offset)
>>>> +{
>>>> + lpae_t *entry;
>>>> + int ret;
>>>> +
>>>> + entry = *table + offset;
>>>> +
>>>> + if ( !lpae_is_valid(*entry) )
>>>> + {
>>>> + if ( read_only )
>>>> + return XEN_TABLE_MAP_FAILED;
>>>> +
>>>> + ret = create_xen_table(entry);
>>>> + if ( ret )
>>>> + return XEN_TABLE_MAP_FAILED;
>>>> + }
>>>> +
>>>> + ASSERT(lpae_is_valid(*entry));
>>>
>>> Why the ASSERT just after the lpae_is_valid check above?
>>
>> When the entry is invalid, the new page table will be allocated and the entry
>> will be generated. The rest of the function will then be executed. The
>> ASSERT() here confirms the entry we have in hand is valid in all the cases.
>
> So it's to double-check that after getting into the `if' statement, the
> entry becomes valid, which is kind of redundant due to the two errors
> check above but it is still valid. OK.
While I agree that we have 2 ifs above, we only check "rc" and not "entry".
I ought to think I wrote perfect code, sadly this is not always the case ;).
Here, it would catch any mistake if "rc" is zero but "entry" is still
invalid. The risk here is the "entry" would be invalid but the mistake
may be spotted a long time after (i.e any access to the mapping will
fault). This would potentially cost a lot of debug.
I agree this is probably over cautious, I can't remember if I hit the
problem before. Anyway, I am happy to drop the ASSERT() if you think it
is too redundant.
Regardless that, are you happy with the rest of the patch? If so, can I
get your acked-by/reviewed-by?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update
2019-06-13 21:32 ` Julien Grall
@ 2019-06-13 22:57 ` Stefano Stabellini
0 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 22:57 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, nd, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 13/06/2019 18:59, Stefano Stabellini wrote:
> > On Thu, 13 Jun 2019, Julien Grall wrote: >>>> + * Return values:
> >>>> + * XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
> >>>> + * was empty, or allocating a new page failed.
> >>>> + * XEN_TABLE_NORMAL_PAGE: next level mapped normally
> >>>> + * XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
> >>>> + */
> >>>> +static int xen_pt_next_level(bool read_only, unsigned int level,
> >>>> + lpae_t **table, unsigned int offset)
> >>>> +{
> >>>> + lpae_t *entry;
> >>>> + int ret;
> >>>> +
> >>>> + entry = *table + offset;
> >>>> +
> >>>> + if ( !lpae_is_valid(*entry) )
> >>>> + {
> >>>> + if ( read_only )
> >>>> + return XEN_TABLE_MAP_FAILED;
> >>>> +
> >>>> + ret = create_xen_table(entry);
> >>>> + if ( ret )
> >>>> + return XEN_TABLE_MAP_FAILED;
> >>>> + }
> >>>> +
> >>>> + ASSERT(lpae_is_valid(*entry));
> >>>
> >>> Why the ASSERT just after the lpae_is_valid check above?
> >>
> >> When the entry is invalid, the new page table will be allocated and the entry
> >> will be generated. The rest of the function will then be executed. The
> >> ASSERT() here confirms the entry we have in hand is valid in all the cases.
> >
> > So it's to double-check that after getting into the `if' statement, the
> > entry becomes valid, which is kind of redundant due to the two errors
> > check above but it is still valid. OK.
>
> While I agree that we have 2 ifs above, we only check "rc" and not "entry".
>
> I ought to think I wrote perfect code, sadly this is not always the case ;).
>
> Here, it would catch any mistake if "rc" is zero but "entry" is still
> invalid. The risk here is the "entry" would be invalid but the mistake
> may be spotted a long time after (i.e any access to the mapping will
> fault). This would potentially cost a lot of debug.
>
> I agree this is probably over cautious, I can't remember if I hit the
> problem before. Anyway, I am happy to drop the ASSERT() if you think it
> is too redundant.
I would drop it, but I don't care much about it.
> Regardless that, are you happy with the rest of the patch? If so, can I
> get your acked-by/reviewed-by?
Yes, either way
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
{set, clear}_fixmap() are currently open-coding update to the Xen
page-tables. This can be avoided by using the generic helpers
map_pages_to_xen() and destroy_xen_mappings().
Both function are not meant to fail for fixmap, hence the BUG_ON()
checking the return.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9a40754f44..23ca61e8f0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -348,19 +348,19 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
/* Map a 4k page in a fixmap entry */
void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
{
- lpae_t pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.table = 1; /* 4k mappings always have this bit set */
- pte.pt.xn = 1;
- write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
- flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+ int res;
+
+ res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags);
+ BUG_ON(res != 0);
}
/* Remove a mapping from a fixmap entry */
void clear_fixmap(unsigned map)
{
- lpae_t pte = {0};
- write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
- flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+ int res;
+
+ res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
+ BUG_ON(res != 0);
}
/* Create Xen's mappings of memory.
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
{set, clear}_fixmap() are currently open-coding update to the Xen
page-tables. This can be avoided by using the generic helpers
map_pages_to_xen() and destroy_xen_mappings().
Both function are not meant to fail for fixmap, hence the BUG_ON()
checking the return.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9a40754f44..23ca61e8f0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -348,19 +348,19 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
/* Map a 4k page in a fixmap entry */
void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
{
- lpae_t pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
- pte.pt.table = 1; /* 4k mappings always have this bit set */
- pte.pt.xn = 1;
- write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
- flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+ int res;
+
+ res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags);
+ BUG_ON(res != 0);
}
/* Remove a mapping from a fixmap entry */
void clear_fixmap(unsigned map)
{
- lpae_t pte = {0};
- write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
- flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+ int res;
+
+ res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
+ BUG_ON(res != 0);
}
/* Create Xen's mappings of memory.
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 22:33 ` Stefano Stabellini
2019-06-13 8:31 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 22:33 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> {set, clear}_fixmap() are currently open-coding update to the Xen
> page-tables. This can be avoided by using the generic helpers
> map_pages_to_xen() and destroy_xen_mappings().
>
> Both function are not meant to fail for fixmap, hence the BUG_ON()
> checking the return.
BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
be a better choice?
The changes in this patch checks out OK.
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 9a40754f44..23ca61e8f0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -348,19 +348,19 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
> /* Map a 4k page in a fixmap entry */
> void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
> {
> - lpae_t pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> - pte.pt.table = 1; /* 4k mappings always have this bit set */
> - pte.pt.xn = 1;
> - write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
> - flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> + int res;
> +
> + res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags);
> + BUG_ON(res != 0);
> }
>
> /* Remove a mapping from a fixmap entry */
> void clear_fixmap(unsigned map)
> {
> - lpae_t pte = {0};
> - write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
> - flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> + int res;
> +
> + res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
> + BUG_ON(res != 0);
> }
>
> /* Create Xen's mappings of memory.
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
2019-06-12 22:33 ` Stefano Stabellini
@ 2019-06-13 8:31 ` Julien Grall
2019-06-13 18:51 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 8:31 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> {set, clear}_fixmap() are currently open-coding update to the Xen
>> page-tables. This can be avoided by using the generic helpers
>> map_pages_to_xen() and destroy_xen_mappings().
>>
>> Both function are not meant to fail for fixmap, hence the BUG_ON()
>> checking the return.
>
> BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
> be a better choice?
The ASSERT() would disappear in non-debug potentially leading to unknown
consequence.
If we imagine that map_pages_to_xen() fails, then it likely means that
mapping has not been done/removed.
As set_fixmap() does not return an error, this means that the user may
try to access an invalid mapping and therefore crash the hypervisor.
As clear_fixmap() does not return an error, this means that subsequent
set_fixmap() may fail because map_pages_to_xen() does not allow to
replace valid mapping.
Ideally we would want to propagate the error, however all the call to
the functions happen during boot. So most likely the user will
panic/BUG_ON as you this hint something has gone really wrong and we
don't want to continue further.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
2019-06-13 8:31 ` Julien Grall
@ 2019-06-13 18:51 ` Stefano Stabellini
2019-06-13 21:21 ` Julien Grall
0 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 18:51 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > {set, clear}_fixmap() are currently open-coding update to the Xen
> > > page-tables. This can be avoided by using the generic helpers
> > > map_pages_to_xen() and destroy_xen_mappings().
> > >
> > > Both function are not meant to fail for fixmap, hence the BUG_ON()
> > > checking the return.
> >
> > BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
> > be a better choice?
> The ASSERT() would disappear in non-debug potentially leading to unknown
> consequence.
>
> If we imagine that map_pages_to_xen() fails, then it likely means that mapping
> has not been done/removed.
>
> As set_fixmap() does not return an error, this means that the user may try to
> access an invalid mapping and therefore crash the hypervisor.
>
> As clear_fixmap() does not return an error, this means that subsequent
> set_fixmap() may fail because map_pages_to_xen() does not allow to replace
> valid mapping.
>
> Ideally we would want to propagate the error, however all the call to the
> functions happen during boot. So most likely the user will panic/BUG_ON as you
> this hint something has gone really wrong and we don't want to continue
> further.
I think the basic principle is that with BUG_ON is "easy" for a guest to
be able to trigger it, potentially causing a DOS. Without the BUG_ON,
the guest is unlikely to be able to trigger a crash. However, if all the
calls happen during boot in regards to operations that have nothing to
do with guests behavior, then it is fine.
I checked all the call sites and I agree that in this case they are all
done during boot only. So in this case it is OK to have the
panic/BUG_ON.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
2019-06-13 18:51 ` Stefano Stabellini
@ 2019-06-13 21:21 ` Julien Grall
2019-06-13 22:55 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 21:21 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 13/06/2019 19:51, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote:
>> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
>>> On Tue, 14 May 2019, Julien Grall wrote:
> I think the basic principle is that with BUG_ON is "easy" for a guest to
> be able to trigger it, potentially causing a DOS. Without the BUG_ON,
> the guest is unlikely to be able to trigger a crash. However, if all the
> calls happen during boot in regards to operations that have nothing to
> do with guests behavior, then it is fine.
Sadly, we don't seem to have used that approach on Arm so far. We have
quite a few BUG_ON() that could be triggered by the guest. For instance,
we used it to confirm that we interpreted correctly the spec... (see
GUEST_BUG_ON). The rationale was that a DOS is better than data leak.
I have a series to try to reduce such BUG_ON.
>
> I checked all the call sites and I agree that in this case they are all
> done during boot only. So in this case it is OK to have the
> panic/BUG_ON.
Can I consider this as an acked-by/reviewed-by?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
2019-06-13 21:21 ` Julien Grall
@ 2019-06-13 22:55 ` Stefano Stabellini
0 siblings, 0 replies; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 22:55 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, nd, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 13/06/2019 19:51, Stefano Stabellini wrote:
> > On Thu, 13 Jun 2019, Julien Grall wrote:
> >> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> >>> On Tue, 14 May 2019, Julien Grall wrote:
> > I think the basic principle is that with BUG_ON is "easy" for a guest to
> > be able to trigger it, potentially causing a DOS. Without the BUG_ON,
> > the guest is unlikely to be able to trigger a crash. However, if all the
> > calls happen during boot in regards to operations that have nothing to
> > do with guests behavior, then it is fine.
>
> Sadly, we don't seem to have used that approach on Arm so far. We have
> quite a few BUG_ON() that could be triggered by the guest. For instance,
> we used it to confirm that we interpreted correctly the spec... (see
> GUEST_BUG_ON). The rationale was that a DOS is better than data leak.
>
> I have a series to try to reduce such BUG_ON.
Good!
> >
> > I checked all the call sites and I agree that in this case they are all
> > done during boot only. So in this case it is OK to have the
> > panic/BUG_ON.
>
> Can I consider this as an acked-by/reviewed-by?
Yes
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
set_pte_flags_on_range() is yet another function that will open-code
update to a specific range in the Xen page-tables. It can be completely
dropped by using either modify_xen_mappings() or destroy_xen_mappings().
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add missing newline in panic
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
1 file changed, 10 insertions(+), 48 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 23ca61e8f0..d74101bcd2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
-enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
-static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
-{
- lpae_t pte;
- int i;
-
- ASSERT(is_kernel(p) && is_kernel(p + l));
-
- /* Can only guard in page granularity */
- ASSERT(!((unsigned long) p & ~PAGE_MASK));
- ASSERT(!(l & ~PAGE_MASK));
-
- for ( i = (p - _start) / PAGE_SIZE;
- i < (p + l - _start) / PAGE_SIZE;
- i++ )
- {
- pte = xen_xenmap[i];
- switch ( mg )
- {
- case mg_clear:
- pte.pt.valid = 0;
- break;
- case mg_ro:
- pte.pt.valid = 1;
- pte.pt.pxn = 1;
- pte.pt.xn = 1;
- pte.pt.ro = 1;
- break;
- case mg_rw:
- pte.pt.valid = 1;
- pte.pt.pxn = 1;
- pte.pt.xn = 1;
- pte.pt.ro = 0;
- break;
- case mg_rx:
- pte.pt.valid = 1;
- pte.pt.pxn = 0;
- pte.pt.xn = 0;
- pte.pt.ro = 1;
- break;
- }
- write_pte(xen_xenmap + i, pte);
- }
- flush_xen_tlb_local();
-}
-
/* Release all __init and __initdata ranges to be reused */
void free_init_memory(void)
{
@@ -1331,8 +1285,12 @@ void free_init_memory(void)
uint32_t insn;
unsigned int i, nr = len / sizeof(insn);
uint32_t *p;
+ int rc;
- set_pte_flags_on_range(__init_begin, len, mg_rw);
+ rc = modify_xen_mappings((unsigned long)__init_begin,
+ (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
+ if ( rc )
+ panic("Unable to map RW the init section (rc = %d)\n", rc);
/*
* From now on, init will not be used for execution anymore,
@@ -1350,7 +1308,11 @@ void free_init_memory(void)
for ( i = 0; i < nr; i++ )
*(p + i) = insn;
- set_pte_flags_on_range(__init_begin, len, mg_clear);
+ rc = destroy_xen_mappings((unsigned long)__init_begin,
+ (unsigned long)__init_end);
+ if ( rc )
+ panic("Unable to remove the init section (rc = %d)\n", rc);
+
init_domheap_pages(pa, pa + len);
printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
@ 2019-05-14 12:31 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-14 12:31 UTC (permalink / raw)
To: xen-devel
Cc: Oleksandr_Tyshchenko, Julien Grall, Stefano Stabellini, Andrii Anisov
set_pte_flags_on_range() is yet another function that will open-code
update to a specific range in the Xen page-tables. It can be completely
dropped by using either modify_xen_mappings() or destroy_xen_mappings().
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
---
Changes in v2:
- Add missing newline in panic
- Add Andrii's reviewed-by
---
xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
1 file changed, 10 insertions(+), 48 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 23ca61e8f0..d74101bcd2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
}
-enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
-static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
-{
- lpae_t pte;
- int i;
-
- ASSERT(is_kernel(p) && is_kernel(p + l));
-
- /* Can only guard in page granularity */
- ASSERT(!((unsigned long) p & ~PAGE_MASK));
- ASSERT(!(l & ~PAGE_MASK));
-
- for ( i = (p - _start) / PAGE_SIZE;
- i < (p + l - _start) / PAGE_SIZE;
- i++ )
- {
- pte = xen_xenmap[i];
- switch ( mg )
- {
- case mg_clear:
- pte.pt.valid = 0;
- break;
- case mg_ro:
- pte.pt.valid = 1;
- pte.pt.pxn = 1;
- pte.pt.xn = 1;
- pte.pt.ro = 1;
- break;
- case mg_rw:
- pte.pt.valid = 1;
- pte.pt.pxn = 1;
- pte.pt.xn = 1;
- pte.pt.ro = 0;
- break;
- case mg_rx:
- pte.pt.valid = 1;
- pte.pt.pxn = 0;
- pte.pt.xn = 0;
- pte.pt.ro = 1;
- break;
- }
- write_pte(xen_xenmap + i, pte);
- }
- flush_xen_tlb_local();
-}
-
/* Release all __init and __initdata ranges to be reused */
void free_init_memory(void)
{
@@ -1331,8 +1285,12 @@ void free_init_memory(void)
uint32_t insn;
unsigned int i, nr = len / sizeof(insn);
uint32_t *p;
+ int rc;
- set_pte_flags_on_range(__init_begin, len, mg_rw);
+ rc = modify_xen_mappings((unsigned long)__init_begin,
+ (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
+ if ( rc )
+ panic("Unable to map RW the init section (rc = %d)\n", rc);
/*
* From now on, init will not be used for execution anymore,
@@ -1350,7 +1308,11 @@ void free_init_memory(void)
for ( i = 0; i < nr; i++ )
*(p + i) = insn;
- set_pte_flags_on_range(__init_begin, len, mg_clear);
+ rc = destroy_xen_mappings((unsigned long)__init_begin,
+ (unsigned long)__init_end);
+ if ( rc )
+ panic("Unable to remove the init section (rc = %d)\n", rc);
+
init_domheap_pages(pa, pa + len);
printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
2019-05-14 12:31 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-12 22:41 ` Stefano Stabellini
2019-06-13 8:51 ` Julien Grall
-1 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-12 22:41 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Tue, 14 May 2019, Julien Grall wrote:
> set_pte_flags_on_range() is yet another function that will open-code
> update to a specific range in the Xen page-tables. It can be completely
> dropped by using either modify_xen_mappings() or destroy_xen_mappings().
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
> Changes in v2:
> - Add missing newline in panic
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
> 1 file changed, 10 insertions(+), 48 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 23ca61e8f0..d74101bcd2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> }
>
> -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
> -{
> - lpae_t pte;
> - int i;
> -
> - ASSERT(is_kernel(p) && is_kernel(p + l));
> -
> - /* Can only guard in page granularity */
> - ASSERT(!((unsigned long) p & ~PAGE_MASK));
> - ASSERT(!(l & ~PAGE_MASK));
> -
> - for ( i = (p - _start) / PAGE_SIZE;
> - i < (p + l - _start) / PAGE_SIZE;
> - i++ )
> - {
> - pte = xen_xenmap[i];
> - switch ( mg )
> - {
> - case mg_clear:
> - pte.pt.valid = 0;
> - break;
> - case mg_ro:
> - pte.pt.valid = 1;
> - pte.pt.pxn = 1;
> - pte.pt.xn = 1;
> - pte.pt.ro = 1;
> - break;
> - case mg_rw:
> - pte.pt.valid = 1;
> - pte.pt.pxn = 1;
It shouldn't make any difference, but FYI we don't set pxn in
xen_pt_update.
> - pte.pt.xn = 1;
> - pte.pt.ro = 0;
> - break;
> - case mg_rx:
> - pte.pt.valid = 1;
> - pte.pt.pxn = 0;
> - pte.pt.xn = 0;
> - pte.pt.ro = 1;
> - break;
> - }
> - write_pte(xen_xenmap + i, pte);
> - }
> - flush_xen_tlb_local();
> -}
> -
> /* Release all __init and __initdata ranges to be reused */
> void free_init_memory(void)
> {
> @@ -1331,8 +1285,12 @@ void free_init_memory(void)
> uint32_t insn;
> unsigned int i, nr = len / sizeof(insn);
> uint32_t *p;
> + int rc;
>
> - set_pte_flags_on_range(__init_begin, len, mg_rw);
> + rc = modify_xen_mappings((unsigned long)__init_begin,
> + (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
> + if ( rc )
> + panic("Unable to map RW the init section (rc = %d)\n", rc);
Like for the previous patch, I wonder if we should replace ASSERTs with
panics: ASSERTs don't cause issues in non-debug builds. We don't really
have an "official policy" about this, but I have been going by the rule
of thumb that ASSERTs are really good to have while we need to be
careful with BUG_ON/panic because they might introduce instability (see
Linux policy not to have any.)
>
> /*
> * From now on, init will not be used for execution anymore,
> @@ -1350,7 +1308,11 @@ void free_init_memory(void)
> for ( i = 0; i < nr; i++ )
> *(p + i) = insn;
>
> - set_pte_flags_on_range(__init_begin, len, mg_clear);
> + rc = destroy_xen_mappings((unsigned long)__init_begin,
> + (unsigned long)__init_end);
> + if ( rc )
> + panic("Unable to remove the init section (rc = %d)\n", rc);
> +
> init_domheap_pages(pa, pa + len);
> printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
> }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
2019-06-12 22:41 ` Stefano Stabellini
@ 2019-06-13 8:51 ` Julien Grall
2019-06-13 18:04 ` Stefano Stabellini
0 siblings, 1 reply; 64+ messages in thread
From: Julien Grall @ 2019-06-13 8:51 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Andrii Anisov, Oleksandr_Tyshchenko
Hi Stefano,
On 6/12/19 11:41 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> set_pte_flags_on_range() is yet another function that will open-code
>> update to a specific range in the Xen page-tables. It can be completely
>> dropped by using either modify_xen_mappings() or destroy_xen_mappings().
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>> Changes in v2:
>> - Add missing newline in panic
>> - Add Andrii's reviewed-by
>> ---
>> xen/arch/arm/mm.c | 58 ++++++++++---------------------------------------------
>> 1 file changed, 10 insertions(+), 48 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 23ca61e8f0..d74101bcd2 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c.
>> @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
>> return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
>> }
>>
>> -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
>> -static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>> -{
>> - lpae_t pte;
>> - int i;
>> -
>> - ASSERT(is_kernel(p) && is_kernel(p + l));
>> -
>> - /* Can only guard in page granularity */
>> - ASSERT(!((unsigned long) p & ~PAGE_MASK));
>> - ASSERT(!(l & ~PAGE_MASK));
>> -
>> - for ( i = (p - _start) / PAGE_SIZE;
>> - i < (p + l - _start) / PAGE_SIZE;
>> - i++ )
>> - {
>> - pte = xen_xenmap[i];
>> - switch ( mg )
>> - {
>> - case mg_clear:
>> - pte.pt.valid = 0;
>> - break;
>> - case mg_ro:
>> - pte.pt.valid = 1;
>> - pte.pt.pxn = 1;
>> - pte.pt.xn = 1;
>> - pte.pt.ro = 1;
>> - break;
>> - case mg_rw:
>> - pte.pt.valid = 1;
>> - pte.pt.pxn = 1;
>
> It shouldn't make any difference, but FYI we don't set pxn in
> xen_pt_update.
Per D5.4.5 in DDI0487D.a, the PXN bit should be RES0 for any stage-1
that supports only a single VA range.
The hypervisor stage-1 only supports a single VA range (we have only one
TTBR), so this bit should be RES0. Any other value would be wrong and
could lead to undefined behavior in the future.
So the current code was wrong. I will mention it in the commit message.
>
>
>> - pte.pt.xn = 1;
>> - pte.pt.ro = 0;
>> - break;
>> - case mg_rx:
>> - pte.pt.valid = 1;
>> - pte.pt.pxn = 0;
>> - pte.pt.xn = 0;
>> - pte.pt.ro = 1;
>> - break;
>> - }
>> - write_pte(xen_xenmap + i, pte);
>> - }
>> - flush_xen_tlb_local();
>> -}
>> -
>> /* Release all __init and __initdata ranges to be reused */
>> void free_init_memory(void)
>> {
>> @@ -1331,8 +1285,12 @@ void free_init_memory(void)
>> uint32_t insn;
>> unsigned int i, nr = len / sizeof(insn);
>> uint32_t *p;
>> + int rc;
>>
>> - set_pte_flags_on_range(__init_begin, len, mg_rw);
>> + rc = modify_xen_mappings((unsigned long)__init_begin,
>> + (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
>> + if ( rc )
>> + panic("Unable to map RW the init section (rc = %d)\n", rc);
>
> Like for the previous patch, I wonder if we should replace ASSERTs with
> panics: ASSERTs don't cause issues in non-debug builds. We don't really
> have an "official policy" about this, but I have been going by the rule
> of thumb that ASSERTs are really good to have while we need to be
> careful with BUG_ON/panic because they might introduce instability (see
> Linux policy not to have any.)
We do have a policy docs/misc/xen-error-handling.txt. While I agree that
we have to be careful with BUG_ON()/panic... you also have to take into
account from where it is called.
In this case, replacing by an ASSERT here is going to make much worst.
This function is only called once at the end of the boot to remove any
part of Xen that is not used anymore. If this were to fail, then this
means that something goes really wrong and this is better to stop here
rather than continuing with in an unstable state.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
2019-06-13 8:51 ` Julien Grall
@ 2019-06-13 18:04 ` Stefano Stabellini
2019-06-13 21:22 ` Julien Grall
0 siblings, 1 reply; 64+ messages in thread
From: Stefano Stabellini @ 2019-06-13 18:04 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Stefano Stabellini, Andrii Anisov, Oleksandr_Tyshchenko
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
>
> On 6/12/19 11:41 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > set_pte_flags_on_range() is yet another function that will open-code
> > > update to a specific range in the Xen page-tables. It can be completely
> > > dropped by using either modify_xen_mappings() or destroy_xen_mappings().
> > >
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > >
> > > ---
> > > Changes in v2:
> > > - Add missing newline in panic
> > > - Add Andrii's reviewed-by
> > > ---
> > > xen/arch/arm/mm.c | 58
> > > ++++++++++---------------------------------------------
> > > 1 file changed, 10 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 23ca61e8f0..d74101bcd2 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c.
> > > @@ -1277,52 +1277,6 @@ int modify_xen_mappings(unsigned long s, unsigned
> > > long e, unsigned int flags)
> > > return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> > > }
> > > -enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
> > > -static void set_pte_flags_on_range(const char *p, unsigned long l, enum
> > > mg mg)
> > > -{
> > > - lpae_t pte;
> > > - int i;
> > > -
> > > - ASSERT(is_kernel(p) && is_kernel(p + l));
> > > -
> > > - /* Can only guard in page granularity */
> > > - ASSERT(!((unsigned long) p & ~PAGE_MASK));
> > > - ASSERT(!(l & ~PAGE_MASK));
> > > -
> > > - for ( i = (p - _start) / PAGE_SIZE;
> > > - i < (p + l - _start) / PAGE_SIZE;
> > > - i++ )
> > > - {
> > > - pte = xen_xenmap[i];
> > > - switch ( mg )
> > > - {
> > > - case mg_clear:
> > > - pte.pt.valid = 0;
> > > - break;
> > > - case mg_ro:
> > > - pte.pt.valid = 1;
> > > - pte.pt.pxn = 1;
> > > - pte.pt.xn = 1;
> > > - pte.pt.ro = 1;
> > > - break;
> > > - case mg_rw:
> > > - pte.pt.valid = 1;
> > > - pte.pt.pxn = 1;
> >
> > It shouldn't make any difference, but FYI we don't set pxn in
> > xen_pt_update.
>
> Per D5.4.5 in DDI0487D.a, the PXN bit should be RES0 for any stage-1 that
> supports only a single VA range.
>
> The hypervisor stage-1 only supports a single VA range (we have only one
> TTBR), so this bit should be RES0. Any other value would be wrong and could
> lead to undefined behavior in the future.
>
> So the current code was wrong. I will mention it in the commit message.
>
> >
> >
> > > - pte.pt.xn = 1;
> > > - pte.pt.ro = 0;
> > > - break;
> > > - case mg_rx:
> > > - pte.pt.valid = 1;
> > > - pte.pt.pxn = 0;
> > > - pte.pt.xn = 0;
> > > - pte.pt.ro = 1;
> > > - break;
> > > - }
> > > - write_pte(xen_xenmap + i, pte);
> > > - }
> > > - flush_xen_tlb_local();
> > > -}
> > > -
> > > /* Release all __init and __initdata ranges to be reused */
> > > void free_init_memory(void)
> > > {
> > > @@ -1331,8 +1285,12 @@ void free_init_memory(void)
> > > uint32_t insn;
> > > unsigned int i, nr = len / sizeof(insn);
> > > uint32_t *p;
> > > + int rc;
> > > - set_pte_flags_on_range(__init_begin, len, mg_rw);
> > > + rc = modify_xen_mappings((unsigned long)__init_begin,
> > > + (unsigned long)__init_end,
> > > PAGE_HYPERVISOR_RW);
> > > + if ( rc )
> > > + panic("Unable to map RW the init section (rc = %d)\n", rc);
> >
> > Like for the previous patch, I wonder if we should replace ASSERTs with
> > panics: ASSERTs don't cause issues in non-debug builds. We don't really
> > have an "official policy" about this, but I have been going by the rule
> > of thumb that ASSERTs are really good to have while we need to be
> > careful with BUG_ON/panic because they might introduce instability (see
> > Linux policy not to have any.)
>
> We do have a policy docs/misc/xen-error-handling.txt. While I agree that we
> have to be careful with BUG_ON()/panic... you also have to take into account
> from where it is called.
>
> In this case, replacing by an ASSERT here is going to make much worst.
> This function is only called once at the end of the boot to remove any part of
> Xen that is not used anymore. If this were to fail, then this means that
> something goes really wrong and this is better to stop here rather than
> continuing with in an unstable state.
Yes, on second thought, a BUG_ON is fine here. The risk is only when the
BUG_ON could somehow be trigger by guest behavior, which is not the case
here.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
2019-06-13 18:04 ` Stefano Stabellini
@ 2019-06-13 21:22 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-13 21:22 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, nd, Andrii Anisov, Oleksandr_Tyshchenko
On 13/06/2019 19:04, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote:
>
> Yes, on second thought, a BUG_ON is fine here. The risk is only when the
> BUG_ON could somehow be trigger by guest behavior, which is not the case
> here.
Agreed. I think we are safe to use BUG_ON(...)/panic in any init
function. Everywhere else, we should think twice before adding new one.
I will respin the patch as discussed.
Cheers,
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH MM-PART3 v2 00/12] xen/arm: Provide a generic function to update Xen PT
@ 2019-05-29 17:23 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-29 17:23 UTC (permalink / raw)
To: xen-devel; +Cc: Oleksandr_Tyshchenko, Stefano Stabellini, Andrii_Anisov
Hi,
Gentle ping...
Cheers,
On 14/05/2019 13:31, Julien Grall wrote:
> Hi all,
>
> This is the third part of the boot/memory rework for Xen on Arm. At the
> moment, the update to Xen PT is scattered all around mm.c. This makes
> difficult to rework Xen memory layout or even ensuring we are following the
> Arm Arm properly (and we are not so far!).
>
> This part contains code to provide a generic function to update Xen PT.
> While I could have started from scratch, I decided to base the new function
> on create_xen_entries() (now renamed xen_pt_update()). This makes slightly
> easier to follow the changes.
>
> In this series, the new generic function will only support 3rd-level update
> and cannot be used in early boot (i.e because xenheap is not initialized).
> This will be extended in follow-up series to allow more use within mm.c.
>
> There are probably some optimization possible around the TLBs flush. I haven't
> looked at it so far.
>
> The last two patches of this series is to show how existing callers can be
> converted. There are more conversion to come in follow-up series.
>
> This series is based on the first two parts sent separately (see [1] and [2]).
>
> For convenience, I provided a branch with all the patches applied based on
> staging:
>
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part3/v2
>
> Cheers,
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01109.html
> [2] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01149.html
>
> Julien Grall (12):
> xen/arm: lpae: Add a macro to generate offsets from an address
> xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
> xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
> xen/arm: mm: Only increment mfn when valid in xen_pt_update
> xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
> xen/arm: mm: Sanity check any update of Xen page tables
> xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
> xen/arm: mm: Remove enum xenmap_operation
> xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
> xen/arm: mm: Rework Xen page-tables walk during update
> xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
> xen/arm: mm: Remove set_pte_flags_on_range()
>
> xen/arch/arm/mm.c | 421 ++++++++++++++++++++++++++++++---------------
> xen/arch/arm/p2m.c | 23 +--
> xen/include/asm-arm/lpae.h | 9 +
> xen/include/asm-arm/page.h | 9 +-
> 4 files changed, 305 insertions(+), 157 deletions(-)
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 00/12] xen/arm: Provide a generic function to update Xen PT
@ 2019-05-29 17:23 ` Julien Grall
0 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-05-29 17:23 UTC (permalink / raw)
To: xen-devel; +Cc: Oleksandr_Tyshchenko, Stefano Stabellini, Andrii_Anisov
Hi,
Gentle ping...
Cheers,
On 14/05/2019 13:31, Julien Grall wrote:
> Hi all,
>
> This is the third part of the boot/memory rework for Xen on Arm. At the
> moment, the update to Xen PT is scattered all around mm.c. This makes
> difficult to rework Xen memory layout or even ensuring we are following the
> Arm Arm properly (and we are not so far!).
>
> This part contains code to provide a generic function to update Xen PT.
> While I could have started from scratch, I decided to base the new function
> on create_xen_entries() (now renamed xen_pt_update()). This makes slightly
> easier to follow the changes.
>
> In this series, the new generic function will only support 3rd-level update
> and cannot be used in early boot (i.e because xenheap is not initialized).
> This will be extended in follow-up series to allow more use within mm.c.
>
> There are probably some optimization possible around the TLBs flush. I haven't
> looked at it so far.
>
> The last two patches of this series is to show how existing callers can be
> converted. There are more conversion to come in follow-up series.
>
> This series is based on the first two parts sent separately (see [1] and [2]).
>
> For convenience, I provided a branch with all the patches applied based on
> staging:
>
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part3/v2
>
> Cheers,
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01109.html
> [2] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01149.html
>
> Julien Grall (12):
> xen/arm: lpae: Add a macro to generate offsets from an address
> xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
> xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
> xen/arm: mm: Only increment mfn when valid in xen_pt_update
> xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
> xen/arm: mm: Sanity check any update of Xen page tables
> xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
> xen/arm: mm: Remove enum xenmap_operation
> xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
> xen/arm: mm: Rework Xen page-tables walk during update
> xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
> xen/arm: mm: Remove set_pte_flags_on_range()
>
> xen/arch/arm/mm.c | 421 ++++++++++++++++++++++++++++++---------------
> xen/arch/arm/p2m.c | 23 +--
> xen/include/asm-arm/lpae.h | 9 +
> xen/include/asm-arm/page.h | 9 +-
> 4 files changed, 305 insertions(+), 157 deletions(-)
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [Xen-devel] [PATCH MM-PART3 v2 00/12] xen/arm: Provide a generic function to update Xen PT
2019-05-29 17:23 ` [Xen-devel] " Julien Grall
(?)
@ 2019-06-10 10:08 ` Julien Grall
-1 siblings, 0 replies; 64+ messages in thread
From: Julien Grall @ 2019-06-10 10:08 UTC (permalink / raw)
To: xen-devel; +Cc: Oleksandr_Tyshchenko, Stefano Stabellini, Andrii_Anisov
(+ Committers)
Ping again... I have some upcoming patches that are blocked on this work...
Cheers,
On 29/05/2019 18:23, Julien Grall wrote:
> Hi,
>
> Gentle ping...
>
> Cheers,
>
> On 14/05/2019 13:31, Julien Grall wrote:
>> Hi all,
>>
>> This is the third part of the boot/memory rework for Xen on Arm. At the
>> moment, the update to Xen PT is scattered all around mm.c. This makes
>> difficult to rework Xen memory layout or even ensuring we are following the
>> Arm Arm properly (and we are not so far!).
>>
>> This part contains code to provide a generic function to update Xen PT.
>> While I could have started from scratch, I decided to base the new function
>> on create_xen_entries() (now renamed xen_pt_update()). This makes slightly
>> easier to follow the changes.
>>
>> In this series, the new generic function will only support 3rd-level update
>> and cannot be used in early boot (i.e because xenheap is not initialized).
>> This will be extended in follow-up series to allow more use within mm.c.
>>
>> There are probably some optimization possible around the TLBs flush. I haven't
>> looked at it so far.
>>
>> The last two patches of this series is to show how existing callers can be
>> converted. There are more conversion to come in follow-up series.
>>
>> This series is based on the first two parts sent separately (see [1] and [2]).
>>
>> For convenience, I provided a branch with all the patches applied based on
>> staging:
>>
>> git://xenbits.xen.org/people/julieng/xen-unstable.git branch mm/part3/v2
>>
>> Cheers,
>>
>> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01109.html
>> [2] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01149.html
>>
>> Julien Grall (12):
>> xen/arm: lpae: Add a macro to generate offsets from an address
>> xen/arm: mm: Rename create_xen_entries() to xen_pt_update()
>> xen/arm: mm: Move out of xen_pt_update() the logic to update an entry
>> xen/arm: mm: Only increment mfn when valid in xen_pt_update
>> xen/arm: mm: Introduce _PAGE_PRESENT and _PAGE_POPULATE
>> xen/arm: mm: Sanity check any update of Xen page tables
>> xen/arm: mm: Rework xen_pt_update_entry to avoid use xenmap_operation
>> xen/arm: mm: Remove enum xenmap_operation
>> xen/arm: mm: Use {, un}map_domain_page() to map/unmap Xen page-tables
>> xen/arm: mm: Rework Xen page-tables walk during update
>> xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()
>> xen/arm: mm: Remove set_pte_flags_on_range()
>>
>> xen/arch/arm/mm.c | 421 ++++++++++++++++++++++++++++++---------------
>> xen/arch/arm/p2m.c | 23 +--
>> xen/include/asm-arm/lpae.h | 9 +
>> xen/include/asm-arm/page.h | 9 +-
>> 4 files changed, 305 insertions(+), 157 deletions(-)
>>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread