| Message ID | 1248811515-6877-1-git-send-email-glommer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/28/2009 11:05 PM, Glauber Costa wrote: > use cpuid code from upstream. By doing that, we lose the following snippet > in kvm_get_supported_cpuid(): > > ret |= 1<< 12; /* MTRR */ > ret |= 1<< 16; /* PAT */ > ret |= 1<< 7; /* MCE */ > ret |= 1<< 14; /* MCA */ > > A quick search in mailing lists says this code is not really necessary, and we're > keeping it just for backwards compatibility. This is not that important, because > we'd lose it anyway in the golden day in which we totally merge with qemu. > Anyway, if it do _is_ important, we can send a patch to qemu with it. > It is important. Please don't introduce regressions (if you do, introduce them in separate patches). The procedure to drop such workarounds for kernel bugs is to verify that major distros have the kernel fixes in their supported kernels.
On Mon, Aug 03, 2009 at 02:57:47PM +0300, Avi Kivity wrote: > On 07/28/2009 11:05 PM, Glauber Costa wrote: >> use cpuid code from upstream. By doing that, we lose the following snippet >> in kvm_get_supported_cpuid(): >> >> ret |= 1<< 12; /* MTRR */ >> ret |= 1<< 16; /* PAT */ >> ret |= 1<< 7; /* MCE */ >> ret |= 1<< 14; /* MCA */ >> >> A quick search in mailing lists says this code is not really necessary, and we're >> keeping it just for backwards compatibility. This is not that important, because >> we'd lose it anyway in the golden day in which we totally merge with qemu. >> Anyway, if it do _is_ important, we can send a patch to qemu with it. >> > > It is important. Please don't introduce regressions (if you do, > introduce them in separate patches). The procedure to drop such > workarounds for kernel bugs is to verify that major distros have the > kernel fixes in their supported kernels. Since this was introduced to fix a bug that did not even existed in Windows, I can't see what you mean by "distro kernels" here. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2009 04:22 PM, Glauber Costa wrote: > On Mon, Aug 03, 2009 at 02:57:47PM +0300, Avi Kivity wrote: > >> On 07/28/2009 11:05 PM, Glauber Costa wrote: >> >>> use cpuid code from upstream. By doing that, we lose the following snippet >>> in kvm_get_supported_cpuid(): >>> >>> ret |= 1<< 12; /* MTRR */ >>> ret |= 1<< 16; /* PAT */ >>> ret |= 1<< 7; /* MCE */ >>> ret |= 1<< 14; /* MCA */ >>> >>> A quick search in mailing lists says this code is not really necessary, and we're >>> keeping it just for backwards compatibility. This is not that important, because >>> we'd lose it anyway in the golden day in which we totally merge with qemu. >>> Anyway, if it do _is_ important, we can send a patch to qemu with it. >>> >>> >> It is important. Please don't introduce regressions (if you do, >> introduce them in separate patches). The procedure to drop such >> workarounds for kernel bugs is to verify that major distros have the >> kernel fixes in their supported kernels. >> > Since this was introduced to fix a bug that did not even existed in Windows, > I can't see what you mean by "distro kernels" here. > If qemu-kvm with this patch works on Fedora 10 (latest kernel) and the equivalent opensuse and Ubuntu kernels, then we can safely remove the bug workaround. If not, if we apply the patch we just cause users needless pain.
On Mon, Aug 03, 2009 at 05:06:28PM +0300, Avi Kivity wrote: > On 08/03/2009 04:22 PM, Glauber Costa wrote: >> On Mon, Aug 03, 2009 at 02:57:47PM +0300, Avi Kivity wrote: >> >>> On 07/28/2009 11:05 PM, Glauber Costa wrote: >>> >>>> use cpuid code from upstream. By doing that, we lose the following snippet >>>> in kvm_get_supported_cpuid(): >>>> >>>> ret |= 1<< 12; /* MTRR */ >>>> ret |= 1<< 16; /* PAT */ >>>> ret |= 1<< 7; /* MCE */ >>>> ret |= 1<< 14; /* MCA */ >>>> >>>> A quick search in mailing lists says this code is not really necessary, and we're >>>> keeping it just for backwards compatibility. This is not that important, because >>>> we'd lose it anyway in the golden day in which we totally merge with qemu. >>>> Anyway, if it do _is_ important, we can send a patch to qemu with it. >>>> >>>> >>> It is important. Please don't introduce regressions (if you do, >>> introduce them in separate patches). The procedure to drop such >>> workarounds for kernel bugs is to verify that major distros have the >>> kernel fixes in their supported kernels. >>> >> Since this was introduced to fix a bug that did not even existed in Windows, >> I can't see what you mean by "distro kernels" here. >> > > If qemu-kvm with this patch works on Fedora 10 (latest kernel) and the > equivalent opensuse and Ubuntu kernels, then we can safely remove the > bug workaround. If not, if we apply the patch we just cause users > needless pain. Again, since it was reported to fix a problem (that did not even existed in the first place) with Windows Vista, I don't really know why shouldn'it it work with any of the Linux guests (since they were not affected, to begin with) However, if you point me to a simple test case, I can definitely test it to make sure no weird condition is taking place. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2009 05:12 PM, Glauber Costa wrote: >> If qemu-kvm with this patch works on Fedora 10 (latest kernel) and the >> equivalent opensuse and Ubuntu kernels, then we can safely remove the >> bug workaround. If not, if we apply the patch we just cause users >> needless pain. >> > Again, since it was reported to fix a problem (that did not even existed in > the first place) with Windows Vista, I don't really know why shouldn'it > it work with any of the Linux guests (since they were not affected, to begin > with) > We aim to support more than just Linux guests. I don't understand what you mean by "the problem did not even existed in the first place". I assure you it existed. > However, if you point me to a simple test case, I can definitely test it to make > sure no weird condition is taking place Install Windows Vista x64 on a host running a distro kernel.
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c index 492dbc5..c12bc78 100644 --- a/qemu-kvm-x86.c +++ b/qemu-kvm-x86.c @@ -660,106 +660,6 @@ int kvm_disable_tpr_access_reporting(kvm_vcpu_context_t vcpu) #endif -#ifdef KVM_CAP_EXT_CPUID - -static struct kvm_cpuid2 *try_get_cpuid(kvm_context_t kvm, int max) -{ - struct kvm_cpuid2 *cpuid; - int r, size; - - size = sizeof(*cpuid) + max * sizeof(*cpuid->entries); - cpuid = qemu_malloc(size); - cpuid->nent = max; - r = kvm_ioctl(kvm_state, KVM_GET_SUPPORTED_CPUID, cpuid); - if (r == 0 && cpuid->nent >= max) - r = -E2BIG; - if (r < 0) { - if (r == -E2BIG) { - free(cpuid); - return NULL; - } else { - fprintf(stderr, "KVM_GET_SUPPORTED_CPUID failed: %s\n", - strerror(-r)); - exit(1); - } - } - return cpuid; -} - -#define R_EAX 0 -#define R_ECX 1 -#define R_EDX 2 -#define R_EBX 3 -#define R_ESP 4 -#define R_EBP 5 -#define R_ESI 6 -#define R_EDI 7 - -uint32_t kvm_get_supported_cpuid(kvm_context_t kvm, uint32_t function, int reg) -{ - struct kvm_cpuid2 *cpuid; - int i, max; - uint32_t ret = 0; - uint32_t cpuid_1_edx; - - if (!kvm_check_extension(kvm_state, KVM_CAP_EXT_CPUID)) { - return -1U; - } - - max = 1; - while ((cpuid = try_get_cpuid(kvm, max)) == NULL) { - max *= 2; - } - - for (i = 0; i < cpuid->nent; ++i) { - if (cpuid->entries[i].function == function) { - switch (reg) { - case R_EAX: - ret = cpuid->entries[i].eax; - break; - case R_EBX: - ret = cpuid->entries[i].ebx; - break; - case R_ECX: - ret = cpuid->entries[i].ecx; - break; - case R_EDX: - ret = cpuid->entries[i].edx; - if (function == 1) { - /* kvm misreports the following features - */ - ret |= 1 << 12; /* MTRR */ - ret |= 1 << 16; /* PAT */ - ret |= 1 << 7; /* MCE */ - ret |= 1 << 14; /* MCA */ - } - - /* On Intel, kvm returns cpuid according to - * the Intel spec, so add missing bits - * according to the AMD spec: - */ - if (function == 0x80000001) { - cpuid_1_edx = kvm_get_supported_cpuid(kvm, 1, R_EDX); - ret |= cpuid_1_edx & 0xdfeff7ff; - } - break; - } - } - } - - free(cpuid); - - return ret; -} - -#else - -uint32_t kvm_get_supported_cpuid(kvm_context_t kvm, uint32_t function, int reg) -{ - return -1U; -} - -#endif int kvm_qemu_create_memory_alias(uint64_t phys_start, uint64_t len, uint64_t target_phys) @@ -1241,19 +1141,6 @@ static int get_para_features(kvm_context_t kvm_context) return features; } -static void kvm_trim_features(uint32_t *features, uint32_t supported) -{ - int i; - uint32_t mask; - - for (i = 0; i < 32; ++i) { - mask = 1U << i; - if ((*features & mask) && !(supported & mask)) { - *features &= ~mask; - } - } -} - int kvm_arch_qemu_init_env(CPUState *cenv) { struct kvm_cpuid_entry2 cpuid_ent[100]; @@ -1671,12 +1558,6 @@ int kvm_arch_init_irq_routing(void) return 0; } -uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function, - int reg) -{ - return kvm_get_supported_cpuid(kvm_context, function, reg); -} - void kvm_arch_process_irqchip_events(CPUState *env) { kvm_arch_save_regs(env); diff --git a/target-i386/kvm.c b/target-i386/kvm.c index cfa5b80..359d27d 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -35,6 +35,7 @@ do { } while (0) #endif +#endif #ifdef KVM_CAP_EXT_CPUID static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max) @@ -130,6 +131,7 @@ static void kvm_trim_features(uint32_t *features, uint32_t supported) } } } +#ifdef KVM_UPSTREAM int kvm_arch_init_vcpu(CPUState *env) {
use cpuid code from upstream. By doing that, we lose the following snippet in kvm_get_supported_cpuid(): ret |= 1 << 12; /* MTRR */ ret |= 1 << 16; /* PAT */ ret |= 1 << 7; /* MCE */ ret |= 1 << 14; /* MCA */ A quick search in mailing lists says this code is not really necessary, and we're keeping it just for backwards compatibility. This is not that important, because we'd lose it anyway in the golden day in which we totally merge with qemu. Anyway, if it do _is_ important, we can send a patch to qemu with it. Signed-off-by: Glauber Costa <glommer@redhat.com> --- qemu-kvm-x86.c | 119 ----------------------------------------------------- target-i386/kvm.c | 2 + 2 files changed, 2 insertions(+), 119 deletions(-)