From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 24B83C7618F for ; Tue, 16 Jul 2019 00:10:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E1B292080A for ; Tue, 16 Jul 2019 00:10:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="X/lLfEmt" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732839AbfGPAKs (ORCPT ); Mon, 15 Jul 2019 20:10:48 -0400 Received: from mail-ed1-f65.google.com ([209.85.208.65]:37780 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731522AbfGPAKr (ORCPT ); Mon, 15 Jul 2019 20:10:47 -0400 Received: by mail-ed1-f65.google.com with SMTP id w13so17140759eds.4 for ; Mon, 15 Jul 2019 17:10:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4TuRNoOkVfCigw2Wuxe3UI/VXS3zxgaiTK+rQEL9mkw=; b=X/lLfEmtOPhgzR9DUEsjJjuG5SasTk1hdZSopvddMrr6Vcm9O8K8p7MrHX42jSHP8I Rx/Dz3masCIvmwkN0lqgILNwGErRuEgGdySqyRkXWvOptFRMuQlYczwmrv6x8l9FsLrn jeVytrwjfkWoYAhOjvEQq1Cghv3JWsinNOb4fHC/YVMwiLkU0+tRq+zSnSLanJsRM/Iu vzhL25p9JSKEeFDa9fWyu1Zc0IdiRFh8TBDQT+MQwu+AZ+smN8mpE2Ewqa5Y3t8VZoJ1 ZSTTE4XITYetKccfa/zD40BA4iEyKgTzpaHb4c1Jx/ojjt7dWSqTLWN4gU6CBGwkm5kk sj5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4TuRNoOkVfCigw2Wuxe3UI/VXS3zxgaiTK+rQEL9mkw=; b=F5PrUzUHwhSuXznBe7IP2/QBWGLwyjkA3a9XGtlq80lZupS9Kg9WZ1pXJqwFd74RUC AbO6bIQQ1xbEYa/SWtrBu8THy//MoYBy3q+z7za5gGkI7jr2rNww/Hm4ZqNnafqiLcgU U/T5dahn8eh2KcjBOcaYAZEMQrhmLG7fjaUNwy8FTrzXYvFJsZVsZ8Gj8zvI2PI0kdB7 bwKZz5VTqpE8hUEPB1jBPBLVi+7gcNk7afIqQfAQVP0ouKdrEUrd5JJBAb4E6OvjcMIc 6yckukDNL8Xu/J7A78wcNynDiYReXQ9vHQfIxZhX4ghmGx4eNaDhtPFWGUfirh5hDIgI Z0CA== X-Gm-Message-State: APjAAAVf5CKOK88KWzJkrwiBFkjilItv4xY/WBlqxZpQL5DJIZhUdcBT kxTPVYAQr3oARJ6gA21X4qsjKMkiE/bC7W6DYskqKQ== X-Google-Smtp-Source: APXvYqx/gW5mUFTFQ2dxFq7BZJg+MSLr0owyUAdpNjN8J5d0UL6/bimFkuLU5MbxXB1DVPLAD3VhWbGpUg1L4udN49E= X-Received: by 2002:a17:906:d7ab:: with SMTP id pk11mr22960862ejb.216.1563235845844; Mon, 15 Jul 2019 17:10:45 -0700 (PDT) MIME-Version: 1.0 References: <5D27FE26.1050002@intel.com> In-Reply-To: <5D27FE26.1050002@intel.com> From: Eric Hankland Date: Mon, 15 Jul 2019 17:10:34 -0700 Message-ID: Subject: Re: [PATCH v2] KVM: x86: PMU Event Filter To: Wei Wang Cc: Paolo Bonzini , rkrcmar@redhat.com, linux-kernel@vger.kernel.org, Stephane Eranian , kvm@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > I think just disabling guest cpuid might not be enough, since guest > could write to the msr without checking the cpuid. > > Why not just add a bitmap for fixed counter? > e.g. fixed_counter_reject_bitmap > > At the beginning of reprogram_fixed_counter, we could add the check: > > if (test_bit(idx, &kvm->arch.fixed_counter_reject_bitmap)) > return -EACCES; > > (Please test with your old guest and see if they have issues if we > inject #GP when > they try to set the fixed_ctrl msr. If there is, we could drop -EACCESS > above) > > The bitmap could be set at kvm_vm_ioctl_set_pmu_event_filter. intel_pmu_refresh() checks the guest cpuid and sets the number of fixed counters according to that: pmu->nr_arch_fixed_counters = min_t(int, edx.split.num_counters_fixed, INTEL_PMC_MAX_FIXED); and reprogram_fixed_counters()/get_fixed_pmc() respect this so the guest can't just ignore the cpuid. Adding a bitmap does let you do things like disable the first counter but keep the second and third, but given that there are only three and the events are likely to be on a whitelist anyway, it seemed like adding the bitmap wasn't worth it. If you still feel the same way even though we can disable them via the cpuid, I can add this in. > I think it would be better to add more, please see below: > > enum kvm_pmu_action_type { > KVM_PMU_EVENT_ACTION_NONE = 0, > KVM_PMU_EVENT_ACTION_ACCEPT = 1, > KVM_PMU_EVENT_ACTION_REJECT = 2, > KVM_PMU_EVENT_ACTION_MAX > }; > > and do a check in kvm_vm_ioctl_set_pmu_event_filter() > if (filter->action >= KVM_PMU_EVENT_ACTION_MAX) > return -EINVAL; > > This is for detecting the case that we add a new action in > userspace, while the kvm hasn't been updated to support that. > > KVM_PMU_EVENT_ACTION_NONE is for userspace to remove > the filter after they set it. We can achieve the same result by using a reject action with an empty set of events - is there some advantage to "none" over that? I can add that check for valid actions. > > +#define KVM_PMU_EVENT_FILTER_MAX_EVENTS 63 > > Why is this limit needed? Serves to keep the filters on the smaller side and ensures the size calculation can't overflow if users attempt to. Keeping the filter under 4k is nicer for allocation - also, if we want really large filters we might want to do something smarter than a linear traversal of the filter when guests program counters. > I think it looks tidier to wrap the changes above into a function: > > if (kvm_pmu_filter_event(kvm, eventsel & AMD64_RAW_EVENT_MASK_NB)) > return; Okay - I can do that. > > + kvfree(filter); > > Probably better to have it conditionally? > > if (filter) { > synchronize_srcu(); > kfree(filter) > } > > You may want to factor it out, so that kvm_pmu_destroy could reuse. Do you mean kvm_arch_destroy_vm? It looks like that's where kvm_arch members are freed. I can do that. Eric