Hi Raghu,
On Thu, Aug 17, 2023 at 12:30:25AM +0000, Raghavendra Rao Ananta wrote:
> From: Reiji Watanabe <[email protected]>
>
> KVM does not yet support userspace modifying PMCR_EL0.N (With
> the previous patch, KVM ignores what is written by upserspace).
typo: userspace
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index ce7de6bbdc967..39ad56a71ad20 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> * while the latter does not.
> */
> kvm->arch.pmcr_n = arm_pmu->num_events - 1;
> + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
Can't we just get at this through the arm_pmu instance rather than
copying it into kvm_arch?
> return 0;
> }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2075901356c5b..c01d62afa7db4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> return 0;
> }
>
> +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> + u64 val)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + u64 new_n, mutable_mask;
> + int ret = 0;
> +
> + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
> +
> + mutex_lock(&kvm->arch.config_lock);
> + if (unlikely(new_n != kvm->arch.pmcr_n)) {
> + /*
> + * The vCPU can't have more counters than the PMU
> + * hardware implements.
> + */
> + if (new_n <= kvm->arch.pmcr_n_limit)
> + kvm->arch.pmcr_n = new_n;
> + else
> + ret = -EINVAL;
> + }
Hmm, I'm not so sure about returning an error here. ABI has it that
userspace can write any value to PMCR_EL0 successfully. Can we just
ignore writes that attempt to set PMCR_EL0.N to something higher than
supported by hardware? Our general stance should be that system register
fields responsible for feature identification are immutable after the VM
has started.
--
Thanks,
Oliver
On Fri, Sep 15, 2023 at 08:53:16PM +0000, Oliver Upton wrote:
> Hi Raghu,
>
> On Thu, Aug 17, 2023 at 12:30:25AM +0000, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <[email protected]>
> >
> > KVM does not yet support userspace modifying PMCR_EL0.N (With
> > the previous patch, KVM ignores what is written by upserspace).
>
> typo: userspace
>
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index ce7de6bbdc967..39ad56a71ad20 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > * while the latter does not.
> > */
> > kvm->arch.pmcr_n = arm_pmu->num_events - 1;
> > + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
>
> Can't we just get at this through the arm_pmu instance rather than
> copying it into kvm_arch?
>
> > return 0;
> > }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 2075901356c5b..c01d62afa7db4 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > return 0;
> > }
> >
> > +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > + u64 val)
> > +{
> > + struct kvm *kvm = vcpu->kvm;
> > + u64 new_n, mutable_mask;
> > + int ret = 0;
> > +
> > + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
> > +
> > + mutex_lock(&kvm->arch.config_lock);
> > + if (unlikely(new_n != kvm->arch.pmcr_n)) {
> > + /*
> > + * The vCPU can't have more counters than the PMU
> > + * hardware implements.
> > + */
> > + if (new_n <= kvm->arch.pmcr_n_limit)
> > + kvm->arch.pmcr_n = new_n;
> > + else
> > + ret = -EINVAL;
> > + }
>
> Hmm, I'm not so sure about returning an error here. ABI has it that
> userspace can write any value to PMCR_EL0 successfully. Can we just
> ignore writes that attempt to set PMCR_EL0.N to something higher than
> supported by hardware? Our general stance should be that system register
> fields responsible for feature identification are immutable after the VM
> has started.
I hacked up my reply and dropped some context; this doesn't read right.
Shaoqin made the point about preventing changes to PMCR_EL0.N after the
VM has started and I firmly agree. The behavior should be:
- Writes to PMCR always succeed
- PMCR_EL0.N values greater than what's supported by hardware are
ignored
- Changes to N after the VM has started are ignored.
--
Thanks,
Oliver
On Fri, Sep 15, 2023 at 2:54 PM Oliver Upton <[email protected]> wrote:
>
> On Fri, Sep 15, 2023 at 08:53:16PM +0000, Oliver Upton wrote:
> > Hi Raghu,
> >
> > On Thu, Aug 17, 2023 at 12:30:25AM +0000, Raghavendra Rao Ananta wrote:
> > > From: Reiji Watanabe <[email protected]>
> > >
> > > KVM does not yet support userspace modifying PMCR_EL0.N (With
> > > the previous patch, KVM ignores what is written by upserspace).
> >
> > typo: userspace
> >
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index ce7de6bbdc967..39ad56a71ad20 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > > * while the latter does not.
> > > */
> > > kvm->arch.pmcr_n = arm_pmu->num_events - 1;
> > > + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
> >
> > Can't we just get at this through the arm_pmu instance rather than
> > copying it into kvm_arch?
> >
> > > return 0;
> > > }
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 2075901356c5b..c01d62afa7db4 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > > return 0;
> > > }
> > >
> > > +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > > + u64 val)
> > > +{
> > > + struct kvm *kvm = vcpu->kvm;
> > > + u64 new_n, mutable_mask;
> > > + int ret = 0;
> > > +
> > > + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
> > > +
> > > + mutex_lock(&kvm->arch.config_lock);
> > > + if (unlikely(new_n != kvm->arch.pmcr_n)) {
> > > + /*
> > > + * The vCPU can't have more counters than the PMU
> > > + * hardware implements.
> > > + */
> > > + if (new_n <= kvm->arch.pmcr_n_limit)
> > > + kvm->arch.pmcr_n = new_n;
> > > + else
> > > + ret = -EINVAL;
> > > + }
> >
> > Hmm, I'm not so sure about returning an error here. ABI has it that
> > userspace can write any value to PMCR_EL0 successfully. Can we just
> > ignore writes that attempt to set PMCR_EL0.N to something higher than
> > supported by hardware? Our general stance should be that system register
> > fields responsible for feature identification are immutable after the VM
> > has started.
>
> I hacked up my reply and dropped some context; this doesn't read right.
> Shaoqin made the point about preventing changes to PMCR_EL0.N after the
> VM has started and I firmly agree. The behavior should be:
>
> - Writes to PMCR always succeed
>
> - PMCR_EL0.N values greater than what's supported by hardware are
> ignored
>
> - Changes to N after the VM has started are ignored.
>
Reiji and I were wondering if we should proceed with this as this
would change userspace expectation. BTW, when you said "ignored", does
that mean we silently return to userspace with a success or with EBUSY
(changing the expectations)?
Thank you.
Raghavendra
> --
> Thanks,
> Oliver
Hi Oliver,
On Mon, Sep 18, 2023 at 10:11 AM Raghavendra Rao Ananta
<[email protected]> wrote:
>
> On Fri, Sep 15, 2023 at 2:54 PM Oliver Upton <[email protected]> wrote:
> >
> > On Fri, Sep 15, 2023 at 08:53:16PM +0000, Oliver Upton wrote:
> > > Hi Raghu,
> > >
> > > On Thu, Aug 17, 2023 at 12:30:25AM +0000, Raghavendra Rao Ananta wrote:
> > > > From: Reiji Watanabe <[email protected]>
> > > >
> > > > KVM does not yet support userspace modifying PMCR_EL0.N (With
> > > > the previous patch, KVM ignores what is written by upserspace).
> > >
> > > typo: userspace
> > >
> > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > > index ce7de6bbdc967..39ad56a71ad20 100644
> > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > > > * while the latter does not.
> > > > */
> > > > kvm->arch.pmcr_n = arm_pmu->num_events - 1;
> > > > + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
> > >
> > > Can't we just get at this through the arm_pmu instance rather than
> > > copying it into kvm_arch?
> > >
> > > > return 0;
> > > > }
> > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > > index 2075901356c5b..c01d62afa7db4 100644
> > > > --- a/arch/arm64/kvm/sys_regs.c
> > > > +++ b/arch/arm64/kvm/sys_regs.c
> > > > @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > > > return 0;
> > > > }
> > > >
> > > > +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > > > + u64 val)
> > > > +{
> > > > + struct kvm *kvm = vcpu->kvm;
> > > > + u64 new_n, mutable_mask;
> > > > + int ret = 0;
> > > > +
> > > > + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
> > > > +
> > > > + mutex_lock(&kvm->arch.config_lock);
> > > > + if (unlikely(new_n != kvm->arch.pmcr_n)) {
> > > > + /*
> > > > + * The vCPU can't have more counters than the PMU
> > > > + * hardware implements.
> > > > + */
> > > > + if (new_n <= kvm->arch.pmcr_n_limit)
> > > > + kvm->arch.pmcr_n = new_n;
> > > > + else
> > > > + ret = -EINVAL;
> > > > + }
> > >
> > > Hmm, I'm not so sure about returning an error here. ABI has it that
> > > userspace can write any value to PMCR_EL0 successfully. Can we just
> > > ignore writes that attempt to set PMCR_EL0.N to something higher than
> > > supported by hardware? Our general stance should be that system register
> > > fields responsible for feature identification are immutable after the VM
> > > has started.
> >
> > I hacked up my reply and dropped some context; this doesn't read right.
> > Shaoqin made the point about preventing changes to PMCR_EL0.N after the
> > VM has started and I firmly agree. The behavior should be:
> >
> > - Writes to PMCR always succeed
> >
> > - PMCR_EL0.N values greater than what's supported by hardware are
> > ignored
> >
> > - Changes to N after the VM has started are ignored.
> >
> Reiji and I were wondering if we should proceed with this as this
> would change userspace expectation. BTW, when you said "ignored", does
> that mean we silently return to userspace with a success or with EBUSY
> (changing the expectations)?
>
Sorry, I just read your earlier comment (one before you detailed the
behavior), from which I'm guessing "ignore" means simply disregard the
change and return success to userspace. But wouldn't that cause issues
in debugging?
Thank you.
Raghavendra
> Thank you.
> Raghavendra
> > --
> > Thanks,
> > Oliver
On Fri, Sep 15, 2023 at 1:53 PM Oliver Upton <[email protected]> wrote:
>
> Hi Raghu,
>
> On Thu, Aug 17, 2023 at 12:30:25AM +0000, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <[email protected]>
> >
> > KVM does not yet support userspace modifying PMCR_EL0.N (With
> > the previous patch, KVM ignores what is written by upserspace).
>
> typo: userspace
>
Noted.
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index ce7de6bbdc967..39ad56a71ad20 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -896,6 +896,7 @@ int kvm_arm_set_vm_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> > * while the latter does not.
> > */
> > kvm->arch.pmcr_n = arm_pmu->num_events - 1;
> > + kvm->arch.pmcr_n_limit = arm_pmu->num_events - 1;
>
> Can't we just get at this through the arm_pmu instance rather than
> copying it into kvm_arch?
>
Yeah, I suppose we can directly access it in set_pmcr().
Thank you.
Raghavendra
> > return 0;
> > }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 2075901356c5b..c01d62afa7db4 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1086,6 +1086,51 @@ static int get_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > return 0;
> > }
> >
> > +static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> > + u64 val)
> > +{
> > + struct kvm *kvm = vcpu->kvm;
> > + u64 new_n, mutable_mask;
> > + int ret = 0;
> > +
> > + new_n = FIELD_GET(ARMV8_PMU_PMCR_N, val);
> > +
> > + mutex_lock(&kvm->arch.config_lock);
> > + if (unlikely(new_n != kvm->arch.pmcr_n)) {
> > + /*
> > + * The vCPU can't have more counters than the PMU
> > + * hardware implements.
> > + */
> > + if (new_n <= kvm->arch.pmcr_n_limit)
> > + kvm->arch.pmcr_n = new_n;
> > + else
> > + ret = -EINVAL;
> > + }
>
> Hmm, I'm not so sure about returning an error here. ABI has it that
> userspace can write any value to PMCR_EL0 successfully. Can we just
> ignore writes that attempt to set PMCR_EL0.N to something higher than
> supported by hardware? Our general stance should be that system register
> fields responsible for feature identification are immutable after the VM
> has started.
>
> --
> Thanks,
> Oliver