| Message ID | 20231009230858.3444834-4-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PMU: Allow userspace to limit the number of PMCs on vCPU | expand |
Hi Raghavendra, On 10/10/23 01:08, Raghavendra Rao Ananta wrote: > From: Reiji Watanabe <reijiw@google.com> > > 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 <reijiw@google.com> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > 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 <eauger@redhat.com> wrote: > > Hi Raghavendra, > > On 10/10/23 01:08, Raghavendra Rao Ananta wrote: > > From: Reiji Watanabe <reijiw@google.com> > > > > 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 <reijiw@google.com> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > --- > > 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 <eauger@redhat.com> wrote: >> >> Hi Raghavendra, >> >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote: >>> From: Reiji Watanabe <reijiw@google.com> >>> >>> 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 <reijiw@google.com> >>> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> >>> --- >>> 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 <eauger@redhat.com> wrote: > > Hi, > On 10/16/23 23:28, Raghavendra Rao Ananta wrote: > > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <eauger@redhat.com> wrote: > >> > >> Hi Raghavendra, > >> > >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote: > >>> From: Reiji Watanabe <reijiw@google.com> > >>> > >>> 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 <reijiw@google.com> > >>> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > >>> --- > >>> 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 <rananta@google.com> wrote: > > Hi Eric, > On Tue, Oct 17, 2023 at 2:23 AM Eric Auger <eauger@redhat.com> wrote: > > > > Hi, > > On 10/16/23 23:28, Raghavendra Rao Ananta wrote: > > > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <eauger@redhat.com> wrote: > > >> > > >> Hi Raghavendra, > > >> > > >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote: > > >>> From: Reiji Watanabe <reijiw@google.com> > > >>> > > >>> 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 <reijiw@google.com> > > >>> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > >>> --- > > >>> 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.
On Wed, Oct 18, 2023 at 2:16 PM Raghavendra Rao Ananta <rananta@google.com> wrote: > > On Tue, Oct 17, 2023 at 9:59 AM Raghavendra Rao Ananta > <rananta@google.com> wrote: > > > > Hi Eric, > > On Tue, Oct 17, 2023 at 2:23 AM Eric Auger <eauger@redhat.com> wrote: > > > > > > Hi, > > > On 10/16/23 23:28, Raghavendra Rao Ananta wrote: > > > > On Mon, Oct 16, 2023 at 12:45 PM Eric Auger <eauger@redhat.com> wrote: > > > >> > > > >> Hi Raghavendra, > > > >> > > > >> On 10/10/23 01:08, Raghavendra Rao Ananta wrote: > > > >>> From: Reiji Watanabe <reijiw@google.com> > > > >>> > > > >>> 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 <reijiw@google.com> > > > >>> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > > >>> --- > > > >>> 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 > <rananta@google.com> 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.
On Thu, Oct 19, 2023 at 12:06 PM Oliver Upton <oliver.upton@linux.dev> 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 > > <rananta@google.com> 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
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 */