Hi Avi, Thanks for the comments on the patches. please see bellow for my response to your comments.
Thanks & Regards,
Nitin
On Tue, 2008-11-04 at 07:59 -0700, Avi Kivity wrote:
> You need changelog entries that describe the patches. Also, one patch
> per email.
>
sounds right. I will cut my patch into smaller patches and send them one
by one.
> > @ -1194,36 +1194,14 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2
> > *entry, u32 function,
> > static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> > u32 index, int *nent, int maxnent)
> > {
> > - const u32 kvm_supported_word0_x86_features = bit(X86_FEATURE_FPU) |
> > - bit(X86_FEATURE_VME) | bit(X86_FEATURE_DE) |
> > - bit(X86_FEATURE_PSE) | bit(X86_FEATURE_TSC) |
> > - bit(X86_FEATURE_MSR) | bit(X86_FEATURE_PAE) |
> > - bit(X86_FEATURE_CX8) | bit(X86_FEATURE_APIC) |
> > - bit(X86_FEATURE_SEP) | bit(X86_FEATURE_PGE) |
> > - bit(X86_FEATURE_CMOV) | bit(X86_FEATURE_PSE36) |
> > - bit(X86_FEATURE_CLFLSH) | bit(X86_FEATURE_MMX) |
> > - bit(X86_FEATURE_FXSR) | bit(X86_FEATURE_XMM) |
> > - bit(X86_FEATURE_XMM2) | bit(X86_FEATURE_SELFSNOOP);
> > - const u32 kvm_supported_word1_x86_features = bit(X86_FEATURE_FPU) |
> > - bit(X86_FEATURE_VME) | bit(X86_FEATURE_DE) |
> > - bit(X86_FEATURE_PSE) | bit(X86_FEATURE_TSC) |
> > - bit(X86_FEATURE_MSR) | bit(X86_FEATURE_PAE) |
> > - bit(X86_FEATURE_CX8) | bit(X86_FEATURE_APIC) |
> > - bit(X86_FEATURE_PGE) |
> > - bit(X86_FEATURE_CMOV) | bit(X86_FEATURE_PSE36) |
> > - bit(X86_FEATURE_MMX) | bit(X86_FEATURE_FXSR) |
> > - bit(X86_FEATURE_SYSCALL) |
> > - (bit(X86_FEATURE_NX) && is_efer_nx()) |
> > -#ifdef CONFIG_X86_64
> > + const u32 kvm_unsupported_word0_x86_features = bit(X86_FEATURE_ACC);
> > + const u32 kvm_unsupported_word1_x86_features =
> > bit(X86_FEATURE_RDTSCP) |
> > +#ifndef CONFIG_X86_64
> >
>
> What's the motivation for this change?
>
> A potential problem is that a newer cpu might define new bits, which we
> don't support, yet we report them as supported.
The motivation is: IMO There would be very few features which KVM will
not be able to support by default. Also lots of cpuid bits are being
blocked unnecessarily by the current code.
If there is a new feature which would break KVM implementation then it
will easy to notice, and easy to block that bit in the KVM at that time.
But if the new feature is not affecting KVM then it will not be noticed,
and KVM will end up unsupporting that cpu feature.
>
> > get_cpu() before
> > @@ -1246,6 +1224,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2
> > *entry, u32 function,
> > int t, times = entry->eax & 0xff;
> >
> > entry->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
> > + entry->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
> > for (t = 1; t < times && *nent < maxnent; ++t) {
> > do_cpuid_1_ent(&entry[t], function, 0);
> > entry[t].flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
> >
>
> Why?
This is a bug fix. Without this change the code does not find a leaf 2
entry in the list.
>
> > @@ -1276,7 +1255,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2
> > *entry, u32 function,
> > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> > /* read more entries until level_type is zero */
> > for (i = 1; *nent < maxnent; ++i) {
> > - level_type = entry[i - 1].ecx & 0xff;
> > + level_type = entry[i - 1].ecx & 0xff00;
> > if (!level_type)
> > break;
> > do_cpuid_1_ent(&entry[i], function, i);
> >
>
> Why?
This is also a bugfix. The type field is in bits 8-15. As per the code,
the bits 0-7 would not define the end of counting. You can look at IA32
processor Software developer's manual for this.
>
> > @@ -1302,10 +1281,14 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct
> > kvm_cpuid2 *cpuid,
> > {
> > struct kvm_cpuid_entry2 *cpuid_entries;
> > int limit, nent = 0, r = -E2BIG;
> > + int sizer = 0;
> > u32 func;
> >
> > - if (cpuid->nent < 1)
> > - goto out;
> > + if (cpuid->nent == 0) {
> > + sizer = 1;
> > + cpuid->nent = KVM_MAX_CPUID_ENTRIES;
> > + }
> > +
> >
>
> That's an ABI change. New userspace could call an older kernel with
> nent = 0, and get an unexpected response.
That is right. What do you suggest for solution? Currently this ABI is
not utilized. Would adding another ioctl would work?
> > @@ -2860,6 +2845,13 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
> > kvm_register_write(vcpu, VCPU_REGS_RCX, best->ecx);
> > kvm_register_write(vcpu, VCPU_REGS_RDX, best->edx);
> > }
> > + if (function == 0x1) { /* set the per-cpu apicid */
> > + u32 ebx = kvm_register_read(vcpu, VCPU_REGS_RBX);
> > + ebx &= 0x00ffffff;
> > + ebx |= (vcpu->vcpu_id << 24);
> > + kvm_register_write(vcpu, VCPU_REGS_RBX, ebx);
> > + }
> > +
> >
>
> ABI change again. This is userspace's job.
This does not look like an ABI change to me. I will look if vcpu_id is
available in the userspace, if yes, then this code can be moved into
userspace.
>
> --
> error compiling committee.c: too many arguments to function
>
--
Thanks & Regards,
Nitin
Open Source Technology Center, Intel Corporation
-----------------------------------------------------------------
The mind is like a parachute; it works much better when it's open
signature.asc
Description: This is a digitally signed message part
