Xen devel - Dec 2007 - [PATCH] Enable Core 2 Duo Performance Counters in HVM guest

Hi, Keir,

Currently, HVM guests do not have access to performance counters. So it
is not possible to use performance analyzer software such as vtune in
HVM guest to analyze programme performance. Other usage of performance
counters , for example, the NMI watchdog, won''t function either.
This patch will enable performance counters in HVM guest. Currently,
only Core 2 Duo is implemented. Tests are carried out using Intel Vtune
Performance Analyzer in Windows XP Professional with 2 vcpus.
There are some notes at the beginning of the patch, describing
implementation choices in this patch.

Signed-off-by:    Haitao Shan <haitao.shan@intel.com>
 <<pmu.patch>> 

Best Regards
Haitao Shan




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Don¹t define new MSR macros ­ use the existing ones (or fixup the
eixsting ones to have better names if you really think that is necessary,
although I doubt it is in this case).

 * Some of the indentation is rather wacky.

 * Poking the LVTPC as a one-off event only when the VCPU writes its virtual
LVTPC is not going to fly. What if the VCPU is subsequently migrated to a
different physical CPU? In any case the LVTPC is a shared resource that
needs synchronised access via e.g., context switching or mutual exclusion
(see the next point below).

 * How does this interact with, for example, xenoprof? Ideally we should be
able to run xenoprof and this concurrently over disjoint subsets of domains.
At the very least xenoprof and this patch should avoid stomping on each
other by implementing some form of mutual exclusion. Probably some resource
sharing (e.g., the PMC interrupt vector) also is possible. I expect this
will require some design thought, but unfortunately that is the price for
being second into the tree.

 * Impact on save/restore to/from Core-2 processors: has this been tested at
all?

 -- Keir

On 11/12/07 10:06, "Shan, Haitao" <haitao.shan@intel.com> wrote:
> Hi, Keir, 
> 
> Currently, HVM guests do not have access to performance counters. So it is
not
> possible to use performance analyzer software such as vtune in HVM guest to
> analyze programme performance. Other usage of performance counters , for
> example, the NMI watchdog, won''t function either.
> 
> This patch will enable performance counters in HVM guest. Currently, only
Core
> 2 Duo is implemented. Tests are carried out using Intel Vtune Performance
> Analyzer in Windows XP Professional with 2 vcpus.
> 
> There are some notes at the beginning of the patch, describing
implementation
> choices in this patch.
> 
> Signed-off-by:    Haitao Shan <haitao.shan@intel.com>
>  <<pmu.patch>> 
> 
> Best Regards 
> Haitao Shan 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Keir

 

Please see my comments embedded.

 

________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
Sent: 2007年12月11日 18:38
To: Shan, Haitao
Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: Re: [PATCH] Enable Core 2 Duo Performance Counters in HVM guest

 

* Don’t define new MSR macros - use the existing ones (or fixup the eixsting
ones to have better names if you really think that is necessary, although I
doubt it is in this case).

I will update.

 * Some of the indentation is rather wacky.

I will update.

 * Poking the LVTPC as a one-off event only when the VCPU writes its virtual
LVTPC is not going to fly. What if the VCPU is subsequently migrated to a
different physical CPU? In any case the LVTPC is a shared resource that needs
synchronised access via e.g., context switching or mutual exclusion (see the
next point below).

I think in the patch I will save/load LVTPC during context switch. 

 * How does this interact with, for example, xenoprof? Ideally we should be able
to run xenoprof and this concurrently over disjoint subsets of domains. At the
very least xenoprof and this patch should avoid stomping on each other by
implementing some form of mutual exclusion. Probably some resource sharing
(e.g., the PMC interrupt vector) also is possible. I expect this will require
some design thought, but unfortunately that is the price for being second into
the tree.

Yes. How to share PMU resources will need careful design. But I really don’t
think it possible to run both concurrently unless we do some sort of PMC’s
partition. Of course, it is silly of me not to implement a mechanism for mutual
exclusion. I will implement one.

 * Impact on save/restore to/from Core-2 processors: has this been tested at
all?

I will try doing the test. I think whether it should support save/restore can be
argued. Does anyone want to run VTune/Oprofile during save/restore? This can be
hardly a good usage model. But at least, I update it to ensure it does not break
current save/restore.

 -- Keir

On 11/12/07 10:06, "Shan, Haitao" <haitao.shan@intel.com> wrote:

Hi, Keir, 

Currently, HVM guests do not have access to performance counters. So it is not
possible to use performance analyzer software such as vtune in HVM guest to
analyze programme performance. Other usage of performance counters , for
example, the NMI watchdog, won''t function either.

This patch will enable performance counters in HVM guest. Currently, only Core 2
Duo is implemented. Tests are carried out using Intel Vtune Performance Analyzer
in Windows XP Professional with 2 vcpus.

There are some notes at the beginning of the patch, describing implementation
choices in this patch.

Signed-off-by:    Haitao Shan <haitao.shan@intel.com> 
 <<pmu.patch>> 

Best Regards 
Haitao Shan 

 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Oh yes, I see you save/restore MSR state and LVTPC across context switch.
That¹s fine then. But I don¹t think you should pass through any of the
virtual LVTPC register fields at all, except to toggle the mask field of the
real LVTPC depending on whether or not performance counter interrupt
delivery is currently enabled for the VCPU (I¹d implement this just to avoid
unnecessary real interrupts which are certainly not going to turn into
virtual interrupts). There are no real LVTPC fields which should really be
under guest control, and so cooking the guest LVTPC value and poking it into
the real LVTPC in the vlapic code looks odd. As does save/restore of the
whole LVTPC on context switch -- at most you should need to track
masked/not-masked. And you¹re letting the guest have access to reserved bits
of the real LVTPC, which is not good...

Ignore my comment about save/restore ‹ I misread your context-switching code
as HVM save/restore code!

 -- Keir

On 11/12/07 13:32, "Shan, Haitao" <haitao.shan@intel.com> wrote:
>  * Poking the LVTPC as a one-off event only when the VCPU writes its
virtual
> LVTPC is not going to fly. What if the VCPU is subsequently migrated to a
> different physical CPU? In any case the LVTPC is a shared resource that
needs
> synchronised access via e.g., context switching or mutual exclusion (see
the
> next point below).
> I think in the patch I will save/load LVTPC during context switch.
> 
>  * How does this interact with, for example, xenoprof? Ideally we should be
> able to run xenoprof and this concurrently over disjoint subsets of
domains.
> At the very least xenoprof and this patch should avoid stomping on each
other
> by implementing some form of mutual exclusion. Probably some resource
sharing
> (e.g., the PMC interrupt vector) also is possible. I expect this will
require
> some design thought, but unfortunately that is the price for being second
into
> the tree.
> Yes. How to share PMU resources will need careful design. But I really
don¹t
> think it possible to run both concurrently unless we do some sort of PMC¹s
> partition. Of course, it is silly of me not to implement a mechanism for
> mutual exclusion. I will implement one.
> 
>  * Impact on save/restore to/from Core-2 processors: has this been tested
at
> all?
> I will try doing the test. I think whether it should support save/restore
can
> be argued. Does anyone want to run VTune/Oprofile during save/restore? This
> can be hardly a good usage model. But at least, I update it to ensure it
does
> not break current save/restore.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Keir,
 
Thanks for your detailed comments. I have worked out an updated patch.
 
I removed my own MSR macros definitions and hard TAB indentation. Also, I
removed LVTPC write-through in vlapic_write. Now, only when guest both enables
counting and interrupt, physical LVTPC is written. And when vcpu is scheduled
out, LVTPC is masked.
In addition, I employed a "first come, first service" policy to grant
PMU access to xenoprof/hvm_guest. When access is granted to hvm guest, oprofile
will get "Device Busy" . On the contrary, guest can not use PMU like
before.
HVM save/restore is tested, the patch will not break current code.
 
Can you have a look and give me your comments? Thanks in advance!

Best Regards 
Haitao Shan 

 

________________________________

From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2007年12月11日 23:02
To: Shan, Haitao
Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: [Xen-devel] Re: [PATCH] Enable Core 2 Duo Performance Counters inHVM
guest


Oh yes, I see you save/restore MSR state and LVTPC across context switch. That’s
fine then. But I don’t think you should pass through any of the virtual LVTPC
register fields at all, except to toggle the mask field of the real LVTPC
depending on whether or not performance counter interrupt delivery is currently
enabled for the VCPU (I’d implement this just to avoid unnecessary real
interrupts which are certainly not going to turn into virtual interrupts). There
are no real LVTPC fields which should really be under guest control, and so
cooking the guest LVTPC value and poking it into the real LVTPC in the vlapic
code looks odd. As does save/restore of the whole LVTPC on context switch -- at
most you should need to track masked/not-masked. And you’re letting the guest
have access to reserved bits of the real LVTPC, which is not good...

Ignore my comment about save/restore — I misread your context-switching code as
HVM save/restore code!

 -- Keir

On 11/12/07 13:32, "Shan, Haitao" <haitao.shan@intel.com> wrote:



	* Poking the LVTPC as a one-off event only when the VCPU writes its virtual
LVTPC is not going to fly. What if the VCPU is subsequently migrated to a
different physical CPU? In any case the LVTPC is a shared resource that needs
synchronised access via e.g., context switching or mutual exclusion (see the
next point below).
	I think in the patch I will save/load LVTPC during context switch. 
	
	 * How does this interact with, for example, xenoprof? Ideally we should be
able to run xenoprof and this concurrently over disjoint subsets of domains. At
the very least xenoprof and this patch should avoid stomping on each other by
implementing some form of mutual exclusion. Probably some resource sharing
(e.g., the PMC interrupt vector) also is possible. I expect this will require
some design thought, but unfortunately that is the price for being second into
the tree.
	Yes. How to share PMU resources will need careful design. But I really don’t
think it possible to run both concurrently unless we do some sort of PMC’s
partition. Of course, it is silly of me not to implement a mechanism for mutual
exclusion. I will implement one.
	
	 * Impact on save/restore to/from Core-2 processors: has this been tested at
all?
	I will try doing the test. I think whether it should support save/restore can
be argued. Does anyone want to run VTune/Oprofile during save/restore? This can
be hardly a good usage model. But at least, I update it to ensure it does not
break current save/restore.
	






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The code style is still odd, but that’s easily fixed. Coding style in files
derived from Linux is often Linux style (including hard tabs). Otherwise
look in e.g., common/domain.c for Xen coding style.

I don’t understand why the MSR-write handler is so complicated, and tracks a
bunch of stuff, yet you appear to give direct access to all the MSRs by
clearing bits in the MSR bitmap. Is the MSR-write function more complicated
than it needs to be? Or is direct MSR access by the guest unsafe?

Is the virtualisation for Core, or Core 2, or both?

I don’t think you need to statically allocate a PMU_VECTOR. request_irq()
yourself a vector when VMX is initialised at boot time. This will avoid
touching a bunch of generic files.

 -- Keir

On 14/12/07 07:49, "Shan, Haitao" <haitao.shan@intel.com> wrote:
> Hi, Keir,
>  
> Thanks for your detailed comments. I have worked out an updated patch.
>  
> I removed my own MSR macros definitions and hard TAB indentation. Also, I
> removed LVTPC write-through in vlapic_write. Now, only when guest both
enables
> counting and interrupt, physical LVTPC is written. And when vcpu is
scheduled
> out, LVTPC is masked.
> In addition, I employed a "first come, first service" policy to
grant PMU
> access to xenoprof/hvm_guest. When access is granted to hvm guest, oprofile
> will get "Device Busy" . On the contrary, guest can not use PMU
like before.
> HVM save/restore is tested, the patch will not break current code.
>  
> Can you have a look and give me your comments? Thanks in advance!
> Best Regards 
> Haitao Shan 
> 
>  
> 
> 
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
> Sent: 2007年12月11日 23:02
> To: Shan, Haitao
> Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
> Subject: [Xen-devel] Re: [PATCH] Enable Core 2 Duo Performance Counters
inHVM
> guest
> 
> Oh yes, I see you save/restore MSR state and LVTPC across context switch.
> That’s fine then. But I don’t think you should pass through any of the
virtual
> LVTPC register fields at all, except to toggle the mask field of the real
> LVTPC depending on whether or not performance counter interrupt delivery is
> currently enabled for the VCPU (I’d implement this just to avoid
unnecessary
> real interrupts which are certainly not going to turn into virtual
> interrupts). There are no real LVTPC fields which should really be under
guest
> control, and so cooking the guest LVTPC value and poking it into the real
> LVTPC in the vlapic code looks odd. As does save/restore of the whole LVTPC
on
> context switch -- at most you should need to track masked/not-masked. And
> you’re letting the guest have access to reserved bits of the real LVTPC,
which
> is not good...
> 
> Ignore my comment about save/restore — I misread your context-switching
code
> as HVM save/restore code!
> 
>  -- Keir
> 
> On 11/12/07 13:32, "Shan, Haitao" <haitao.shan@intel.com>
wrote:
> 
>> * Poking the LVTPC as a one-off event only when the  VCPU writes its
virtual
>> LVTPC is not going to fly. What if the VCPU is  subsequently migrated
to a
>> different physical CPU? In any case the LVTPC is a  shared resource
that
>> needs synchronised access via e.g., context switching or  mutual
exclusion
>> (see the next point below).
>> I think in  the patch I will save/load LVTPC during context switch.
>> 
>>  *  How does this interact with, for example, xenoprof? Ideally we
should be
>> able  to run xenoprof and this concurrently over disjoint subsets of
domains.
>> At the  very least xenoprof and this patch should avoid stomping on
each
>> other by  implementing some form of mutual exclusion. Probably some
resource
>> sharing  (e.g., the PMC interrupt vector) also is possible. I expect
this
>> will require  some design thought, but unfortunately that is the price
for
>> being second into  the tree.
>> Yes. How to share PMU resources will need  careful design. But I really
don’t
>> think it possible to run both concurrently  unless we do some sort of
PMC’s
>> partition. Of course, it is silly of me not to  implement a mechanism
for
>> mutual exclusion. I will implement one.
>> 
>>  * Impact on save/restore to/from Core-2 processors: has  this been
tested at
>> all?
>> I will try doing the test. I  think whether it should support
save/restore
>> can be argued. Does anyone want  to run VTune/Oprofile during
save/restore?
>> This can be hardly a good usage  model. But at least, I update it to
ensure
>> it does not break current  save/restore.
> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Thanks for your quick reply. I will fully check my coding style, sorry for that.
Please see my comments embedded, thanks.

Best Regards 
Haitao Shan 

 

________________________________

From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
Sent: 2007年12月14日 16:31
To: Shan, Haitao
Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo Performance Counters
inHVM guest


The code style is still odd, but that’s easily fixed. Coding style in files
derived from Linux is often Linux style (including hard tabs). Otherwise look in
e.g., common/domain.c for Xen coding style.
 

I don’t understand why the MSR-write handler is so complicated, and tracks a
bunch of stuff, yet you appear to give direct access to all the MSRs by clearing
bits in the MSR bitmap. Is the MSR-write function more complicated than it needs
to be? Or is direct MSR access by the guest unsafe?
Guest can directly access counter MSRs and read-access non-global-control MSRs.
So guest write to control MSRs are always trapped. The reason is:
We want to implement a "lazy load" mechanism. But only when none of
the counters are running, should "lazy load" be employed. If counters
are running, we must load its contents every time the vcpu is scheduled back to
run. Currently each counter has two control MSRs: a single bit in global control
and a dedicate MSR. Those code will calculate which one is enabled. The result
can be used to do the "lazy load" staff.
 Another approach is calculate the running status of all the counters at context
switching time. But from my observation, vcpu migration tends to be quite
frequent. However, the times that guest writes to control MSRs are much less. So
I decide it is better to do it in msr write handler.

Is the virtualisation for Core, or Core 2, or both? 
Only Core 2. The reason is that Core do not have global control MSR. This MSR is
the only one which will use VMX''s HW capability to save and load on
vmentry/vmexit. The benefit is the all the other MSRs can be handled with
software flexibility, like the "lazy load" mechanism.

I don’t think you need to statically allocate a PMU_VECTOR. request_irq()
yourself a vector when VMX is initialised at boot time. This will avoid touching
a bunch of generic files.
But request_irq can not ensure PMU can be assigned with a high vector. High
vector will help to handle PMIs in time so that gain accurate performance data.

 -- Keir

On 14/12/07 07:49, "Shan, Haitao" <haitao.shan@intel.com> wrote:



	Hi, Keir,
	
	Thanks for your detailed comments. I have worked out an updated patch.
	
	I removed my own MSR macros definitions and hard TAB indentation. Also, I
removed LVTPC write-through in vlapic_write. Now, only when guest both enables
counting and interrupt, physical LVTPC is written. And when vcpu is scheduled
out, LVTPC is masked.
	In addition, I employed a "first come, first service" policy to grant
PMU access to xenoprof/hvm_guest. When access is granted to hvm guest, oprofile
will get "Device Busy" . On the contrary, guest can not use PMU like
before.
	HVM save/restore is tested, the patch will not break current code.
	
	Can you have a look and give me your comments? Thanks in advance!
	Best Regards 
	Haitao Shan 
	
	 
	
	
________________________________

	From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
	Sent: 2007年12月11日 23:02
	To: Shan, Haitao
	Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
	Subject: [Xen-devel] Re: [PATCH] Enable Core 2 Duo Performance Counters inHVM
guest
	
	Oh yes, I see you save/restore MSR state and LVTPC across context switch.
That’s fine then. But I don’t think you should pass through any of the virtual
LVTPC register fields at all, except to toggle the mask field of the real LVTPC
depending on whether or not performance counter interrupt delivery is currently
enabled for the VCPU (I’d implement this just to avoid unnecessary real
interrupts which are certainly not going to turn into virtual interrupts). There
are no real LVTPC fields which should really be under guest control, and so
cooking the guest LVTPC value and poking it into the real LVTPC in the vlapic
code looks odd. As does save/restore of the whole LVTPC on context switch -- at
most you should need to track masked/not-masked. And you’re letting the guest
have access to reserved bits of the real LVTPC, which is not good...
	
	Ignore my comment about save/restore — I misread your context-switching code as
HVM save/restore code!
	
	 -- Keir
	
	On 11/12/07 13:32, "Shan, Haitao" <haitao.shan@intel.com>
wrote:
	
	

		* Poking the LVTPC as a one-off event only when the  VCPU writes its virtual
LVTPC is not going to fly. What if the VCPU is  subsequently migrated to a
different physical CPU? In any case the LVTPC is a  shared resource that needs
synchronised access via e.g., context switching or  mutual exclusion (see the
next point below).
		I think in  the patch I will save/load LVTPC during context switch. 
		
		 *  How does this interact with, for example, xenoprof? Ideally we should be
able  to run xenoprof and this concurrently over disjoint subsets of domains. At
the  very least xenoprof and this patch should avoid stomping on each other by 
implementing some form of mutual exclusion. Probably some resource sharing 
(e.g., the PMC interrupt vector) also is possible. I expect this will require 
some design thought, but unfortunately that is the price for being second into 
the tree.
		Yes. How to share PMU resources will need  careful design. But I really don’t
think it possible to run both concurrently  unless we do some sort of PMC’s
partition. Of course, it is silly of me not to  implement a mechanism for mutual
exclusion. I will implement one.
		
		 * Impact on save/restore to/from Core-2 processors: has  this been tested at
all?
		I will try doing the test. I  think whether it should support save/restore can
be argued. Does anyone want  to run VTune/Oprofile during save/restore? This can
be hardly a good usage  model. But at least, I update it to ensure it does not
break current  save/restore.
		

	
	
	





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

1. Shouldn¹t the family-specific file be called vpmu_core2.c then?

2. Ah yes, actually setup_irq()/request_irq() is not a suitable interface.
The current code is fine.

 -- Keir

On 14/12/07 09:32, "Shan, Haitao" <haitao.shan@intel.com> wrote:
> Is the virtualisation for Core, or Core 2, or both?
> Only Core 2. The reason is that Core do not have global control MSR. This
MSR
> is the only one which will use VMX''s HW capability to save and
load on
> vmentry/vmexit. The benefit is the all the other MSRs can be handled with
> software flexibility, like the "lazy load" mechanism.
> 
> I don¹t think you need to statically allocate a PMU_VECTOR. request_irq()
> yourself a vector when VMX is initialised at boot time. This will avoid
> touching a bunch of generic files.
> But request_irq can not ensure PMU can be assigned with a high vector. High
> vector will help to handle PMIs in time so that gain accurate performance
> data. 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Thanks, Keir!
I will rename the file. Actually I mean the Core Architecture here. But it will
definitly cause confusions.

Best Regards 
Haitao Shan 

 

________________________________

From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2007年12月14日 17:55
To: Shan, Haitao
Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo PerformanceCounters inHVM
guest


1. Shouldn’t the family-specific file be called vpmu_core2.c then?

2. Ah yes, actually setup_irq()/request_irq() is not a suitable interface. The
current code is fine.

 -- Keir

On 14/12/07 09:32, "Shan, Haitao" <haitao.shan@intel.com> wrote:



	Is the virtualisation for Core, or Core 2, or both? 
	Only Core 2. The reason is that Core do not have global control MSR. This MSR
is the only one which will use VMX''s HW capability to save and load on
vmentry/vmexit. The benefit is the all the other MSRs can be handled with
software flexibility, like the "lazy load" mechanism.
	
	I don’t think you need to statically allocate a PMU_VECTOR. request_irq()
yourself a vector when VMX is initialised at boot time. This will avoid touching
a bunch of generic files.
	But request_irq can not ensure PMU can be assigned with a high vector. High
vector will help to handle PMIs in time so that gain accurate performance data.
	





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Keir,

 

I have carefully checked my coding style according to your comments. 

For those which contained hard TABs originally, I only updated what was touched
by my patch.

Do you have any comments?

 

Thanks!

 

Haitao Shan

 

________________________________

From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2007年12月14日 17:55
To: Shan, Haitao
Cc: xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo PerformanceCounters inHVM
guest

 

1. Shouldn’t the family-specific file be called vpmu_core2.c then?

2. Ah yes, actually setup_irq()/request_irq() is not a suitable interface. The
current code is fine.

 -- Keir

On 14/12/07 09:32, "Shan, Haitao" <haitao.shan@intel.com> wrote:

Is the virtualisation for Core, or Core 2, or both? 
Only Core 2. The reason is that Core do not have global control MSR. This MSR is
the only one which will use VMX''s HW capability to save and load on
vmentry/vmexit. The benefit is the all the other MSRs can be handled with
software flexibility, like the "lazy load" mechanism.

I don’t think you need to statically allocate a PMU_VECTOR. request_irq()
yourself a vector when VMX is initialised at boot time. This will avoid touching
a bunch of generic files.
But request_irq can not ensure PMU can be assigned with a high vector. High
vector will help to handle PMIs in time so that gain accurate performance data.

 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

"Shan, Haitao" <haitao.shan@intel.com> writes:

Hello Haitao,

I''ve done a look on the patch and found two places with wrong coding
style. Take a look bellow:
> diff -r 8f0cbfc478d6 xen/arch/x86/oprofile/op_model_ppro.c
> --- a/xen/arch/x86/oprofile/op_model_ppro.c	Thu Dec 13 09:31:03 2007 +0000
> +++ b/xen/arch/x86/oprofile/op_model_ppro.c	Fri Dec 14 07:21:43 2007 +0800
> @@ -126,6 +127,11 @@ static void ppro_start(struct op_msrs co
>  	CTRL_READ(low, high, msrs, 0);
>  	CTRL_SET_ACTIVE(low);
>  	CTRL_WRITE(low, high, msrs, 0);
> +    /* Global Control MSR is enabled by default when system power on.
> +     * However, this may not hold true when xenoprof starts to run.
> +     */
> +    if ( ppro_has_global_ctrl )
> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 1);
>  }
>  
>  
> @@ -135,6 +141,8 @@ static void ppro_stop(struct op_msrs con
>  	CTRL_READ(low, high, msrs, 0);
>  	CTRL_SET_INACTIVE(low);
>  	CTRL_WRITE(low, high, msrs, 0);
> +    if ( ppro_has_global_ctrl )
> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>  }
Cheers,

-- 
Otavio Salvador                  O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854         http://projetos.ossystems.com.br

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Salvador,

Do you mean the strange indentation around CTRL_WRITE? That is because there are
three hard TABs before CTRL_WRITE. I am not sure whether I should fix the
original code, so I just leave them there untouched.

Haitao Shan

-----Original Message-----
From: Otavio Salvador [mailto:otavio@ossystems.com.br] 
Sent: 2007年12月15日 3:05
To: Shan, Haitao
Cc: Keir Fraser; xen-devel@lists.xensource.com; Jiang, Yunhong
Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo PerformanceCounters inHVM
guest

"Shan, Haitao" <haitao.shan@intel.com> writes:

Hello Haitao,

I''ve done a look on the patch and found two places with wrong coding
style. Take a look bellow:
> diff -r 8f0cbfc478d6 xen/arch/x86/oprofile/op_model_ppro.c
> --- a/xen/arch/x86/oprofile/op_model_ppro.c	Thu Dec 13 09:31:03 2007 +0000
> +++ b/xen/arch/x86/oprofile/op_model_ppro.c	Fri Dec 14 07:21:43 2007 +0800
> @@ -126,6 +127,11 @@ static void ppro_start(struct op_msrs co
>  	CTRL_READ(low, high, msrs, 0);
>  	CTRL_SET_ACTIVE(low);
>  	CTRL_WRITE(low, high, msrs, 0);
> +    /* Global Control MSR is enabled by default when system power on.
> +     * However, this may not hold true when xenoprof starts to run.
> +     */
> +    if ( ppro_has_global_ctrl )
> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 1);
>  }
>  
>  
> @@ -135,6 +141,8 @@ static void ppro_stop(struct op_msrs con
>  	CTRL_READ(low, high, msrs, 0);
>  	CTRL_SET_INACTIVE(low);
>  	CTRL_WRITE(low, high, msrs, 0);
> +    if ( ppro_has_global_ctrl )
> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>  }
Cheers,

-- 
Otavio Salvador                  O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854         http://projetos.ossystems.com.br

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, Keir,
 
I have baked an updated patch. In this patch, I changed some logic to shorten
the lengthy part of core2_vpmu_do_wrmsr. And also, I did a rebase, since there
is one new changeset recently which made changes to xenoprofile.
Can you have a look?
 <<pmu.patch>> 
Best Regards
Haitao Shan

Shan, Haitao wrote:
> Hi, Salvador,
> 
> Do you mean the strange indentation around CTRL_WRITE? That is
> because there are three hard TABs before CTRL_WRITE. I am not sure
> whether I should fix the original code, so I just leave them there
> untouched.   
> 
> Haitao Shan
> 
> -----Original Message-----
> From: Otavio Salvador [mailto:otavio@ossystems.com.br]
> Sent: 2007年12月15日 3:05
> To: Shan, Haitao
> Cc: Keir Fraser; xen-devel@lists.xensource.com; Jiang, Yunhong
> Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo
> PerformanceCounters inHVM guest 
> 
> "Shan, Haitao" <haitao.shan@intel.com> writes:
> 
> Hello Haitao,
> 
> I've done a look on the patch and found two places with wrong coding
> style. Take a look bellow:
> 
>> diff -r 8f0cbfc478d6 xen/arch/x86/oprofile/op_model_ppro.c
>> --- a/xen/arch/x86/oprofile/op_model_ppro.c	Thu Dec 13 09:31:03 2007
>> +0000 +++ b/xen/arch/x86/oprofile/op_model_ppro.c	Fri Dec 14
>> 07:21:43 2007 +0800 @@ -126,6 +127,11 @@ static void
>>  	ppro_start(struct op_msrs co CTRL_READ(low, high, msrs, 0);
>>  	CTRL_SET_ACTIVE(low);
>>  	CTRL_WRITE(low, high, msrs, 0);
>> +    /* Global Control MSR is enabled by default when system power
>> on. +     * However, this may not hold true when xenoprof starts to
>> run. +     */ +    if ( ppro_has_global_ctrl )
>> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 1);
>>  }
>> 
>> 
>> @@ -135,6 +141,8 @@ static void ppro_stop(struct op_msrs con
>>  	CTRL_READ(low, high, msrs, 0);
>>  	CTRL_SET_INACTIVE(low);
>>  	CTRL_WRITE(low, high, msrs, 0);
>> +    if ( ppro_has_global_ctrl )
>> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>>  }
> 
> Cheers,


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I will. This isn’t for 3.2.0 though, so I may not look until after the 3.2.0
branch.

 -- Keir

On 17/12/07 07:22, "Shan, Haitao" <haitao.shan@intel.com> wrote:
> Hi, Keir, 
> ? 
> I have baked an updated patch. In this patch, I changed some logic?to
shorten
> the lengthy part of core2_vpmu_do_wrmsr. And also, I did a rebase, since
> there?is?one?new changeset recently which made changes to xenoprofile.
> 
> Can you have a look?
>  <<pmu.patch>> 
> Best Regards 
> Haitao Shan 
> 
> Shan, Haitao wrote:
>> > Hi, Salvador, 
>> > 
>> > Do you mean the strange indentation around CTRL_WRITE? That is
>> > because there are three hard TABs before CTRL_WRITE. I am not sure
>> > whether I should fix the original code, so I just leave them there
>> > untouched.   
>> > 
>> > Haitao Shan 
>> > 
>> > -----Original Message-----
>> > From: Otavio Salvador [mailto:otavio@ossystems.com.br
>> <mailto:otavio@ossystems.com.br> ]
>> > Sent: 2007年12月15日 3:05
>> > To: Shan, Haitao
>> > Cc: Keir Fraser; xen-devel@lists.xensource.com; Jiang, Yunhong
>> > Subject: Re: [Xen-devel] Re: [PATCH] Enable Core 2 Duo
>> > PerformanceCounters inHVM guest
>> > 
>> > "Shan, Haitao" <haitao.shan@intel.com> writes:
>> > 
>> > Hello Haitao, 
>> > 
>> > I''ve done a look on the patch and found two places with
wrong coding
>> > style. Take a look bellow:
>> > 
>>> >> diff -r 8f0cbfc478d6 xen/arch/x86/oprofile/op_model_ppro.c
>>> >> --- a/xen/arch/x86/oprofile/op_model_ppro.c  Thu Dec 13
09:31:03 2007
>>> >> +0000 +++ b/xen/arch/x86/oprofile/op_model_ppro.c    Fri
Dec 14
>>> >> 07:21:43 2007 +0800 @@ -126,6 +127,11 @@ static void
>>> >>      ppro_start(struct op_msrs co CTRL_READ(low, high,
msrs, 0);
>>> >>      CTRL_SET_ACTIVE(low);
>>> >>      CTRL_WRITE(low, high, msrs, 0);
>>> >> +    /* Global Control MSR is enabled by default when
system power
>>> >> on. +     * However, this may not hold true when xenoprof
starts to
>>> >> run. +     */ +    if ( ppro_has_global_ctrl )
>>> >> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 1);
>>> >>  } 
>>> >> 
>>> >> 
>>> >> @@ -135,6 +141,8 @@ static void ppro_stop(struct op_msrs
con
>>> >>      CTRL_READ(low, high, msrs, 0);
>>> >>      CTRL_SET_INACTIVE(low);
>>> >>      CTRL_WRITE(low, high, msrs, 0);
>>> >> +    if ( ppro_has_global_ctrl )
>>> >> +        wrmsrl(MSR_CORE_PERF_GLOBAL_CTRL, 0);
>>> >>  } 
>> > 
>> > Cheers, 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel