linux-kernel - Re: [PATCH v2 01/18] KVM: arm64: Change the layout of enum pkvm_page

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1hbKrUPXdEokXpP@google.com>
Date: Tue, 10 Dec 2024 15:15:54 +0000
From: Quentin Perret <qperret@...gle.com>
To: Fuad Tabba <tabba@...gle.com>
Cc: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Vincent Donnefort <vdonnefort@...gle.com>,
	Sebastian Ene <sebastianene@...gle.com>,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/18] KVM: arm64: Change the layout of enum
 pkvm_page_state

Hey Fuad,

On Tuesday 10 Dec 2024 at 12:59:44 (+0000), Fuad Tabba wrote:
> Hi Quentin,
> 
> On Tue, 3 Dec 2024 at 10:37, Quentin Perret <qperret@...gle.com> wrote:
> >
> > The 'concrete' (a.k.a non-meta) page states are currently encoded using
> > software bits in PTEs. For performance reasons, the abstract
> > pkvm_page_state enum uses the same bits to encode these states as that
> > makes conversions from and to PTEs easy.
> >
> > In order to prepare the ground for moving the 'concrete' state storage
> > to the hyp vmemmap, re-arrange the enum to use bits 0 and 1 for this
> > purpose.
> >
> > No functional changes intended.
> >
> > Signed-off-by: Quentin Perret <qperret@...gle.com>
> > ---
> >  arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > index 0972faccc2af..ca3177481b78 100644
> > --- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > +++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
> > @@ -24,25 +24,28 @@
> >   */
> >  enum pkvm_page_state {
> >         PKVM_PAGE_OWNED                 = 0ULL,
> > -       PKVM_PAGE_SHARED_OWNED          = KVM_PGTABLE_PROT_SW0,
> > -       PKVM_PAGE_SHARED_BORROWED       = KVM_PGTABLE_PROT_SW1,
> > -       __PKVM_PAGE_RESERVED            = KVM_PGTABLE_PROT_SW0 |
> > -                                         KVM_PGTABLE_PROT_SW1,
> > +       PKVM_PAGE_SHARED_OWNED          = BIT(0),
> > +       PKVM_PAGE_SHARED_BORROWED       = BIT(1),
> > +       __PKVM_PAGE_RESERVED            = BIT(0) | BIT(1),
> >
> >         /* Meta-states which aren't encoded directly in the PTE's SW bits */
> > -       PKVM_NOPAGE,
> > +       PKVM_NOPAGE                     = BIT(2),
> >  };
> > +#define PKVM_PAGE_META_STATES_MASK     (~(BIT(0) | BIT(1)))
> >
> >  #define PKVM_PAGE_STATE_PROT_MASK      (KVM_PGTABLE_PROT_SW0 | KVM_PGTABLE_PROT_SW1)
> >  static inline enum kvm_pgtable_prot pkvm_mkstate(enum kvm_pgtable_prot prot,
> >                                                  enum pkvm_page_state state)
> >  {
> > -       return (prot & ~PKVM_PAGE_STATE_PROT_MASK) | state;
> > +       BUG_ON(state & PKVM_PAGE_META_STATES_MASK);
> 
> This is a slight change in functionality, having a BUG_ON instead of
> just masking out illegal states. Is it necessary?

Yep, this is arguably a bit zealous. Passing e.g. PKVM_NOPAGE to
pkvm_mkstate() would be properly bogus, so having a WARN_ON() or
BUG_ON() in there is still a good thing, but it should be done in a
separate patch.

I'll rework in v3.

> > +       prot &= ~PKVM_PAGE_STATE_PROT_MASK;
> > +       prot |= FIELD_PREP(PKVM_PAGE_STATE_PROT_MASK, state);
> > +       return prot;
> >  }
> >
> >  static inline enum pkvm_page_state pkvm_getstate(enum kvm_pgtable_prot prot)
> >  {
> > -       return prot & PKVM_PAGE_STATE_PROT_MASK;
> > +       return FIELD_GET(PKVM_PAGE_STATE_PROT_MASK, prot);
> >  }
> >
> >  struct host_mmu {
> > --
> > 2.47.0.338.g60cca15819-goog
> >