Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <cf329949-b81c-3e8c-0f38-4a28de22c456@redhat.com>
Date:   Wed, 15 Dec 2021 14:42:50 +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
To:     "Wang, Wei W" <wei.w.wang@intel.com>,
        "Zhong, Yang" <yang.zhong@intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Cc:     "seanjc@google.com" <seanjc@google.com>,
        "Nakajima, Jun" <jun.nakajima@intel.com>,
        "Tian, Kevin" <kevin.tian@intel.com>,
        "jing2.liu@linux.intel.com" <jing2.liu@linux.intel.com>,
        "Liu, Jing2" <jing2.liu@intel.com>,
        "Zeng, Guang" <guang.zeng@intel.com>
References: <20211208000359.2853257-1-yang.zhong@intel.com>
 <20211208000359.2853257-17-yang.zhong@intel.com>
 <d16aab21-0f81-f758-a61e-5919f223be78@redhat.com>
 <26ea7039-3186-c23f-daba-d039bb8d6f48@redhat.com>
 <86d3c3a5d61649079800a2038370365b@intel.com>
 <bdda79b5-79e4-22fd-9af8-ec6e87a412ab@redhat.com>
 <3ec6019a551249d6994063e56a448625@intel.com>
 <ba78d142-6a97-99dd-9d00-465f7d6aa712@redhat.com>
 <0c2dae4264ae4d3b87d023879c51833c@intel.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <0c2dae4264ae4d3b87d023879c51833c@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/15/21 03:39, Wang, Wei W wrote:
>>> Why would KVM_GET_XSAVE2 still be needed in this case?
>>>
>>> I'm thinking it would also be possible to reuse KVM_GET_XSAVE:
>>>
>>> - If userspace calls to KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2),
>>>    then KVM knows that the userspace is a new version and it works with
>> larger xsave buffer using the "size" that it returns via KVM_CAP_XSAVE2.
>>>    So we can add a flag "kvm->xsave2_enabled", which gets set upon
>> userspace checks KVM_CAP_XSAVE2.
>>
>> You can use KVM_ENABLE_CAP(KVM_CAP_XSAVE2) for that, yes.  In that case
>> you don't need KVM_GET_XSAVE2.
>
> On more thing here, what size should KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) return?
> If the size still comes from the guest CPUID(0xd, 0)::RCX, would it be better to just return 1?
> This requires that the QEMU CPUID info has been set to KVM before checking the cap.
> QEMU already has this CPUID info to get the size (seems no need to inquire KVM for it).

It's still easier to return the full size of the buffer from 
KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2).  It makes the userspace code a bit 
easier.

I'm also thinking that I prefer KVM_GET_XSAVE2 to 
KVM_ENABLE_CAP(KVM_CAP_XSAVE2), after all.  Since it would be a 
backwards-incompatible change to an _old_ ioctl (KVM_GET_XSAVE), I 
prefer to limit the ways that userspace can shoot itself in the foot.

Paolo