On Mon, Apr 27, 2020 at 11:16:40AM +0800, Like Xu wrote:
> On 2020/4/24 20:16, Peter Zijlstra wrote:
> > And I suppose that is why you need that horrible:
> > needs_guest_lbr_without_counter() thing to begin with.
>
> Do you suggest to use event->attr.config check to replace
> "needs_branch_stack(event) && is_kernel_event(event) &&
> event->attr.exclude_host" check for guest LBR event ?
That's what the BTS thing does.
> > Please allocate yourself an event from the pseudo event range:
> > event==0x00. Currently we only have umask==3 for Fixed2 and umask==4
> > for Fixed3, given you claim 58, which is effectively Fixed25,
> > umask==0x1a might be appropriate.
>
> OK, I assume that adding one more field ".config = 0x1a00" is
> efficient enough for perf_event_attr to allocate guest LBR events.
Uh what? The code is already setting .config. You just have to change it
do another value.
> > Also, I suppose we need to claim 0x0000 as an error, so that other
> > people won't try this again.
>
> Does the following fix address your concern on this ?
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 2405926e2dba..32d2a3f8c51f 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -498,6 +498,9 @@ int x86_pmu_max_precise(void)
>
> int x86_pmu_hw_config(struct perf_event *event)
> {
> + if (!unlikely(event->attr.config & X86_ARCH_EVENT_MASK))
> + return -EINVAL;
> +
> if (event->attr.precise_ip) {
> int precise = x86_pmu_max_precise();
That wouldn't work right for AMD. But yes, something like that.
> > Also, what happens if you fail programming due to a conflicting cpu
> > event? That pinned doesn't guarantee you'll get the event, it just means
> > you'll error instead of getting RR.
> >
> > I didn't find any code checking the event state.
> >
>
> Error instead of RR is expected.
>
> If the KVM fails programming due to a conflicting cpu event
> the LBR registers will not be passthrough to the guest,
> and KVM would return zero for any guest LBR records accesses
> until the next attempt to program the guest LBR event.
>
> Every time before cpu enters the non-root mode where irq is
> disabled, the "event-> oncpu! =-1" check will be applied.
> (more details in the comment around intel_pmu_availability_check())
>
> The guests administer is supposed to know the result of guest
> LBR records is inaccurate if someone is using LBR to record
> guest or hypervisor on the host side.
>
> Is this acceptable to you？
>
> If there is anything needs to be improved, please let me know.
It might be nice to emit a pr_warn() or something on the host when this
happens. Then at least the host admin can know he wrecked things for
which guest.
