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 - 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); > 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
