From: Wei Fu <[email protected]>
This patch follows the standard pure RISC-V Svpbmt extension in
privilege spec to solve the non-coherent SOC dma synchronization
issues.
Here is the svpbmt PTE format:
| 63 | 62-61 | 60-8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
N MT RSW D A G U X W R V
^
Of the Reserved bits [63:54] in a leaf PTE, the high bit is already
allocated (as the N bit), so bits [62:61] are used as the MT (aka
MemType) field. This field specifies one of three memory types that
are close equivalents (or equivalent in effect) to the three main x86
and ARMv8 memory types - as shown in the following table.
RISC-V
Encoding &
MemType RISC-V Description
---------- ------------------------------------------------
00 - PMA Normal Cacheable, No change to implied PMA memory type
01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
11 - Rsvd Reserved for future standard use
The standard protection_map[] needn't be modified because the "PMA"
type keeps the highest bits zero. And the whole modification is
limited in the arch/riscv/* and using a global variable
(__riscv_svpbmt) as _PAGE_DMA_MASK/IO/NC for pgprot_noncached
(&writecombine) in pgtable.h. We also add _PAGE_CHG_MASK to filter
PFN than before.
Enable it in devicetree - (Add "mmu-supports-svpbmt" in cpu node)
- mmu-supports-svpbmt
Signed-off-by: Wei Fu <[email protected]>
Co-developed-by: Liu Shaohua <[email protected]>
Signed-off-by: Liu Shaohua <[email protected]>
Co-developed-by: Guo Ren <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Atish Patra <[email protected]>
Cc: Drew Fustini <[email protected]>
Cc: Wei Fu <[email protected]>
Cc: Wei Wu <[email protected]>
Cc: Chen-Yu Tsai <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Daniel Lustig <[email protected]>
Cc: Greg Favor <[email protected]>
Cc: Andrea Mondelli <[email protected]>
Cc: Jonathan Behrens <[email protected]>
Cc: Xinhaoqu (Freddie) <[email protected]>
Cc: Bill Huffman <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: Allen Baum <[email protected]>
Cc: Josh Scheid <[email protected]>
Cc: Richard Trauben <[email protected]>
---
arch/riscv/include/asm/fixmap.h | 2 +-
arch/riscv/include/asm/pgtable-64.h | 8 ++++--
arch/riscv/include/asm/pgtable-bits.h | 41 +++++++++++++++++++++++++--
arch/riscv/include/asm/pgtable.h | 39 +++++++++++++++++++------
arch/riscv/kernel/cpufeature.c | 32 +++++++++++++++++++++
arch/riscv/mm/init.c | 5 ++++
6 files changed, 112 insertions(+), 15 deletions(-)
diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
index 54cbf07fb4e9..5acd99d08e74 100644
--- a/arch/riscv/include/asm/fixmap.h
+++ b/arch/riscv/include/asm/fixmap.h
@@ -43,7 +43,7 @@ enum fixed_addresses {
__end_of_fixed_addresses
};
-#define FIXMAP_PAGE_IO PAGE_KERNEL
+#define FIXMAP_PAGE_IO PAGE_IOREMAP
#define __early_set_fixmap __set_fixmap
diff --git a/arch/riscv/include/asm/pgtable-64.h b/arch/riscv/include/asm/pgtable-64.h
index 228261aa9628..0b53ea67e91a 100644
--- a/arch/riscv/include/asm/pgtable-64.h
+++ b/arch/riscv/include/asm/pgtable-64.h
@@ -61,12 +61,14 @@ static inline void pud_clear(pud_t *pudp)
static inline pmd_t *pud_pgtable(pud_t pud)
{
- return (pmd_t *)pfn_to_virt(pud_val(pud) >> _PAGE_PFN_SHIFT);
+ return (pmd_t *)pfn_to_virt((pud_val(pud) & _PAGE_CHG_MASK)
+ >> _PAGE_PFN_SHIFT);
}
static inline struct page *pud_page(pud_t pud)
{
- return pfn_to_page(pud_val(pud) >> _PAGE_PFN_SHIFT);
+ return pfn_to_page((pud_val(pud) & _PAGE_CHG_MASK)
+ >> _PAGE_PFN_SHIFT);
}
static inline pmd_t pfn_pmd(unsigned long pfn, pgprot_t prot)
@@ -76,7 +78,7 @@ static inline pmd_t pfn_pmd(unsigned long pfn, pgprot_t prot)
static inline unsigned long _pmd_pfn(pmd_t pmd)
{
- return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
+ return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
}
#define mk_pmd(page, prot) pfn_pmd(page_to_pfn(page), prot)
diff --git a/arch/riscv/include/asm/pgtable-bits.h b/arch/riscv/include/asm/pgtable-bits.h
index 2ee413912926..3b38fe14f169 100644
--- a/arch/riscv/include/asm/pgtable-bits.h
+++ b/arch/riscv/include/asm/pgtable-bits.h
@@ -7,7 +7,7 @@
#define _ASM_RISCV_PGTABLE_BITS_H
/*
- * PTE format:
+ * rv32 PTE format:
* | XLEN-1 10 | 9 8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
* PFN reserved for SW D A G U X W R V
*/
@@ -24,6 +24,42 @@
#define _PAGE_DIRTY (1 << 7) /* Set by hardware on any write */
#define _PAGE_SOFT (1 << 8) /* Reserved for software */
+#ifndef __ASSEMBLY__
+#ifdef CONFIG_64BIT
+/*
+ * rv64 PTE format:
+ * | 63 | 62 61 | 60 54 | 53 10 | 9 8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
+ * N MT RSV PFN reserved for SW D A G U X W R V
+ * [62:61] Memory Type definitions:
+ * 00 - PMA Normal Cacheable, No change to implied PMA memory type
+ * 01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
+ * 10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
+ * 11 - Rsvd Reserved for future standard use
+ */
+#define _SVPBMT_PMA ((unsigned long)0x0 << 61)
+#define _SVPBMT_NC ((unsigned long)0x1 << 61)
+#define _SVPBMT_IO ((unsigned long)0x2 << 61)
+#define _SVPBMT_MASK (_SVPBMT_PMA | _SVPBMT_NC | _SVPBMT_IO)
+
+extern struct __riscv_svpbmt_struct {
+ unsigned long mask;
+ unsigned long mt_pma;
+ unsigned long mt_nc;
+ unsigned long mt_io;
+} __riscv_svpbmt;
+
+#define _PAGE_MT_MASK __riscv_svpbmt.mask
+#define _PAGE_MT_PMA __riscv_svpbmt.mt_pma
+#define _PAGE_MT_NC __riscv_svpbmt.mt_nc
+#define _PAGE_MT_IO __riscv_svpbmt.mt_io
+#else
+#define _PAGE_MT_MASK 0
+#define _PAGE_MT_PMA 0
+#define _PAGE_MT_NC 0
+#define _PAGE_MT_IO 0
+#endif /* CONFIG_64BIT */
+#endif /* __ASSEMBLY__ */
+
#define _PAGE_SPECIAL _PAGE_SOFT
#define _PAGE_TABLE _PAGE_PRESENT
@@ -38,7 +74,8 @@
/* Set of bits to preserve across pte_modify() */
#define _PAGE_CHG_MASK (~(unsigned long)(_PAGE_PRESENT | _PAGE_READ | \
_PAGE_WRITE | _PAGE_EXEC | \
- _PAGE_USER | _PAGE_GLOBAL))
+ _PAGE_USER | _PAGE_GLOBAL | \
+ _PAGE_MT_MASK))
/*
* when all of R/W/X are zero, the PTE is a pointer to the next level
* of the page table; otherwise, it is a leaf PTE.
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 39b550310ec6..3fc70a63e395 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -136,7 +136,8 @@
| _PAGE_PRESENT \
| _PAGE_ACCESSED \
| _PAGE_DIRTY \
- | _PAGE_GLOBAL)
+ | _PAGE_GLOBAL \
+ | _PAGE_MT_PMA)
#define PAGE_KERNEL __pgprot(_PAGE_KERNEL)
#define PAGE_KERNEL_READ __pgprot(_PAGE_KERNEL & ~_PAGE_WRITE)
@@ -146,11 +147,9 @@
#define PAGE_TABLE __pgprot(_PAGE_TABLE)
-/*
- * The RISC-V ISA doesn't yet specify how to query or modify PMAs, so we can't
- * change the properties of memory regions.
- */
-#define _PAGE_IOREMAP _PAGE_KERNEL
+#define _PAGE_IOREMAP ((_PAGE_KERNEL & ~_PAGE_MT_MASK) | _PAGE_MT_IO)
+
+#define PAGE_IOREMAP __pgprot(_PAGE_IOREMAP)
extern pgd_t swapper_pg_dir[];
@@ -230,12 +229,12 @@ static inline unsigned long _pgd_pfn(pgd_t pgd)
static inline struct page *pmd_page(pmd_t pmd)
{
- return pfn_to_page(pmd_val(pmd) >> _PAGE_PFN_SHIFT);
+ return pfn_to_page((pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
}
static inline unsigned long pmd_page_vaddr(pmd_t pmd)
{
- return (unsigned long)pfn_to_virt(pmd_val(pmd) >> _PAGE_PFN_SHIFT);
+ return (unsigned long)pfn_to_virt((pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
}
static inline pte_t pmd_pte(pmd_t pmd)
@@ -251,7 +250,7 @@ static inline pte_t pud_pte(pud_t pud)
/* Yields the page frame number (PFN) of a page table entry */
static inline unsigned long pte_pfn(pte_t pte)
{
- return (pte_val(pte) >> _PAGE_PFN_SHIFT);
+ return ((pte_val(pte) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
}
#define pte_page(x) pfn_to_page(pte_pfn(x))
@@ -490,6 +489,28 @@ static inline int ptep_clear_flush_young(struct vm_area_struct *vma,
return ptep_test_and_clear_young(vma, address, ptep);
}
+#define pgprot_noncached pgprot_noncached
+static inline pgprot_t pgprot_noncached(pgprot_t _prot)
+{
+ unsigned long prot = pgprot_val(_prot);
+
+ prot &= ~_PAGE_MT_MASK;
+ prot |= _PAGE_MT_IO;
+
+ return __pgprot(prot);
+}
+
+#define pgprot_writecombine pgprot_writecombine
+static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
+{
+ unsigned long prot = pgprot_val(_prot);
+
+ prot &= ~_PAGE_MT_MASK;
+ prot |= _PAGE_MT_NC;
+
+ return __pgprot(prot);
+}
+
/*
* THP functions
*/
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index d959d207a40d..a71ebebc468c 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -8,6 +8,7 @@
#include <linux/bitmap.h>
#include <linux/of.h>
+#include <linux/pgtable.h>
#include <asm/processor.h>
#include <asm/hwcap.h>
#include <asm/smp.h>
@@ -59,6 +60,35 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit)
}
EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
+static void __init mmu_supports_svpbmt(void)
+{
+#if defined(CONFIG_MMU) && defined(CONFIG_64BIT)
+ struct device_node *node;
+ const char *str;
+
+ for_each_of_cpu_node(node) {
+ if (of_property_read_string(node, "mmu-type", &str))
+ continue;
+
+ if (!strncmp(str + 6, "none", 4))
+ continue;
+
+ if (!of_property_read_bool(node, "mmu-supports-svpbmt"))
+ return;
+ }
+
+ __riscv_svpbmt.mask = _SVPBMT_MASK;
+ __riscv_svpbmt.mt_pma = _SVPBMT_PMA;
+ __riscv_svpbmt.mt_nc = _SVPBMT_NC;
+ __riscv_svpbmt.mt_io = _SVPBMT_IO;
+#endif
+}
+
+static void __init mmu_supports(void)
+{
+ mmu_supports_svpbmt();
+}
+
void __init riscv_fill_hwcap(void)
{
struct device_node *node;
@@ -67,6 +97,8 @@ void __init riscv_fill_hwcap(void)
size_t i, j, isa_len;
static unsigned long isa2hwcap[256] = {0};
+ mmu_supports();
+
isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I;
isa2hwcap['m'] = isa2hwcap['M'] = COMPAT_HWCAP_ISA_M;
isa2hwcap['a'] = isa2hwcap['A'] = COMPAT_HWCAP_ISA_A;
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index c0cddf0fc22d..d198eabe55d4 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -855,3 +855,8 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
return vmemmap_populate_basepages(start, end, node, NULL);
}
#endif
+
+#ifdef CONFIG_64BIT
+struct __riscv_svpbmt_struct __riscv_svpbmt __ro_after_init;
+EXPORT_SYMBOL(__riscv_svpbmt);
+#endif
--
2.25.4
On Mon, Oct 25, 2021 at 12:06:07PM +0800, [email protected] wrote:
> static inline pmd_t *pud_pgtable(pud_t pud)
> {
> - return (pmd_t *)pfn_to_virt(pud_val(pud) >> _PAGE_PFN_SHIFT);
> + return (pmd_t *)pfn_to_virt((pud_val(pud) & _PAGE_CHG_MASK)
> + >> _PAGE_PFN_SHIFT);
> }
>
> static inline struct page *pud_page(pud_t pud)
> {
> - return pfn_to_page(pud_val(pud) >> _PAGE_PFN_SHIFT);
> + return pfn_to_page((pud_val(pud) & _PAGE_CHG_MASK)
> + >> _PAGE_PFN_SHIFT);
> static inline unsigned long _pmd_pfn(pmd_t pmd)
> {
> - return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
> + return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
> }
The "(pud_val(pud) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT" expression begs
for readable and well-documented helper.
> +#define _SVPBMT_PMA ((unsigned long)0x0 << 61)
> +#define _SVPBMT_NC ((unsigned long)0x1 << 61)
> +#define _SVPBMT_IO ((unsigned long)0x2 << 61)
0UL << 61
1UL << 61
...
> +#define _SVPBMT_MASK (_SVPBMT_PMA | _SVPBMT_NC | _SVPBMT_IO)
> +
> +extern struct __riscv_svpbmt_struct {
> + unsigned long mask;
> + unsigned long mt_pma;
> + unsigned long mt_nc;
> + unsigned long mt_io;
> +} __riscv_svpbmt;
> +
> +#define _PAGE_MT_MASK __riscv_svpbmt.mask
> +#define _PAGE_MT_PMA __riscv_svpbmt.mt_pma
> +#define _PAGE_MT_NC __riscv_svpbmt.mt_nc
> +#define _PAGE_MT_IO __riscv_svpbmt.mt_io
Using a struct over individual variables seems a little odd here.
Also why not use the standard names for these _PAGE bits used by
most other architectures?
> - return (unsigned long)pfn_to_virt(pmd_val(pmd) >> _PAGE_PFN_SHIFT);
> + return (unsigned long)pfn_to_virt((pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
Overly long line, could use a helper. Btw, what is the point in having
this _PAGE_PFN_SHIFT macro to start with?
Hi Christoph,
Great thanks for your review.
On Mon, Oct 25, 2021 at 2:57 PM Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Oct 25, 2021 at 12:06:07PM +0800, [email protected] wrote:
> > static inline pmd_t *pud_pgtable(pud_t pud)
> > {
> > - return (pmd_t *)pfn_to_virt(pud_val(pud) >> _PAGE_PFN_SHIFT);
> > + return (pmd_t *)pfn_to_virt((pud_val(pud) & _PAGE_CHG_MASK)
> > + >> _PAGE_PFN_SHIFT);
> > }
> >
> > static inline struct page *pud_page(pud_t pud)
> > {
> > - return pfn_to_page(pud_val(pud) >> _PAGE_PFN_SHIFT);
> > + return pfn_to_page((pud_val(pud) & _PAGE_CHG_MASK)
> > + >> _PAGE_PFN_SHIFT);
>
> > static inline unsigned long _pmd_pfn(pmd_t pmd)
> > {
> > - return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
> > + return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
> > }
>
> The "(pud_val(pud) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT" expression begs
> for readable and well-documented helper.
>
> > +#define _SVPBMT_PMA ((unsigned long)0x0 << 61)
> > +#define _SVPBMT_NC ((unsigned long)0x1 << 61)
> > +#define _SVPBMT_IO ((unsigned long)0x2 << 61)
>
> 0UL << 61
> 1UL << 61
How about this macro:
#define _SVPBMT_PMA 0x0UL
#define _SVPBMT_NC BIT(61)
#define _SVPBMT_IO BIT(62)
#define _SVPBMT_MASK GENMASK(62, 61)
> ...
>
> > +#define _SVPBMT_MASK (_SVPBMT_PMA | _SVPBMT_NC | _SVPBMT_IO)
> > +
> > +extern struct __riscv_svpbmt_struct {
> > + unsigned long mask;
> > + unsigned long mt_pma;
> > + unsigned long mt_nc;
> > + unsigned long mt_io;
> > +} __riscv_svpbmt;
> > +
> > +#define _PAGE_MT_MASK __riscv_svpbmt.mask
> > +#define _PAGE_MT_PMA __riscv_svpbmt.mt_pma
> > +#define _PAGE_MT_NC __riscv_svpbmt.mt_nc
> > +#define _PAGE_MT_IO __riscv_svpbmt.mt_io
>
> Using a struct over individual variables seems a little odd here.
>
> Also why not use the standard names for these _PAGE bits used by
> most other architectures?
Which names are you suggesting? Would you mind providing an example ?
_PAGE_BIT_ for _PAGE_KERNEL_ ??
>
> > - return (unsigned long)pfn_to_virt(pmd_val(pmd) >> _PAGE_PFN_SHIFT);
> > + return (unsigned long)pfn_to_virt((pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
>
> Overly long line, could use a helper. Btw, what is the point in having
> this _PAGE_PFN_SHIFT macro to start with?
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
Hi Christoph,
I hope I understand this correctly,
On Mon, Oct 25, 2021 at 2:57 PM Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Oct 25, 2021 at 12:06:07PM +0800, [email protected] wrote:
> > static inline pmd_t *pud_pgtable(pud_t pud)
> > {
> > - return (pmd_t *)pfn_to_virt(pud_val(pud) >> _PAGE_PFN_SHIFT);
> > + return (pmd_t *)pfn_to_virt((pud_val(pud) & _PAGE_CHG_MASK)
> > + >> _PAGE_PFN_SHIFT);
> > }
> >
> > static inline struct page *pud_page(pud_t pud)
> > {
> > - return pfn_to_page(pud_val(pud) >> _PAGE_PFN_SHIFT);
> > + return pfn_to_page((pud_val(pud) & _PAGE_CHG_MASK)
> > + >> _PAGE_PFN_SHIFT);
>
> > static inline unsigned long _pmd_pfn(pmd_t pmd)
> > {
> > - return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
> > + return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
> > }
>
> The "(pud_val(pud) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT" expression begs
> for readable and well-documented helper.
How about these:
#define _chg_of_pmd(pmd) (pmd_val(pmd) & _PAGE_CHG_MASK)
#define _chg_of_pud(pud) (pud_val(pud) & _PAGE_CHG_MASK)
>
> > +#define _SVPBMT_PMA ((unsigned long)0x0 << 61)
> > +#define _SVPBMT_NC ((unsigned long)0x1 << 61)
> > +#define _SVPBMT_IO ((unsigned long)0x2 << 61)
>
> 0UL << 61
> 1UL << 61
> ...
>
> > +#define _SVPBMT_MASK (_SVPBMT_PMA | _SVPBMT_NC | _SVPBMT_IO)
> > +
> > +extern struct __riscv_svpbmt_struct {
> > + unsigned long mask;
> > + unsigned long mt_pma;
> > + unsigned long mt_nc;
> > + unsigned long mt_io;
> > +} __riscv_svpbmt;
> > +
> > +#define _PAGE_MT_MASK __riscv_svpbmt.mask
> > +#define _PAGE_MT_PMA __riscv_svpbmt.mt_pma
> > +#define _PAGE_MT_NC __riscv_svpbmt.mt_nc
> > +#define _PAGE_MT_IO __riscv_svpbmt.mt_io
>
> Using a struct over individual variables seems a little odd here.
>
How about :
extern unsigned long _svpbmt_mask
extern unsigned long _svpbmt_mt_pma
extern unsigned long _svpbmt_mt_nc
extern unsigned long _svpbmt_mt_io
#define _PAGE_MT_MASK _svpbmt_mask
#define _PAGE_MT_PMA _svpbmt_mt_pma
#define _PAGE_MT_NC _svpbmt_mt_nc
#define _PAGE_MT_IO _svpbmt_mt_io
> Also why not use the standard names for these _PAGE bits used by
> most other architectures?
>
> > - return (unsigned long)pfn_to_virt(pmd_val(pmd) >> _PAGE_PFN_SHIFT);
> > + return (unsigned long)pfn_to_virt((pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT);
>
> Overly long line, could use a helper. Btw, what is the point in having
> this _PAGE_PFN_SHIFT macro to start with?
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
On Mon, Oct 25, 2021 at 10:49:11PM +0800, Wei Fu wrote:
> > > + >> _PAGE_PFN_SHIFT);
> >
> > > static inline unsigned long _pmd_pfn(pmd_t pmd)
> > > {
> > > - return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
> > > + return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
> > > }
> >
> > The "(pud_val(pud) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT" expression begs
> > for readable and well-documented helper.
>
> How about these:
> #define _chg_of_pmd(pmd) (pmd_val(pmd) & _PAGE_CHG_MASK)
> #define _chg_of_pud(pud) (pud_val(pud) & _PAGE_CHG_MASK)
I'd use inline functions instead of macros if possible, but otherwise
yes.
> How about :
>
> extern unsigned long _svpbmt_mask
> extern unsigned long _svpbmt_mt_pma
> extern unsigned long _svpbmt_mt_nc
> extern unsigned long _svpbmt_mt_io
>
> #define _PAGE_MT_MASK _svpbmt_mask
> #define _PAGE_MT_PMA _svpbmt_mt_pma
> #define _PAGE_MT_NC _svpbmt_mt_nc
> #define _PAGE_MT_IO _svpbmt_mt_io
Looks much better.
On Mon, Oct 25, 2021 at 06:55:09PM +0800, Wei Fu wrote:
> How about this macro:
> #define _SVPBMT_PMA 0x0UL
> #define _SVPBMT_NC BIT(61)
> #define _SVPBMT_IO BIT(62)
> #define _SVPBMT_MASK GENMASK(62, 61)
Personally I find these macros highly confusing.
#define _SVPBMT_PMA 0UL
#define _SVPBMT_NC (1UL << 61)
#define _SVPBMT_IO (1UL << 62).
#define _SVPBMT_MASK (_SVPBMT_NC | _SVPBMT_IO)
is much eaier to follow. Note that we can probably just drop
_SVPBMT_PMA entirely to make this even more readable.
> > Also why not use the standard names for these _PAGE bits used by
> > most other architectures?
>
> Which names are you suggesting? Would you mind providing an example ?
> _PAGE_BIT_ for _PAGE_KERNEL_ ??
Use _PAGE_NOCACHE for _SVPBMT_NC, and _PAGE_IO for _SVPBMT_IO.
Hi Christoph,
Great thanks for your review.
On Tue, Nov 2, 2021 at 2:04 PM Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Oct 25, 2021 at 10:49:11PM +0800, Wei Fu wrote:
> > > > + >> _PAGE_PFN_SHIFT);
> > >
> > > > static inline unsigned long _pmd_pfn(pmd_t pmd)
> > > > {
> > > > - return pmd_val(pmd) >> _PAGE_PFN_SHIFT;
> > > > + return (pmd_val(pmd) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT;
> > > > }
> > >
> > > The "(pud_val(pud) & _PAGE_CHG_MASK) >> _PAGE_PFN_SHIFT" expression begs
> > > for readable and well-documented helper.
> >
> > How about these:
> > #define _chg_of_pmd(pmd) (pmd_val(pmd) & _PAGE_CHG_MASK)
> > #define _chg_of_pud(pud) (pud_val(pud) & _PAGE_CHG_MASK)
>
> I'd use inline functions instead of macros if possible, but otherwise
> yes.
Great thanks for your suggestion, I will change this to inline :-)
>
> > How about :
> >
> > extern unsigned long _svpbmt_mask
> > extern unsigned long _svpbmt_mt_pma
> > extern unsigned long _svpbmt_mt_nc
> > extern unsigned long _svpbmt_mt_io
> >
> > #define _PAGE_MT_MASK _svpbmt_mask
> > #define _PAGE_MT_PMA _svpbmt_mt_pma
> > #define _PAGE_MT_NC _svpbmt_mt_nc
> > #define _PAGE_MT_IO _svpbmt_mt_io
>
> Looks much better.
Thanks , will do this way
>
Hi Chistoph,
On Tue, Nov 2, 2021 at 2:07 PM Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Oct 25, 2021 at 06:55:09PM +0800, Wei Fu wrote:
> > How about this macro:
> > #define _SVPBMT_PMA 0x0UL
> > #define _SVPBMT_NC BIT(61)
> > #define _SVPBMT_IO BIT(62)
> > #define _SVPBMT_MASK GENMASK(62, 61)
>
> Personally I find these macros highly confusing.
>
> #define _SVPBMT_PMA 0UL
> #define _SVPBMT_NC (1UL << 61)
> #define _SVPBMT_IO (1UL << 62).
> #define _SVPBMT_MASK (_SVPBMT_NC | _SVPBMT_IO)
>
> is much eaier to follow. Note that we can probably just drop
> _SVPBMT_PMA entirely to make this even more readable.
sure, I can do this , thanks
>
> > > Also why not use the standard names for these _PAGE bits used by
> > > most other architectures?
> >
> > Which names are you suggesting? Would you mind providing an example ?
> > _PAGE_BIT_ for _PAGE_KERNEL_ ??
>
> Use _PAGE_NOCACHE for _SVPBMT_NC, and _PAGE_IO for _SVPBMT_IO.
OK, Sure , will do
Great thanks
>