lkml.org 
[lkml]   [2021]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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
>

\
 
 \ /
  Last update: 2021-10-25 16:50    [W:0.077 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site