On 2022-09-19 9:46 a.m., Wang, Wei W wrote: > On Friday, September 16, 2022 9:27 PM, Liang, Kan wrote: >>> Did you mean to handle the PT event in the proposed driver API? Event >>> status is just one of the things. There are other things if we want to >>> make it complete for this, e.g. event->oncpu = -1, and eventually seems we will >> re-implement perf_event_disable_*. >>> >> >> As my understand, perf always check the status first. If it's a stopped or >> inactivated event, I don't think event->oncpu will be touched. That's why I think >> the proposed driver API should be acceptable. > > That's the implementation thing. We need to make it architecturally clean though. > >> >>> Btw, Xiaoyao has made it work with perf_event_disable_local, and don’t have >> that many changes. >>> If necessary, we can post the 2nd version out to double check. >>> >> >> I'm not worry about which ways (either perf_event_disable_local() or the >> proposed PT driver API) are chosen to stop the PT. If the existing perf_event >> interfaces can meet your requirement, that's perfect. >> >> My real concern is the pt_save_msr()/pt_load_msr(). I don't think it's a job for >> KVM. See atomic_switch_perf_msrs(). It is the perf core driver rather than KVM >> that tells which MSRs should be saved/restored in VMCS. >> We should do the same thing for PT. (Actually, I think we already encounter >> issues with the current KVM-dominated method. KVM saves/restores >> unnecessary MSRs. Right?) >> > > Right. It's on my plan to improve the current PT virtualization, and > planed to be the next step after this fix. The general rule is the same: make KVM a user > of perf, that is, we leave those save/restore work to be completely done by the > perf (driver) side, so we will eventually remove the KVM side pt_save/load_msr. > To be more precise, it will work as below: > - we will create a guest event, like what we did for lbr virtualization Another fake event? We have to specially handle it in the perf code. I don't think it's a clean way for perf. > - on VMEnter: > -- perf_disable_event_local(host_event); > -- perf_enable_event_local(guest_event); > - on VMExit: > -- perf_disable_event_local(guest_event); > -- perf_enable_event_local(host_event); Why we cannot use the same way as the perf core driver to switch the MSRs in the VMCS? You just need one generic function, perf_guest_get_msrs(), for both PT and core driver. If you have to disable PT explicitly before VMCS, I think you can do it in the PT specific perf_guest_get_msrs(). Anyway, that's an improvement for the current code. I don't have a problem, if you prefer to separate the fix patch and improvement patch. Thanks, Kan > >> To do so, I think there may be two ways. >> - Since MSRs have to be switched for both PT and core drivers, it sounds >> reasonable to provide a new generic interface in the perf_event. The new >> interface is to tell KVM which MSRs should be saved/restored. Then KVM can >> decide to save/restore via VMCS or direct MSR access. I suspect this way >> requires big change, but it will benefit all the drivers which have similar >> requirements. >> - The proposed driver API. The MSRs are saved/restored in the PT driver. > > As shown above, no need for those. We can completely reuse the > perf side save/restore. > > Thanks, > Wei
