[RESEND PATCH V3 2/2] riscv: add RISC-V Svpbmt extension supports
Wei Fu
wefu at redhat.com
Mon Oct 25 03:55:09 PDT 2021
Hi Christoph,
Great thanks for your review.
On Mon, Oct 25, 2021 at 2:57 PM Christoph Hellwig <hch at infradead.org> wrote:
>
> On Mon, Oct 25, 2021 at 12:06:07PM +0800, wefu at 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.
>
> > +#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
> linux-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
More information about the linux-riscv
mailing list