Messages in this thread | | | From | Wei Fu <> | Date | Mon, 25 Oct 2021 22:49:11 +0800 | Subject | Re: [RESEND PATCH V3 2/2] riscv: add RISC-V Svpbmt extension supports |
| |
Hi Christoph,
I hope I understand this correctly,
On Mon, Oct 25, 2021 at 2:57 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Oct 25, 2021 at 12:06:07PM +0800, wefu@redhat.com 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 > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv >
| |