| Message ID | 1370273624-26976-1-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-06-03 at 16:33 +0100, Stefano Stabellini wrote: > Define xen_remap as ioremap_cache (MT_MEMORY and MT_DEVICE_CACHED end up > having the same AttrIndx encoding). The entries in static struct mem_type mem_types[] look entirely different to me. What am I missing? [MT_DEVICE_CACHED] = { /* ioremap_cached */ .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_CACHED, .prot_l1 = PMD_TYPE_TABLE, .prot_sect = PROT_SECT_DEVICE | PMD_SECT_WB, .domain = DOMAIN_IO, }, [MT_MEMORY] = { .prot_pte = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY, .prot_l1 = PMD_TYPE_TABLE, .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE, .domain = DOMAIN_KERNEL, }, I can see in pgtable-3level.h how L_PTE_MT_DEV_CACHED and L_PTE_MT_WRITEBACK are the same but not where the MT_WRITEBACK comes from for MT_MEMORY. Things are less clear in pgtable-2level.h, where one is 0x3 and the other is 0xb. I can see that the entries are the same in armv6_mt_table but how that would apply to a v7 processor? Anyhow, even if I'm prepared to believe that MT_MEMORY and MT_DEVICE_CACHED end up being the same thing (which TBH I am) it seems that the level of abstraction involved makes us vulnerable to future changes subtly breaking things for us. What about: /* Device shared memory */ #define ioremap_shm(cookie,size) __arm_ioremap((cookie), (size), MT_MEMORY) Ian. > Remove include asm/mach/map.h, not unneeded. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > arch/arm/include/asm/xen/page.h | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h > index 30cdacb..359a7b5 100644 > --- a/arch/arm/include/asm/xen/page.h > +++ b/arch/arm/include/asm/xen/page.h > @@ -1,7 +1,6 @@ > #ifndef _ASM_ARM_XEN_PAGE_H > #define _ASM_ARM_XEN_PAGE_H > > -#include <asm/mach/map.h> > #include <asm/page.h> > #include <asm/pgtable.h> > > @@ -88,6 +87,6 @@ static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn) > return __set_phys_to_machine(pfn, mfn); > } > > -#define xen_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY); > +#define xen_remap(cookie, size) ioremap_cached((cookie), (size)); > > #endif /* _ASM_ARM_XEN_PAGE_H */
On Tue, Jun 04, 2013 at 10:20:50AM +0100, Ian Campbell wrote: > On Mon, 2013-06-03 at 16:33 +0100, Stefano Stabellini wrote: > > Define xen_remap as ioremap_cache (MT_MEMORY and MT_DEVICE_CACHED end up > > having the same AttrIndx encoding). > > The entries in static struct mem_type mem_types[] look entirely > different to me. What am I missing? > [MT_DEVICE_CACHED] = { /* ioremap_cached */ > .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_CACHED, > .prot_l1 = PMD_TYPE_TABLE, > .prot_sect = PROT_SECT_DEVICE | PMD_SECT_WB, > .domain = DOMAIN_IO, > }, > [MT_MEMORY] = { > .prot_pte = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY, > .prot_l1 = PMD_TYPE_TABLE, > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE, > .domain = DOMAIN_KERNEL, > }, > > I can see in pgtable-3level.h how L_PTE_MT_DEV_CACHED and > L_PTE_MT_WRITEBACK are the same but not where the MT_WRITEBACK comes > from for MT_MEMORY. Things are less clear in pgtable-2level.h, where one > is 0x3 and the other is 0xb. I can see that the entries are the same in > armv6_mt_table but how that would apply to a v7 processor? PROT_PTE_DEVICE and PROT_SECT_DEVICE above don't contain any memory type information, just attributes/permission - present, young, dirty and XN: #define PROT_PTE_DEVICE L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN #define PROT_SECT_DEVICE PMD_TYPE_SECT|PMD_SECT_AP_WRITE The memory type is given by the L_PTE_MT_DEV_CACHED and PMD_SECT_WB macros. Let's take prot_sect first as it's simpler. For MT_DEVICE_CACHED we have: .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_WB For MT_MEMORY we have: .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE The cache policy is added later to MT_MEMORY which is either WB or WBWA (based on SMP, no particular reason as these are just processor hints; for some historical reasons we enabled WBWA for ARM11MPCore but we could leave it on all the time). Similarly for prot_pte, present, young, dirty are the same. Regarding the type, on ARMv7 (with or without LPAE) we use TEX remapping and L_PTE_MT_DEVICE has the same index (3-bit TEX[0], C, B for NMRR/PRRR or TEX[2:0] for MAIR0/MAIR1 registers) as Normal Cacheable Writeback memory (there is no such thing as Device memory with cacheability attributes, only Normal Cacheable memory). We have XN in addition for MT_DEVICE_CACHED to prevent speculative instruction fetches. However, you still get speculative D-cache line fills since the memory is Normal Cacheable. > Anyhow, even if I'm prepared to believe that MT_MEMORY and > MT_DEVICE_CACHED end up being the same thing (which TBH I am) it seems > that the level of abstraction involved makes us vulnerable to future > changes subtly breaking things for us. What about: > > /* Device shared memory */ > #define ioremap_shm(cookie,size) __arm_ioremap((cookie), (size), MT_MEMORY) For my understanding, what is Xen doing with such mapping? I would rather make ioremap_cached() use MT_MEMORY on ARMv6/v7 (or make it closer to that, I'm not sure about the implications on ARMv5 and earlier but for newer architectures I don't see the point in pretending to have Cacheable Device memory). That's however for Russell to comment.
On Tue, 4 Jun 2013, Catalin Marinas wrote: > On Tue, Jun 04, 2013 at 10:20:50AM +0100, Ian Campbell wrote: > > On Mon, 2013-06-03 at 16:33 +0100, Stefano Stabellini wrote: > > > Define xen_remap as ioremap_cache (MT_MEMORY and MT_DEVICE_CACHED end up > > > having the same AttrIndx encoding). > > > > The entries in static struct mem_type mem_types[] look entirely > > different to me. What am I missing? > > [MT_DEVICE_CACHED] = { /* ioremap_cached */ > > .prot_pte = PROT_PTE_DEVICE | L_PTE_MT_DEV_CACHED, > > .prot_l1 = PMD_TYPE_TABLE, > > .prot_sect = PROT_SECT_DEVICE | PMD_SECT_WB, > > .domain = DOMAIN_IO, > > }, > > [MT_MEMORY] = { > > .prot_pte = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY, > > .prot_l1 = PMD_TYPE_TABLE, > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE, > > .domain = DOMAIN_KERNEL, > > }, > > > > I can see in pgtable-3level.h how L_PTE_MT_DEV_CACHED and > > L_PTE_MT_WRITEBACK are the same but not where the MT_WRITEBACK comes > > from for MT_MEMORY. Things are less clear in pgtable-2level.h, where one > > is 0x3 and the other is 0xb. I can see that the entries are the same in > > armv6_mt_table but how that would apply to a v7 processor? > > PROT_PTE_DEVICE and PROT_SECT_DEVICE above don't contain any memory type > information, just attributes/permission - present, young, dirty and XN: > > #define PROT_PTE_DEVICE L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN > #define PROT_SECT_DEVICE PMD_TYPE_SECT|PMD_SECT_AP_WRITE > > The memory type is given by the L_PTE_MT_DEV_CACHED and PMD_SECT_WB > macros. Let's take prot_sect first as it's simpler. For MT_DEVICE_CACHED > we have: > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_WB > > For MT_MEMORY we have: > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE > > The cache policy is added later to MT_MEMORY which is either WB or WBWA > (based on SMP, no particular reason as these are just processor hints; > for some historical reasons we enabled WBWA for ARM11MPCore but we could > leave it on all the time). > > Similarly for prot_pte, present, young, dirty are the same. > > Regarding the type, on ARMv7 (with or without LPAE) we use TEX remapping > and L_PTE_MT_DEVICE has the same index (3-bit TEX[0], C, B for NMRR/PRRR > or TEX[2:0] for MAIR0/MAIR1 registers) as Normal Cacheable Writeback > memory (there is no such thing as Device memory with cacheability > attributes, only Normal Cacheable memory). > > We have XN in addition for MT_DEVICE_CACHED to prevent speculative > instruction fetches. However, you still get speculative D-cache line > fills since the memory is Normal Cacheable. > > > Anyhow, even if I'm prepared to believe that MT_MEMORY and > > MT_DEVICE_CACHED end up being the same thing (which TBH I am) it seems > > that the level of abstraction involved makes us vulnerable to future > > changes subtly breaking things for us. What about: > > > > /* Device shared memory */ > > #define ioremap_shm(cookie,size) __arm_ioremap((cookie), (size), MT_MEMORY) > > For my understanding, what is Xen doing with such mapping? I would > rather make ioremap_cached() use MT_MEMORY on ARMv6/v7 (or make it > closer to that, I'm not sure about the implications on ARMv5 and earlier > but for newer architectures I don't see the point in pretending to have > Cacheable Device memory). That's however for Russell to comment. Xen guests share these pages with one another and place a lockless ring buffer on it for bidirectional communication. So the page that is being ioremapped actually belongs to another guest and it's RAM.
On Tue, Jun 04, 2013 at 12:28:34PM +0100, Catalin Marinas wrote: > PROT_PTE_DEVICE and PROT_SECT_DEVICE above don't contain any memory type > information, just attributes/permission - present, young, dirty and XN: > > #define PROT_PTE_DEVICE L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN > #define PROT_SECT_DEVICE PMD_TYPE_SECT|PMD_SECT_AP_WRITE > > The memory type is given by the L_PTE_MT_DEV_CACHED and PMD_SECT_WB > macros. Let's take prot_sect first as it's simpler. For MT_DEVICE_CACHED > we have: > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_WB > > For MT_MEMORY we have: > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE > > The cache policy is added later to MT_MEMORY which is either WB or WBWA > (based on SMP, no particular reason as these are just processor hints; > for some historical reasons we enabled WBWA for ARM11MPCore but we could > leave it on all the time). They may be reported to be just hints, but SMP, particularly ARM11MPCore, the SMP guys at ARM Ltd were very particular about _requiring_ and stating that it is required that WBWA must be used in the page tables for SMP, and not WB. That suggests while the ARM ARM may say that they're hints, there's slightly more to it when it comes to SMP, and WBWA is a hard requirement there. Indeed, that's something we enforce in the kernel because of the statements from the SMP development group. > Similarly for prot_pte, present, young, dirty are the same. > > Regarding the type, on ARMv7 (with or without LPAE) we use TEX remapping > and L_PTE_MT_DEVICE has the same index (3-bit TEX[0], C, B for NMRR/PRRR > or TEX[2:0] for MAIR0/MAIR1 registers) as Normal Cacheable Writeback > memory (there is no such thing as Device memory with cacheability > attributes, only Normal Cacheable memory). You don't mean L_PTE_MT_DEVICE there. Thankfully, L_PTE_MT_DEVICE doesn't exist, so it's not that confusing. What you mean is L_PTE_MT_DEV_CACHED, which _does_ map to "normal cacheable writeback memory" irrespective of the mappings which the kernel uses for RAM. However, that mapping type (which is used for MT_DEVICE_CACHED) should not be used if you really do want normal memory. And using MT_* definitions _not_ in asm/io.h with ioremap() is really... silly. It's not something that is intended, nor is it something which I intend to be supportable into the future on ARM. That's why the definitions are separate - see the comments in asm/io.h about "types 4 onwards are undefined for ioremap" - I'm not sure how more explicit to make that statement. And as I _have_ made the statement, if I see people violating it, I won't care about their code if/when I decide to change the ioremap() behaviour.
On Tue, 4 Jun 2013, Russell King - ARM Linux wrote: > On Tue, Jun 04, 2013 at 12:28:34PM +0100, Catalin Marinas wrote: > > PROT_PTE_DEVICE and PROT_SECT_DEVICE above don't contain any memory type > > information, just attributes/permission - present, young, dirty and XN: > > > > #define PROT_PTE_DEVICE L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN > > #define PROT_SECT_DEVICE PMD_TYPE_SECT|PMD_SECT_AP_WRITE > > > > The memory type is given by the L_PTE_MT_DEV_CACHED and PMD_SECT_WB > > macros. Let's take prot_sect first as it's simpler. For MT_DEVICE_CACHED > > we have: > > > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_WB > > > > For MT_MEMORY we have: > > > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE > > > > The cache policy is added later to MT_MEMORY which is either WB or WBWA > > (based on SMP, no particular reason as these are just processor hints; > > for some historical reasons we enabled WBWA for ARM11MPCore but we could > > leave it on all the time). > > They may be reported to be just hints, but SMP, particularly ARM11MPCore, > the SMP guys at ARM Ltd were very particular about _requiring_ and stating > that it is required that WBWA must be used in the page tables for SMP, > and not WB. That suggests while the ARM ARM may say that they're hints, > there's slightly more to it when it comes to SMP, and WBWA is a hard > requirement there. > > Indeed, that's something we enforce in the kernel because of the statements > from the SMP development group. > > > Similarly for prot_pte, present, young, dirty are the same. > > > > Regarding the type, on ARMv7 (with or without LPAE) we use TEX remapping > > and L_PTE_MT_DEVICE has the same index (3-bit TEX[0], C, B for NMRR/PRRR > > or TEX[2:0] for MAIR0/MAIR1 registers) as Normal Cacheable Writeback > > memory (there is no such thing as Device memory with cacheability > > attributes, only Normal Cacheable memory). > > You don't mean L_PTE_MT_DEVICE there. Thankfully, L_PTE_MT_DEVICE doesn't > exist, so it's not that confusing. What you mean is L_PTE_MT_DEV_CACHED, > which _does_ map to "normal cacheable writeback memory" irrespective of > the mappings which the kernel uses for RAM. > > However, that mapping type (which is used for MT_DEVICE_CACHED) should > not be used if you really do want normal memory. And using MT_* definitions > _not_ in asm/io.h with ioremap() is really... silly. It's not something > that is intended, nor is it something which I intend to be supportable > into the future on ARM. That's why the definitions are separate - see > the comments in asm/io.h about "types 4 onwards are undefined for > ioremap" - I'm not sure how more explicit to make that statement. And > as I _have_ made the statement, if I see people violating it, I won't > care about their code if/when I decide to change the ioremap() behaviour. Fair enough. In that case what do you suggest we should do? Given that ioremap_cached is already taken for "device cacheable memory", maybe we could introduce ioremap_memory for "normal memory" on ARM and ARM64: #define ioremap_memory(cookie,size) __arm_ioremap((cookie), (size), MT_MEMORY) that would require us moving the MT_MEMORY definition to arch/arm/include/asm/io.h though. I am open to any suggestions.
On Tue, Jun 04, 2013 at 02:58:57PM +0100, Russell King - ARM Linux wrote: > On Tue, Jun 04, 2013 at 12:28:34PM +0100, Catalin Marinas wrote: > > PROT_PTE_DEVICE and PROT_SECT_DEVICE above don't contain any memory type > > information, just attributes/permission - present, young, dirty and XN: > > > > #define PROT_PTE_DEVICE L_PTE_PRESENT|L_PTE_YOUNG|L_PTE_DIRTY|L_PTE_XN > > #define PROT_SECT_DEVICE PMD_TYPE_SECT|PMD_SECT_AP_WRITE > > > > The memory type is given by the L_PTE_MT_DEV_CACHED and PMD_SECT_WB > > macros. Let's take prot_sect first as it's simpler. For MT_DEVICE_CACHED > > we have: > > > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_WB > > > > For MT_MEMORY we have: > > > > .prot_sect = PMD_TYPE_SECT | PMD_SECT_AP_WRITE > > > > The cache policy is added later to MT_MEMORY which is either WB or WBWA > > (based on SMP, no particular reason as these are just processor hints; > > for some historical reasons we enabled WBWA for ARM11MPCore but we could > > leave it on all the time). > > They may be reported to be just hints, but SMP, particularly ARM11MPCore, > the SMP guys at ARM Ltd were very particular about _requiring_ and stating > that it is required that WBWA must be used in the page tables for SMP, > and not WB. That suggests while the ARM ARM may say that they're hints, > there's slightly more to it when it comes to SMP, and WBWA is a hard > requirement there. It may have been a hard requirement in the early days (FPGA?), I don't fully remember. The ARM11MPCore TRM for r1p0 states "Inner Write-Back No Allocate on Write behaves as Write-back Write-Allocate". Anyway, my comment above was for leaving it on all the time (v6 and v7). > > Similarly for prot_pte, present, young, dirty are the same. > > > > Regarding the type, on ARMv7 (with or without LPAE) we use TEX remapping > > and L_PTE_MT_DEVICE has the same index (3-bit TEX[0], C, B for NMRR/PRRR > > or TEX[2:0] for MAIR0/MAIR1 registers) as Normal Cacheable Writeback > > memory (there is no such thing as Device memory with cacheability > > attributes, only Normal Cacheable memory). > > You don't mean L_PTE_MT_DEVICE there. Thankfully, L_PTE_MT_DEVICE doesn't > exist, so it's not that confusing. What you mean is L_PTE_MT_DEV_CACHED, > which _does_ map to "normal cacheable writeback memory" irrespective of > the mappings which the kernel uses for RAM. Yes, that's what I meant. > However, that mapping type (which is used for MT_DEVICE_CACHED) should > not be used if you really do want normal memory. Well, you want some range mapped as cacheable, so on ARM MT_DEVICE_CACHED gives you normal memory. I'm not sure in the Xen context where this memory comes from, it looks like some physical address not under kernel control. There are other ways to map this but ioremap_cached() seems right for this situation (x86 has a similar definition for xen_remap). BTW, it looks like 3 architectures call this 'ioremap_cached' while other 3 (or 4) call it 'ioremap_cache'. Not a standard API. > And using MT_* definitions > _not_ in asm/io.h with ioremap() is really... silly. It's not something > that is intended, nor is it something which I intend to be supportable > into the future on ARM. That's why the definitions are separate - see > the comments in asm/io.h about "types 4 onwards are undefined for > ioremap" - I'm not sure how more explicit to make that statement. And > as I _have_ made the statement, if I see people violating it, I won't > care about their code if/when I decide to change the ioremap() behaviour. That's a fair point.
diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h index 30cdacb..359a7b5 100644 --- a/arch/arm/include/asm/xen/page.h +++ b/arch/arm/include/asm/xen/page.h @@ -1,7 +1,6 @@ #ifndef _ASM_ARM_XEN_PAGE_H #define _ASM_ARM_XEN_PAGE_H -#include <asm/mach/map.h> #include <asm/page.h> #include <asm/pgtable.h> @@ -88,6 +87,6 @@ static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn) return __set_phys_to_machine(pfn, mfn); } -#define xen_remap(cookie, size) __arm_ioremap((cookie), (size), MT_MEMORY); +#define xen_remap(cookie, size) ioremap_cached((cookie), (size)); #endif /* _ASM_ARM_XEN_PAGE_H */
Define xen_remap as ioremap_cache (MT_MEMORY and MT_DEVICE_CACHED end up having the same AttrIndx encoding). Remove include asm/mach/map.h, not unneeded. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- arch/arm/include/asm/xen/page.h | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)