On Jan 16, 2008, at 9:12 PM, Dan Kenigsberg wrote:
> On Wed, Jan 16, 2008 at 06:34:08PM +0100, Alexander Graf wrote:
>> Dan Kenigsberg wrote:
>>> On Tue, Jan 15, 2008 at 08:57:45AM +0100, Alexander Graf wrote:
>>>
>>>> Dan Kenigsberg wrote:
>>>>
>>>>> On Mon, Jan 14, 2008 at 02:49:31PM +0100, Alexander Graf wrote:
>>>>>
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Currently CPUID function 4 is broken. This function's values
>>>>>> rely on the
>>>>>> value of ECX.
>>>>>> To solve the issue cleanly, there is already a new API for cpuid
>>>>>> settings, which is not used yet.
>>>>>> Using the current interface, the function 4 can be easily passed
>>>>>> through, by giving multiple function 4 outputs and increasing the
>>>>>> index-identifier on the fly. This does not break compatibility.
>>>>>>
>>>>>> This fix is really important for Mac OS X, as it requires cache
>>>>>> information. Please also see my previous patches for Mac OS X
>>>>>> (or rather
>>>>>> core duo target) compatibility.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>> diff --git a/kernel/x86.c b/kernel/x86.c
>>>>>> index b55c177..73312e9 100644
>>>>>> --- a/kernel/x86.c
>>>>>> +++ b/kernel/x86.c
>>>>>> @@ -783,7 +783,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct
>>>>>> kvm_vcpu *vcpu,
>>>>>> struct kvm_cpuid *cpuid,
>>>>>> struct kvm_cpuid_entry __user *entries)
>>>>>> {
>>>>>> - int r, i;
>>>>>> + int r, i, n = 0;
>>>>>> struct kvm_cpuid_entry *cpuid_entries;
>>>>>>
>>>>>> r = -E2BIG;
>>>>>> @@ -803,8 +803,17 @@ static int kvm_vcpu_ioctl_set_cpuid(struct
>>>>>> kvm_vcpu *vcpu,
>>>>>> vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>>>>>> vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>>>>>> vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
>>>>>> - vcpu->arch.cpuid_entries[i].index = 0;
>>>>>> - vcpu->arch.cpuid_entries[i].flags = 0;
>>>>>> + switch(vcpu->arch.cpuid_entries[i].function) {
>>>>>> + case 4:
>>>>>> + vcpu->arch.cpuid_entries[i].index = n;
>>>>>> + vcpu->arch.cpuid_entries[i].flags =
>>>>>> KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>>>>>> + n++;
>>>>>> + break;
>>>>>> + default:
>>>>>> + vcpu->arch.cpuid_entries[i].index = 0;
>>>>>> + vcpu->arch.cpuid_entries[i].flags = 0;
>>>>>> + break;
>>>>>> + }
>>>>>>
>>>>>>
>>>>> I will not mention the whitespace damage here :-). Instead, I'd
>>>>> ask you
>>>>>
>>>>>
>>>> Oh well, after having been into qemu source, I just got used to use
>>>> spaces instead of tabs ;-).
>>>>
>>>>
>>>>> to review, comment, and even try, the patch that I posted here
>>>>> not long
>>>>> ago, exposing all safe host cpuid functions to guests.
>>>>>
>>>>>
>>>> Sure.
>>>> Basically your patch targets at a completely different use case
>>>> than
>>>> mine though. You want to expose the host features on the virtual
>>>> CPU,
>>>> whereas my goal is to have a virtual Core Duo/Solo CPU, even if
>>>> your
>>>> host CPU is actually an SVM capable one.
>>>>
>>>> So my CoreDuo CPU definition still fails to populate a proper CPUID
>>>> function 4. With the -cpu host option, Linux works (as it's bright
>>>> enough to know that some values are just plain wrong), but Darwin
>>>> crashes. I am not exactly sure why it is, but I guess it's due to
>>>> the
>>>> function 4 values exposing a 2-core CPU, which kvm simply doesn't
>>>> emulate.
>>>>
>>>
>>> What I wanted to say is that the fact that the usermode support is
>>> not
>>> used, is not IMHO a good-enough reason to change the kernel:
>>> kvm_vcpu_ioctl_set_cpuid() was ment to be a stupid function, to be
>>> used
>>> only with old usermode. I hate to teach it the true complex logic
>>> of Intel's
>>> CPUID.
>>>
>>>
>>
>> The funny part is, you don't have to. Every complex I know of so
>> far is
>> simply repetitive. If the userspace just sends x cpuid values and the
>> kernel takes x, where's the problem?
>>
>> Of course having a full descriptionary approach is way better, but
>> I see
>> no real need to not use a stupid interface.
>
> The only reason is that a smarter interface exists, and I want it to
> be used,
> not hacked arround.
>
This is a valid complaint. Still, one wouldn't have needed the smart
interface in the first place. Now that it is in, one should of course
use it.
>>> What I would like to see is something that uses the cpuid2 API,
>>> and not
>>> circumvene it... For this to happen, I need a deep review of my
>>> code.
>>>
>>
>> I have to admin that I am really bad at reviewing, so don't expect
>> anything glorious from me.
>
> Anything beyond silence would be glorious.
>
Let's break it and get cpuid2 support in libkvm upstream, then! I
believe having the host CPU's features exposed is a really beneficial
feature here. I do believe this might be useful in kqemu as well
though, so you might want to keep it as independent from kvm as
possible.
Any complaints about these patches from anyone?
>>> How about the (untested) attched kvm-cpuid.patch, on top of the
>>> attached
>>> cpuid-user patch?
>>>
>>
>> Is there any real difference between this kvm-cpuid.patch and the
>> one I
>> sent?
>
> There is none. I just wanted to recruit you to test my own patch.
>
Ok, I will.
>> What I was really wondering about is, why do you fetch the cpuid
>> information about the host from the kernel module?
>
> Because only the kernel knows which cpu features are safe to be
> exposed. You've
> added the SSSSSSSSE3 bit there, haven't you?
>
Right. Maybe I did get something wrong, but where are the host cpuid
flags received from?
>> CPUID does not get
>> intercepted and can be easily triggered from userspace.
>> All the fancy processing of capabilities could be done in userspace
>> as
>> well (except for features that'd need to be implemented in the
>> kernel,
>> like MTRR) and this might even reduce the code, and in any case the
>> amount of code changes in the kernel.
>
> I don't understand. Are you speaking about the cpuid2 api already
> committed
> into kvm.git? Guest cpuid operations are intercepted and handled by
> the kernel,
> don't they?
>
I'm talking about normal userspace code running on the host OS. Guest
CPUIDs of course get intercepted and handled by kvm.
>> Furthermore most people probably don't even want their host cpu to be
>> the default one. It renders migration near impossible.
>
> I think "most people" are end users who want to run kvm as fast as
> they can on
> their PC, and care less about migration between different hosts.
> People who run
> kvm in large farms would probably use their own least-common-
> denominator and
> not any default I choose.
>
I wouldn't bet on this, but it's a valid thought. I don't know which
direction kvm is going to develop to. S390 support definitely is
nothing for "end users on their PCs".
>>
>>> #else
>>> cpu_model = "qemu32";
>>> #endif
>>> +#ifdef USE_KVM
>>
>> Why is every other ifdef set on KVM_CAP_EXT_CPUID? Couldn't this be
>> used
>> here as well?
>
> Right. Thanks.
>
>>> + if (kvm_allowed)
>>> + cpu_model = "host";
>>> +#endif
>>
>> [...]
>>
>>> + } else {
>>> + copy.regs[R_EAX] = 0;
>>> + qemu_kvm_cpuid_on_env(©);
>>> + limit = copy.regs[R_EAX];
>>> +
>>> + for (i = 0; i <= limit; ++i)
>>> + do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, ©);
>>
>> I don't see any index here?
>>
>
> That's the job of your kvm-cpuid.patch...
I assumed you touched it, sorry ;-).
Regards,
Alex
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
