Re: [PATCH v2 1/6] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang <mizhang@google.com>]

On Tue, Jan 17, 2023 at 12:34 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> On 1/13/2023 6:43 AM, Aaron Lewis wrote:
> >
> > I'd still like to clean up CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[17] by
> > keeping it consistent with CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[18] in
> > the guest, but it's not clear to me what the best way to do that is.
> > The crux of the issue is that xstate_get_guest_group_perm() returns
> > partial support for AMX when userspace doesn't call
> > prctl(ARCH_REQ_XCOMP_GUEST_PERM), I.e. the guest CPUID will report
> > XTILECFG=1 and XTILEDATA=0 in that case.  In that situation, XTILECFG
> > should be cleared for it to be consistent.  I can see two ways of
> > potentially doing that:
> >
> > 1. We can ensure that perm->__state_perm never stores partial support.
> >
> > 2. We can sanitize the bits in xstate_get_guest_group_perm() before
> > they are returned, to ensure KVM never sees partial support.
> >
> > I like the idea of #1, but if that has negative effects on the host or
> > XFD I'm open to #2.  Though, XFD has its own field, so I thought that
> > wouldn't be an issue.  Would it work to set __state_perm and/or
> > default_features (what originally sets __state_perm) to a consistent
> > view, so partial support is never returned from
> > xstate_get_guest_group_perm()?
>
> FWIW, I was trying to clarify that ARCH_GET_XCOMP_GUEST_PERM is a
> variant of ARCH_GET_XCOMP_PERM in the documentation [1]. So, I guess #1
> will have some side-effect (at least confusion) for this semantics.

Right, before talking in this thread, I was not aware of the other
arch_prctl(2) for AMX. But when I look into it, it looks reasonable to
me from an engineering point of view since it seems reusing almost all
of the code in the host.

ARCH_GET_XCOMP_GUEST_PERM is to ask for "guest permission", but it is
not just about the "permission" (the fp_state size increase for AMX).
It is also about the CPUID feature bits exposure. So for the host side
of AMX usage, this is fine, since there is no partial CPUID exposure
problem. But the guest side does have it.

So, can we just grant two bits instead of 1 bit? For that, I find 1)
seems more reasonable than 2). Of course, if there are many side
effects, option #2 could be considered as well. But before that, it
will be good to understand where the side effects are.

>
> There may be some ways to transform the permission bits to the
> XCR0-capable bits. For instance, when considering this permission
> support in host, the highest feature number was considered to denote the
> rest feature bits [2].

Hmm, this is out of my concern since it is about the host-level
enabling. I have no problem with the existing host side permission
control for AMX.

However, for me, [2] does not seem a little hacky but I get the point.
The concern is that how do we know in the future, the highest bit
always indicates lower bits? Will Intel CPU features always follow
this style?  Even so, there is no such guidance/guarantee that other
CPU vendors (e.g., AMD) will do the same. Also what if there are more
than 1 dynamic features for a feature? Please kindly correct me.

Thanks.
-Mingwei


>
> Thanks,
> Chang
>
> [1]
> https://lore.kernel.org/lkml/20220922195810.23248-5-chang.seok.bae@intel.com/
> [2] https://lore.kernel.org/lkml/878rz7fyhe.ffs@tglx/