Hi Jing,
On Thu, Feb 9, 2023 at 1:25 PM Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> On Mon, Feb 6, 2023 at 9:30 PM Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> >
> > Hi Jing,
> >
> > On Tue, Jan 31, 2023 at 6:51 PM Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> > >
> > > With per guest ID registers, PMUver settings from userspace
> > > can be stored in its corresponding ID register.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > > ---
> > > arch/arm64/include/asm/kvm_host.h | 5 -----
> > > arch/arm64/kvm/arm.c | 6 ------
> > > arch/arm64/kvm/id_regs.c | 33 ++++++++++++++++++++-----------
> > > include/kvm/arm_pmu.h | 6 ++++--
> > > 4 files changed, 25 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index fabb30185a4a..1ab443b52c46 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -225,11 +225,6 @@ struct kvm_arch {
> > >
> > > cpumask_var_t supported_cpus;
> > >
> > > - struct {
> > > - u8 imp:4;
> > > - u8 unimp:4;
> > > - } dfr0_pmuver;
> > > -
> > > /* Hypercall features firmware registers' descriptor */
> > > struct kvm_smccc_features smccc_feat;
> > >
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index d8ba5106bf51..25bd95650223 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -138,12 +138,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > > kvm_arm_set_default_id_regs(kvm);
> > > kvm_arm_init_hypercalls(kvm);
> > >
> > > - /*
> > > - * Initialise the default PMUver before there is a chance to
> > > - * create an actual PMU.
> > > - */
> > > - kvm->arch.dfr0_pmuver.imp = kvm_arm_pmu_get_pmuver_limit();
> > > -
> > > return 0;
> > >
> > > err_free_cpumask:
> > > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > > index bc5d9bc84eb1..5eade7d380af 100644
> > > --- a/arch/arm64/kvm/id_regs.c
> > > +++ b/arch/arm64/kvm/id_regs.c
> > > @@ -19,10 +19,12 @@
> > >
> > > static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
> > > {
> > > - if (kvm_vcpu_has_pmu(vcpu))
> > > - return vcpu->kvm->arch.dfr0_pmuver.imp;
> > > -
> > > - return vcpu->kvm->arch.dfr0_pmuver.unimp;
> > > + u8 pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> > > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1));
> > > + if (kvm_vcpu_has_pmu(vcpu) || pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> > > + return pmuver;
> > > + else
> > > + return 0;
> > > }
> > >
> > > static u8 perfmon_to_pmuver(u8 perfmon)
> > > @@ -263,10 +265,9 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > > if (val)
> > > return -EINVAL;
> > >
> > > - if (valid_pmu)
> > > - vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
> > > - else
> > > - vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
> > > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) |=
> > > + FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), pmuver);
> >
> > Did you consider there could be guests that have vCPUs with and
> > without PMU ? It looks like the code doesn't work for such guests.
> > (e.g. for such guests, if setting the register is done for vCPUs
> > without PMU, this code seems to make PMUVer zero or 0xf
> > even for vCPUs with PMU)
> Yes, I did. The PMUVer field is a per-VCPU field, whose value was
> determined on the fly in the get_user function. Check the function
> kvm_arm_read_id_reg_with_encoding, vcpu_pmuver is called to set the
> real value for the VCPU.
> The perVM ID registers array we put in the kvm structure save
> correctly only for those perVM field, for those pre-VCPU filed, their
> real value is determined on the fly during the register reading
> (get_user).
Yes, I understand that part.
But, if a guest has 2 VCPUs, one of which has a PMU and the other
does not, the PMUVer for them will be different. Which value do
you try to save in the per VM field ?
It appears that the code always saves the last value set by
userspace in the per VM field, regardless of whether the vCPU
has PMU or not.
If the last value is set for the vCPU without PMU, the per VM
field will be either 0 or 0xf. Since the per VM field is always
used by vcpu_pmuver() for vCPUs with PMU, I would think the
PMUVer for the vCPU with PMU will end up being 0 or 0xf in this
case (NOTE: the PMUVer for vCPUs with PMU should not be 0 or 0xf).
Or am I missing something??
Thanks,
Reiji
