[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH MM-PART3 v2 12/12] xen/arm: mm: Remove set_pte_flags_on_range()
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@xxxxxxx>
Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
