Hi Reiji,
On Thu, Feb 9, 2023 at 10:16 PM Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
>
> 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 for the clarification. You are right about the issue. I'll fix
it in the next version by utilizing a bit in kvm->arch.flags to
indicate the IMPDEF PMU. The value stored in id_regs would be the one
for vCPU with PMU support.
>
> Thanks,
> Reiji
