Received-SPF: pass (google.com: domain of linux-kernel+bounces-178089-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 13 May 2024 13:56:19 -0700
In-Reply-To: <20240513203839.GA168153@ls.amr.corp.intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240227232100.478238-1-pbonzini@redhat.com> <20240227232100.478238-3-pbonzini@redhat.com>
 <6bd61607-9491-4517-8fc8-8d61d9416cab@linux.intel.com> <4d0d9f64-4cc4-4c1e-ba27-ff70c9827570@linux.intel.com>
 <20240513203839.GA168153@ls.amr.corp.intel.com>
Message-ID: <ZkJ-c5El-sXc-GZB@google.com>
Subject: Re: [PATCH 02/21] KVM: Allow page-sized MMU caches to be initialized
 with custom 64-bit values
From: Sean Christopherson <seanjc@google.com>
To: Isaku Yamahata <isaku.yamahata@intel.com>
Cc: Binbin Wu <binbin.wu@linux.intel.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org, michael.roth@amd.com, 
	thomas.lendacky@amd.com, isaku.yamahata@linux.intel.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 13, 2024, Isaku Yamahata wrote:
> On Tue, Mar 26, 2024 at 11:56:35PM +0800, Binbin Wu <binbin.wu@linux.inte=
l.com> wrote:
> > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > > index d93f6522b2c3..827ecc0b7e10 100644
> > > > --- a/include/linux/kvm_types.h
> > > > +++ b/include/linux/kvm_types.h
> > > > @@ -86,6 +86,7 @@ struct gfn_to_pfn_cache {
> > > > =C2=A0 struct kvm_mmu_memory_cache {
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gfp_t gfp_zero;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gfp_t gfp_custom;
> > > > +=C2=A0=C2=A0=C2=A0 u64 init_value;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct kmem_cache *kmem_cache;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int capacity;
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int nobjs;
> > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > > index 9c99c9373a3e..c9828feb7a1c 100644
> > > > --- a/virt/kvm/kvm_main.c
> > > > +++ b/virt/kvm/kvm_main.c
> > > > @@ -401,12 +401,17 @@ static void kvm_flush_shadow_all(struct kvm *=
kvm)
> > > > =C2=A0 static inline void *mmu_memory_cache_alloc_obj(struct
> > > > kvm_mmu_memory_cache *mc,
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 gfp_t gfp_flags)
> > > > =C2=A0 {
> > > > +=C2=A0=C2=A0=C2=A0 void *page;
> > > > +
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gfp_flags |=3D mc->gfp_zero;
> > > > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (mc->kmem_cache)
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return kmem_=
cache_alloc(mc->kmem_cache, gfp_flags);
> > > > -=C2=A0=C2=A0=C2=A0 else
> > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return (void *)__get_fr=
ee_page(gfp_flags);
> > > > +
> > > > +=C2=A0=C2=A0=C2=A0 page =3D (void *)__get_free_page(gfp_flags);
> > > > +=C2=A0=C2=A0=C2=A0 if (page && mc->init_value)
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 memset64(page, mc->init=
_value, PAGE_SIZE /
> > > > sizeof(mc->init_value));
> >=20
> > Do we need a static_assert() to make sure mc->init_value is 64bit?
>=20
> That's overkill because EPT entry is defined as 64bit and KVM uses u64 fo=
r it
> uniformly.

I'm pretty sure Binbin is talking about passing init_value to memset64(), n=
ot
about whether or not that suffices for EPT.  So I wouldn't say it's overkil=
l.

However, I don't think a static assert is warranted.  Functionally, trackin=
g
init_value as a u32 or even a u8 would be a-ok as it's a copy-by-value para=
meter
that won't be sign-extended or truncated.  I.e. the real reqiurement comes =
from
TDX wanting to set a 64-bit value.

And trying to set bit 63 in a 32-bit field _will_ make the compiler unhappy=
:

arch/x86/kvm/mmu/mmu.c: In function =E2=80=98kvm_mmu_create=E2=80=99:
include/vdso/bits.h:8:33: error: conversion from =E2=80=98long long unsigne=
d int=E2=80=99 to =E2=80=98u32=E2=80=99 {aka =E2=80=98unsigned int=E2=80=99=
} changes value from =E2=80=989223372036854775808=E2=80=99 to =E2=80=980=E2=
=80=99 [-Werror=3Doverflow]
    8 | #define BIT_ULL(nr)             (ULL(1) << (nr))
      |                                 ^
arch/x86/kvm/mmu/spte.h:162:33: note: in expansion of macro =E2=80=98BIT_UL=
L=E2=80=99
  162 | #define SHADOW_NONPRESENT_VALUE BIT_ULL(63)
      |                                 ^~~~~~~
arch/x86/kvm/mmu/mmu.c:6225:17: note: in expansion of macro =E2=80=98SHADOW=
_NONPRESENT_VALUE=E2=80=99
 6225 |                 SHADOW_NONPRESENT_VALUE;
      |                 ^~~~~~~~~~~~~~~~~~~~~~~


I suppose one could argue that changing init_value to a u128 could result i=
n
undetected truncation, but IMO that firmly crosses into ridiculous territor=
y.