linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core
@ 2021-08-09  7:48 Like Xu
  2021-08-09 14:12 ` Liang, Kan
  0 siblings, 1 reply; 5+ messages in thread
From: Like Xu @ 2021-08-09  7:48 UTC (permalink / raw)
  To: Paolo Bonzini, Kan Liang
  Cc: Andi Kleen, Tony Luck, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, x86, linux-kernel

From: Like Xu <likexu@tencent.com>

According to Intel SDM, the Last Branch Record Filtering Select Register
(R/W) is defined as shared per physical core rather than per logical core
on some older Intel platforms: Silvermont, Airmont, Goldmont and Nehalem.

To avoid LBR attacks or accidental data leakage, on these specific
platforms, KVM should not expose guest LBR capability even if HT is
disabled on the host, considering that the HT state can be dynamically
changed, yet the KVM capabilities are initialized at module initialisation.

Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the MSR_IA32_PERF_CAPABILITIES")
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/include/asm/intel-family.h |  1 +
 arch/x86/kvm/vmx/capabilities.h     | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h
index 27158436f322..f35c915566e3 100644
--- a/arch/x86/include/asm/intel-family.h
+++ b/arch/x86/include/asm/intel-family.h
@@ -119,6 +119,7 @@
 
 #define INTEL_FAM6_ATOM_SILVERMONT	0x37 /* Bay Trail, Valleyview */
 #define INTEL_FAM6_ATOM_SILVERMONT_D	0x4D /* Avaton, Rangely */
+#define INTEL_FAM6_ATOM_SILVERMONT_X3	0x5D /* X3-C3000 based on Silvermont */
 #define INTEL_FAM6_ATOM_SILVERMONT_MID	0x4A /* Merriefield */
 
 #define INTEL_FAM6_ATOM_AIRMONT		0x4C /* Cherry Trail, Braswell */
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index 4705ad55abb5..ff9596d7112d 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -3,6 +3,7 @@
 #define __KVM_X86_VMX_CAPS_H
 
 #include <asm/vmx.h>
+#include <asm/cpu_device_id.h>
 
 #include "lapic.h"
 
@@ -376,6 +377,21 @@ static inline bool vmx_pt_mode_is_host_guest(void)
 	return pt_mode == PT_MODE_HOST_GUEST;
 }
 
+static const struct x86_cpu_id lbr_select_shared_cpu[] = {
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_X3, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT_MID, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_PLUS, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EP, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_G, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EX, NULL),
+	{}
+};
+
 static inline u64 vmx_get_perf_capabilities(void)
 {
 	u64 perf_cap = 0;
@@ -383,7 +399,8 @@ static inline u64 vmx_get_perf_capabilities(void)
 	if (boot_cpu_has(X86_FEATURE_PDCM))
 		rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
 
-	perf_cap &= PMU_CAP_LBR_FMT;
+	if (!x86_match_cpu(lbr_select_shared_cpu))
+		perf_cap &= PMU_CAP_LBR_FMT;
 
 	/*
 	 * Since counters are virtualized, KVM would support full
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core
  2021-08-09  7:48 [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core Like Xu
@ 2021-08-09 14:12 ` Liang, Kan
  2021-08-09 15:08   ` Like Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Liang, Kan @ 2021-08-09 14:12 UTC (permalink / raw)
  To: Like Xu, Paolo Bonzini
  Cc: Andi Kleen, Tony Luck, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, x86, linux-kernel



On 8/9/2021 3:48 AM, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> According to Intel SDM, the Last Branch Record Filtering Select Register
> (R/W) is defined as shared per physical core rather than per logical core
> on some older Intel platforms: Silvermont, Airmont, Goldmont and Nehalem.
> 
> To avoid LBR attacks or accidental data leakage, on these specific
> platforms, KVM should not expose guest LBR capability even if HT is
> disabled on the host, considering that the HT state can be dynamically
> changed, yet the KVM capabilities are initialized at module initialisation.
> 
> Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the MSR_IA32_PERF_CAPABILITIES")
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>   arch/x86/include/asm/intel-family.h |  1 +
>   arch/x86/kvm/vmx/capabilities.h     | 19 ++++++++++++++++++-
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h
> index 27158436f322..f35c915566e3 100644
> --- a/arch/x86/include/asm/intel-family.h
> +++ b/arch/x86/include/asm/intel-family.h
> @@ -119,6 +119,7 @@
>   
>   #define INTEL_FAM6_ATOM_SILVERMONT	0x37 /* Bay Trail, Valleyview */
>   #define INTEL_FAM6_ATOM_SILVERMONT_D	0x4D /* Avaton, Rangely */
> +#define INTEL_FAM6_ATOM_SILVERMONT_X3	0x5D /* X3-C3000 based on Silvermont */


Please submit a separate patch if you want to add a new CPU ID. Also, 
the comments should be platform code name, not the model.

AFAIK, Atom X3 should be SoFIA which is for mobile phone. It's an old 
product. I don't think I enabled it in perf. I have no idea why you want 
to add it here for KVM. If you have a product and want to enable it, I 
guess you may want to enable it for perf first.

Thanks,
Kan

>   #define INTEL_FAM6_ATOM_SILVERMONT_MID	0x4A /* Merriefield */
>   
>   #define INTEL_FAM6_ATOM_AIRMONT		0x4C /* Cherry Trail, Braswell */
> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index 4705ad55abb5..ff9596d7112d 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -3,6 +3,7 @@
>   #define __KVM_X86_VMX_CAPS_H
>   
>   #include <asm/vmx.h>
> +#include <asm/cpu_device_id.h>
>   
>   #include "lapic.h"
>   
> @@ -376,6 +377,21 @@ static inline bool vmx_pt_mode_is_host_guest(void)
>   	return pt_mode == PT_MODE_HOST_GUEST;
>   }
>   
> +static const struct x86_cpu_id lbr_select_shared_cpu[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_X3, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT_MID, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_PLUS, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EP, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_G, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EX, NULL),
> +	{}
> +};
> +
>   static inline u64 vmx_get_perf_capabilities(void)
>   {
>   	u64 perf_cap = 0;
> @@ -383,7 +399,8 @@ static inline u64 vmx_get_perf_capabilities(void)
>   	if (boot_cpu_has(X86_FEATURE_PDCM))
>   		rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
>   
> -	perf_cap &= PMU_CAP_LBR_FMT;
> +	if (!x86_match_cpu(lbr_select_shared_cpu))
> +		perf_cap &= PMU_CAP_LBR_FMT;
>   
>   	/*
>   	 * Since counters are virtualized, KVM would support full
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core
  2021-08-09 14:12 ` Liang, Kan
@ 2021-08-09 15:08   ` Like Xu
  2021-08-09 18:03     ` Liang, Kan
  0 siblings, 1 reply; 5+ messages in thread
From: Like Xu @ 2021-08-09 15:08 UTC (permalink / raw)
  To: Liang, Kan
  Cc: Paolo Bonzini, Andi Kleen, Tony Luck, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	x86, linux-kernel

On Mon, Aug 9, 2021 at 10:12 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 8/9/2021 3:48 AM, Like Xu wrote:
> > From: Like Xu <likexu@tencent.com>
> >
> > According to Intel SDM, the Last Branch Record Filtering Select Register
> > (R/W) is defined as shared per physical core rather than per logical core
> > on some older Intel platforms: Silvermont, Airmont, Goldmont and Nehalem.
> >
> > To avoid LBR attacks or accidental data leakage, on these specific
> > platforms, KVM should not expose guest LBR capability even if HT is
> > disabled on the host, considering that the HT state can be dynamically
> > changed, yet the KVM capabilities are initialized at module initialisation.
> >
> > Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the MSR_IA32_PERF_CAPABILITIES")
> > Signed-off-by: Like Xu <likexu@tencent.com>
> > ---
> >   arch/x86/include/asm/intel-family.h |  1 +
> >   arch/x86/kvm/vmx/capabilities.h     | 19 ++++++++++++++++++-
> >   2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h
> > index 27158436f322..f35c915566e3 100644
> > --- a/arch/x86/include/asm/intel-family.h
> > +++ b/arch/x86/include/asm/intel-family.h
> > @@ -119,6 +119,7 @@
> >
> >   #define INTEL_FAM6_ATOM_SILVERMONT  0x37 /* Bay Trail, Valleyview */
> >   #define INTEL_FAM6_ATOM_SILVERMONT_D        0x4D /* Avaton, Rangely */
> > +#define INTEL_FAM6_ATOM_SILVERMONT_X3        0x5D /* X3-C3000 based on Silvermont */
>
>
> Please submit a separate patch if you want to add a new CPU ID. Also,
> the comments should be platform code name, not the model.
>
> AFAIK, Atom X3 should be SoFIA which is for mobile phone. It's an old
> product. I don't think I enabled it in perf. I have no idea why you want
> to add it here for KVM. If you have a product and want to enable it, I
> guess you may want to enable it for perf first.

Thanks for your clarification about SoFIA. I'll drop 0x5D check
for V2 since we doesn't have host support as you said.

Do the other models here and the idea of banning guest LBR make sense to you ?

>
> Thanks,
> Kan
>
> >   #define INTEL_FAM6_ATOM_SILVERMONT_MID      0x4A /* Merriefield */
> >
> >   #define INTEL_FAM6_ATOM_AIRMONT             0x4C /* Cherry Trail, Braswell */
> > diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> > index 4705ad55abb5..ff9596d7112d 100644
> > --- a/arch/x86/kvm/vmx/capabilities.h
> > +++ b/arch/x86/kvm/vmx/capabilities.h
> > @@ -3,6 +3,7 @@
> >   #define __KVM_X86_VMX_CAPS_H
> >
> >   #include <asm/vmx.h>
> > +#include <asm/cpu_device_id.h>
> >
> >   #include "lapic.h"
> >
> > @@ -376,6 +377,21 @@ static inline bool vmx_pt_mode_is_host_guest(void)
> >       return pt_mode == PT_MODE_HOST_GUEST;
> >   }
> >
> > +static const struct x86_cpu_id lbr_select_shared_cpu[] = {
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_X3, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT_MID, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_PLUS, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EP, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_G, NULL),
> > +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EX, NULL),
> > +     {}
> > +};
> > +
> >   static inline u64 vmx_get_perf_capabilities(void)
> >   {
> >       u64 perf_cap = 0;
> > @@ -383,7 +399,8 @@ static inline u64 vmx_get_perf_capabilities(void)
> >       if (boot_cpu_has(X86_FEATURE_PDCM))
> >               rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
> >
> > -     perf_cap &= PMU_CAP_LBR_FMT;
> > +     if (!x86_match_cpu(lbr_select_shared_cpu))
> > +             perf_cap &= PMU_CAP_LBR_FMT;
> >
> >       /*
> >        * Since counters are virtualized, KVM would support full
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core
  2021-08-09 15:08   ` Like Xu
@ 2021-08-09 18:03     ` Liang, Kan
  2021-08-10  3:21       ` [NAK][PATCH] " Like Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Liang, Kan @ 2021-08-09 18:03 UTC (permalink / raw)
  To: Like Xu
  Cc: Paolo Bonzini, Andi Kleen, Tony Luck, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	x86, linux-kernel



On 8/9/2021 11:08 AM, Like Xu wrote:
> On Mon, Aug 9, 2021 at 10:12 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 8/9/2021 3:48 AM, Like Xu wrote:
>>> From: Like Xu <likexu@tencent.com>
>>>
>>> According to Intel SDM, the Last Branch Record Filtering Select Register
>>> (R/W) is defined as shared per physical core rather than per logical core
>>> on some older Intel platforms: Silvermont, Airmont, Goldmont and Nehalem.
>>>
>>> To avoid LBR attacks or accidental data leakage, on these specific
>>> platforms, KVM should not expose guest LBR capability even if HT is
>>> disabled on the host, considering that the HT state can be dynamically
>>> changed, yet the KVM capabilities are initialized at module initialisation.
>>>
>>> Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the MSR_IA32_PERF_CAPABILITIES")
>>> Signed-off-by: Like Xu <likexu@tencent.com>
>>> ---
>>>    arch/x86/include/asm/intel-family.h |  1 +
>>>    arch/x86/kvm/vmx/capabilities.h     | 19 ++++++++++++++++++-
>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h
>>> index 27158436f322..f35c915566e3 100644
>>> --- a/arch/x86/include/asm/intel-family.h
>>> +++ b/arch/x86/include/asm/intel-family.h
>>> @@ -119,6 +119,7 @@
>>>
>>>    #define INTEL_FAM6_ATOM_SILVERMONT  0x37 /* Bay Trail, Valleyview */
>>>    #define INTEL_FAM6_ATOM_SILVERMONT_D        0x4D /* Avaton, Rangely */
>>> +#define INTEL_FAM6_ATOM_SILVERMONT_X3        0x5D /* X3-C3000 based on Silvermont */
>>
>>
>> Please submit a separate patch if you want to add a new CPU ID. Also,
>> the comments should be platform code name, not the model.
>>
>> AFAIK, Atom X3 should be SoFIA which is for mobile phone. It's an old
>> product. I don't think I enabled it in perf. I have no idea why you want
>> to add it here for KVM. If you have a product and want to enable it, I
>> guess you may want to enable it for perf first.
> 
> Thanks for your clarification about SoFIA. I'll drop 0x5D check
> for V2 since we doesn't have host support as you said.
> 
> Do the other models here and the idea of banning guest LBR make sense to you ?
> 

For the Atom after Silvermont, I don't think hyper-threading is 
supported. That's why it's per physical core. I don't think we should 
disable LBR because of it.

For Nehalem, it seems possible that the MSR_LBR_SELECT can be overridden 
if the other logical core has a different configure. But I'm not sure 
whether it brings any severe problems. Logical core A may miss some LBRs 
or get extra LBRs, but I don't think LBRs can be leaked to Logical core 
B. Also, Nehalem is a 13+ year old machine. Not sure how many people 
still use it.

LBR format 0 is also a valid format version, LBR_FORMAT_32. It seems 
this patch just forces the format to LBR_FORMAT_32 for these machines. 
It doesn't sound correct.

Thanks,
Kan
>>
>>>    #define INTEL_FAM6_ATOM_SILVERMONT_MID      0x4A /* Merriefield */
>>>
>>>    #define INTEL_FAM6_ATOM_AIRMONT             0x4C /* Cherry Trail, Braswell */
>>> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
>>> index 4705ad55abb5..ff9596d7112d 100644
>>> --- a/arch/x86/kvm/vmx/capabilities.h
>>> +++ b/arch/x86/kvm/vmx/capabilities.h
>>> @@ -3,6 +3,7 @@
>>>    #define __KVM_X86_VMX_CAPS_H
>>>
>>>    #include <asm/vmx.h>
>>> +#include <asm/cpu_device_id.h>
>>>
>>>    #include "lapic.h"
>>>
>>> @@ -376,6 +377,21 @@ static inline bool vmx_pt_mode_is_host_guest(void)
>>>        return pt_mode == PT_MODE_HOST_GUEST;
>>>    }
>>>
>>> +static const struct x86_cpu_id lbr_select_shared_cpu[] = {
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_X3, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT_MID, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_PLUS, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EP, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_G, NULL),
>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EX, NULL),
>>> +     {}
>>> +};
>>> +
>>>    static inline u64 vmx_get_perf_capabilities(void)
>>>    {
>>>        u64 perf_cap = 0;
>>> @@ -383,7 +399,8 @@ static inline u64 vmx_get_perf_capabilities(void)
>>>        if (boot_cpu_has(X86_FEATURE_PDCM))
>>>                rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
>>>
>>> -     perf_cap &= PMU_CAP_LBR_FMT;
>>> +     if (!x86_match_cpu(lbr_select_shared_cpu))
>>> +             perf_cap &= PMU_CAP_LBR_FMT;
>>>
>>>        /*
>>>         * Since counters are virtualized, KVM would support full
>>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [NAK][PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core
  2021-08-09 18:03     ` Liang, Kan
@ 2021-08-10  3:21       ` Like Xu
  0 siblings, 0 replies; 5+ messages in thread
From: Like Xu @ 2021-08-10  3:21 UTC (permalink / raw)
  To: Liang, Kan
  Cc: Paolo Bonzini, Andi Kleen, Tony Luck, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	x86, linux-kernel

Thank you, Kan.

On 10/8/2021 2:03 am, Liang, Kan wrote:
> 
> 
> On 8/9/2021 11:08 AM, Like Xu wrote:
>> On Mon, Aug 9, 2021 at 10:12 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>
>>>
>>>
>>> On 8/9/2021 3:48 AM, Like Xu wrote:
>>>> From: Like Xu <likexu@tencent.com>
>>>>
>>>> According to Intel SDM, the Last Branch Record Filtering Select Register
>>>> (R/W) is defined as shared per physical core rather than per logical core
>>>> on some older Intel platforms: Silvermont, Airmont, Goldmont and Nehalem.
>>>>
>>>> To avoid LBR attacks or accidental data leakage, on these specific
>>>> platforms, KVM should not expose guest LBR capability even if HT is
>>>> disabled on the host, considering that the HT state can be dynamically
>>>> changed, yet the KVM capabilities are initialized at module initialisation.
>>>>
>>>> Fixes: be635e34c284 ("KVM: vmx/pmu: Expose LBR_FMT in the 
>>>> MSR_IA32_PERF_CAPABILITIES")
>>>> Signed-off-by: Like Xu <likexu@tencent.com>
>>>> ---
>>>>    arch/x86/include/asm/intel-family.h |  1 +
>>>>    arch/x86/kvm/vmx/capabilities.h     | 19 ++++++++++++++++++-
>>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/intel-family.h 
>>>> b/arch/x86/include/asm/intel-family.h
>>>> index 27158436f322..f35c915566e3 100644
>>>> --- a/arch/x86/include/asm/intel-family.h
>>>> +++ b/arch/x86/include/asm/intel-family.h
>>>> @@ -119,6 +119,7 @@
>>>>
>>>>    #define INTEL_FAM6_ATOM_SILVERMONT  0x37 /* Bay Trail, Valleyview */
>>>>    #define INTEL_FAM6_ATOM_SILVERMONT_D        0x4D /* Avaton, Rangely */
>>>> +#define INTEL_FAM6_ATOM_SILVERMONT_X3        0x5D /* X3-C3000 based on 
>>>> Silvermont */
>>>
>>>
>>> Please submit a separate patch if you want to add a new CPU ID. Also,
>>> the comments should be platform code name, not the model.
>>>
>>> AFAIK, Atom X3 should be SoFIA which is for mobile phone. It's an old
>>> product. I don't think I enabled it in perf. I have no idea why you want
>>> to add it here for KVM. If you have a product and want to enable it, I
>>> guess you may want to enable it for perf first.
>>
>> Thanks for your clarification about SoFIA. I'll drop 0x5D check
>> for V2 since we doesn't have host support as you said.
>>
>> Do the other models here and the idea of banning guest LBR make sense to you ?
>>
> 
> For the Atom after Silvermont, I don't think hyper-threading is supported. 
> That's why it's per physical core. I don't think we should disable LBR because 
> of it.

In addition to your clarification below, it makes sense to keep it as it is.

> 
> For Nehalem, it seems possible that the MSR_LBR_SELECT can be overridden if the 
> other logical core has a different configure. But I'm not sure whether it brings 
> any severe problems. Logical core A may miss some LBRs or get extra LBRs, but I 
> don't think LBRs can be leaked to Logical core B. Also, Nehalem is a 13+ year 
> old machine. Not sure how many people still use it.

Allowing one guest to prevent another guest from using the LBR (writing zero
consistently) is quite a serious flaw, but considering that only such an old model
is affected, adding a maintenance burden to KVM here is not a good choice.

> 
> LBR format 0 is also a valid format version, LBR_FORMAT_32. It seems this patch 
> just forces the format to LBR_FORMAT_32 for these machines. It doesn't sound 
> correct.

Sigh, I assume that the platform reporting LBR_FORMAT_32 is older than Nehalem.

> 
> Thanks,
> Kan
>>>
>>>>    #define INTEL_FAM6_ATOM_SILVERMONT_MID      0x4A /* Merriefield */
>>>>
>>>>    #define INTEL_FAM6_ATOM_AIRMONT             0x4C /* Cherry Trail, 
>>>> Braswell */
>>>> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
>>>> index 4705ad55abb5..ff9596d7112d 100644
>>>> --- a/arch/x86/kvm/vmx/capabilities.h
>>>> +++ b/arch/x86/kvm/vmx/capabilities.h
>>>> @@ -3,6 +3,7 @@
>>>>    #define __KVM_X86_VMX_CAPS_H
>>>>
>>>>    #include <asm/vmx.h>
>>>> +#include <asm/cpu_device_id.h>
>>>>
>>>>    #include "lapic.h"
>>>>
>>>> @@ -376,6 +377,21 @@ static inline bool vmx_pt_mode_is_host_guest(void)
>>>>        return pt_mode == PT_MODE_HOST_GUEST;
>>>>    }
>>>>
>>>> +static const struct x86_cpu_id lbr_select_shared_cpu[] = {
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_D, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_X3, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT_MID, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(ATOM_GOLDMONT_PLUS, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EP, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_G, NULL),
>>>> +     X86_MATCH_INTEL_FAM6_MODEL(NEHALEM_EX, NULL),
>>>> +     {}
>>>> +};
>>>> +
>>>>    static inline u64 vmx_get_perf_capabilities(void)
>>>>    {
>>>>        u64 perf_cap = 0;
>>>> @@ -383,7 +399,8 @@ static inline u64 vmx_get_perf_capabilities(void)
>>>>        if (boot_cpu_has(X86_FEATURE_PDCM))
>>>>                rdmsrl(MSR_IA32_PERF_CAPABILITIES, perf_cap);
>>>>
>>>> -     perf_cap &= PMU_CAP_LBR_FMT;
>>>> +     if (!x86_match_cpu(lbr_select_shared_cpu))
>>>> +             perf_cap &= PMU_CAP_LBR_FMT;
>>>>
>>>>        /*
>>>>         * Since counters are virtualized, KVM would support full
>>>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-10  3:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09  7:48 [PATCH] KVM: x86/pmu: Don't expose guest LBR if the LBR_SELECT is shared per physical core Like Xu
2021-08-09 14:12 ` Liang, Kan
2021-08-09 15:08   ` Like Xu
2021-08-09 18:03     ` Liang, Kan
2021-08-10  3:21       ` [NAK][PATCH] " Like Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).