Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Tue, 24 Oct 2023 10:07:12 -0700
In-Reply-To: <1347cf03-4598-f923-74e4-a3d193d9d2e9@intel.com>
Mime-Version: 1.0
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-7-weijiang.yang@intel.com> <e0db6ffd-5d92-2a1a-bdfb-a190fe1ccd25@intel.com>
 <1347cf03-4598-f923-74e4-a3d193d9d2e9@intel.com>
Message-ID: <ZTf5wPKXuHBQk0AN@google.com>
Subject: Re: [PATCH v6 06/25] x86/fpu/xstate: Opt-in kernel dynamic bits when
 calculate guest xstate size
From:   Sean Christopherson <seanjc@google.com>
To:     Weijiang Yang <weijiang.yang@intel.com>
Cc:     Dave Hansen <dave.hansen@intel.com>, pbonzini@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        peterz@infradead.org, chao.gao@intel.com,
        rick.p.edgecombe@intel.com, john.allen@amd.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Sep 15, 2023, Weijiang Yang wrote:
> On 9/15/2023 1:40 AM, Dave Hansen wrote:
> > On 9/13/23 23:33, Yang Weijiang wrote:
> > > --- a/arch/x86/kernel/fpu/xstate.c
> > > +++ b/arch/x86/kernel/fpu/xstate.c
> > > @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted=
, u64 requested, bool guest)
> > >   	/* Calculate the resulting kernel state size */
> > >   	mask =3D permitted | requested;
> > > -	/* Take supervisor states into account on the host */
> > > +	/*
> > > +	 * Take supervisor states into account on the host. And add
> > > +	 * kernel dynamic xfeatures to guest since guest kernel may
> > > +	 * enable corresponding CPU feaures and the xstate registers
> > > +	 * need to be saved/restored properly.
> > > +	 */
> > >   	if (!guest)
> > >   		mask |=3D xfeatures_mask_supervisor();
> > > +	else
> > > +		mask |=3D fpu_kernel_dynamic_xfeatures;

This looks wrong.  Per commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervi=
sor
states in XSTATE permissions"), mask at this point only contains user featu=
res,
which somewhat unintuitively doesn't include CET_USER (I get that they're M=
SRs
and thus supervisor state, it's just the name that's odd).

IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_su=
pervisor()
conatins PASID, CET_USER, and CET_KERNEL.  PASID isn't virtualized by KVM, =
but
doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/=
XTILE
enabling?

The existing code also seems odd, but I might be missing something.  Won't =
the
kernel drop PASID if the guest request AMX/XTILE?  I'm not at all familiar =
with
what PASID state is managed via XSAVE, so I've no idea if that's an actual =
problem
or just an oddity.

> > >   	ksize =3D xstate_calculate_size(mask, compacted);
> > Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures"
> > but didn't change the logic.
> >=20
> > As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are
> > guest xfeatures.  So, they're different in name only.

...

> > Would there ever be any reason for KVM to be on a system which supports=
 a
> > dynamic kernel feature but where it doesn't get enabled for guest use, =
or
> > at least shouldn't have the FPU space allocated?
>=20
> I haven't heard of that kind of usage for other features so far, CET
> supervisor xstate is the only dynamic kernel feature now,=C2=A0 not sure =
whether
> other CPU features having supervisor xstate would share the handling logi=
c
> like CET does one day.

There are definitely scenarios where CET will not be exposed to KVM guests,=
 but
I don't see any reason to make the guest FPU space dynamically sized for CE=
T.
It's what, 40 bytes?

I would much prefer to avoid the whole "dynamic" thing and instead make CET
explicitly guest-only.  E.g. fpu_kernel_guest_only_xfeatures?  Or even bett=
er
if it doesn't cause weirdness elsewhere, a dedicated fpu_guest_cfg.  For me=
 at
least, a fpu_guest_cfg would make it easier to understand what all is going=
 on.=20