Re: [RESEND PATCH V3 2/2] riscv: add RISC-V Svpbmt extension supports

From: Wei Fu <wefu@redhat.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: "Anup Patel" <anup.patel@wdc.com>,
	"Atish Patra" <atish.patra@wdc.com>,
	"Palmer Dabbelt" <palmerdabbelt@google.com>,
	guoren@kernel.org, christoph.muellner@vrull.eu,
	"Philipp Tomsich" <philipp.tomsich@vrull.eu>,
	"Christoph Hellwig" <hch@lst.de>,
	"Liu Shaohua" <liush@allwinnertech.com>,
	"Wei Wu (吴伟)" <lazyparser@gmail.com>,
	"Drew Fustini" <drew@beagleboard.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	taiten.peng@canonical.com, aniket.ponkshe@canonical.com,
	heinrich.schuchardt@canonical.com, gordan.markus@canonical.com,
	guoren@linux.alibaba.com, "Arnd Bergmann" <arnd@arndb.de>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Maxime Ripard" <maxime@cerno.tech>,
	"Daniel Lustig" <dlustig@nvidia.com>,
	"Greg Favor" <gfavor@ventanamicro.com>,
	"Andrea Mondelli" <andrea.mondelli@huawei.com>,
	"Jonathan Behrens" <behrensj@mit.edu>,
	Xinhaoqu <xinhaoqu@huawei.com>,
	"Bill Huffman" <huffman@cadence.com>,
	"Nick Kossifidis" <mick@ics.forth.gr>,
	"Allen Baum" <allen.baum@esperantotech.com>,
	"Josh Scheid" <jscheid@ventanamicro.com>,
	"Richard Trauben" <rtrauben@gmail.com>
Subject: Re: [RESEND PATCH V3 2/2] riscv: add RISC-V Svpbmt extension supports
Date: Mon, 25 Oct 2021 18:55:09 +0800	[thread overview]
Message-ID: <CA+YCwKkX64A8p9vfLJHC3+LqOWyDg99eDBR0mjfB=H45BOyJLQ@mail.gmail.com> (raw)
In-Reply-To: <YXZU/3/YmRGFrOXK@infradead.org>

Hi Christoph,

Great thanks for your review.

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.
>
> > +#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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>