On 1/17/2023 2:39 PM, Mingwei Zhang wrote:
On Tue, Jan 17, 2023 at 12:34 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> 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.
I think this is still about permission. While lazy allocation is
possible, this pre-allocation makes it simple. So this is how that
allocation part is implemented, right?
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.
Sorry, I don't get it. But I guess maintainers will dictate it.
Also, I would feel the code can be more easily adjusted for the KVM/Qemu
use if it is part of KVM API [3]. Then, the generic arch_prctl options
can remain as it is.
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.
At least, that works for AMX I thought. I was not saying anything for
the future feature. The code can be feature-specific here, no?
Thanks,
Chang
[3]
https://lore.kernel.org/kvm/20220823231402.7839-1-chang.seok.bae@xxxxxxxxx/
