On Tue, 24 Nov 2020 17:28:30 +0000,
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi Marc,
>
> I believe there is something missing from this series.
>
> The original behaviour, which this series changes, was not to do
> register emulation and PMU state update if the PMU wasn't ready,
> where vcpu->arch.pmu.ready was set to true if the PMU was
> initialized properly in kvm_vcpu_first_run_init() ->
> kvm_arm_pmu_v3_enable().
>
> The series changes PMU emulation such that register emulation and
> pmu state update is gated only on the VCPU feature being set. This
> means that now userspace can set the VCPU feature, don't do any
> initialization, and run a guest which can access PMU registers. Also
> kvm_pmu_update_state() will now be called before each VM entry. I'm
> not exactly sure what happens if we call kvm_vgic_inject_irq() for
> an irq_num = 0 and not owned by the PMU (the owner is set
> KVM_ARM_VCPU_PMU_V3_INIT -> kvm_arm_pmu_v3_init()), but I don't
> think that's allowed.
That's a very good point. I dropped the "ready" state a bit
carelessly, and nothing guards a half baked PMU anymore.
>
> I was also able to trigger this warning with a modified version of kvmtool:
[ugly warning]
> I removed all KVM_ARM_VCPU_PMU_V3_CTRL ioctl calls from kvmtool's
> pmu emulation, and I started the pmu test from kvm-unit-tests:
>
> $ ./lkvm-pmu run -c1 -m64 -f arm/pmu.flat --pmu -p cycle-counter
>
> The reason for the warning is that the correct value for
> kvm->arch.pmuver is set in kvm_arm_pmu_v3_set_attr(), which is not
> called anymore.
>
> This diff seems to solve the issue:
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 643cf819f3c0..150b9cb0f741 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -825,9 +825,12 @@ bool kvm_arm_support_pmu_v3(void)
>
> int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
> {
> - if (!vcpu->arch.pmu.created)
> + if (!kvm_vcpu_has_pmu(vcpu))
> return 0;
>
> + if (!vcpu->arch.pmu.created)
> + return -ENOEXEC;
> +
> /*
> * A valid interrupt configuration for the PMU is either to have a
> * properly configured interrupt number and using an in-kernel
>
> If you agree with the fix, I can send a proper patch.
> vcpu->arch.pmu.created is set in kvm_arm_pmu_v3_init(), which checks
> if the interrupt ID has been set. I chose to return -ENOEXEC
> because that's what KVM_RUN returns if the vcpu isn't initialized in
> kvm_arch_vcpu_ioctl_run().
Yes, this seems reasonable. The first run will fail, as for an
uninitialised vcpu.
Whist you're doing that, can you please document the ENOEXEC return
value? We only document EINTR so far.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
