diff mbox

x86, kvm: Add MSR_AMD64_BU_CFG2 to the list of ignored MSRs

Message ID 1360755761-725-1-git-send-email-bp@alien8.de (mailing list archive)
State New, archived
Headers show

Commit Message

Borislav Petkov Feb. 13, 2013, 11:42 a.m. UTC 
From: Borislav Petkov <bp@suse.de>

The "x86, AMD: Enable WC+ memory type on family 10 processors" patch
currently in -tip added a workaround for AMD F10h CPUs which #GPs my
guest when booted in kvm. This is because it accesses MSR_AMD64_BU_CFG2
which is not currently ignored by kvm. Do that because this MSR is only
baremetal-relevant anyway. While at it, move the ignored MSRs at the
beginning of kvm_set_msr_common so that we exit then and there.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Boris Ostrovsky <boris.ostrovsky@amd.com>
Cc: Andre Przywara <andre@andrep.de>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/x86.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Gleb Natapov Feb. 13, 2013, 11:47 a.m. UTC | #1 
On Wed, Feb 13, 2013 at 12:42:41PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> The "x86, AMD: Enable WC+ memory type on family 10 processors" patch
> currently in -tip added a workaround for AMD F10h CPUs which #GPs my
> guest when booted in kvm. This is because it accesses MSR_AMD64_BU_CFG2
> which is not currently ignored by kvm. Do that because this MSR is only
> baremetal-relevant anyway. While at it, move the ignored MSRs at the
> beginning of kvm_set_msr_common so that we exit then and there.
> 
Is your guest compiled without PV support? With PV Linux traps #GP for
all MSRs and it saves us in more than one places. 

> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Boris Ostrovsky <boris.ostrovsky@amd.com>
> Cc: Andre Przywara <andre@andrep.de>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c243b81e3c74..37040079cd6b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1881,6 +1881,14 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 data = msr_info->data;
>  
>  	switch (msr) {
> +	case MSR_AMD64_NB_CFG:
> +	case MSR_IA32_UCODE_REV:
> +	case MSR_IA32_UCODE_WRITE:
> +	case MSR_VM_HSAVE_PA:
> +	case MSR_AMD64_PATCH_LOADER:
> +	case MSR_AMD64_BU_CFG2:
> +		break;
> +
>  	case MSR_EFER:
>  		return set_efer(vcpu, data);
>  	case MSR_K7_HWCR:
> @@ -1900,8 +1908,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			return 1;
>  		}
>  		break;
> -	case MSR_AMD64_NB_CFG:
> -		break;
>  	case MSR_IA32_DEBUGCTLMSR:
>  		if (!data) {
>  			/* We support the non-activated case already */
> @@ -1914,11 +1920,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
>  			    __func__, data);
>  		break;
> -	case MSR_IA32_UCODE_REV:
> -	case MSR_IA32_UCODE_WRITE:
> -	case MSR_VM_HSAVE_PA:
> -	case MSR_AMD64_PATCH_LOADER:
> -		break;
>  	case 0x200 ... 0x2ff:
>  		return set_msr_mtrr(vcpu, msr, data);
>  	case MSR_IA32_APICBASE:
> @@ -2253,6 +2254,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>  	case MSR_K8_INT_PENDING_MSG:
>  	case MSR_AMD64_NB_CFG:
>  	case MSR_FAM10H_MMIO_CONF_BASE:
> +	case MSR_AMD64_BU_CFG2:
>  		data = 0;
>  		break;
>  	case MSR_P6_PERFCTR0:
> -- 
> 1.8.1.3.535.ga923c31

--
			Gleb.
--
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
Borislav Petkov Feb. 13, 2013, 11:55 a.m. UTC | #2 
On Wed, Feb 13, 2013 at 01:47:47PM +0200, Gleb Natapov wrote:
> On Wed, Feb 13, 2013 at 12:42:41PM +0100, Borislav Petkov wrote:
> > From: Borislav Petkov <bp@suse.de>
> > 
> > The "x86, AMD: Enable WC+ memory type on family 10 processors" patch
> > currently in -tip added a workaround for AMD F10h CPUs which #GPs my
> > guest when booted in kvm. This is because it accesses MSR_AMD64_BU_CFG2
> > which is not currently ignored by kvm. Do that because this MSR is only
> > baremetal-relevant anyway. While at it, move the ignored MSRs at the
> > beginning of kvm_set_msr_common so that we exit then and there.
> > 
> Is your guest compiled without PV support? With PV Linux traps #GP for
> all MSRs and it saves us in more than one places.

Yes, CONFIG_PARAVIRT_GUEST is not set on the guest kernel.
Gleb Natapov Feb. 13, 2013, 12:10 p.m. UTC | #3 
On Wed, Feb 13, 2013 at 12:55:26PM +0100, Borislav Petkov wrote:
> On Wed, Feb 13, 2013 at 01:47:47PM +0200, Gleb Natapov wrote:
> > On Wed, Feb 13, 2013 at 12:42:41PM +0100, Borislav Petkov wrote:
> > > From: Borislav Petkov <bp@suse.de>
> > > 
> > > The "x86, AMD: Enable WC+ memory type on family 10 processors" patch
> > > currently in -tip added a workaround for AMD F10h CPUs which #GPs my
> > > guest when booted in kvm. This is because it accesses MSR_AMD64_BU_CFG2
> > > which is not currently ignored by kvm. Do that because this MSR is only
> > > baremetal-relevant anyway. While at it, move the ignored MSRs at the
> > > beginning of kvm_set_msr_common so that we exit then and there.
> > > 
> > Is your guest compiled without PV support? With PV Linux traps #GP for
> > all MSRs and it saves us in more than one places.
> 
> Yes, CONFIG_PARAVIRT_GUEST is not set on the guest kernel.
> 
Thanks. It does not mean that the patch should not be applied though.
I cannot seems to find the documentation for the MSR anywhere, do you
have a pointer?

--
			Gleb.
--
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
Borislav Petkov Feb. 13, 2013, 1:35 p.m. UTC | #4 
On Wed, Feb 13, 2013 at 02:10:02PM +0200, Gleb Natapov wrote:
> > > Is your guest compiled without PV support? With PV Linux traps #GP for
> > > all MSRs and it saves us in more than one places.
> > 
> > Yes, CONFIG_PARAVIRT_GUEST is not set on the guest kernel.
> > 
> Thanks. It does not mean that the patch should not be applied though.

Right,

but, come to think of it, adding an MSR each time to those functions
could turn out to be a PITA. The PV solution with trapping on the MSR
accesses might be better so maybe CONFIG_KVM should do

select KVM_GUEST

?

This is even a good forward-looking solution.

> I cannot seems to find the documentation for the MSR anywhere, do you
> have a pointer?

http://support.amd.com/us/Processor_TechDocs/31116.pdf, p.438

Thanks.
Gleb Natapov Feb. 13, 2013, 3:37 p.m. UTC | #5 
On Wed, Feb 13, 2013 at 02:35:49PM +0100, Borislav Petkov wrote:
> On Wed, Feb 13, 2013 at 02:10:02PM +0200, Gleb Natapov wrote:
> > > > Is your guest compiled without PV support? With PV Linux traps #GP for
> > > > all MSRs and it saves us in more than one places.
> > > 
> > > Yes, CONFIG_PARAVIRT_GUEST is not set on the guest kernel.
> > > 
> > Thanks. It does not mean that the patch should not be applied though.
> 
> Right,
> 
> but, come to think of it, adding an MSR each time to those functions
> could turn out to be a PITA. The PV solution with trapping on the MSR
> accesses might be better so maybe CONFIG_KVM should do
> 
PV solution does not exists for some other guests.

> select KVM_GUEST
> 
> ?
I can easily imaging the situation where one whats to build different
kernels for host and guest and do not have PV in the host one.

> 
> This is even a good forward-looking solution.
> 
> > I cannot seems to find the documentation for the MSR anywhere, do you
> > have a pointer?
> 
> http://support.amd.com/us/Processor_TechDocs/31116.pdf, p.438
> 
Thanks,

--
			Gleb.
--
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
Gleb Natapov Feb. 13, 2013, 3:39 p.m. UTC | #6 
On Wed, Feb 13, 2013 at 12:42:41PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> The "x86, AMD: Enable WC+ memory type on family 10 processors" patch
> currently in -tip added a workaround for AMD F10h CPUs which #GPs my
> guest when booted in kvm. This is because it accesses MSR_AMD64_BU_CFG2
> which is not currently ignored by kvm. Do that because this MSR is only
> baremetal-relevant anyway. While at it, move the ignored MSRs at the
> beginning of kvm_set_msr_common so that we exit then and there.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Boris Ostrovsky <boris.ostrovsky@amd.com>
> Cc: Andre Przywara <andre@andrep.de>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>

Acked-by: Gleb Natapov <gleb@redhat.com>

Ingo, MSR_AMD64_BU_CFG2 exists only on tip. Can you get this through the
tip too?

> ---
>  arch/x86/kvm/x86.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c243b81e3c74..37040079cd6b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1881,6 +1881,14 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 data = msr_info->data;
>  
>  	switch (msr) {
> +	case MSR_AMD64_NB_CFG:
> +	case MSR_IA32_UCODE_REV:
> +	case MSR_IA32_UCODE_WRITE:
> +	case MSR_VM_HSAVE_PA:
> +	case MSR_AMD64_PATCH_LOADER:
> +	case MSR_AMD64_BU_CFG2:
> +		break;
> +
>  	case MSR_EFER:
>  		return set_efer(vcpu, data);
>  	case MSR_K7_HWCR:
> @@ -1900,8 +1908,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			return 1;
>  		}
>  		break;
> -	case MSR_AMD64_NB_CFG:
> -		break;
>  	case MSR_IA32_DEBUGCTLMSR:
>  		if (!data) {
>  			/* We support the non-activated case already */
> @@ -1914,11 +1920,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
>  			    __func__, data);
>  		break;
> -	case MSR_IA32_UCODE_REV:
> -	case MSR_IA32_UCODE_WRITE:
> -	case MSR_VM_HSAVE_PA:
> -	case MSR_AMD64_PATCH_LOADER:
> -		break;
>  	case 0x200 ... 0x2ff:
>  		return set_msr_mtrr(vcpu, msr, data);
>  	case MSR_IA32_APICBASE:
> @@ -2253,6 +2254,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>  	case MSR_K8_INT_PENDING_MSG:
>  	case MSR_AMD64_NB_CFG:
>  	case MSR_FAM10H_MMIO_CONF_BASE:
> +	case MSR_AMD64_BU_CFG2:
>  		data = 0;
>  		break;
>  	case MSR_P6_PERFCTR0:
> -- 
> 1.8.1.3.535.ga923c31

--
			Gleb.
--
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
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c243b81e3c74..37040079cd6b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1881,6 +1881,14 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 data = msr_info->data;
 
 	switch (msr) {
+	case MSR_AMD64_NB_CFG:
+	case MSR_IA32_UCODE_REV:
+	case MSR_IA32_UCODE_WRITE:
+	case MSR_VM_HSAVE_PA:
+	case MSR_AMD64_PATCH_LOADER:
+	case MSR_AMD64_BU_CFG2:
+		break;
+
 	case MSR_EFER:
 		return set_efer(vcpu, data);
 	case MSR_K7_HWCR:
@@ -1900,8 +1908,6 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 		}
 		break;
-	case MSR_AMD64_NB_CFG:
-		break;
 	case MSR_IA32_DEBUGCTLMSR:
 		if (!data) {
 			/* We support the non-activated case already */
@@ -1914,11 +1920,6 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
 			    __func__, data);
 		break;
-	case MSR_IA32_UCODE_REV:
-	case MSR_IA32_UCODE_WRITE:
-	case MSR_VM_HSAVE_PA:
-	case MSR_AMD64_PATCH_LOADER:
-		break;
 	case 0x200 ... 0x2ff:
 		return set_msr_mtrr(vcpu, msr, data);
 	case MSR_IA32_APICBASE:
@@ -2253,6 +2254,7 @@  int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_K8_INT_PENDING_MSG:
 	case MSR_AMD64_NB_CFG:
 	case MSR_FAM10H_MMIO_CONF_BASE:
+	case MSR_AMD64_BU_CFG2:
 		data = 0;
 		break;
 	case MSR_P6_PERFCTR0: