| Message ID | 20190514123125.29086-13-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Provide a generic function to update Xen PT | expand |
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); > }
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,
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.
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, >
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); }