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.
> 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.
> 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?
What I was really wondering about is, why do you fetch the cpuid
information about the host from the kernel module? 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.
Furthermore most people probably don't even want their host cpu to be
the default one. It renders migration near impossible.
> #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?
> + 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?
> +
> + copy.regs[R_EAX] = 0x80000000;
> + qemu_kvm_cpuid_on_env(©);
> + limit = copy.regs[R_EAX];
> +
> + for (i = 0x80000000; i <= limit; ++i)
> + do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, ©);
Same here.
> + }
> +
-------------------------------------------------------------------------
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
