* [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
@ 2015-12-17 14:01 Jan Beulich
2015-12-17 14:05 ` Andrew Cooper
2015-12-17 14:12 ` Boris Ostrovsky
0 siblings, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2015-12-17 14:01 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Boris Ostrovsky,
Jun Nakajima
[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]
For one, loading the MSR with a possibly non-canonical address was
possible since the verification is conditional, while the MSR load
wasn't. And then for PV guests we need to further limit the range of
valid addresses to exclude the hypervisor range.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -366,7 +366,8 @@ static inline void __core2_vpmu_load(str
}
wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
- wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
+ if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
+ wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
if ( !has_hvm_container_vcpu(v) )
@@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
enabled_cntrs |= (1ULL << i);
}
- if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
- !is_canonical_address(core2_vpmu_cxt->ds_area) )
+ if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
+ !(has_hvm_container_vcpu(v)
+ ? is_canonical_address(core2_vpmu_cxt->ds_area)
+ : __addr_ok(core2_vpmu_cxt->ds_area)) )
return -EINVAL;
if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) ||
[-- Attachment #2: x86-ds-area-constraints.patch --]
[-- Type: text/plain, Size: 1381 bytes --]
x86/vPMU: constrain MSR_IA32_DS_AREA loads
For one, loading the MSR with a possibly non-canonical address was
possible since the verification is conditional, while the MSR load
wasn't. And then for PV guests we need to further limit the range of
valid addresses to exclude the hypervisor range.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -366,7 +366,8 @@ static inline void __core2_vpmu_load(str
}
wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
- wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
+ if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
+ wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
if ( !has_hvm_container_vcpu(v) )
@@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
enabled_cntrs |= (1ULL << i);
}
- if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
- !is_canonical_address(core2_vpmu_cxt->ds_area) )
+ if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
+ !(has_hvm_container_vcpu(v)
+ ? is_canonical_address(core2_vpmu_cxt->ds_area)
+ : __addr_ok(core2_vpmu_cxt->ds_area)) )
return -EINVAL;
if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) ||
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:01 [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads Jan Beulich
@ 2015-12-17 14:05 ` Andrew Cooper
2015-12-17 14:12 ` Boris Ostrovsky
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2015-12-17 14:05 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Kevin Tian, Keir Fraser, Boris Ostrovsky, Jun Nakajima
On 17/12/15 14:01, Jan Beulich wrote:
> For one, loading the MSR with a possibly non-canonical address was
> possible since the verification is conditional, while the MSR load
> wasn't. And then for PV guests we need to further limit the range of
> valid addresses to exclude the hypervisor range.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:01 [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads Jan Beulich
2015-12-17 14:05 ` Andrew Cooper
@ 2015-12-17 14:12 ` Boris Ostrovsky
2015-12-17 14:18 ` Jan Beulich
1 sibling, 1 reply; 11+ messages in thread
From: Boris Ostrovsky @ 2015-12-17 14:12 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima
On 12/17/2015 09:01 AM, Jan Beulich wrote:
> For one, loading the MSR with a possibly non-canonical address was
> possible since the verification is conditional, while the MSR load
> wasn't. And then for PV guests we need to further limit the range of
> valid addresses to exclude the hypervisor range.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -366,7 +366,8 @@ static inline void __core2_vpmu_load(str
> }
>
> wrmsrl(MSR_CORE_PERF_FIXED_CTR_CTRL, core2_vpmu_cxt->fixed_ctrl);
> - wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
> + if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) )
> + wrmsrl(MSR_IA32_DS_AREA, core2_vpmu_cxt->ds_area);
> wrmsrl(MSR_IA32_PEBS_ENABLE, core2_vpmu_cxt->pebs_enable);
>
> if ( !has_hvm_container_vcpu(v) )
> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
> enabled_cntrs |= (1ULL << i);
> }
>
> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
> + !(has_hvm_container_vcpu(v)
> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
Should we instead of (or in addition to) this also make the same change
in core2_vpmu_do_wrmsr()?
-boris
> return -EINVAL;
>
> if ( (core2_vpmu_cxt->global_ctrl & enabled_cntrs) ||
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:12 ` Boris Ostrovsky
@ 2015-12-17 14:18 ` Jan Beulich
2015-12-17 14:26 ` Boris Ostrovsky
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2015-12-17 14:18 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel
>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>> enabled_cntrs |= (1ULL << i);
>> }
>>
>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>> + !(has_hvm_container_vcpu(v)
>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>
> Should we instead of (or in addition to) this also make the same change
> in core2_vpmu_do_wrmsr()?
Currently there's no need for this since - afaict - PV guests can't
write this MSR directly (it's not among the white listed set in
traps.c).
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:18 ` Jan Beulich
@ 2015-12-17 14:26 ` Boris Ostrovsky
2015-12-17 14:29 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Boris Ostrovsky @ 2015-12-17 14:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel
On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>> enabled_cntrs |= (1ULL << i);
>>> }
>>>
>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>> + !(has_hvm_container_vcpu(v)
>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>> Should we instead of (or in addition to) this also make the same change
>> in core2_vpmu_do_wrmsr()?
> Currently there's no need for this since - afaict - PV guests can't
> write this MSR directly (it's not among the white listed set in
> traps.c).
Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
-boris
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:26 ` Boris Ostrovsky
@ 2015-12-17 14:29 ` Jan Beulich
2015-12-17 14:41 ` Boris Ostrovsky
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2015-12-17 14:29 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel
>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>>> enabled_cntrs |= (1ULL << i);
>>>> }
>>>>
>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>>> + !(has_hvm_container_vcpu(v)
>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>>> Should we instead of (or in addition to) this also make the same change
>>> in core2_vpmu_do_wrmsr()?
>> Currently there's no need for this since - afaict - PV guests can't
>> write this MSR directly (it's not among the white listed set in
>> traps.c).
>
> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
Or add the MSR to the permitted set. You know better than I
what the best route here is.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:29 ` Jan Beulich
@ 2015-12-17 14:41 ` Boris Ostrovsky
2015-12-18 6:21 ` Tian, Kevin
0 siblings, 1 reply; 11+ messages in thread
From: Boris Ostrovsky @ 2015-12-17 14:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Kevin Tian, Keir Fraser, Jun Nakajima, xen-devel
On 12/17/2015 09:29 AM, Jan Beulich wrote:
>>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
>> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>>>> enabled_cntrs |= (1ULL << i);
>>>>> }
>>>>>
>>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>>>> + !(has_hvm_container_vcpu(v)
>>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>>>> Should we instead of (or in addition to) this also make the same change
>>>> in core2_vpmu_do_wrmsr()?
>>> Currently there's no need for this since - afaict - PV guests can't
>>> write this MSR directly (it's not among the white listed set in
>>> traps.c).
>> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
> Or add the MSR to the permitted set. You know better than I
> what the best route here is.
I vaguely recall a conversation where we weren't sure whether BTS (which
needs DS area) will work for PV. Something to do with DS address being
in the right context (guest or host). I'd need to find that conversation
(or test BTS on PV).
-boris
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-17 14:41 ` Boris Ostrovsky
@ 2015-12-18 6:21 ` Tian, Kevin
2015-12-18 15:12 ` Boris Ostrovsky
0 siblings, 1 reply; 11+ messages in thread
From: Tian, Kevin @ 2015-12-18 6:21 UTC (permalink / raw)
To: Boris Ostrovsky, Jan Beulich
Cc: Andrew Cooper, Keir Fraser, Nakajima, Jun, xen-devel
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: Thursday, December 17, 2015 10:42 PM
>
> On 12/17/2015 09:29 AM, Jan Beulich wrote:
> >>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
> >> On 12/17/2015 09:18 AM, Jan Beulich wrote:
> >>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
> >>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
> >>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
> >>>>> enabled_cntrs |= (1ULL << i);
> >>>>> }
> >>>>>
> >>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
> >>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
> >>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
> >>>>> + !(has_hvm_container_vcpu(v)
> >>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
> >>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
> >>>> Should we instead of (or in addition to) this also make the same change
> >>>> in core2_vpmu_do_wrmsr()?
> >>> Currently there's no need for this since - afaict - PV guests can't
> >>> write this MSR directly (it's not among the white listed set in
> >>> traps.c).
> >> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
> > Or add the MSR to the permitted set. You know better than I
> > what the best route here is.
>
> I vaguely recall a conversation where we weren't sure whether BTS (which
> needs DS area) will work for PV. Something to do with DS address being
> in the right context (guest or host). I'd need to find that conversation
> (or test BTS on PV).
>
I guess I don't need to review current patch until you have a conclusion, right? :-)
Thanks
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-18 6:21 ` Tian, Kevin
@ 2015-12-18 15:12 ` Boris Ostrovsky
2015-12-18 15:19 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Boris Ostrovsky @ 2015-12-18 15:12 UTC (permalink / raw)
To: Tian, Kevin, Jan Beulich
Cc: Andrew Cooper, Keir Fraser, Nakajima, Jun, xen-devel
On 12/18/2015 01:21 AM, Tian, Kevin wrote:
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: Thursday, December 17, 2015 10:42 PM
>>
>> On 12/17/2015 09:29 AM, Jan Beulich wrote:
>>>>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
>>>> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>>>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>>>>>> enabled_cntrs |= (1ULL << i);
>>>>>>> }
>>>>>>>
>>>>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>>>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>>>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>>>>>> + !(has_hvm_container_vcpu(v)
>>>>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>>>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>>>>>> Should we instead of (or in addition to) this also make the same change
>>>>>> in core2_vpmu_do_wrmsr()?
>>>>> Currently there's no need for this since - afaict - PV guests can't
>>>>> write this MSR directly (it's not among the white listed set in
>>>>> traps.c).
>>>> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
>>> Or add the MSR to the permitted set. You know better than I
>>> what the best route here is.
>> I vaguely recall a conversation where we weren't sure whether BTS (which
>> needs DS area) will work for PV. Something to do with DS address being
>> in the right context (guest or host). I'd need to find that conversation
>> (or test BTS on PV).
>>
> I guess I don't need to review current patch until you have a conclusion, right? :-)
All I can say that is BTS does not work on PV (at least as far as perf
is concerned, which is the only tool I know that could use it). Which is
not surprising given that we can't access DS_AREA MSR.
-boris
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-18 15:12 ` Boris Ostrovsky
@ 2015-12-18 15:19 ` Jan Beulich
2015-12-18 15:32 ` Boris Ostrovsky
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2015-12-18 15:19 UTC (permalink / raw)
To: Kevin Tian, Boris Ostrovsky
Cc: Andrew Cooper, Keir Fraser, Jun Nakajima, xen-devel
>>> On 18.12.15 at 16:12, <boris.ostrovsky@oracle.com> wrote:
> On 12/18/2015 01:21 AM, Tian, Kevin wrote:
>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>> Sent: Thursday, December 17, 2015 10:42 PM
>>>
>>> On 12/17/2015 09:29 AM, Jan Beulich wrote:
>>>>>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>>>>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>>>>>>> enabled_cntrs |= (1ULL << i);
>>>>>>>> }
>>>>>>>>
>>>>>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>>>>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>>>>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>>>>>>> + !(has_hvm_container_vcpu(v)
>>>>>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>>>>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>>>>>>> Should we instead of (or in addition to) this also make the same change
>>>>>>> in core2_vpmu_do_wrmsr()?
>>>>>> Currently there's no need for this since - afaict - PV guests can't
>>>>>> write this MSR directly (it's not among the white listed set in
>>>>>> traps.c).
>>>>> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
>>>> Or add the MSR to the permitted set. You know better than I
>>>> what the best route here is.
>>> I vaguely recall a conversation where we weren't sure whether BTS (which
>>> needs DS area) will work for PV. Something to do with DS address being
>>> in the right context (guest or host). I'd need to find that conversation
>>> (or test BTS on PV).
>>>
>> I guess I don't need to review current patch until you have a conclusion,
> right? :-)
>
> All I can say that is BTS does not work on PV (at least as far as perf
> is concerned, which is the only tool I know that could use it). Which is
> not surprising given that we can't access DS_AREA MSR.
But in the context of this patch (and what direction a v2 might
need to go) we'd need to assume that MSR can be accessed.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads
2015-12-18 15:19 ` Jan Beulich
@ 2015-12-18 15:32 ` Boris Ostrovsky
0 siblings, 0 replies; 11+ messages in thread
From: Boris Ostrovsky @ 2015-12-18 15:32 UTC (permalink / raw)
To: Jan Beulich, Kevin Tian
Cc: Andrew Cooper, Keir Fraser, Jun Nakajima, xen-devel
On 12/18/2015 10:19 AM, Jan Beulich wrote:
>>>> On 18.12.15 at 16:12, <boris.ostrovsky@oracle.com> wrote:
>> On 12/18/2015 01:21 AM, Tian, Kevin wrote:
>>>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>>>> Sent: Thursday, December 17, 2015 10:42 PM
>>>>
>>>> On 12/17/2015 09:29 AM, Jan Beulich wrote:
>>>>>>>> On 17.12.15 at 15:26, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 12/17/2015 09:18 AM, Jan Beulich wrote:
>>>>>>>>>> On 17.12.15 at 15:12, <boris.ostrovsky@oracle.com> wrote:
>>>>>>>> On 12/17/2015 09:01 AM, Jan Beulich wrote:
>>>>>>>>> @@ -415,8 +416,10 @@ static int core2_vpmu_verify(struct vcpu
>>>>>>>>> enabled_cntrs |= (1ULL << i);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> - if ( vpmu_is_set(vcpu_vpmu(v), VPMU_CPU_HAS_DS) &&
>>>>>>>>> - !is_canonical_address(core2_vpmu_cxt->ds_area) )
>>>>>>>>> + if ( vpmu_is_set(vpmu, VPMU_CPU_HAS_DS) &&
>>>>>>>>> + !(has_hvm_container_vcpu(v)
>>>>>>>>> + ? is_canonical_address(core2_vpmu_cxt->ds_area)
>>>>>>>>> + : __addr_ok(core2_vpmu_cxt->ds_area)) )
>>>>>>>> Should we instead of (or in addition to) this also make the same change
>>>>>>>> in core2_vpmu_do_wrmsr()?
>>>>>>> Currently there's no need for this since - afaict - PV guests can't
>>>>>>> write this MSR directly (it's not among the white listed set in
>>>>>>> traps.c).
>>>>>> Then we probably shouldn't set VPMU_CPU_HAS_DS for PV guests.
>>>>> Or add the MSR to the permitted set. You know better than I
>>>>> what the best route here is.
>>>> I vaguely recall a conversation where we weren't sure whether BTS (which
>>>> needs DS area) will work for PV. Something to do with DS address being
>>>> in the right context (guest or host). I'd need to find that conversation
>>>> (or test BTS on PV).
>>>>
>>> I guess I don't need to review current patch until you have a conclusion,
>> right? :-)
>>
>> All I can say that is BTS does not work on PV (at least as far as perf
>> is concerned, which is the only tool I know that could use it). Which is
>> not surprising given that we can't access DS_AREA MSR.
> But in the context of this patch (and what direction a v2 might
> need to go) we'd need to assume that MSR can be accessed.
In which case I think we need to add the same test to
core2_vpmu_do_wrmsr() as well.
My earlier statement that we perform this test there *instead of* doing
in core2_vpmu_verify() was incorrect --- we need to do it in both places
since the value can come from the guest in the vpmu_context that we are
loading thus bypassing core2_vpmu_do_wrmsr() path.
-boris
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-12-18 15:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-17 14:01 [PATCH] x86/vPMU: constrain MSR_IA32_DS_AREA loads Jan Beulich
2015-12-17 14:05 ` Andrew Cooper
2015-12-17 14:12 ` Boris Ostrovsky
2015-12-17 14:18 ` Jan Beulich
2015-12-17 14:26 ` Boris Ostrovsky
2015-12-17 14:29 ` Jan Beulich
2015-12-17 14:41 ` Boris Ostrovsky
2015-12-18 6:21 ` Tian, Kevin
2015-12-18 15:12 ` Boris Ostrovsky
2015-12-18 15:19 ` Jan Beulich
2015-12-18 15:32 ` Boris Ostrovsky
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).