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

Open Source and information security mailing list archives

Message-ID: <CA+YCwKn1VA8=NSa=bdhP99jfpokYGHBvqVSKvZmdndQOz6UUPA@mail.gmail.com>
Date:   Mon, 25 Oct 2021 22:49:11 +0800
From:   Wei Fu <wefu@...hat.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Anup Patel <anup.patel@....com>, Atish Patra <atish.patra@....com>,
        Palmer Dabbelt <palmerdabbelt@...gle.com>, guoren@...nel.org,
        christoph.muellner@...ll.eu,
        Philipp Tomsich <philipp.tomsich@...ll.eu>,
        Christoph Hellwig <hch@....de>,
        Liu Shaohua <liush@...winnertech.com>,
        Wei Wu (吴伟) <lazyparser@...il.com>,
        Drew Fustini <drew@...gleboard.org>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        taiten.peng@...onical.com, aniket.ponkshe@...onical.com,
        heinrich.schuchardt@...onical.com, gordan.markus@...onical.com,
        guoren@...ux.alibaba.com, Arnd Bergmann <arnd@...db.de>,
        Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime@...no.tech>,
        Daniel Lustig <dlustig@...dia.com>,
        Greg Favor <gfavor@...tanamicro.com>,
        Andrea Mondelli <andrea.mondelli@...wei.com>,
        Jonathan Behrens <behrensj@....edu>,
        Xinhaoqu <xinhaoqu@...wei.com>,
        Bill Huffman <huffman@...ence.com>,
        Nick Kossifidis <mick@....forth.gr>,
        Allen Baum <allen.baum@...erantotech.com>,
        Josh Scheid <jscheid@...tanamicro.com>,
        Richard Trauben <rtrauben@...il.com>
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@...radead.org> wrote:
>
> On Mon, Oct 25, 2021 at 12:06:07PM +0800, wefu@...hat.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives