Message ID | 20160913060250.GA24733@linux-7smt.suse |
---|---|
State | RFC |
Delegated to: | Tom Rini |
Headers | show |
On Tue, Sep 13, 2016 at 02:02:53PM +0800, Peng Fan wrote: > Hi Tom, > > On Mon, Sep 12, 2016 at 08:04:01AM -0400, Tom Rini wrote: > >On Mon, Sep 12, 2016 at 05:18:53PM +0800, Peng Fan wrote: > >> On Mon, Sep 12, 2016 at 05:07:58PM +0800, Peng Fan wrote: > >> >We should not use "bi_dram[0].start + text_offset" as the image dst. > >> >The text_offset maybe 0 for some images, such as XEN. Then the dst > >> >is actually bi_dram[0].start, which maybe the location of spin table. > >> > > >> >Let's use "images->ep & ~(ih->text_offset)" as the dst address. > >> > >> This patch maybe not that correct according to the doc from Linux kernel. > >> " > >> The Image must be placed text_offset bytes from a 2MB aligned base > >> address anywhere in usable system RAM and called there. The region > >> between the 2 MB aligned base address and the start of the image has no > >> special significance to the kernel, and may be used for other purposes. > >> " > >> > >> Now I do not have a good idea that we may have a spin table in the start > >> of DRAM or even a small firmware. > >> > >> Is it better to change gd->bd->bi_dram[0].start? > >> For example physical dram starts from 0x80000000, but 4K is reserved at the beginning. > >> So is it ok to change gd->bd->bi_dram[0].start to 0x80001000? > > > >So, to be more precise, with v4.5 the memory location restrictions were > >relaxed. So we first need to update cmd/booti.c to know that res1 is > >now 'flags' and that we must check bit3 to determine where / how to > >align the image. In the case of bit3 == 0 we must continue to do what > >we do today. In the case of bit3 == 1, we have to decide what exactly > >to do. My first thought is to see if images->ep is 2MB aligned and if > >so move to images->ep + ih->text_offset. If not 2MB aligned, align that > >and then add ih->text_offset. > > > >Then any areas that need to be reserved in memory, wherever they are, > >can still be marked off as reserved in the DT. > > > How about the following patch? > If it is ok, I'll send out a formal one. Pretty close: > > diff --git a/cmd/booti.c b/cmd/booti.c > index 6c1c998..efa8bc8 100644 > --- a/cmd/booti.c > +++ b/cmd/booti.c > @@ -20,7 +20,7 @@ struct Image_header { > uint32_t code1; /* Executable code */ > uint64_t text_offset; /* Image load offset, LE */ > uint64_t image_size; /* Effective Image size, LE */ > - uint64_t res1; /* reserved */ > + uint64_t flags; /* Information flags, LE */ > uint64_t res2; /* reserved */ > uint64_t res3; /* reserved */ > uint64_t res4; /* reserved */ > @@ -29,6 +29,7 @@ struct Image_header { > }; > > #define LINUX_ARM64_IMAGE_MAGIC 0x644d5241 > +#define LINUX_PHYSICAL_PLACEMENT (0x1 << 3) Just '1' not '0x1' > > static int booti_setup(bootm_headers_t *images) > { > @@ -54,7 +55,11 @@ static int booti_setup(bootm_headers_t *images) > * If we are not at the correct run-time location, set the new > * correct location and then move the image there. > */ > - dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset); > + dst = gd->bd->bi_dram[0].start; > + if (le64_to_cpu(ih->flags) & LINUX_PHYSICAL_PLACEMENT) > + dst = round_up(images->ep, SZ_2M); Can you do this as if/else and expand the comment so that it explains that earlier it was recommended to be as close to the start of memory as possible but later a flag was introduced to allow a more flexible placement? Thanks!
diff --git a/cmd/booti.c b/cmd/booti.c index 6c1c998..efa8bc8 100644 --- a/cmd/booti.c +++ b/cmd/booti.c @@ -20,7 +20,7 @@ struct Image_header { uint32_t code1; /* Executable code */ uint64_t text_offset; /* Image load offset, LE */ uint64_t image_size; /* Effective Image size, LE */ - uint64_t res1; /* reserved */ + uint64_t flags; /* Information flags, LE */ uint64_t res2; /* reserved */ uint64_t res3; /* reserved */ uint64_t res4; /* reserved */ @@ -29,6 +29,7 @@ struct Image_header { }; #define LINUX_ARM64_IMAGE_MAGIC 0x644d5241 +#define LINUX_PHYSICAL_PLACEMENT (0x1 << 3) static int booti_setup(bootm_headers_t *images) { @@ -54,7 +55,11 @@ static int booti_setup(bootm_headers_t *images) * If we are not at the correct run-time location, set the new * correct location and then move the image there. */ - dst = gd->bd->bi_dram[0].start + le64_to_cpu(ih->text_offset); + dst = gd->bd->bi_dram[0].start; + if (le64_to_cpu(ih->flags) & LINUX_PHYSICAL_PLACEMENT) + dst = round_up(images->ep, SZ_2M); + + dst += le64_to_cpu(ih->text_offset); unmap_sysmem(ih);