Hi Marc,
On Fri, Nov 4, 2022 at 5:21 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> 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;
Thanks, much better!
>
> /* 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.
Yes, right.
BTW, if we have no intention of supporting a mix of vCPUs with and
without PMU, I think it would be nice if we have a clear comment on
that in the code. Or I'm hoping to disallow it if possible though.
Thank you,
Reiji
