| Message ID | 0dddfee6c0817bd548ed4f8c69d7032455eec5a9.1406133930.git.ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 597eaa8db932741f4b49336d5638c5a1833d4f2f |
| Headers | show |
On Wed, Jul 23, 2014 at 9:45 AM, Ian Campbell <ian.campbell@citrix.com> wrote: > They might be in e.g. flash or something but more likely they could > bein a bank of RAM which we aren't handling or in RAM which the > bootloader hasn't told us about for some reason. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Fu Wei <fu.wei@linaro.org> > Cc: Roy Franz <roy.franz@linaro.org> > --- > xen/arch/arm/setup.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index e53e491..446b4dc 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -247,8 +247,13 @@ void __init discard_initial_modules(void) > paddr_t s = mi->module[i].start; > paddr_t e = s + PAGE_ALIGN(mi->module[i].size); > > - if ( mi->module[i].kind != BOOTMOD_XEN ) > - dt_unreserved_regions(s, e, init_domheap_pages, 0); > + if ( mi->module[i].kind == BOOTMOD_XEN ) > + continue; > + > + if ( !mfn_valid(paddr_to_pfn(s)) || !mfn_valid(paddr_to_pfn(e))) > + continue; > + > + dt_unreserved_regions(s, e, init_domheap_pages, 0); > } > > mi->nr_mods = 0; This looks good - I'll give it a spin. I think this will be sufficient to allow the stub (or GRUB) to use any available EFI memory for loading modules without worrying about whether XEN will map it. Thanks, Roy
Hi Ian, On 23/07/14 17:45, Ian Campbell wrote: > They might be in e.g. flash or something but more likely they could > bein a bank of RAM which we aren't handling or in RAM which the being? > bootloader hasn't told us about for some reason. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Fu Wei <fu.wei@linaro.org> > Cc: Roy Franz <roy.franz@linaro.org> > --- > xen/arch/arm/setup.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index e53e491..446b4dc 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -247,8 +247,13 @@ void __init discard_initial_modules(void) > paddr_t s = mi->module[i].start; > paddr_t e = s + PAGE_ALIGN(mi->module[i].size); > > - if ( mi->module[i].kind != BOOTMOD_XEN ) > - dt_unreserved_regions(s, e, init_domheap_pages, 0); > + if ( mi->module[i].kind == BOOTMOD_XEN ) > + continue; > + > + if ( !mfn_valid(paddr_to_pfn(s)) || !mfn_valid(paddr_to_pfn(e))) > + continue; What happen if the bootloader decide to put the module between 2 banks and having the hole in the middle. Such as: start of the module end of bank 0 hole start of bank 1 end of the module Regards,
On Thu, 2014-07-24 at 11:40 +0100, Julien Grall wrote: > > - if ( mi->module[i].kind != BOOTMOD_XEN ) > > - dt_unreserved_regions(s, e, init_domheap_pages, 0); > > + if ( mi->module[i].kind == BOOTMOD_XEN ) > > + continue; > > + > > + if ( !mfn_valid(paddr_to_pfn(s)) || !mfn_valid(paddr_to_pfn(e))) > > + continue; > > What happen if the bootloader decide to put the module between 2 banks > and having the hole in the middle. Such as: > > start of the module > > end of bank 0 > > hole > > start of bank 1 > > end of the module Either we will ignore bank 1, in which case these checks will prevent us adding them to the heap or the frame table will span bank0..1 and include the hole. We don't really handle the latter case very well, but the first one is the one which is actually biting people today. Ian.
Hi Ian, On 07/24/2014 11:44 AM, Ian Campbell wrote: > On Thu, 2014-07-24 at 11:40 +0100, Julien Grall wrote: >>> - if ( mi->module[i].kind != BOOTMOD_XEN ) >>> - dt_unreserved_regions(s, e, init_domheap_pages, 0); >>> + if ( mi->module[i].kind == BOOTMOD_XEN ) >>> + continue; >>> + >>> + if ( !mfn_valid(paddr_to_pfn(s)) || !mfn_valid(paddr_to_pfn(e))) >>> + continue; >> >> What happen if the bootloader decide to put the module between 2 banks >> and having the hole in the middle. Such as: >> >> start of the module >> >> end of bank 0 >> >> hole >> >> start of bank 1 >> >> end of the module > > Either we will ignore bank 1, in which case these checks will prevent us > adding them to the heap or the frame table will span bank0..1 and > include the hole. > > We don't really handle the latter case very well, but the first one is > the one which is actually biting people today. Thanks for the explanation. It might be worse to add a TODO to help the person who will support sparse frame table. Anyway: Acked-by: Julien Grall <julien.grall@linaro.org> Regards,
On Thu, 2014-07-24 at 11:40 +0100, Julien Grall wrote: > Hi Ian, > > On 23/07/14 17:45, Ian Campbell wrote: > > They might be in e.g. flash or something but more likely they could > > bein a bank of RAM which we aren't handling or in RAM which the > > being? "be in". Oops! I've made a note for myself about the sparseness thing (since I hope to work on that soon) and applied with your Ack. Ian.
On Thu, Jul 24, 2014 at 8:54 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Thu, 2014-07-24 at 11:40 +0100, Julien Grall wrote: > > Hi Ian, > > > > On 23/07/14 17:45, Ian Campbell wrote: > > > They might be in e.g. flash or something but more likely they could > > > bein a bank of RAM which we aren't handling or in RAM which the > > > > being? > > "be in". Oops! > > I've made a note for myself about the sparseness thing (since I hope to > work on that soon) and applied with your Ack. > > Ian. > > > > > Thanks Ian - this should allow me to simplify the allocation of buffers for the modules. I just got back from vacation today, so I'll give this a try soon. Roy
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index e53e491..446b4dc 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -247,8 +247,13 @@ void __init discard_initial_modules(void) paddr_t s = mi->module[i].start; paddr_t e = s + PAGE_ALIGN(mi->module[i].size); - if ( mi->module[i].kind != BOOTMOD_XEN ) - dt_unreserved_regions(s, e, init_domheap_pages, 0); + if ( mi->module[i].kind == BOOTMOD_XEN ) + continue; + + if ( !mfn_valid(paddr_to_pfn(s)) || !mfn_valid(paddr_to_pfn(e))) + continue; + + dt_unreserved_regions(s, e, init_domheap_pages, 0); } mi->nr_mods = 0;
They might be in e.g. flash or something but more likely they could bein a bank of RAM which we aren't handling or in RAM which the bootloader hasn't told us about for some reason. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Fu Wei <fu.wei@linaro.org> Cc: Roy Franz <roy.franz@linaro.org> --- xen/arch/arm/setup.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)