| Message ID | 39b51904-37d8-f0c0-2ad3-6a0dd7df59d7@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: refine link time stub area related assertion | expand |
On 15/01/2020 10:26, Jan Beulich wrote: > While it has been me to introduce this, the use of | there has become > (and perhaps was from the very beginning) misleading. Rather than > avoiding the right side of it when linking the xen.efi intermediate file > at a different base address, make the expression cope with that case, > thus verifying placement on every step. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> as this is simply a rearranging, but... > > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -351,8 +351,8 @@ SECTIONS > .comment 0 : { *(.comment) } > } > > -ASSERT(__image_base__ > XEN_VIRT_START | > - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, > +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - > + NR_CPUS * PAGE_SIZE, ... doesn't this want a stubs_per_page term? We don't have 4k per cpu. Wei: FYI, if you do introduce executable fixmap entries, this ASSERT() wants adjusting by NR_X_FIXMAP * PAGE_SIZE as well. ~Andrew
On Wed, Jan 15, 2020 at 02:36:24PM +0000, Andrew Cooper wrote: > On 15/01/2020 10:26, Jan Beulich wrote: > > While it has been me to introduce this, the use of | there has become > > (and perhaps was from the very beginning) misleading. Rather than > > avoiding the right side of it when linking the xen.efi intermediate file > > at a different base address, make the expression cope with that case, > > thus verifying placement on every step. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> as this is simply a > rearranging, but... > > > > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -351,8 +351,8 @@ SECTIONS > > .comment 0 : { *(.comment) } > > } > > > > -ASSERT(__image_base__ > XEN_VIRT_START | > > - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, > > +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - > > + NR_CPUS * PAGE_SIZE, > > ... doesn't this want a stubs_per_page term? We don't have 4k per cpu. > > Wei: FYI, if you do introduce executable fixmap entries, this ASSERT() > wants adjusting by NR_X_FIXMAP * PAGE_SIZE as well. It is a bit problematic to do that if NR_X_FIXMAP is calculated from the last enum element (see FIXADDR_SIZE) I can expose a constant for the largest possible numbers of executable fixed mappings to put in the ASSERT. Wei. > > ~Andrew
On 15.01.2020 15:36, Andrew Cooper wrote: > On 15/01/2020 10:26, Jan Beulich wrote: >> While it has been me to introduce this, the use of | there has become >> (and perhaps was from the very beginning) misleading. Rather than >> avoiding the right side of it when linking the xen.efi intermediate file >> at a different base address, make the expression cope with that case, >> thus verifying placement on every step. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> as this is simply a > rearranging, but... > >> >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -351,8 +351,8 @@ SECTIONS >> .comment 0 : { *(.comment) } >> } >> >> -ASSERT(__image_base__ > XEN_VIRT_START | >> - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, >> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - >> + NR_CPUS * PAGE_SIZE, > > ... doesn't this want a stubs_per_page term? We don't have 4k per cpu. Good point - let me see if I can fix this at this same occasion. I guess when introducing these pages I had one per CPU initially, and then forgot to update the assertion here (it being too strict of course is better than it being too lax). Jan
On 15/01/2020 16:27, Jan Beulich wrote: > On 15.01.2020 15:36, Andrew Cooper wrote: >> On 15/01/2020 10:26, Jan Beulich wrote: >>> While it has been me to introduce this, the use of | there has become >>> (and perhaps was from the very beginning) misleading. Rather than >>> avoiding the right side of it when linking the xen.efi intermediate file >>> at a different base address, make the expression cope with that case, >>> thus verifying placement on every step. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> as this is simply a >> rearranging, but... >> >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -351,8 +351,8 @@ SECTIONS >>> .comment 0 : { *(.comment) } >>> } >>> >>> -ASSERT(__image_base__ > XEN_VIRT_START | >>> - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, >>> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - >>> + NR_CPUS * PAGE_SIZE, >> ... doesn't this want a stubs_per_page term? We don't have 4k per cpu. > Good point - let me see if I can fix this at this same occasion. > I guess when introducing these pages I had one per CPU initially, > and then forgot to update the assertion here (it being too strict > of course is better than it being too lax). With some suitable term, feel free to up my A-by to R-by. ~Andrew
On 15.01.2020 17:29, Andrew Cooper wrote: > On 15/01/2020 16:27, Jan Beulich wrote: >> On 15.01.2020 15:36, Andrew Cooper wrote: >>> On 15/01/2020 10:26, Jan Beulich wrote: >>>> While it has been me to introduce this, the use of | there has become >>>> (and perhaps was from the very beginning) misleading. Rather than >>>> avoiding the right side of it when linking the xen.efi intermediate file >>>> at a different base address, make the expression cope with that case, >>>> thus verifying placement on every step. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> as this is simply a >>> rearranging, but... >>> >>>> --- a/xen/arch/x86/xen.lds.S >>>> +++ b/xen/arch/x86/xen.lds.S >>>> @@ -351,8 +351,8 @@ SECTIONS >>>> .comment 0 : { *(.comment) } >>>> } >>>> >>>> -ASSERT(__image_base__ > XEN_VIRT_START | >>>> - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, >>>> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - >>>> + NR_CPUS * PAGE_SIZE, >>> ... doesn't this want a stubs_per_page term? We don't have 4k per cpu. >> Good point - let me see if I can fix this at this same occasion. >> I guess when introducing these pages I had one per CPU initially, >> and then forgot to update the assertion here (it being too strict >> of course is better than it being too lax). > > With some suitable term, feel free to up my A-by to R-by. Let me rather post what I have, because there's going to be some ugliness (and hence room for mistakes) because of the rounding needed. But thanks for the offer. Jan
--- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -351,8 +351,8 @@ SECTIONS .comment 0 : { *(.comment) } } -ASSERT(__image_base__ > XEN_VIRT_START | - __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE, +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - + NR_CPUS * PAGE_SIZE, "Xen image overlaps stubs area") #ifdef CONFIG_KEXEC
While it has been me to introduce this, the use of | there has become (and perhaps was from the very beginning) misleading. Rather than avoiding the right side of it when linking the xen.efi intermediate file at a different base address, make the expression cope with that case, thus verifying placement on every step. Signed-off-by: Jan Beulich <jbeulich@suse.com>