Re: [Qemu-devel] Semantics of "-cpu host" (was Re: [PATCH 2/2] Expose tsc deadline timer cpuid to guest)

[Alexander Graf <agraf@suse.de>]

On 05/09/2012 11:38 AM, Gleb Natapov wrote:
> On Wed, May 09, 2012 at 11:05:58AM +0200, Alexander Graf wrote:
>> On 09.05.2012, at 10:51, Gleb Natapov wrote:
>>
>>> On Wed, May 09, 2012 at 10:42:26AM +0200, Alexander Graf wrote:
>>>>
>>>> On 09.05.2012, at 10:14, Gleb Natapov<gleb@redhat.com>  wrote:
>>>>
>>>>> On Wed, May 09, 2012 at 12:07:04AM +0200, Alexander Graf wrote:
>>>>>> On 08.05.2012, at 22:14, Eduardo Habkost wrote:
>>>>>>
>>>>>>> On Tue, May 08, 2012 at 02:58:11AM +0200, Alexander Graf wrote:
>>>>>>>> On 07.05.2012, at 20:21, Eduardo Habkost wrote:
>>>>>>>>
>>>>>>>>> Andre? Are you able to help to answer the question below?
>>>>>>>>>
>>>>>>>>> I would like to clarify what's the expected behavior of "-cpu host" to
>>>>>>>>> be able to continue working on it. I believe the code will need to be
>>>>>>>>> fixed on either case, but first we need to figure out what are the
>>>>>>>>> expectations/requirements, to know _which_ changes will be needed.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Apr 24, 2012 at 02:19:25PM -0300, Eduardo Habkost wrote:
>>>>>>>>>> (CCing Andre Przywara, in case he can help to clarify what's the
>>>>>>>>>> expected meaning of "-cpu host")
>>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>> I am not sure I understand what you are proposing. Let me explain the
>>>>>>>>>> use case I am thinking about:
>>>>>>>>>>
>>>>>>>>>> - Feature FOO is of type (A) (e.g. just a new instruction set that
>>>>>>>>>> doesn't require additional userspace support)
>>>>>>>>>> - User has a Qemu vesion that doesn't know anything about feature FOO
>>>>>>>>>> - User gets a new CPU that supports feature FOO
>>>>>>>>>> - User gets a new kernel that supports feature FOO (i.e. has FOO in
>>>>>>>>>> GET_SUPPORTED_CPUID)
>>>>>>>>>> - User does _not_ upgrade Qemu.
>>>>>>>>>> - User expects to get feature FOO enabled if using "-cpu host", without
>>>>>>>>>> upgrading Qemu.
>>>>>>>>>>
>>>>>>>>>> The problem here is: to support the above use-case, userspace need a
>>>>>>>>>> probing mechanism that can differentiate _new_ (previously unknown)
>>>>>>>>>> features that are in group (A) (safe to blindly enable) from features
>>>>>>>>>> that are in group (B) (that can't be enabled without an userspace
>>>>>>>>>> upgrade).
>>>>>>>>>>
>>>>>>>>>> In short, it becomes a problem if we consider the following case:
>>>>>>>>>>
>>>>>>>>>> - Feature BAR is of type (B) (it can't be enabled without extra
>>>>>>>>>> userspace support)
>>>>>>>>>> - User has a Qemu version that doesn't know anything about feature BAR
>>>>>>>>>> - User gets a new CPU that supports feature BAR
>>>>>>>>>> - User gets a new kernel that supports feature BAR (i.e. has BAR in
>>>>>>>>>> GET_SUPPORTED_CPUID)
>>>>>>>>>> - User does _not_ upgrade Qemu.
>>>>>>>>>> - User simply shouldn't get feature BAR enabled, even if using "-cpu
>>>>>>>>>> host", otherwise Qemu would break.
>>>>>>>>>>
>>>>>>>>>> If userspace always limited itself to features it knows about, it would
>>>>>>>>>> be really easy to implement the feature without any new probing
>>>>>>>>>> mechanism from the kernel. But that's not how I think users expect "-cpu
>>>>>>>>>> host" to work. Maybe I am wrong, I don't know. I am CCing Andre, who
>>>>>>>>>> introduced the "-cpu host" feature, in case he can explain what's the
>>>>>>>>>> expected semantics on the cases above.
>>>>>>>> Can you think of any feature that'd go into category B?
>>>>>>> - TSC-deadline: can't be enabled unless userspace takes care to enable
>>>>>>> the in-kernel irqchip.
>>>>>> The kernel can check if in-kernel irqchip has it enabled and otherwise mask it out, no?
>>>>>>
>>>>> How kernel should know that userspace does not emulate it?
>>>> You have to enable the in-kernel apic to use it, at which point the kernel knows it's in use, right?
>>>>
>>>>>>> - x2apic: ditto.
>>>>>> Same here. For user space irqchip the kernel side doesn't care. If in-kernel APIC is enabled, check for its capabilities.
>>>>>>
>>>>> Same here.
>>>>>
>>>>> Well, technically both of those features can't be implemented in
>>>>> userspace right now since MSRs are terminated in the kernel, but I
>>>> Doesn't sound like the greatest design - unless you deprecate the non-in-kernel apic case.
>>>>
>>> You mean terminating MSRs in kernel does not sound like the greatest
>>> design? I do not disagree. That is why IMO kernel can't filter out
>>> TSC-deadline and x2apic like you suggest.
>> I still don't see why it can't.
>>
>> Imagine we would filter TSC-deadline and x2apic by default in the kernel - they are not known to exist yet.
>> Now, we implement TSC-deadline in the kernel. We still filter TSC-deadline out in GET_SUPORTED_CPUID in the kernel. But we provide an interface to user space that says "call me to enable TSC-deadline CPUID, but only if you're using the in-kernel apic"
>> New user space calls that ioctl when it's using the in-kernel apic, it doesn't when it's using the user space apic.
>> Old user space doesn't call that ioctl.
> First of all we already have TSC-deadline in GET_SUPORTED_CPUID without
> any additional ioctls. And second I do not see why we need additional
> iostls here.

Yeah, some times our ABI is already broken :(. What a shame...

> Hmm, so may be I misunderstood you. You propose to mask TSC-deadline and
> x2apic out from GET_SUPORTED_CPUID if irq chip is not in kernel, not
> from KVM_SET_CPUID? For those two features it may make sense indeed. Not
> sure there won't be others that are not dependent on irq chip presence.
> You propose to add additional ioctls to enable them if they appear?

That's the only backwards compatible way to design this without putting 
a plethora of knowledge into user space I can see, yes.

>
>> So at the end all bits in GET_SUPPORTED_CPUID are consistent with what user space is capable of.
>>
> GET_SUPPORTED_CPUID should not be necessary consistent with what user
> space is capable of. Userspace may emulate features that are not in
> GET_SUPPORTED_CPUID.

If it does, it can OR them in. GET_SUPPORTED_CPUID should return "these 
are the bits that your currently configured kvm is capable of".


Alex