lkml.org 
[lkml]   [2023]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 08/12] KVM: arm64: PMU: Allow userspace to limit PMCR_EL0.N for the guest
    On Tue, Aug 22, 2023 at 11:26:23AM +0800, Shaoqin Huang wrote:

    [...]

    > > > > +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?
    > >
    > It depends on how userspace use this api to limit the number of pmcr. I
    > think what you mean in the code is that userspace need to set every vcpu's
    > pmcr to the same value, so the `unlikely` here is right, only one vcpu can
    > change the kvm->arch.pmcr.n, it saves the cpu cycles.
    >
    > What suggest above might be wrong. Since I think when userspace want to
    > limit the number of pmcr, it may just set the new_n on one vcpu, since the
    > kvm->arch.pmcr_n is a VM-local value, every vcpu can see it, so it's
    > `likely` the (new_n <= kvm->arch.pmcr_n_limit), it can decrease one checking
    > statement.

    How about we just do away with branch hints in the first place? This is
    _not_ a hot path.

    --
    Thanks,
    Oliver

    \
     
     \ /
      Last update: 2023-09-15 22:37    [W:2.997 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site