linux-kernel - Re: [PATCH v8 02/13] KVM: arm64: PMU: Set the default PMU for the guest before vCPU reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZTa6cigtBpyq5IwD@linux.dev>
Date:   Mon, 23 Oct 2023 18:24:50 +0000
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Raghavendra Rao Ananta <rananta@...gle.com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        James Morse <james.morse@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Zenghui Yu <yuzenghui@...wei.com>,
        Shaoqin Huang <shahuang@...hat.com>,
        Jing Zhang <jingzhangos@...gle.com>,
        Reiji Watanabe <reijiw@...gle.com>,
        Colton Lewis <coltonlewis@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v8 02/13] KVM: arm64: PMU: Set the default PMU for the
 guest before vCPU reset

On Mon, Oct 23, 2023 at 11:40:50AM +0100, Marc Zyngier wrote:

[...]

> > +static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm *kvm = vcpu->kvm;
> > +
> > +	/*
> > +	 * When the vCPU has a PMU, but no PMU is set for the guest
> > +	 * yet, set the default one.
> > +	 */
> > +	if (kvm_vcpu_has_pmu(vcpu) && !kvm->arch.arm_pmu &&
> > +	    kvm_arm_set_default_pmu(kvm))
> > +		return -EINVAL;
> 
> nit: I'm not keen on re-interpreting the error code. If
> kvm_arm_set_default_pmu() returns an error, we should return *that*
> particular error, and not any other. Something like:

The code took this shape because I had an issue with returning ENODEV on
the KVM_ARM_VCPU_INIT ioctl, which is not a documented error code.
Now that the vCPU flags are sanitised early in the ioctl, KVM has
decided at this point that vPMU is a supported feature.

Given that, I think ENODEV is fine now as the unexpected return value
would indicate a bug in KVM.

> Hmmm. Contrary to what the commit message says, the default PMU is not
> picked at reset time, but at the point where the target is set (the
> very first vcpu init). Which is pretty different from reset, which
> happens more than once.
> 
> I also can't say I'm over the moon with yet another function that does
> a very tiny bit of initialisation outside of the rest of the code that
> performs the vcpu init. Following things is an absolute maze...

I'm fine with this being inlined into __kvm_vcpu_set_target() so long as
we maintain the clear distinction between one-time setup and vCPU reset.

-- 
Thanks,
Oliver