| Message ID | 1394640969-25583-3-git-send-email-julien.grall@linaro.org |
|---|---|
| State | RFC, archived |
| Headers | show |
On Wed, 2014-03-12 at 16:15 +0000, Julien Grall wrote: > copy_addr s/copy_p?addr/copy_from_paddr/ throughout the subject and changelog. > is only used with BUFFERABLE, there is some place where DEV_SHARED > was used by mistake. A hangover from flash support or something else? > The parameter "attrindx" can be safely remove and let copy_paddr to map > every page with BUFFERABLE attribute. What about if e.g. the firmware provides a dtb which is in flash or where the kernel/initrd which it references are in flash. There's nothing inherently wrong with that is there? Maybe BUFFERABLE is still appropriate there for r/o use. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/domain_build.c | 2 +- > xen/arch/arm/kernel.c | 15 +++++++-------- > xen/arch/arm/setup.c | 2 +- > xen/include/asm-arm/setup.h | 2 +- > 4 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 5ca2f15..7bb2c28 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -976,7 +976,7 @@ static void initrd_load(struct kernel_info *kinfo) > > dst = map_domain_page(ma>>PAGE_SHIFT); > > - copy_from_paddr(dst + s, paddr + offs, l, BUFFERABLE); > + copy_from_paddr(dst + s, paddr + offs, l); > > unmap_domain_page(dst); > offs += l; > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index 492ce6d..0bc7eb1 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -40,7 +40,7 @@ struct minimal_dtb_header { > * @paddr: source physical address > * @len: length to copy > */ > -void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx) > +void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len) > { > void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC); > > @@ -52,7 +52,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx) > s = paddr & (PAGE_SIZE-1); > l = min(PAGE_SIZE - s, len); > > - set_fixmap(FIXMAP_MISC, p, attrindx); > + set_fixmap(FIXMAP_MISC, p, BUFFERABLE); > memcpy(dst, src + s, l); > clean_xen_dcache_va_range(dst, l); > > @@ -145,7 +145,7 @@ static void kernel_zimage_load(struct kernel_info *info) > > dst = map_domain_page(ma>>PAGE_SHIFT); > > - copy_from_paddr(dst + s, paddr + offs, l, BUFFERABLE); > + copy_from_paddr(dst + s, paddr + offs, l); > > unmap_domain_page(dst); > offs += l; > @@ -178,7 +178,7 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info, > if ( size < sizeof(zimage) ) > return -EINVAL; > > - copy_from_paddr(&zimage, addr, sizeof(zimage), DEV_SHARED); > + copy_from_paddr(&zimage, addr, sizeof(zimage)); > > if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && > zimage.magic1 != ZIMAGE64_MAGIC_V1 ) > @@ -223,7 +223,7 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info, > if ( size < ZIMAGE32_HEADER_LEN ) > return -EINVAL; > > - copy_from_paddr(zimage, addr, sizeof(zimage), DEV_SHARED); > + copy_from_paddr(zimage, addr, sizeof(zimage)); > > if (zimage[ZIMAGE32_MAGIC_OFFSET/4] != ZIMAGE32_MAGIC) > return -EINVAL; > @@ -239,8 +239,7 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info, > */ > if ( addr + end - start + sizeof(dtb_hdr) <= size ) > { > - copy_from_paddr(&dtb_hdr, addr + end - start, > - sizeof(dtb_hdr), DEV_SHARED); > + copy_from_paddr(&dtb_hdr, addr + end - start, sizeof(dtb_hdr)); > if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { > end += be32_to_cpu(dtb_hdr.total_size); > > @@ -311,7 +310,7 @@ static int kernel_try_elf_prepare(struct kernel_info *info, > if ( info->kernel_img == NULL ) > panic("Cannot allocate temporary buffer for kernel"); > > - copy_from_paddr(info->kernel_img, addr, size, BUFFERABLE); > + copy_from_paddr(info->kernel_img, addr, size); > > if ( (rc = elf_init(&info->elf.elf, info->kernel_img, size )) != 0 ) > goto err; > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 9480f42..959744e 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -488,7 +488,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) > > /* Copy the DTB. */ > fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1)); > - copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE); > + copy_from_paddr(fdt, dtb_paddr, dtb_size); > device_tree_flattened = fdt; > > /* Add non-xenheap memory */ > diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h > index 44a3b4d..b09f688 100644 > --- a/xen/include/asm-arm/setup.h > +++ b/xen/include/asm-arm/setup.h > @@ -5,7 +5,7 @@ > > void arch_init_memory(void); > > -void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx); > +void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > void arch_get_xen_caps(xen_capabilities_info_t *info); >
Hi Ian, On 03/14/2014 05:14 PM, Ian Campbell wrote: > On Wed, 2014-03-12 at 16:15 +0000, Julien Grall wrote: >> copy_addr > > s/copy_p?addr/copy_from_paddr/ throughout the subject and changelog. Right. I will fix it in the next series. >> is only used with BUFFERABLE, there is some place where DEV_SHARED >> was used by mistake. > > A hangover from flash support or something else? I guess, DEV_SHARED was using in few place mainly when Xen is loading a small amount of data. Even before this patch, info->load_attr should have been used. Do you think I need to send a patch for Xen 4.4? >> The parameter "attrindx" can be safely remove and let copy_paddr to map >> every page with BUFFERABLE attribute. > > What about if e.g. the firmware provides a dtb which is in flash or > where the kernel/initrd which it references are in flash. There's > nothing inherently wrong with that is there? Maybe BUFFERABLE is still > appropriate there for r/o use. Even before this patch, Xen was using BUFFERABLE when the kernel/initrd is described in the device tree. I don't think it's easily possible to know if the range belongs to the RAM or not. Maybe we can use UNCACHE in this context? Regards,
On Fri, 2014-03-14 at 18:02 +0000, Julien Grall wrote: > Hi Ian, > > On 03/14/2014 05:14 PM, Ian Campbell wrote: > > On Wed, 2014-03-12 at 16:15 +0000, Julien Grall wrote: > >> copy_addr > > > > s/copy_p?addr/copy_from_paddr/ throughout the subject and changelog. > > Right. I will fix it in the next series. > > >> is only used with BUFFERABLE, there is some place where DEV_SHARED > >> was used by mistake. > > > > A hangover from flash support or something else? > > I guess, DEV_SHARED was using in few place mainly when Xen is loading a > small amount of data. Even before this patch, info->load_attr should > have been used. > > Do you think I need to send a patch for Xen 4.4? Is it actively harmful or just not ideal? > >> The parameter "attrindx" can be safely remove and let copy_paddr to map > >> every page with BUFFERABLE attribute. > > > > What about if e.g. the firmware provides a dtb which is in flash or > > where the kernel/initrd which it references are in flash. There's > > nothing inherently wrong with that is there? Maybe BUFFERABLE is still > > appropriate there for r/o use. > > Even before this patch, Xen was using BUFFERABLE when the kernel/initrd > is described in the device tree. I don't think it's easily possible to > know if the range belongs to the RAM or not. Maybe we can use UNCACHE in > this context? True. Lets stick with BUFFERABLE (i.e. assume RAM) until someone comes along with things in flash, then we can actually test things. (or they never show up and we save ourselves some effort). Ian.
Hi Ian, On 03/17/2014 10:13 AM, Ian Campbell wrote: > On Fri, 2014-03-14 at 18:02 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 03/14/2014 05:14 PM, Ian Campbell wrote: >>> On Wed, 2014-03-12 at 16:15 +0000, Julien Grall wrote: >>>> copy_addr >>> >>> s/copy_p?addr/copy_from_paddr/ throughout the subject and changelog. >> >> Right. I will fix it in the next series. >> >>>> is only used with BUFFERABLE, there is some place where DEV_SHARED >>>> was used by mistake. >>> >>> A hangover from flash support or something else? >> >> I guess, DEV_SHARED was using in few place mainly when Xen is loading a >> small amount of data. Even before this patch, info->load_attr should >> have been used. >> >> Do you think I need to send a patch for Xen 4.4? > > Is it actively harmful or just not ideal? From the ARM ARM A3.5.1, we should use Normal memory attribute even for the flash. I'm not sure what is the behavior when Normal memory is access with Device memory attribute. Regards,
On Mon, 2014-03-17 at 11:53 +0000, Julien Grall wrote: > Hi Ian, > > On 03/17/2014 10:13 AM, Ian Campbell wrote: > > On Fri, 2014-03-14 at 18:02 +0000, Julien Grall wrote: > >> Hi Ian, > >> > >> On 03/14/2014 05:14 PM, Ian Campbell wrote: > >>> On Wed, 2014-03-12 at 16:15 +0000, Julien Grall wrote: > >>>> copy_addr > >>> > >>> s/copy_p?addr/copy_from_paddr/ throughout the subject and changelog. > >> > >> Right. I will fix it in the next series. > >> > >>>> is only used with BUFFERABLE, there is some place where DEV_SHARED > >>>> was used by mistake. > >>> > >>> A hangover from flash support or something else? > >> > >> I guess, DEV_SHARED was using in few place mainly when Xen is loading a > >> small amount of data. Even before this patch, info->load_attr should > >> have been used. > >> > >> Do you think I need to send a patch for Xen 4.4? > > > > Is it actively harmful or just not ideal? > > From the ARM ARM A3.5.1, we should use Normal memory attribute even for > the flash. For r/o access that does make sense. I expect when programming or sending control commands more cleverness is needed, but we needn't worry here. Good. > I'm not sure what is the behavior when Normal memory is access with > Device memory attribute. It's just pointlessly uncached and strongly ordered I think, so it's a performance hit. Ian.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 5ca2f15..7bb2c28 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -976,7 +976,7 @@ static void initrd_load(struct kernel_info *kinfo) dst = map_domain_page(ma>>PAGE_SHIFT); - copy_from_paddr(dst + s, paddr + offs, l, BUFFERABLE); + copy_from_paddr(dst + s, paddr + offs, l); unmap_domain_page(dst); offs += l; diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 492ce6d..0bc7eb1 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -40,7 +40,7 @@ struct minimal_dtb_header { * @paddr: source physical address * @len: length to copy */ -void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx) +void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len) { void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC); @@ -52,7 +52,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx) s = paddr & (PAGE_SIZE-1); l = min(PAGE_SIZE - s, len); - set_fixmap(FIXMAP_MISC, p, attrindx); + set_fixmap(FIXMAP_MISC, p, BUFFERABLE); memcpy(dst, src + s, l); clean_xen_dcache_va_range(dst, l); @@ -145,7 +145,7 @@ static void kernel_zimage_load(struct kernel_info *info) dst = map_domain_page(ma>>PAGE_SHIFT); - copy_from_paddr(dst + s, paddr + offs, l, BUFFERABLE); + copy_from_paddr(dst + s, paddr + offs, l); unmap_domain_page(dst); offs += l; @@ -178,7 +178,7 @@ static int kernel_try_zimage64_prepare(struct kernel_info *info, if ( size < sizeof(zimage) ) return -EINVAL; - copy_from_paddr(&zimage, addr, sizeof(zimage), DEV_SHARED); + copy_from_paddr(&zimage, addr, sizeof(zimage)); if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && zimage.magic1 != ZIMAGE64_MAGIC_V1 ) @@ -223,7 +223,7 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info, if ( size < ZIMAGE32_HEADER_LEN ) return -EINVAL; - copy_from_paddr(zimage, addr, sizeof(zimage), DEV_SHARED); + copy_from_paddr(zimage, addr, sizeof(zimage)); if (zimage[ZIMAGE32_MAGIC_OFFSET/4] != ZIMAGE32_MAGIC) return -EINVAL; @@ -239,8 +239,7 @@ static int kernel_try_zimage32_prepare(struct kernel_info *info, */ if ( addr + end - start + sizeof(dtb_hdr) <= size ) { - copy_from_paddr(&dtb_hdr, addr + end - start, - sizeof(dtb_hdr), DEV_SHARED); + copy_from_paddr(&dtb_hdr, addr + end - start, sizeof(dtb_hdr)); if (be32_to_cpu(dtb_hdr.magic) == DTB_MAGIC) { end += be32_to_cpu(dtb_hdr.total_size); @@ -311,7 +310,7 @@ static int kernel_try_elf_prepare(struct kernel_info *info, if ( info->kernel_img == NULL ) panic("Cannot allocate temporary buffer for kernel"); - copy_from_paddr(info->kernel_img, addr, size, BUFFERABLE); + copy_from_paddr(info->kernel_img, addr, size); if ( (rc = elf_init(&info->elf.elf, info->kernel_img, size )) != 0 ) goto err; diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 9480f42..959744e 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -488,7 +488,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) /* Copy the DTB. */ fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1)); - copy_from_paddr(fdt, dtb_paddr, dtb_size, BUFFERABLE); + copy_from_paddr(fdt, dtb_paddr, dtb_size); device_tree_flattened = fdt; /* Add non-xenheap memory */ diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index 44a3b4d..b09f688 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -5,7 +5,7 @@ void arch_init_memory(void); -void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len, int attrindx); +void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); void arch_get_xen_caps(xen_capabilities_info_t *info);
copy_addr is only used with BUFFERABLE, there is some place where DEV_SHARED was used by mistake. The parameter "attrindx" can be safely remove and let copy_paddr to map every page with BUFFERABLE attribute. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/domain_build.c | 2 +- xen/arch/arm/kernel.c | 15 +++++++-------- xen/arch/arm/setup.c | 2 +- xen/include/asm-arm/setup.h | 2 +- 4 files changed, 10 insertions(+), 11 deletions(-)