| Message ID | 1368152307-598-12-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2013-05-10 at 03:17 +0100, Julien Grall wrote: > Map physical range in virtual memory with a specific mapping attribute. > Also add new mapping attributes for ARM: PAGE_HYPERVISOR_NOCACHE > and PAGE_HYPERVISOR_WC. I think it would be useful, e.g. if when we want to reuse Linux SYS MMU code, to follow the Linux convention here which is ioremap_(no)cache, ioremap_wc etc. > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 29447ef..a667db4 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -429,6 +429,8 @@ void __init start_xen(unsigned long boot_phys_offset, > setup_pagetables(boot_phys_offset, get_xen_paddr()); > setup_mm(fdt_paddr, fdt_size); > > + vm_init(); > + > #ifdef EARLY_UART_ADDRESS > /* TODO Need to get device tree or command line for UART address */ > pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); > @@ -483,8 +485,6 @@ void __init start_xen(unsigned long boot_phys_offset, > > console_init_postirq(); > > - vm_init(); > - > do_presmp_initcalls(); > > for_each_present_cpu ( i ) This movement seems to be unrelated to the purpose of this patch?
> @@ -59,6 +59,8 @@ > #define DEV_CACHED WRITEBACK > > #define PAGE_HYPERVISOR (MATTR_MEM) I should have noticed this when Stefano's original vmap patch went in but this is wrong. MATTR_* are Second stage paging attributes (i.e. p2m ones) and are not suitable for the Xen first stage page tables. MATTR_MEM == 0xf, which I think will be truncated to 0x7 when written to the ai field in the PT (which is only 3 bits) and so this is equivalent to using WRITEALLOC. I suspect that isn't at all desirable... Stefano, can you fix this please? Ian.
On 05/10/2013 10:13 AM, Ian Campbell wrote: > On Fri, 2013-05-10 at 03:17 +0100, Julien Grall wrote: >> Map physical range in virtual memory with a specific mapping attribute. >> Also add new mapping attributes for ARM: PAGE_HYPERVISOR_NOCACHE >> and PAGE_HYPERVISOR_WC. > > I think it would be useful, e.g. if when we want to reuse Linux SYS MMU > code, to follow the Linux convention here which is ioremap_(no)cache, > ioremap_wc etc. Is macro for those functions is sufficient? >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 29447ef..a667db4 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -429,6 +429,8 @@ void __init start_xen(unsigned long boot_phys_offset, >> setup_pagetables(boot_phys_offset, get_xen_paddr()); >> setup_mm(fdt_paddr, fdt_size); >> >> + vm_init(); >> + >> #ifdef EARLY_UART_ADDRESS >> /* TODO Need to get device tree or command line for UART address */ >> pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); >> @@ -483,8 +485,6 @@ void __init start_xen(unsigned long boot_phys_offset, >> >> console_init_postirq(); >> >> - vm_init(); >> - >> do_presmp_initcalls(); >> >> for_each_present_cpu ( i ) > > This movement seems to be unrelated to the purpose of this patch? vm_init initialize vmap code, which is used by ioremap_attr. The function was called to late.
On Fri, 2013-05-10 at 13:00 +0100, Julien Grall wrote: > On 05/10/2013 10:13 AM, Ian Campbell wrote: > > > On Fri, 2013-05-10 at 03:17 +0100, Julien Grall wrote: > >> Map physical range in virtual memory with a specific mapping attribute. > >> Also add new mapping attributes for ARM: PAGE_HYPERVISOR_NOCACHE > >> and PAGE_HYPERVISOR_WC. > > > > I think it would be useful, e.g. if when we want to reuse Linux SYS MMU > > code, to follow the Linux convention here which is ioremap_(no)cache, > > ioremap_wc etc. > Is macro for those functions is sufficient? If need be that's fine, is there anything which breaks if you use a static inline? > >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > >> index 29447ef..a667db4 100644 > >> --- a/xen/arch/arm/setup.c > >> +++ b/xen/arch/arm/setup.c > >> @@ -429,6 +429,8 @@ void __init start_xen(unsigned long boot_phys_offset, > >> setup_pagetables(boot_phys_offset, get_xen_paddr()); > >> setup_mm(fdt_paddr, fdt_size); > >> > >> + vm_init(); > >> + > >> #ifdef EARLY_UART_ADDRESS > >> /* TODO Need to get device tree or command line for UART address */ > >> pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); > >> @@ -483,8 +485,6 @@ void __init start_xen(unsigned long boot_phys_offset, > >> > >> console_init_postirq(); > >> > >> - vm_init(); > >> - > >> do_presmp_initcalls(); > >> > >> for_each_present_cpu ( i ) > > > > This movement seems to be unrelated to the purpose of this patch? > > vm_init initialize vmap code, which is used by ioremap_attr. The > function was called to late. But this patch doesn't add any callers, I presume that a caller will subsequently be introduce which then requires it to be earlier. I think it would be sufficient to simply mention this upcoming caller in the commit message and refer to the need to init sooner. Ian.
On 05/10/2013 01:07 PM, Ian Campbell wrote: > On Fri, 2013-05-10 at 13:00 +0100, Julien Grall wrote: >> On 05/10/2013 10:13 AM, Ian Campbell wrote: >> >>> On Fri, 2013-05-10 at 03:17 +0100, Julien Grall wrote: >>>> Map physical range in virtual memory with a specific mapping attribute. >>>> Also add new mapping attributes for ARM: PAGE_HYPERVISOR_NOCACHE >>>> and PAGE_HYPERVISOR_WC. >>> >>> I think it would be useful, e.g. if when we want to reuse Linux SYS MMU >>> code, to follow the Linux convention here which is ioremap_(no)cache, >>> ioremap_wc etc. > >> Is macro for those functions is sufficient? > > If need be that's fine, is there anything which breaks if you use a > static inline? I will use it on the next version of this patch. >>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>>> index 29447ef..a667db4 100644 >>>> --- a/xen/arch/arm/setup.c >>>> +++ b/xen/arch/arm/setup.c >>>> @@ -429,6 +429,8 @@ void __init start_xen(unsigned long boot_phys_offset, >>>> setup_pagetables(boot_phys_offset, get_xen_paddr()); >>>> setup_mm(fdt_paddr, fdt_size); >>>> >>>> + vm_init(); >>>> + >>>> #ifdef EARLY_UART_ADDRESS >>>> /* TODO Need to get device tree or command line for UART address */ >>>> pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); >>>> @@ -483,8 +485,6 @@ void __init start_xen(unsigned long boot_phys_offset, >>>> >>>> console_init_postirq(); >>>> >>>> - vm_init(); >>>> - >>>> do_presmp_initcalls(); >>>> >>>> for_each_present_cpu ( i ) >>> >>> This movement seems to be unrelated to the purpose of this patch? >> >> vm_init initialize vmap code, which is used by ioremap_attr. The >> function was called to late. > > But this patch doesn't add any callers, I presume that a caller will > subsequently be introduce which then requires it to be earlier. I think > it would be sufficient to simply mention this upcoming caller in the > commit message and refer to the need to init sooner. I will mention it on the commit message.
On Fri, 10 May 2013, Ian Campbell wrote: > > @@ -59,6 +59,8 @@ > > #define DEV_CACHED WRITEBACK > > > > #define PAGE_HYPERVISOR (MATTR_MEM) > > I should have noticed this when Stefano's original vmap patch went in > but this is wrong. > > MATTR_* are Second stage paging attributes (i.e. p2m ones) and are not > suitable for the Xen first stage page tables. > > MATTR_MEM == 0xf, which I think will be truncated to 0x7 when written to > the ai field in the PT (which is only 3 bits) and so this is equivalent > to using WRITEALLOC. I suspect that isn't at all desirable... > > Stefano, can you fix this please? Yep
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 96297d3..10d2869 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -38,6 +38,7 @@ #include <xen/sched.h> #include <xen/vmap.h> #include <xsm/xsm.h> +#include <xen/pfn.h> struct domain *dom_xen, *dom_io, *dom_cow; @@ -608,6 +609,19 @@ void *__init arch_vmap_virt_end(void) return (void *)early_vmap_start; } +/* + * This function should only be used to remap device address ranges + * TODO: add a check to verify this assumption + */ +void *ioremap_attr(paddr_t pa, size_t len, unsigned int attributes) +{ + unsigned long pfn = PFN_DOWN(pa); + unsigned int offs = pa & (PAGE_SIZE - 1); + unsigned int nr = PFN_UP(offs + len); + + return (__vmap(&pfn, nr, 1, 1, attributes) + offs); +} + static int create_xen_table(lpae_t *entry) { void *p; diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 29447ef..a667db4 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -429,6 +429,8 @@ void __init start_xen(unsigned long boot_phys_offset, setup_pagetables(boot_phys_offset, get_xen_paddr()); setup_mm(fdt_paddr, fdt_size); + vm_init(); + #ifdef EARLY_UART_ADDRESS /* TODO Need to get device tree or command line for UART address */ pl011_init(0, FIXMAP_ADDR(FIXMAP_CONSOLE)); @@ -483,8 +485,6 @@ void __init start_xen(unsigned long boot_phys_offset, console_init_postirq(); - vm_init(); - do_presmp_initcalls(); for_each_present_cpu ( i ) diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index 26c271e..4ac8fab 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -155,6 +155,8 @@ extern void set_fixmap(unsigned map, unsigned long mfn, unsigned attributes); extern void clear_fixmap(unsigned map); /* map a 2MB aligned physical range in virtual memory. */ void* early_ioremap(paddr_t start, size_t len, unsigned attributes); +/* map a physical range in virtual memory */ +void *ioremap_attr(paddr_t start, size_t len, unsigned attributes); #define mfn_valid(mfn) ({ \ unsigned long __m_f_n = (mfn); \ diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index fd6946e..13fbd78 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -59,6 +59,8 @@ #define DEV_CACHED WRITEBACK #define PAGE_HYPERVISOR (MATTR_MEM) +#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED) +#define PAGE_HYPERVISOR_WC (DEV_WC) #define MAP_SMALL_PAGES PAGE_HYPERVISOR /*
Map physical range in virtual memory with a specific mapping attribute. Also add new mapping attributes for ARM: PAGE_HYPERVISOR_NOCACHE and PAGE_HYPERVISOR_WC. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/mm.c | 14 ++++++++++++++ xen/arch/arm/setup.c | 4 ++-- xen/include/asm-arm/mm.h | 2 ++ xen/include/asm-arm/page.h | 2 ++ 4 files changed, 20 insertions(+), 2 deletions(-)