lkml.org 
[lkml]   [2023]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v5 08/12] KVM: arm64: PMU: Allow userspace to limit PMCR_EL0.N for the guest
    Hi Shaoqin,

    On Mon, Aug 21, 2023 at 5:12 AM Shaoqin Huang <shahuang@redhat.com> wrote:
    >
    > Hi Raghavendra,
    >
    > On 8/17/23 08:30, Raghavendra Rao Ananta wrote:
    > > From: Reiji Watanabe <reijiw@google.com>
    > >
    > > KVM does not yet support userspace modifying PMCR_EL0.N (With
    > > the previous patch, KVM ignores what is written by upserspace).
    > > Add support userspace limiting PMCR_EL0.N.
    > >
    > > Disallow userspace to set PMCR_EL0.N to a value that is greater
    > > than the host value (KVM_SET_ONE_REG will fail), as KVM doesn't
    > > support more event counters than the host HW implements.
    > > Although this is an ABI change, this change only affects
    > > userspace setting PMCR_EL0.N to a larger value than the host.
    > > As accesses to unadvertised event counters indices is CONSTRAINED
    > > UNPREDICTABLE behavior, and PMCR_EL0.N was reset to the host value
    > > on every vCPU reset before this series, I can't think of any
    > > use case where a user space would do that.
    > >
    > > Also, ignore writes to read-only bits that are cleared on vCPU reset,
    > > and RES{0,1} bits (including writable bits that KVM doesn't support
    > > yet), as those bits shouldn't be modified (at least with
    > > the current KVM).
    > >
    > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
    > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
    > > ---
    > > arch/arm64/include/asm/kvm_host.h | 3 ++
    > > arch/arm64/kvm/pmu-emul.c | 1 +
    > > arch/arm64/kvm/sys_regs.c | 49 +++++++++++++++++++++++++++++--
    > > 3 files changed, 51 insertions(+), 2 deletions(-)
    > >
    > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
    > > index 0f2dbbe8f6a7e..c15ec365283d1 100644
    > > --- a/arch/arm64/include/asm/kvm_host.h
    > > +++ b/arch/arm64/include/asm/kvm_host.h
    > > @@ -259,6 +259,9 @@ struct kvm_arch {
    > > /* PMCR_EL0.N value for the guest */
    > > u8 pmcr_n;
    > >
    > > + /* Limit value of PMCR_EL0.N for the guest */
    > > + u8 pmcr_n_limit;
    > > +
    > > /* Hypercall features firmware registers' descriptor */
    > > struct kvm_smccc_features smccc_feat;
    > > struct maple_tree smccc_filter;
    > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
    > > index ce7de6bbdc967..39ad56a71ad20 100644
    > > --- a/arch/arm64/kvm/pmu-emul.c
    > > +++ b/arch/arm64/kvm/pmu-emul.c
    > > @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
    > > * while the latter does not.
    > > */
    > > kvm->arch.pmcr_n = arm_pmu->num_events - 1;
    > > + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
    > >
    > > return 0;
    > > }
    > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
    > > index 2075901356c5b..c01d62afa7db4 100644
    > > --- a/arch/arm64/kvm/sys_regs.c
    > > +++ b/arch/arm64/kvm/sys_regs.c
    > > @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
    > > return 0;
    > > }
    > >
    > > +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
    > > + u64 val)
    > > +{
    > > + struct kvm *kvm = vcpu->kvm;
    > > + u64 new_n, mutable_mask;
    > > + int ret = 0;
    > > +
    > > + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
    > > +
    > > + mutex_lock(&kvm->arch.config_lock);
    > > + if (unlikely(new_n != kvm->arch.pmcr_n)) {
    > > + /*
    > > + * The vCPU can't have more counters than the PMU
    > > + * hardware implements.
    > > + */
    > > + if (new_n <= kvm->arch.pmcr_n_limit)
    > > + kvm->arch.pmcr_n = new_n;
    > > + else
    > > + ret = -EINVAL;
    > > + }
    >
    > Since we have set the default value of pmcr_n, if we want to set a new
    > pmcr_n, shouldn't it be a different value?
    >
    > So how about change the checking to:
    >
    > if (likely(new_n <= kvm->arch.pmcr_n_limit)
    > kvm->arch.pmcr_n = new_n;
    > else
    > ret = -EINVAL;
    >
    > what do you think?
    >
    Sorry, I guess I didn't fully understand your suggestion. Are you
    saying that it's 'likely' that userspace would configure the correct
    value?

    > > + mutex_unlock(&kvm->arch.config_lock);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + /*
    > > + * Ignore writes to RES0 bits, read only bits that are cleared on
    > > + * vCPU reset, and writable bits that KVM doesn't support yet.
    > > + * (i.e. only PMCR.N and bits [7:0] are mutable from userspace)
    > > + * The LP bit is RES0 when FEAT_PMUv3p5 is not supported on the vCPU.
    > > + * But, we leave the bit as it is here, as the vCPU's PMUver might
    > > + * be changed later (NOTE: the bit will be cleared on first vCPU run
    > > + * if necessary).
    > > + */
    > > + mutable_mask = (ARMV8_PMU_PMCR_MASK | ARMV8_PMU_PMCR_N);
    > > + val &= mutable_mask;
    > > + val |= (__vcpu_sys_reg(vcpu, r->reg) & ~mutable_mask);
    > > +
    > > + /* The LC bit is RES1 when AArch32 is not supported */
    > > + if (!kvm_supports_32bit_el0())
    > > + val |= ARMV8_PMU_PMCR_LC;
    > > +
    > > + __vcpu_sys_reg(vcpu, r->reg) = val;
    > > + return 0;
    > > +}
    > > +
    > > /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
    > > #define DBG_BCR_BVR_WCR_WVR_EL1(n) \
    > > { SYS_DESC(SYS_DBGBVRn_EL1(n)), \
    > > @@ -2147,8 +2192,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
    > > { SYS_DESC(SYS_CTR_EL0), access_ctr },
    > > { SYS_DESC(SYS_SVCR), undef_access },
    > >
    > > - { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr,
    > > - .reset = reset_pmcr, .reg = PMCR_EL0, .get_user = get_pmcr },
    > > + { PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
    > > + .reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
    >
    > A little confusing, since the PMU_SYS_REG() defines the default
    > visibility which is pmu_visibility can return REG_HIDDEN, the set_user
    > to pmcr will be blocked, how can it being set?
    >
    > Maybe I lose some details.
    >
    pmu_visibility() returns REG_HIDDEN only if userspace has not added
    support for PMUv3 via KVM_ARM_PREFERRED_TARGET ioctl. Else, it should
    return 0, and give access.

    Thank you.
    Raghavendra

    > Thanks,
    > Shaoqin
    >
    > > { PMU_SYS_REG(PMCNTENSET_EL0),
    > > .access = access_pmcnten, .reg = PMCNTENSET_EL0 },
    > > { PMU_SYS_REG(PMCNTENCLR_EL0),
    >
    > --
    > Shaoqin
    >

    \
     
     \ /
      Last update: 2023-08-22 01:29    [W:4.605 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site