Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <26ea7039-3186-c23f-daba-d039bb8d6f48@redhat.com>
Date:   Fri, 10 Dec 2021 23:13:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH 16/19] kvm: x86: Introduce KVM_{G|S}ET_XSAVE2 ioctl
Content-Language: en-US
From:   Paolo Bonzini <pbonzini@redhat.com>
To:     Yang Zhong <yang.zhong@intel.com>, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com
Cc:     seanjc@google.com, jun.nakajima@intel.com, kevin.tian@intel.com,
        jing2.liu@linux.intel.com, jing2.liu@intel.com
References: <20211208000359.2853257-1-yang.zhong@intel.com>
 <20211208000359.2853257-17-yang.zhong@intel.com>
 <d16aab21-0f81-f758-a61e-5919f223be78@redhat.com>
In-Reply-To: <d16aab21-0f81-f758-a61e-5919f223be78@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/10/21 17:30, Paolo Bonzini wrote:
>>
>> +static int kvm_vcpu_ioctl_x86_set_xsave2(struct kvm_vcpu *vcpu, u8 
>> *state)
>> +{
>> +    if (fpstate_is_confidential(&vcpu->arch.guest_fpu))
>> +        return 0;
>> +
>> +    return fpu_copy_uabi_to_guest_fpstate(&vcpu->arch.guest_fpu, state,
>> +                          supported_xcr0, &vcpu->arch.pkru);
>> +}
>> +
> 
> I think fpu_copy_uabi_to_guest_fpstate (and therefore 
> copy_uabi_from_kernel_to_xstate) needs to check that the size is 
> compatible with the components in the input.
> 
> Also, IIUC the size of the AMX state will vary in different processors. 
>   Is this correct?  If so, this should be handled already by 
> KVM_GET/SET_XSAVE2 and therefore should be part of the 
> arch/x86/kernel/fpu APIs.  In the future we want to support migrating a 
> "small AMX" host to a "large AMX" host; and also migrating from a "large 
> AMX" host to a "small AMX" host if the guest CPUID is compatible with 
> the destination of the migration.

So, the size of the AMX state will depend on the active "palette" in 
TILECONFIG, and on the CPUID information.  I have a few questions on how 
Intel intends to handle future extensions to AMX:

- can we assume that, in the future, palette 1 will always have the same 
value (bytes_per_row=64, max_names=8, max_rows=16), and basically that 
the only variable value is really the number of palettes?

- how does Intel plan to handle bigger TILEDATA?  Will it use more XCR0 
bits or will it rather enlarge save state 18?

If it will use more XCR0 bits, I suppose that XCR0 bits will control 
which palettes can be chosen by LDTILECFG.

If not, on the other hand, this will be a first case of one system's 
XSAVE data not being XRSTOR-able on another system even if the 
destination system can set XCR0 to the same value as the source system.

Likewise, if the size and offsets for save state 18 were to vary 
depending on the selected palette, then this would be novel, in that the 
save state size and offsets would not be in CPUID anymore.  It would be 
particularly interesting for non-compacted format, where all save states 
after 18 would also move forward.

So, I hope that save state 18 will be frozen to 8k.  In that case, and 
if palette 1 is frozen to the same values as today, implementing 
migration will not be a problem; it will be essentially the same as 
SSE->AVX (horizontal extension of existing registers) and/or AVX->AVX512 
(both horizontal and vertical extension).

By the way, I think KVM_SET_XSAVE2 is not needed.  Instead:

- KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) should return the size of the 
buffer that is passed to KVM_GET_XSAVE2

- KVM_GET_XSAVE2 should fill in the buffer expecting that its size is 
whatever KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) passes

- KVM_SET_XSAVE can just expect a buffer that is bigger than 4k if the 
save states recorded in the header point to offsets larger than 4k.

Paolo