From: Reiji Watanabe <[email protected]>
On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
This function clears RAZ bits of those registers corresponding
to unimplemented event counters on the vCPU, and sets bits
corresponding to implemented event counters to a predefined
pseudo UNKNOWN value (some bits are set to 1).
The function identifies (un)implemented event counters on the
vCPU based on the PMCR_EL0.N value on the host. Using the host
value for this would be problematic when KVM supports letting
userspace set PMCR_EL0.N to a value different from the host value
(some of the RAZ bits of those registers could end up being set to 1).
Fix this by clearing the registers so that it can ensure
that all the RAZ bits are cleared even when the PMCR_EL0.N value
for the vCPU is different from the host value. Use reset_val() to
do this instead of fixing reset_pmu_reg(), and remove
reset_pmu_reg(), as it is no longer used.
Signed-off-by: Reiji Watanabe <[email protected]>
Signed-off-by: Raghavendra Rao Ananta <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 818a52e257ed..3dbb7d276b0e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
return REG_HIDDEN;
}
-static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
-{
- u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
-
- /* No PMU available, any PMU reg may UNDEF... */
- if (!kvm_arm_support_pmu_v3())
- return 0;
-
- n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
- n &= ARMV8_PMU_PMCR_N_MASK;
- if (n)
- mask |= GENMASK(n - 1, 0);
-
- reset_unknown(vcpu, r);
- __vcpu_sys_reg(vcpu, r->reg) &= mask;
-
- return __vcpu_sys_reg(vcpu, r->reg);
-}
-
static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
reset_unknown(vcpu, r);
@@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
#define PMU_SYS_REG(name) \
- SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
+ SYS_DESC(SYS_##name), .reset = reset_val, \
.visibility = pmu_visibility
/* Macro to expand the PMEVCNTRn_EL0 register */
--
2.42.0.609.gbb76f46606-goog
Hi Raghavendra,
On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
> From: Reiji Watanabe <[email protected]>
>
> On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
> PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
PMOVS{SET,CLR}_EL0?
> This function clears RAZ bits of those registers corresponding
> to unimplemented event counters on the vCPU, and sets bits
> corresponding to implemented event counters to a predefined
> pseudo UNKNOWN value (some bits are set to 1).
>
> The function identifies (un)implemented event counters on the
> vCPU based on the PMCR_EL0.N value on the host. Using the host
> value for this would be problematic when KVM supports letting
> userspace set PMCR_EL0.N to a value different from the host value
> (some of the RAZ bits of those registers could end up being set to 1).
>
> Fix this by clearing the registers so that it can ensure
> that all the RAZ bits are cleared even when the PMCR_EL0.N value
> for the vCPU is different from the host value. Use reset_val() to
> do this instead of fixing reset_pmu_reg(), and remove
> reset_pmu_reg(), as it is no longer used.
do you intend to restore the 'unknown' behavior at some point?
Thanks
Eric
>
> Signed-off-by: Reiji Watanabe <[email protected]>
> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> ---
> arch/arm64/kvm/sys_regs.c | 21 +--------------------
> 1 file changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 818a52e257ed..3dbb7d276b0e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> return REG_HIDDEN;
> }
>
> -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> -{
> - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> -
> - /* No PMU available, any PMU reg may UNDEF... */
> - if (!kvm_arm_support_pmu_v3())
> - return 0;
> -
> - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> - n &= ARMV8_PMU_PMCR_N_MASK;
> - if (n)
> - mask |= GENMASK(n - 1, 0);
> -
> - reset_unknown(vcpu, r);
> - __vcpu_sys_reg(vcpu, r->reg) &= mask;
> -
> - return __vcpu_sys_reg(vcpu, r->reg);
> -}
> -
> static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> {
> reset_unknown(vcpu, r);
> @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
>
> #define PMU_SYS_REG(name) \
> - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
> + SYS_DESC(SYS_##name), .reset = reset_val, \
> .visibility = pmu_visibility
>
> /* Macro to expand the PMEVCNTRn_EL0 register */
On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <[email protected]> wrote:
>
> Hi Raghavendra,
>
> On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <[email protected]>
> >
> > On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
> > PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
> PMOVS{SET,CLR}_EL0?
Ah, yes. It should be PMOVS{SET,CLR}_EL0.
> > This function clears RAZ bits of those registers corresponding
> > to unimplemented event counters on the vCPU, and sets bits
> > corresponding to implemented event counters to a predefined
> > pseudo UNKNOWN value (some bits are set to 1).
> >
> > The function identifies (un)implemented event counters on the
> > vCPU based on the PMCR_EL0.N value on the host. Using the host
> > value for this would be problematic when KVM supports letting
> > userspace set PMCR_EL0.N to a value different from the host value
> > (some of the RAZ bits of those registers could end up being set to 1).
> >
> > Fix this by clearing the registers so that it can ensure
> > that all the RAZ bits are cleared even when the PMCR_EL0.N value
> > for the vCPU is different from the host value. Use reset_val() to
> > do this instead of fixing reset_pmu_reg(), and remove
> > reset_pmu_reg(), as it is no longer used.
> do you intend to restore the 'unknown' behavior at some point?
>
I believe Reiji's (original author) intention was to keep them
cleared, which would still imply an 'unknown' behavior. Do you think
there's an issue with this?
Thank you.
Raghavendra
> Thanks
>
> Eric
> >
> > Signed-off-by: Reiji Watanabe <[email protected]>
> > Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> > ---
> > arch/arm64/kvm/sys_regs.c | 21 +--------------------
> > 1 file changed, 1 insertion(+), 20 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 818a52e257ed..3dbb7d276b0e 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> > return REG_HIDDEN;
> > }
> >
> > -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > -{
> > - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> > -
> > - /* No PMU available, any PMU reg may UNDEF... */
> > - if (!kvm_arm_support_pmu_v3())
> > - return 0;
> > -
> > - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> > - n &= ARMV8_PMU_PMCR_N_MASK;
> > - if (n)
> > - mask |= GENMASK(n - 1, 0);
> > -
> > - reset_unknown(vcpu, r);
> > - __vcpu_sys_reg(vcpu, r->reg) &= mask;
> > -
> > - return __vcpu_sys_reg(vcpu, r->reg);
> > -}
> > -
> > static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > {
> > reset_unknown(vcpu, r);
> > @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
> >
> > #define PMU_SYS_REG(name) \
> > - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
> > + SYS_DESC(SYS_##name), .reset = reset_val, \
> > .visibility = pmu_visibility
> >
> > /* Macro to expand the PMEVCNTRn_EL0 register */
>
Hi,
On 10/16/23 23:28, Raghavendra Rao Ananta wrote:
> On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <[email protected]> wrote:
>>
>> Hi Raghavendra,
>>
>> On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
>>> From: Reiji Watanabe <[email protected]>
>>>
>>> On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
>>> PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
>> PMOVS{SET,CLR}_EL0?
> Ah, yes. It should be PMOVS{SET,CLR}_EL0.
>
>>> This function clears RAZ bits of those registers corresponding
>>> to unimplemented event counters on the vCPU, and sets bits
>>> corresponding to implemented event counters to a predefined
>>> pseudo UNKNOWN value (some bits are set to 1).
>>>
>>> The function identifies (un)implemented event counters on the
>>> vCPU based on the PMCR_EL0.N value on the host. Using the host
>>> value for this would be problematic when KVM supports letting
>>> userspace set PMCR_EL0.N to a value different from the host value
>>> (some of the RAZ bits of those registers could end up being set to 1).
>>>
>>> Fix this by clearing the registers so that it can ensure
>>> that all the RAZ bits are cleared even when the PMCR_EL0.N value
>>> for the vCPU is different from the host value. Use reset_val() to
>>> do this instead of fixing reset_pmu_reg(), and remove
>>> reset_pmu_reg(), as it is no longer used.
>> do you intend to restore the 'unknown' behavior at some point?
>>
> I believe Reiji's (original author) intention was to keep them
> cleared, which would still imply an 'unknown' behavior. Do you think
> there's an issue with this?
Then why do we bother using reset_unknown in the other places if
clearing the bits is enough here?
Thanks
Eric
>
> Thank you.
> Raghavendra
>> Thanks
>>
>> Eric
>>>
>>> Signed-off-by: Reiji Watanabe <[email protected]>
>>> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
>>> ---
>>> arch/arm64/kvm/sys_regs.c | 21 +--------------------
>>> 1 file changed, 1 insertion(+), 20 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 818a52e257ed..3dbb7d276b0e 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
>>> return REG_HIDDEN;
>>> }
>>>
>>> -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>>> -{
>>> - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
>>> -
>>> - /* No PMU available, any PMU reg may UNDEF... */
>>> - if (!kvm_arm_support_pmu_v3())
>>> - return 0;
>>> -
>>> - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
>>> - n &= ARMV8_PMU_PMCR_N_MASK;
>>> - if (n)
>>> - mask |= GENMASK(n - 1, 0);
>>> -
>>> - reset_unknown(vcpu, r);
>>> - __vcpu_sys_reg(vcpu, r->reg) &= mask;
>>> -
>>> - return __vcpu_sys_reg(vcpu, r->reg);
>>> -}
>>> -
>>> static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>>> {
>>> reset_unknown(vcpu, r);
>>> @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>> trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
>>>
>>> #define PMU_SYS_REG(name) \
>>> - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
>>> + SYS_DESC(SYS_##name), .reset = reset_val, \
>>> .visibility = pmu_visibility
>>>
>>> /* Macro to expand the PMEVCNTRn_EL0 register */
>>
>
Hi Eric,
On Tue, Oct 17, 2023 at 2:23 AM Eric Auger <[email protected]> wrote:
>
> Hi,
> On 10/16/23 23:28, Raghavendra Rao Ananta wrote:
> > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <[email protected]> wrote:
> >>
> >> Hi Raghavendra,
> >>
> >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
> >>> From: Reiji Watanabe <[email protected]>
> >>>
> >>> On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
> >>> PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
> >> PMOVS{SET,CLR}_EL0?
> > Ah, yes. It should be PMOVS{SET,CLR}_EL0.
> >
> >>> This function clears RAZ bits of those registers corresponding
> >>> to unimplemented event counters on the vCPU, and sets bits
> >>> corresponding to implemented event counters to a predefined
> >>> pseudo UNKNOWN value (some bits are set to 1).
> >>>
> >>> The function identifies (un)implemented event counters on the
> >>> vCPU based on the PMCR_EL0.N value on the host. Using the host
> >>> value for this would be problematic when KVM supports letting
> >>> userspace set PMCR_EL0.N to a value different from the host value
> >>> (some of the RAZ bits of those registers could end up being set to 1).
> >>>
> >>> Fix this by clearing the registers so that it can ensure
> >>> that all the RAZ bits are cleared even when the PMCR_EL0.N value
> >>> for the vCPU is different from the host value. Use reset_val() to
> >>> do this instead of fixing reset_pmu_reg(), and remove
> >>> reset_pmu_reg(), as it is no longer used.
> >> do you intend to restore the 'unknown' behavior at some point?
> >>
> > I believe Reiji's (original author) intention was to keep them
> > cleared, which would still imply an 'unknown' behavior. Do you think
> > there's an issue with this?
> Then why do we bother using reset_unknown in the other places if
> clearing the bits is enough here?
>
Hmm. Good point. I can bring back reset_unknown to keep the original behavior.
Thank you.
Raghavendra
> Thanks
>
> Eric
> >
> > Thank you.
> > Raghavendra
> >> Thanks
> >>
> >> Eric
> >>>
> >>> Signed-off-by: Reiji Watanabe <[email protected]>
> >>> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> >>> ---
> >>> arch/arm64/kvm/sys_regs.c | 21 +--------------------
> >>> 1 file changed, 1 insertion(+), 20 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>> index 818a52e257ed..3dbb7d276b0e 100644
> >>> --- a/arch/arm64/kvm/sys_regs.c
> >>> +++ b/arch/arm64/kvm/sys_regs.c
> >>> @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> >>> return REG_HIDDEN;
> >>> }
> >>>
> >>> -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >>> -{
> >>> - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> >>> -
> >>> - /* No PMU available, any PMU reg may UNDEF... */
> >>> - if (!kvm_arm_support_pmu_v3())
> >>> - return 0;
> >>> -
> >>> - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> >>> - n &= ARMV8_PMU_PMCR_N_MASK;
> >>> - if (n)
> >>> - mask |= GENMASK(n - 1, 0);
> >>> -
> >>> - reset_unknown(vcpu, r);
> >>> - __vcpu_sys_reg(vcpu, r->reg) &= mask;
> >>> -
> >>> - return __vcpu_sys_reg(vcpu, r->reg);
> >>> -}
> >>> -
> >>> static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> >>> {
> >>> reset_unknown(vcpu, r);
> >>> @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>> trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
> >>>
> >>> #define PMU_SYS_REG(name) \
> >>> - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
> >>> + SYS_DESC(SYS_##name), .reset = reset_val, \
> >>> .visibility = pmu_visibility
> >>>
> >>> /* Macro to expand the PMEVCNTRn_EL0 register */
> >>
> >
>
On Tue, Oct 17, 2023 at 9:59 AM Raghavendra Rao Ananta
<[email protected]> wrote:
>
> Hi Eric,
> On Tue, Oct 17, 2023 at 2:23 AM Eric Auger <[email protected]> wrote:
> >
> > Hi,
> > On 10/16/23 23:28, Raghavendra Rao Ananta wrote:
> > > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <[email protected]> wrote:
> > >>
> > >> Hi Raghavendra,
> > >>
> > >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
> > >>> From: Reiji Watanabe <[email protected]>
> > >>>
> > >>> On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
> > >>> PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
> > >> PMOVS{SET,CLR}_EL0?
> > > Ah, yes. It should be PMOVS{SET,CLR}_EL0.
> > >
> > >>> This function clears RAZ bits of those registers corresponding
> > >>> to unimplemented event counters on the vCPU, and sets bits
> > >>> corresponding to implemented event counters to a predefined
> > >>> pseudo UNKNOWN value (some bits are set to 1).
> > >>>
> > >>> The function identifies (un)implemented event counters on the
> > >>> vCPU based on the PMCR_EL0.N value on the host. Using the host
> > >>> value for this would be problematic when KVM supports letting
> > >>> userspace set PMCR_EL0.N to a value different from the host value
> > >>> (some of the RAZ bits of those registers could end up being set to 1).
> > >>>
> > >>> Fix this by clearing the registers so that it can ensure
> > >>> that all the RAZ bits are cleared even when the PMCR_EL0.N value
> > >>> for the vCPU is different from the host value. Use reset_val() to
> > >>> do this instead of fixing reset_pmu_reg(), and remove
> > >>> reset_pmu_reg(), as it is no longer used.
> > >> do you intend to restore the 'unknown' behavior at some point?
> > >>
> > > I believe Reiji's (original author) intention was to keep them
> > > cleared, which would still imply an 'unknown' behavior. Do you think
> > > there's an issue with this?
> > Then why do we bother using reset_unknown in the other places if
> > clearing the bits is enough here?
> >
> Hmm. Good point. I can bring back reset_unknown to keep the original behavior.
>
I had a brief discussion about this with Oliver, and it looks like we
might need a couple of additional changes for these register accesses:
- For the userspace accesses, we have to implement explicit get_user
and set_user callbacks that to filter out the unimplemented counters
using kvm_pmu_valid_counter_mask().
- For the guest accesses to be correct, we might have to apply the
same mask while serving KVM_REQ_RELOAD_PMU.
Thank you.
Raghavendra
> Thank you.
> Raghavendra
> > Thanks
> >
> > Eric
> > >
> > > Thank you.
> > > Raghavendra
> > >> Thanks
> > >>
> > >> Eric
> > >>>
> > >>> Signed-off-by: Reiji Watanabe <[email protected]>
> > >>> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> > >>> ---
> > >>> arch/arm64/kvm/sys_regs.c | 21 +--------------------
> > >>> 1 file changed, 1 insertion(+), 20 deletions(-)
> > >>>
> > >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > >>> index 818a52e257ed..3dbb7d276b0e 100644
> > >>> --- a/arch/arm64/kvm/sys_regs.c
> > >>> +++ b/arch/arm64/kvm/sys_regs.c
> > >>> @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> > >>> return REG_HIDDEN;
> > >>> }
> > >>>
> > >>> -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > >>> -{
> > >>> - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> > >>> -
> > >>> - /* No PMU available, any PMU reg may UNDEF... */
> > >>> - if (!kvm_arm_support_pmu_v3())
> > >>> - return 0;
> > >>> -
> > >>> - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> > >>> - n &= ARMV8_PMU_PMCR_N_MASK;
> > >>> - if (n)
> > >>> - mask |= GENMASK(n - 1, 0);
> > >>> -
> > >>> - reset_unknown(vcpu, r);
> > >>> - __vcpu_sys_reg(vcpu, r->reg) &= mask;
> > >>> -
> > >>> - return __vcpu_sys_reg(vcpu, r->reg);
> > >>> -}
> > >>> -
> > >>> static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > >>> {
> > >>> reset_unknown(vcpu, r);
> > >>> @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > >>> trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
> > >>>
> > >>> #define PMU_SYS_REG(name) \
> > >>> - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
> > >>> + SYS_DESC(SYS_##name), .reset = reset_val, \
> > >>> .visibility = pmu_visibility
> > >>>
> > >>> /* Macro to expand the PMEVCNTRn_EL0 register */
> > >>
> > >
> >
On Wed, Oct 18, 2023 at 02:16:36PM -0700, Raghavendra Rao Ananta wrote:
[...]
> I had a brief discussion about this with Oliver, and it looks like we
> might need a couple of additional changes for these register accesses:
> - For the userspace accesses, we have to implement explicit get_user
> and set_user callbacks that to filter out the unimplemented counters
> using kvm_pmu_valid_counter_mask().
> - For the guest accesses to be correct, we might have to apply the
> same mask while serving KVM_REQ_RELOAD_PMU.
To be precise, the second issue is that we want to make sure KVM's PMU
emulation never uses an invalid value for the configuration, like
enabling a PMC at an index inaccessible to the guest.
--
Thanks,
Oliver
On Wed, Oct 18, 2023 at 2:16 PM Raghavendra Rao Ananta
<[email protected]> wrote:
>
> On Tue, Oct 17, 2023 at 9:59 AM Raghavendra Rao Ananta
> <[email protected]> wrote:
> >
> > Hi Eric,
> > On Tue, Oct 17, 2023 at 2:23 AM Eric Auger <[email protected]> wrote:
> > >
> > > Hi,
> > > On 10/16/23 23:28, Raghavendra Rao Ananta wrote:
> > > > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <[email protected]> wrote:
> > > >>
> > > >> Hi Raghavendra,
> > > >>
> > > >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote:
> > > >>> From: Reiji Watanabe <[email protected]>
> > > >>>
> > > >>> On vCPU reset, PMCNTEN{SET,CLR}_EL0, PMINTEN{SET,CLR}_EL1, and
> > > >>> PMOVS{SET,CLR}_EL1 for a vCPU are reset by reset_pmu_reg().
> > > >> PMOVS{SET,CLR}_EL0?
> > > > Ah, yes. It should be PMOVS{SET,CLR}_EL0.
> > > >
> > > >>> This function clears RAZ bits of those registers corresponding
> > > >>> to unimplemented event counters on the vCPU, and sets bits
> > > >>> corresponding to implemented event counters to a predefined
> > > >>> pseudo UNKNOWN value (some bits are set to 1).
> > > >>>
> > > >>> The function identifies (un)implemented event counters on the
> > > >>> vCPU based on the PMCR_EL0.N value on the host. Using the host
> > > >>> value for this would be problematic when KVM supports letting
> > > >>> userspace set PMCR_EL0.N to a value different from the host value
> > > >>> (some of the RAZ bits of those registers could end up being set to 1).
> > > >>>
> > > >>> Fix this by clearing the registers so that it can ensure
> > > >>> that all the RAZ bits are cleared even when the PMCR_EL0.N value
> > > >>> for the vCPU is different from the host value. Use reset_val() to
> > > >>> do this instead of fixing reset_pmu_reg(), and remove
> > > >>> reset_pmu_reg(), as it is no longer used.
> > > >> do you intend to restore the 'unknown' behavior at some point?
> > > >>
> > > > I believe Reiji's (original author) intention was to keep them
> > > > cleared, which would still imply an 'unknown' behavior. Do you think
> > > > there's an issue with this?
> > > Then why do we bother using reset_unknown in the other places if
> > > clearing the bits is enough here?
> > >
> > Hmm. Good point. I can bring back reset_unknown to keep the original behavior.
> >
> I had a brief discussion about this with Oliver, and it looks like we
> might need a couple of additional changes for these register accesses:
> - For the userspace accesses, we have to implement explicit get_user
> and set_user callbacks that to filter out the unimplemented counters
> using kvm_pmu_valid_counter_mask().
Re-thinking the first case: Since these registers go through a reset
(reset_pmu_reg()) during initialization, where the valid counter mask
is applied, and since we are sanitizing the registers with the mask
before running the guest (below case), will implementing the
{get,set}_user() add any value, apart from just keeping userspace in
sync with every update of PMCR.N?
> - For the guest accesses to be correct, we might have to apply the
> same mask while serving KVM_REQ_RELOAD_PMU.
>
> Thank you.
> Raghavendra
>
> > Thank you.
> > Raghavendra
> > > Thanks
> > >
> > > Eric
> > > >
> > > > Thank you.
> > > > Raghavendra
> > > >> Thanks
> > > >>
> > > >> Eric
> > > >>>
> > > >>> Signed-off-by: Reiji Watanabe <[email protected]>
> > > >>> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> > > >>> ---
> > > >>> arch/arm64/kvm/sys_regs.c | 21 +--------------------
> > > >>> 1 file changed, 1 insertion(+), 20 deletions(-)
> > > >>>
> > > >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > >>> index 818a52e257ed..3dbb7d276b0e 100644
> > > >>> --- a/arch/arm64/kvm/sys_regs.c
> > > >>> +++ b/arch/arm64/kvm/sys_regs.c
> > > >>> @@ -717,25 +717,6 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
> > > >>> return REG_HIDDEN;
> > > >>> }
> > > >>>
> > > >>> -static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > >>> -{
> > > >>> - u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
> > > >>> -
> > > >>> - /* No PMU available, any PMU reg may UNDEF... */
> > > >>> - if (!kvm_arm_support_pmu_v3())
> > > >>> - return 0;
> > > >>> -
> > > >>> - n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
> > > >>> - n &= ARMV8_PMU_PMCR_N_MASK;
> > > >>> - if (n)
> > > >>> - mask |= GENMASK(n - 1, 0);
> > > >>> -
> > > >>> - reset_unknown(vcpu, r);
> > > >>> - __vcpu_sys_reg(vcpu, r->reg) &= mask;
> > > >>> -
> > > >>> - return __vcpu_sys_reg(vcpu, r->reg);
> > > >>> -}
> > > >>> -
> > > >>> static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > >>> {
> > > >>> reset_unknown(vcpu, r);
> > > >>> @@ -1115,7 +1096,7 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > > >>> trap_wcr, reset_wcr, 0, 0, get_wcr, set_wcr }
> > > >>>
> > > >>> #define PMU_SYS_REG(name) \
> > > >>> - SYS_DESC(SYS_##name), .reset = reset_pmu_reg, \
> > > >>> + SYS_DESC(SYS_##name), .reset = reset_val, \
> > > >>> .visibility = pmu_visibility
> > > >>>
> > > >>> /* Macro to expand the PMEVCNTRn_EL0 register */
> > > >>
> > > >
> > >
Hi Raghu,
Can you please make sure you include leading and trailing whitespace for
your inline replies? The message gets extremely dense and is difficult
to read.
Also -- delete any unrelated context from your replies. If there's a
localized conversation about a particular detail there's no reason to
keep the entire thread in the body.
On Thu, Oct 19, 2023 at 11:46:22AM -0700, Raghavendra Rao Ananta wrote:
> On Wed, Oct 18, 2023 at 2:16 PM Raghavendra Rao Ananta
> <[email protected]> wrote:
> > I had a brief discussion about this with Oliver, and it looks like we
> > might need a couple of additional changes for these register accesses:
> > - For the userspace accesses, we have to implement explicit get_user
> > and set_user callbacks that to filter out the unimplemented counters
> > using kvm_pmu_valid_counter_mask().
> Re-thinking the first case: Since these registers go through a reset
> (reset_pmu_reg()) during initialization, where the valid counter mask
> is applied, and since we are sanitizing the registers with the mask
> before running the guest (below case), will implementing the
> {get,set}_user() add any value, apart from just keeping userspace in
> sync with every update of PMCR.N?
KVM's sysreg emulation (as seen from userspace) fails to uphold the RES0
bits of these registers. That's a bug.
--
Thanks,
Oliver
On Thu, Oct 19, 2023 at 12:06 PM Oliver Upton <[email protected]> wrote:
>
> Hi Raghu,
>
> Can you please make sure you include leading and trailing whitespace for
> your inline replies? The message gets extremely dense and is difficult
> to read.
>
> Also -- delete any unrelated context from your replies. If there's a
> localized conversation about a particular detail there's no reason to
> keep the entire thread in the body.
>
Sorry about that. I'll try to keep it clean.
> On Thu, Oct 19, 2023 at 11:46:22AM -0700, Raghavendra Rao Ananta wrote:
> > On Wed, Oct 18, 2023 at 2:16 PM Raghavendra Rao Ananta
> > <[email protected]> wrote:
> > > I had a brief discussion about this with Oliver, and it looks like we
> > > might need a couple of additional changes for these register accesses:
> > > - For the userspace accesses, we have to implement explicit get_user
> > > and set_user callbacks that to filter out the unimplemented counters
> > > using kvm_pmu_valid_counter_mask().
> > Re-thinking the first case: Since these registers go through a reset
> > (reset_pmu_reg()) during initialization, where the valid counter mask
> > is applied, and since we are sanitizing the registers with the mask
> > before running the guest (below case), will implementing the
> > {get,set}_user() add any value, apart from just keeping userspace in
> > sync with every update of PMCR.N?
>
> KVM's sysreg emulation (as seen from userspace) fails to uphold the RES0
> bits of these registers. That's a bug.
>
Got it. Thanks for the confirmation. I'll implement these as originally planned.
Thank you.
Raghavendra
> --
> Thanks,
> Oliver