Hi Reiji,
On Fri, 04 Nov 2022 07:00:22 +0000,
Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> On Thu, Nov 3, 2022 at 3:25 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Thu, 03 Nov 2022 05:31:56 +0000,
> > Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> > >
> > > It appears the patch allows userspace to set IMPDEF even
> > > when host_pmuver == 0. Shouldn't it be allowed only when
> > > host_pmuver == IMPDEF (as before)?
> > > Probably, it may not cause any real problems though.
> >
> > Given that we don't treat the two cases any differently, I thought it
> > would be reasonable to relax this particular case, and I can't see any
> > reason why we shouldn't tolerate this sort of migration.
>
> That's true. I assume it won't cause any functional issues.
>
> I have another comment related to this.
> KVM allows userspace to create a guest with a mix of vCPUs with and
> without PMU. For such a guest, if the register for the vCPU without
> PMU is set last, I think the PMUVER value for vCPUs with PMU could
> become no PMU (0) or IMPDEF (0xf).
> Also, with the current patch, userspace can set PMUv3 support value
> (non-zero or non-IMPDEF) for vCPUs without the PMU.
> IMHO, KVM shouldn't allow userspace to set PMUVER to the value that
> is inconsistent with PMU configuration for the vCPU.
> What do you think ?
Yes, this seems sensible, and we only do it one way at the moment.
> I'm thinking of the following code (not tested).
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 4fa14b4ae2a6..ddd849027cc3 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1265,10 +1265,17 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver)
> return -EINVAL;
>
> - /* We already have a PMU, don't try to disable it... */
> - if (kvm_vcpu_has_pmu(vcpu) &&
> - (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF))
> - return -EINVAL;
> + if (kvm_vcpu_has_pmu(vcpu)) {
> + /* We already have a PMU, don't try to disable it... */
> + if (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) {
> + return -EINVAL;
> + }
> + } else {
> + /* We don't have a PMU, don't try to enable it... */
> + if (pmuver > 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF) {
> + return -EINVAL;
> + }
> + }
This is a bit ugly. I came up with this instead:
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3b28ef48a525..e104fde1a0ee 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1273,6 +1273,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
u64 val)
{
u8 pmuver, host_pmuver;
+ bool valid_pmu;
host_pmuver = kvm_arm_pmu_get_pmuver_limit();
@@ -1286,9 +1287,10 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
if (pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver)
return -EINVAL;
- /* We already have a PMU, don't try to disable it... */
- if (kvm_vcpu_has_pmu(vcpu) &&
- (pmuver == 0 || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF))
+ valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
+
+ /* Make sure view register and PMU support do match */
+ if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
return -EINVAL;
/* We can only differ with PMUver, and anything else is an error */
and the similar check for the 32bit counterpart.
>
> /* We can only differ with PMUver, and anything else is an error */
> val ^= read_id_reg(vcpu, rd);
> @@ -1276,7 +1283,8 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> if (val)
> return -EINVAL;
>
> - vcpu->kvm->arch.dfr0_pmuver = pmuver;
> + if (kvm_vcpu_has_pmu(vcpu))
> + vcpu->kvm->arch.dfr0_pmuver = pmuver;
We need to update this unconditionally if we want to be able to
restore an IMPDEF PMU view to the guest.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
