Re: [PATCH 2/2] Expose tsc deadline timer cpuid to guest

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-04-23 22:02, Eduardo Habkost wrote:
> On Mon, Apr 23, 2012 at 06:31:25PM +0200, Jan Kiszka wrote:
>> On 2012-04-23 16:48, Eduardo Habkost wrote:
>>> Trying to summarize the points above:
>>>
>>> Groups (A) and (B) are:
>>>
>>> A) a feature that KVM supports and emulate by itself and can be enabled
>>>    by userspace blindly, without requiring any additional userspace
>>>    code to work.
>>> B) a feature that KVM supports but need support from userspace to work.
>>>
>>> We have to differentiate those two groups somehow, otherwise "-cpu host"
>>> will always risk being unstable (in case we can't identify group (B) and
>>> end up enabling a feature that will break) or useless (if group (A) is
>>> considered always empty).
>>>
>>> (If you think this two-group model is not sufficient, please let me know.)
>>>
>>> Note that I am discussing two things above:
>>>
>>> - Whether GET_SUPPORTED_CPUID should expose only features from group
>>>   (A), or group (B) too.
>>>   - One problem here is that today GET_SUPPORTED_CPUIDS have many
>>>     examples of (B) features inside it. Should we stop doing that?
>>
>> We have exactly two for the category that I was concerned about: TSC
>> deadline and X2APIC. The latter is already exposed unconditionally, even
>> if the kernel does not provide emulation. So, you are right, TSC
>> deadline is not a new scenario.
> 
> Oh, that explains why you were seing it differently: if the kernel
> doesn't control anything about the feature exposure, there was no
> obvious need to add it to GET_SUPPORTED_CPUID.
> 
>>
>>>     - TSC-deadline is the first case where we are _not_ doing that. It
>>>       is the first CPU feature in KVM that can be enabled by userspace
>>>       (as long as userspace does the proper setup), but it's not
>>>       included on GET_SUPPORTED_CPUIDs.
>>>   - Even the current documentation implies that group (B) is included:
>>>
>>>     "This ioctl returns x86 cpuid features which are supported by both
>>>     the hardware and kvm.  Userspace can use the information returned by
>>>     this ioctl to construct cpuid information (for KVM_SET_CPUID2) that
>>>     is consistent with hardware, kernel, and userspace capabilities, and
>>>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>     with user requirements (for example, the user may wish to constrain
>>>     cpuid to emulate older hardware, or for feature consistency across a
>>>     cluster)."
>>>
>>>     In the specific case of TSC-deadline, I consider "Qemu knowing that
>>>     TSC-deadline can be enabled only if in-kernel irqchip is enabled" as
>>>     an "userpace capability".
>>>
>>> - How to precisely define the groups (A) and (B)?
>>>   - "requires additional code only if migration is required" qualifies
>>>     as (B) or (A)?
>>>   - "requires in-kernel irqchip to be enabled to work" qualifies as (B),
>>>     doesn't it?
>>
>> My problem is that features like X2APIC and TSC deadline are exposed by
>> the kernel without "contributing" to them (if in-kernel irqchip is off).
> 
> They are not simply "exposed by the kernel": they are enabled by
> userspace, after userspace deciding whether it's safe or not (based on
> multiple factors).
> 
> 
>> However, that was how I interpreted this GET_SUPPORTED_CPUID. In fact,
>> it is used as "kernel or hardware does not _prevent_" already. And in
>> that sense, it's ok to enable even features that are not in
>> kernel/hardware hands. We should point out this fact in the documentation.
> 
> I see GET_SUPPORTED_CPUID as just a "what userspace can enable because
> the kernel and the hardware support it (= don't prevent it), as long as
> userspace has the required support" (meaning A+B). It's a bit like
> KVM_CHECK_EXTENSION, but with the nice feature that that the
> capabilities map directly to CPUID bits.
> 
> So, it's not clear to me: now you are OK with adding TSC_DEADLINE to
> GET_SUPPORTED_CPUID?
> 
> But we still have the issue of "-cpu host" not knowing what can be
> safely enabled (without userspace feature-specific setup code), or not.
> Do you have any suggestion for that? Avi, do you have any suggestion?

First of all, I bet this was already broken with the introduction of
x2apic. So TSC deadline won't make it worse. I guess we need to address
this in userspace, first by masking those features out, later by
actually emulating them.

> 
> And I still don't know the answer to:
> 
>>> - How to precisely define the groups (A) and (B)?
>>>   - "requires additional code only if migration is required" qualifies
>>>     as (B) or (A)?
> 
> 
> Re: documentation, isn't the following paragraph (already present on
> api.txt) sufficient?
> 
> "The entries returned are the host cpuid as returned by the cpuid
> instruction, with unknown or unsupported features masked out.  Some
> features (for example, x2apic), may not be present in the host cpu, but
> are exposed by kvm if it can emulate them efficiently."

That suggests such features are always emulated - which is not true.
They are either emulated, or nothing _prevents_ their emulation by user
space.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux