| Message ID | 20230625204907.57291-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Enable UBSAN support | expand |
On 25/06/2023 22:49, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > At the moment, we are mapping the size of the reserved area for Xen > (i.e. 2MB) even if the binary is smaller. We don't exactly know what's > after Xen, so it is not a good idea to map more than necessary for a > couple of reasons: > * We would need to use break-before-make if the extra PTE needs to > be updated to point to another region > * The extra area mapped may be mapped again by Xen with different > memory attribute. This would result to attribue mismatch. s/attribue/attribute > > Therefore, rework the logic in create_page_tables() to map only what's > necessary. To simplify the logic, we also want to make sure _end > is page-aligned. So align the symbol in the linker and add an assert > to catch any change. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm64/head.S | 15 ++++++++++++++- > xen/arch/arm/xen.lds.S | 3 +++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index f37133cf7ccd..66bc85d4c39e 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -572,6 +572,19 @@ create_page_tables: > create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3 > create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3 > > + /* > + * Find the size of Xen in pages and multiply by the size of a > + * PTE. This will then be compared in the mapping loop below. > + * > + * Note the multiplication is just to avoid using an extra > + * register/instruction per iteration. > + */ > + ldr x0, =_start /* x0 := vaddr(_start) */ x0 is already set to vaddr of _start by the first instruction of create_page_tables and is preserved by create_table_entry. You could just reuse it instead of re-loading. > + ldr x1, =_end /* x1 := vaddr(_end) */ > + sub x0, x1, x0 /* x0 := effective size of Xen */ > + lsr x0, x0, #PAGE_SHIFT /* x0 := Number of pages for Xen */ > + lsl x0, x0, #3 /* x0 := Number of pages * PTE size */ > + > /* Map Xen */ > adr_l x4, boot_third > > @@ -585,7 +598,7 @@ create_page_tables: > 1: str x2, [x4, x1] /* Map vaddr(start) */ > add x2, x2, #PAGE_SIZE /* Next page */ > add x1, x1, #8 /* Next slot */ > - cmp x1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512 entries per page */ > + cmp x1, x0 /* Loop until we map all of Xen */ > b.lt 1b > > /* > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index c5d8c6201423..c4627cea7482 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -212,6 +212,7 @@ SECTIONS > . = ALIGN(POINTER_ALIGN); > __bss_end = .; > } :text > + . = ALIGN(PAGE_SIZE); > _end = . ; > > /* Section for the device tree blob (if any). */ > @@ -241,4 +242,6 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") > ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") > ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") > ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") > +/* To simplify the logic in head.S, we want to _end to be page aligned */ > +ASSERT(IS_ALIGNED(_end, PAGE_SIZE), "_end is not page aligned") one more space if you want to align PAGE_SIZE to POINTER_ALIGN All in all: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, On 26/06/2023 12:28, Michal Orzel wrote: > On 25/06/2023 22:49, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> At the moment, we are mapping the size of the reserved area for Xen >> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's >> after Xen, so it is not a good idea to map more than necessary for a >> couple of reasons: >> * We would need to use break-before-make if the extra PTE needs to >> be updated to point to another region >> * The extra area mapped may be mapped again by Xen with different >> memory attribute. This would result to attribue mismatch. > s/attribue/attribute > >> >> Therefore, rework the logic in create_page_tables() to map only what's >> necessary. To simplify the logic, we also want to make sure _end >> is page-aligned. So align the symbol in the linker and add an assert >> to catch any change. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/arm/arm64/head.S | 15 ++++++++++++++- >> xen/arch/arm/xen.lds.S | 3 +++ >> 2 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >> index f37133cf7ccd..66bc85d4c39e 100644 >> --- a/xen/arch/arm/arm64/head.S >> +++ b/xen/arch/arm/arm64/head.S >> @@ -572,6 +572,19 @@ create_page_tables: >> create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3 >> create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3 >> >> + /* >> + * Find the size of Xen in pages and multiply by the size of a >> + * PTE. This will then be compared in the mapping loop below. >> + * >> + * Note the multiplication is just to avoid using an extra >> + * register/instruction per iteration. >> + */ >> + ldr x0, =_start /* x0 := vaddr(_start) */ > x0 is already set to vaddr of _start by the first instruction of create_page_tables > and is preserved by create_table_entry. You could just reuse it instead of re-loading. I agree that the load is technically redundant. However, I find this approach easier to read (you don't have to remember that _start equals XEN_VIRT_START). > >> + ldr x1, =_end /* x1 := vaddr(_end) */ >> + sub x0, x1, x0 /* x0 := effective size of Xen */ >> + lsr x0, x0, #PAGE_SHIFT /* x0 := Number of pages for Xen */ >> + lsl x0, x0, #3 /* x0 := Number of pages * PTE size */ >> + >> /* Map Xen */ >> adr_l x4, boot_third >> >> @@ -585,7 +598,7 @@ create_page_tables: >> 1: str x2, [x4, x1] /* Map vaddr(start) */ >> add x2, x2, #PAGE_SIZE /* Next page */ >> add x1, x1, #8 /* Next slot */ >> - cmp x1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512 entries per page */ >> + cmp x1, x0 /* Loop until we map all of Xen */ >> b.lt 1b >> >> /* >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index c5d8c6201423..c4627cea7482 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -212,6 +212,7 @@ SECTIONS >> . = ALIGN(POINTER_ALIGN); >> __bss_end = .; >> } :text >> + . = ALIGN(PAGE_SIZE); >> _end = . ; >> >> /* Section for the device tree blob (if any). */ >> @@ -241,4 +242,6 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") >> ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") >> ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") >> ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") >> +/* To simplify the logic in head.S, we want to _end to be page aligned */ >> +ASSERT(IS_ALIGNED(_end, PAGE_SIZE), "_end is not page aligned") > one more space if you want to align PAGE_SIZE to POINTER_ALIGN I have updated in my tree. > > All in all: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks! Cheers,
On 28/06/2023 20:21, Julien Grall wrote: > > > Hi, > > On 26/06/2023 12:28, Michal Orzel wrote: >> On 25/06/2023 22:49, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> At the moment, we are mapping the size of the reserved area for Xen >>> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's >>> after Xen, so it is not a good idea to map more than necessary for a >>> couple of reasons: >>> * We would need to use break-before-make if the extra PTE needs to >>> be updated to point to another region >>> * The extra area mapped may be mapped again by Xen with different >>> memory attribute. This would result to attribue mismatch. >> s/attribue/attribute >> >>> >>> Therefore, rework the logic in create_page_tables() to map only what's >>> necessary. To simplify the logic, we also want to make sure _end >>> is page-aligned. So align the symbol in the linker and add an assert >>> to catch any change. >>> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> --- >>> xen/arch/arm/arm64/head.S | 15 ++++++++++++++- >>> xen/arch/arm/xen.lds.S | 3 +++ >>> 2 files changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index f37133cf7ccd..66bc85d4c39e 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -572,6 +572,19 @@ create_page_tables: >>> create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3 >>> create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3 >>> >>> + /* >>> + * Find the size of Xen in pages and multiply by the size of a >>> + * PTE. This will then be compared in the mapping loop below. >>> + * >>> + * Note the multiplication is just to avoid using an extra >>> + * register/instruction per iteration. >>> + */ >>> + ldr x0, =_start /* x0 := vaddr(_start) */ >> x0 is already set to vaddr of _start by the first instruction of create_page_tables >> and is preserved by create_table_entry. You could just reuse it instead of re-loading. > > I agree that the load is technically redundant. However, I find this > approach easier to read (you don't have to remember that _start equals > XEN_VIRT_START). Ok. As a side note not related to this patch, would it make sense to add an assert in linker script to make sure _start equals XEN_VIRT_START, since we use them interchangeably? ~Michal
Hi, On 29/06/2023 08:26, Michal Orzel wrote: > On 28/06/2023 20:21, Julien Grall wrote: >> >> >> Hi, >> >> On 26/06/2023 12:28, Michal Orzel wrote: >>> On 25/06/2023 22:49, Julien Grall wrote: >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> At the moment, we are mapping the size of the reserved area for Xen >>>> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's >>>> after Xen, so it is not a good idea to map more than necessary for a >>>> couple of reasons: >>>> * We would need to use break-before-make if the extra PTE needs to >>>> be updated to point to another region >>>> * The extra area mapped may be mapped again by Xen with different >>>> memory attribute. This would result to attribue mismatch. >>> s/attribue/attribute >>> >>>> >>>> Therefore, rework the logic in create_page_tables() to map only what's >>>> necessary. To simplify the logic, we also want to make sure _end >>>> is page-aligned. So align the symbol in the linker and add an assert >>>> to catch any change. >>>> >>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>> --- >>>> xen/arch/arm/arm64/head.S | 15 ++++++++++++++- >>>> xen/arch/arm/xen.lds.S | 3 +++ >>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>>> index f37133cf7ccd..66bc85d4c39e 100644 >>>> --- a/xen/arch/arm/arm64/head.S >>>> +++ b/xen/arch/arm/arm64/head.S >>>> @@ -572,6 +572,19 @@ create_page_tables: >>>> create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3 >>>> create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3 >>>> >>>> + /* >>>> + * Find the size of Xen in pages and multiply by the size of a >>>> + * PTE. This will then be compared in the mapping loop below. >>>> + * >>>> + * Note the multiplication is just to avoid using an extra >>>> + * register/instruction per iteration. >>>> + */ >>>> + ldr x0, =_start /* x0 := vaddr(_start) */ >>> x0 is already set to vaddr of _start by the first instruction of create_page_tables >>> and is preserved by create_table_entry. You could just reuse it instead of re-loading. >> >> I agree that the load is technically redundant. However, I find this >> approach easier to read (you don't have to remember that _start equals >> XEN_VIRT_START). > Ok. As a side note not related to this patch, would it make sense to add an assert in linke > script to make sure _start equals XEN_VIRT_START, since we use them interchangeably? Good point. I can add it in this patch. Cheers,
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index f37133cf7ccd..66bc85d4c39e 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -572,6 +572,19 @@ create_page_tables: create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3 create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3 + /* + * Find the size of Xen in pages and multiply by the size of a + * PTE. This will then be compared in the mapping loop below. + * + * Note the multiplication is just to avoid using an extra + * register/instruction per iteration. + */ + ldr x0, =_start /* x0 := vaddr(_start) */ + ldr x1, =_end /* x1 := vaddr(_end) */ + sub x0, x1, x0 /* x0 := effective size of Xen */ + lsr x0, x0, #PAGE_SHIFT /* x0 := Number of pages for Xen */ + lsl x0, x0, #3 /* x0 := Number of pages * PTE size */ + /* Map Xen */ adr_l x4, boot_third @@ -585,7 +598,7 @@ create_page_tables: 1: str x2, [x4, x1] /* Map vaddr(start) */ add x2, x2, #PAGE_SIZE /* Next page */ add x1, x1, #8 /* Next slot */ - cmp x1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512 entries per page */ + cmp x1, x0 /* Loop until we map all of Xen */ b.lt 1b /* diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index c5d8c6201423..c4627cea7482 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -212,6 +212,7 @@ SECTIONS . = ALIGN(POINTER_ALIGN); __bss_end = .; } :text + . = ALIGN(PAGE_SIZE); _end = . ; /* Section for the device tree blob (if any). */ @@ -241,4 +242,6 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") +/* To simplify the logic in head.S, we want to _end to be page aligned */ +ASSERT(IS_ALIGNED(_end, PAGE_SIZE), "_end is not page aligned") ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big")