On Thu, Feb 17, 2022 at 02:44:10PM +0100, Paolo Bonzini wrote:
> On 2/17/22 06:58, Yang Zhong wrote:
> >>+
> >>+ if ((mask & XSTATE_XTILE_DATA_MASK) == XSTATE_XTILE_DATA_MASK) {
> >>+ bitmask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX);
> >>+ if (!(bitmask & XSTATE_XTILE_DATA_MASK)) {
> > Paolo, last time you suggested below changes for here:
> >
> > rc = kvm_arch_get_supported_cpuid(s, 0xd, 0,
> > (xdata_bit < 32 ? R_EAX : R_EDX));
> > if (!(rc & BIT(xdata_bit & 31)) {
> > ...
> > }
> >
> > Since I used "mask" as parameter here, so I had to directly use R_EAX
> > here.
> > Please review and if need change it to like "(xdata_bit < 32 ? R_EAX :
> > R_EDX)",
> > I will change this in next version, thanks!
>
> I looked at this function more closely because it didn't compile on non-Linux
> systems, too. I think it's better to write it already to plan for more
> dynamic features. In the code below, I'm also relying on
> KVM_GET_SUPPORTED_CPUID/KVM_X86_COMP_GUEST_SUPP being executed
> before ARCH_REQ_XCOMP_GUEST_PERM, which therefore cannot fail.
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 377d993438..1d0c006077 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -43,8 +43,6 @@
> #include "disas/capstone.h"
> #include "cpu-internal.h"
> -#include <sys/syscall.h>
> -
> /* Helpers for building CPUID[2] descriptors: */
> struct CPUID2CacheDescriptorInfo {
> @@ -6002,40 +6000,6 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu,
> FeatureWord w)
> }
> }
> -static void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
> -{
> - KVMState *s = kvm_state;
> - uint64_t bitmask;
> - long rc;
> -
> - if ((mask & XSTATE_XTILE_DATA_MASK) == XSTATE_XTILE_DATA_MASK) {
> - bitmask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX);
> - if (!(bitmask & XSTATE_XTILE_DATA_MASK)) {
> - warn_report("no amx support from supported_xcr0, "
> - "bitmask:0x%lx", bitmask);
> - return;
> - }
> -
> - rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_GUEST_PERM,
> - XSTATE_XTILE_DATA_BIT);
> - if (rc) {
> - /*
> - * The older kernel version(<5.15) can't support
> - * ARCH_REQ_XCOMP_GUEST_PERM and directly return.
> - */
> - return;
> - }
> -
> - rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_GUEST_PERM, &bitmask);
> - if (rc) {
> - warn_report("prctl(ARCH_GET_XCOMP_GUEST_PERM) error: %ld", rc);
> - } else if (!(bitmask & XFEATURE_XTILE_MASK)) {
> - warn_report("prctl(ARCH_REQ_XCOMP_GUEST_PERM) failure "
> - "and bitmask=0x%lx", bitmask);
> - }
> - }
> -}
> -
> /* Calculate XSAVE components based on the configured CPU feature flags */
> static void x86_cpu_enable_xsave_components(X86CPU *cpu)
> {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d4ad0f56bd..de949bd63d 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -551,11 +551,8 @@ typedef enum X86Seg {
> #define XSTATE_PKRU_MASK (1ULL << XSTATE_PKRU_BIT)
> #define XSTATE_XTILE_CFG_MASK (1ULL << XSTATE_XTILE_CFG_BIT)
> #define XSTATE_XTILE_DATA_MASK (1ULL << XSTATE_XTILE_DATA_BIT)
> -#define XFEATURE_XTILE_MASK (XSTATE_XTILE_CFG_MASK \
> - | XSTATE_XTILE_DATA_MASK)
> -#define ARCH_GET_XCOMP_GUEST_PERM 0x1024
> -#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> +#define XSTATE_DYNAMIC_MASK (XSTATE_XTILE_DATA_MASK)
> #define ESA_FEATURE_ALIGN64_BIT 1
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 3bdcd724c4..4b07778970 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -17,6 +17,7 @@
> #include "qapi/error.h"
> #include <sys/ioctl.h>
> #include <sys/utsname.h>
> +#include <sys/syscall.h>
> #include <linux/kvm.h>
> #include "standard-headers/asm-x86/kvm_para.h"
> @@ -5168,3 +5169,39 @@ bool kvm_arch_cpu_check_are_resettable(void)
> {
> return !sev_es_enabled();
> }
> +
> +#define ARCH_REQ_XCOMP_GUEST_PERM 0x1025
> +
> +void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask)
> +{
> + KVMState *s = kvm_state;
> + uint64_t supported;
> +
> + mask &= XSTATE_DYNAMIC_MASK;
> + if (!mask) {
> + return;
> + }
> + /*
> + * Just ignore bits that are not in CPUID[EAX=0xD,ECX=0].
> + * ARCH_REQ_XCOMP_GUEST_PERM would fail, and QEMU has warned
> + * about them already because they are not supported features.
> + */
> + supported = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX);
> + supported |= (uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) <<
> 32;
> + mask &= ~supported;
Paolo, thanks for your great help!
Except changing "mask &= ~supported" to "mask &= supported", this patch work
well.
Please re-sync Linux-header since David has pulled linux header to 5.17-rc1
https://lists.nongnu.org/archive/html/qemu-devel/2022-02/msg03763.html
Yang
> +
> + while (mask) {
> + int bit = ctz64(mask);
> + int rc = syscall(SYS_arch_prctl, ARCH_REQ_XCOMP_GUEST_PERM, bit);
> + if (rc) {
> + /*
> + * Older kernel version (<5.17) do not support
> + * ARCH_REQ_XCOMP_GUEST_PERM, but also do not return
> + * any dynamic feature from kvm_arch_get_supported_cpuid.
> + */
> + warn_report("prctl(ARCH_REQ_XCOMP_GUEST_PERM) failure "
> + "for feature bit %d", bit);
> + }
> + mask &= ~BIT_ULL(bit);
> + }
> +}
> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> index a978509d50..4124912c20 100644
> --- a/target/i386/kvm/kvm_i386.h
> +++ b/target/i386/kvm/kvm_i386.h
> @@ -52,5 +52,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
> uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
> bool kvm_enable_sgx_provisioning(KVMState *s);
> +void kvm_request_xsave_components(X86CPU *cpu, uint64_t mask);
> #endif
>
>
> If this works, the rest of the series is good to go!
>
> Thanks,
>
> Paolo
