RE: [Xen-devel] Re: [PATCH] Enable Core 2 Duo Performance Counters inHVM guest

["Shan, Haitao" <haitao.shan@xxxxxxxxx>]

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@xxxxxxxxxxxx]
Sent: 2007年12月14日 16:31
To: Shan, Haitao
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Keir Fraser
Sent: 2007年12月11日 23:02
To: Shan, Haitao
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel