From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH] Ignore DEBUGCTL MSRs Date: Tue, 22 Jul 2008 08:00:45 +0200 Message-ID: <4885778D.5010306@suse.de> References: <4860806D.6000608@suse.de> <486F6904.2040907@qumranet.com> <4874B3B7.7060003@suse.de> <48761651.3030407@qumranet.com> <487DBECD.7000605@suse.de> <48845141.7080701@qumranet.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------040703050600080304010103" Cc: kvm@vger.kernel.org, joerg.roedel@amd.com To: Avi Kivity Return-path: Received: from ns.suse.de ([195.135.220.2]:57272 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757520AbYGVGAx (ORCPT ); Tue, 22 Jul 2008 02:00:53 -0400 In-Reply-To: <48845141.7080701@qumranet.com> Sender: kvm-owner@vger.kernel.org List-ID: This is a multi-part message in MIME format. --------------040703050600080304010103 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Avi Kivity wrote: > Alexander Graf wrote: >>> >>>> + pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n", >>>> + __func__, data); >>>> >>> >>> We can avoid the printout if data == 0, since we support that case >>> fully. >> >> I was thinking a lot about that. Even though we support data == 0, >> usually the kernel log output is useful for people trying to find if >> something is cause a problem. If they see that DEBUGCTL gets set, but >> won't see it getting unset, they'd get confused IMHO. >> So the current behavior is on purpose, but if you oppose to that >> idea, please tell me. >> > > Once it gets set, you can expect brokenness. It doesn't matter if it > gets unset later. So IMO not printing on data == 0 is best: quiet on > the cases we support, and loud on cases we don't. So yes, I'd prefer > it changed. > > Corrected patch follows... --- Netware writes to DEBUGCTL and reads from the DEBUGCTL and LAST*IP MSRs without further checks and is really confused to receive a #GP during that. To make it happy we should just make them stubs, which is exactly what SVM already does. Writes to DEBUGCTL that are vendor-specific are resembled to behave as if the virtual CPU does not know them. Signed-off-by: Alexander Graf --------------040703050600080304010103 Content-Type: text/x-patch; name="kvm-netware.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="kvm-netware.patch" diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fc0721e..10f5e95 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -609,6 +609,15 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) pr_unimpl(vcpu, "%s: MSR_IA32_MCG_CTL 0x%llx, nop\n", __func__, data); break; + case MSR_IA32_DEBUGCTLMSR: + if (!data) { + /* We support the non-activated case already */ + break; + } else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) { + /* Values other than LBR and BTF are vendor-specific, + thus reserved and should throw a #GP */ + return 1; + } + pr_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n", + __func__, data); + break; case MSR_IA32_UCODE_REV: case MSR_IA32_UCODE_WRITE: break; @@ -705,6 +714,11 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) case MSR_IA32_MC0_MISC+16: case MSR_IA32_UCODE_REV: case MSR_IA32_EBL_CR_POWERON: + case MSR_IA32_DEBUGCTLMSR: + case MSR_IA32_LASTBRANCHFROMIP: + case MSR_IA32_LASTBRANCHTOIP: + case MSR_IA32_LASTINTFROMIP: + case MSR_IA32_LASTINTTOIP: data = 0; break; case MSR_MTRRcap: --------------040703050600080304010103--