diff mbox series

[v3,11/14] RISC-V: KVM: Implement trap & emulate for hpmcounters

Message ID 20230127182558.2416400-12-atishp@rivosinc.com (mailing list archive)
State Superseded
Headers show
Series KVM perf support | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 13 and now 13
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 1 this patch: 1
conchuod/alphanumeric_selects success Out of order selects before the patch: 57 and now 57
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 2 this patch: 2
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch fail CHECK: Please use a blank line after function/struct/union/enum declarations ERROR: Macros with complex values should be enclosed in parentheses
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Atish Kumar Patra Jan. 27, 2023, 6:25 p.m. UTC 
As the KVM guests only see the virtual PMU counters, all hpmcounter
access should trap and KVM emulates the read access on behalf of guests.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/include/asm/kvm_vcpu_pmu.h | 16 ++++++++++
 arch/riscv/kvm/vcpu_insn.c            |  4 ++-
 arch/riscv/kvm/vcpu_pmu.c             | 45 ++++++++++++++++++++++++++-
 3 files changed, 63 insertions(+), 2 deletions(-)

Comments

Anup Patel Jan. 29, 2023, 12:44 p.m. UTC | #1 
On Fri, Jan 27, 2023 at 11:56 PM Atish Patra <atishp@rivosinc.com> wrote:
>
> As the KVM guests only see the virtual PMU counters, all hpmcounter
> access should trap and KVM emulates the read access on behalf of guests.
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_pmu.h | 16 ++++++++++
>  arch/riscv/kvm/vcpu_insn.c            |  4 ++-
>  arch/riscv/kvm/vcpu_pmu.c             | 45 ++++++++++++++++++++++++++-
>  3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> index 3f43a43..022d45d 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> @@ -43,6 +43,19 @@ struct kvm_pmu {
>  #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
>  #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
>
> +#if defined(CONFIG_32BIT)
> +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> +{ .base = CSR_CYCLEH,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm }, \
> +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> +#else
> +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> +#endif
> +
> +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> +                               unsigned long *val, unsigned long new_val,
> +                               unsigned long wr_mask);
> +
>  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata);
>  int kvm_riscv_vcpu_pmu_ctr_info(struct kvm_vcpu *vcpu, unsigned long cidx,
>                                 struct kvm_vcpu_sbi_ext_data *edata);
> @@ -65,6 +78,9 @@ void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
>  #else
>  struct kvm_pmu {
>  };
> +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> +{ .base = 0,      .count = 0, .func = NULL },
> +

Redundant newline here.

>
>  static inline int kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu)
>  {
> diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> index 0bb5276..f689337 100644
> --- a/arch/riscv/kvm/vcpu_insn.c
> +++ b/arch/riscv/kvm/vcpu_insn.c
> @@ -213,7 +213,9 @@ struct csr_func {
>                     unsigned long wr_mask);
>  };
>
> -static const struct csr_func csr_funcs[] = { };
> +static const struct csr_func csr_funcs[] = {
> +       KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
> +};
>
>  /**
>   * kvm_riscv_vcpu_csr_return -- Handle CSR read/write after user space
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 7713927..894053a 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -17,6 +17,44 @@
>
>  #define kvm_pmu_num_counters(pmu) ((pmu)->num_hw_ctrs + (pmu)->num_fw_ctrs)
>
> +static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> +                       unsigned long *out_val)
> +{
> +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> +       struct kvm_pmc *pmc;
> +       u64 enabled, running;
> +
> +       pmc = &kvpmu->pmc[cidx];
> +       if (!pmc->perf_event)
> +               return -EINVAL;
> +
> +       pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> +       *out_val = pmc->counter_val;
> +
> +       return 0;
> +}
> +
> +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> +                               unsigned long *val, unsigned long new_val,
> +                               unsigned long wr_mask)
> +{
> +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> +       int cidx, ret = KVM_INSN_CONTINUE_NEXT_SEPC;
> +
> +       if (!kvpmu || !kvpmu->init_done)
> +               return KVM_INSN_EXIT_TO_USER_SPACE;

As discussed previously, this should be KVM_INSN_ILLEGAL_TRAP.

> +
> +       if (wr_mask)
> +               return KVM_INSN_ILLEGAL_TRAP;
> +
> +       cidx = csr_num - CSR_CYCLE;
> +
> +       if (pmu_ctr_read(vcpu, cidx, val) < 0)
> +               return KVM_INSN_EXIT_TO_USER_SPACE;

Same as above.

> +
> +       return ret;
> +}
> +
>  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata)
>  {
>         struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> @@ -69,7 +107,12 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
>  int kvm_riscv_vcpu_pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
>                                 struct kvm_vcpu_sbi_ext_data *edata)
>  {
> -       /* TODO */
> +       int ret;
> +
> +       ret = pmu_ctr_read(vcpu, cidx, &edata->out_val);
> +       if (ret == -EINVAL)
> +               edata->err_val = SBI_ERR_INVALID_PARAM;
> +
>         return 0;
>  }
>
> --
> 2.25.1
>

Regards,
Anup
Atish Patra Jan. 31, 2023, 10:46 p.m. UTC | #2 
On Sun, Jan 29, 2023 at 4:44 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Fri, Jan 27, 2023 at 11:56 PM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > As the KVM guests only see the virtual PMU counters, all hpmcounter
> > access should trap and KVM emulates the read access on behalf of guests.
> >
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_pmu.h | 16 ++++++++++
> >  arch/riscv/kvm/vcpu_insn.c            |  4 ++-
> >  arch/riscv/kvm/vcpu_pmu.c             | 45 ++++++++++++++++++++++++++-
> >  3 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > index 3f43a43..022d45d 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > @@ -43,6 +43,19 @@ struct kvm_pmu {
> >  #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
> >  #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> >
> > +#if defined(CONFIG_32BIT)
> > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > +{ .base = CSR_CYCLEH,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm }, \
> > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > +#else
> > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > +#endif
> > +
> > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > +                               unsigned long *val, unsigned long new_val,
> > +                               unsigned long wr_mask);
> > +
> >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata);
> >  int kvm_riscv_vcpu_pmu_ctr_info(struct kvm_vcpu *vcpu, unsigned long cidx,
> >                                 struct kvm_vcpu_sbi_ext_data *edata);
> > @@ -65,6 +78,9 @@ void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
> >  #else
> >  struct kvm_pmu {
> >  };
> > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > +{ .base = 0,      .count = 0, .func = NULL },
> > +
>
> Redundant newline here.
>

Fixed.

> >
> >  static inline int kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu)
> >  {
> > diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> > index 0bb5276..f689337 100644
> > --- a/arch/riscv/kvm/vcpu_insn.c
> > +++ b/arch/riscv/kvm/vcpu_insn.c
> > @@ -213,7 +213,9 @@ struct csr_func {
> >                     unsigned long wr_mask);
> >  };
> >
> > -static const struct csr_func csr_funcs[] = { };
> > +static const struct csr_func csr_funcs[] = {
> > +       KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
> > +};
> >
> >  /**
> >   * kvm_riscv_vcpu_csr_return -- Handle CSR read/write after user space
> > diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> > index 7713927..894053a 100644
> > --- a/arch/riscv/kvm/vcpu_pmu.c
> > +++ b/arch/riscv/kvm/vcpu_pmu.c
> > @@ -17,6 +17,44 @@
> >
> >  #define kvm_pmu_num_counters(pmu) ((pmu)->num_hw_ctrs + (pmu)->num_fw_ctrs)
> >
> > +static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> > +                       unsigned long *out_val)
> > +{
> > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > +       struct kvm_pmc *pmc;
> > +       u64 enabled, running;
> > +
> > +       pmc = &kvpmu->pmc[cidx];
> > +       if (!pmc->perf_event)
> > +               return -EINVAL;
> > +
> > +       pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> > +       *out_val = pmc->counter_val;
> > +
> > +       return 0;
> > +}
> > +
> > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > +                               unsigned long *val, unsigned long new_val,
> > +                               unsigned long wr_mask)
> > +{
> > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > +       int cidx, ret = KVM_INSN_CONTINUE_NEXT_SEPC;
> > +
> > +       if (!kvpmu || !kvpmu->init_done)
> > +               return KVM_INSN_EXIT_TO_USER_SPACE;
>
> As discussed previously, this should be KVM_INSN_ILLEGAL_TRAP.
>

Done.
> > +
> > +       if (wr_mask)
> > +               return KVM_INSN_ILLEGAL_TRAP;
> > +
> > +       cidx = csr_num - CSR_CYCLE;
> > +
> > +       if (pmu_ctr_read(vcpu, cidx, val) < 0)
> > +               return KVM_INSN_EXIT_TO_USER_SPACE;
>
> Same as above.
>

Done.

> > +
> > +       return ret;
> > +}
> > +
> >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata)
> >  {
> >         struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > @@ -69,7 +107,12 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
> >  int kvm_riscv_vcpu_pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> >                                 struct kvm_vcpu_sbi_ext_data *edata)
> >  {
> > -       /* TODO */
> > +       int ret;
> > +
> > +       ret = pmu_ctr_read(vcpu, cidx, &edata->out_val);
> > +       if (ret == -EINVAL)
> > +               edata->err_val = SBI_ERR_INVALID_PARAM;
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.25.1
> >
>
> Regards,
> Anup
Atish Patra Feb. 1, 2023, 8:58 a.m. UTC | #3 
On Tue, Jan 31, 2023 at 2:46 PM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Sun, Jan 29, 2023 at 4:44 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Jan 27, 2023 at 11:56 PM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > As the KVM guests only see the virtual PMU counters, all hpmcounter
> > > access should trap and KVM emulates the read access on behalf of guests.
> > >
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_pmu.h | 16 ++++++++++
> > >  arch/riscv/kvm/vcpu_insn.c            |  4 ++-
> > >  arch/riscv/kvm/vcpu_pmu.c             | 45 ++++++++++++++++++++++++++-
> > >  3 files changed, 63 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > index 3f43a43..022d45d 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > @@ -43,6 +43,19 @@ struct kvm_pmu {
> > >  #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
> > >  #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> > >
> > > +#if defined(CONFIG_32BIT)
> > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > +{ .base = CSR_CYCLEH,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm }, \
> > > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > > +#else
> > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > > +#endif
> > > +
> > > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > > +                               unsigned long *val, unsigned long new_val,
> > > +                               unsigned long wr_mask);
> > > +
> > >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata);
> > >  int kvm_riscv_vcpu_pmu_ctr_info(struct kvm_vcpu *vcpu, unsigned long cidx,
> > >                                 struct kvm_vcpu_sbi_ext_data *edata);
> > > @@ -65,6 +78,9 @@ void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
> > >  #else
> > >  struct kvm_pmu {
> > >  };
> > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > +{ .base = 0,      .count = 0, .func = NULL },
> > > +
> >
> > Redundant newline here.
> >
>
> Fixed.
>
> > >
> > >  static inline int kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu)
> > >  {
> > > diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> > > index 0bb5276..f689337 100644
> > > --- a/arch/riscv/kvm/vcpu_insn.c
> > > +++ b/arch/riscv/kvm/vcpu_insn.c
> > > @@ -213,7 +213,9 @@ struct csr_func {
> > >                     unsigned long wr_mask);
> > >  };
> > >
> > > -static const struct csr_func csr_funcs[] = { };
> > > +static const struct csr_func csr_funcs[] = {
> > > +       KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
> > > +};
> > >
> > >  /**
> > >   * kvm_riscv_vcpu_csr_return -- Handle CSR read/write after user space
> > > diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> > > index 7713927..894053a 100644
> > > --- a/arch/riscv/kvm/vcpu_pmu.c
> > > +++ b/arch/riscv/kvm/vcpu_pmu.c
> > > @@ -17,6 +17,44 @@
> > >
> > >  #define kvm_pmu_num_counters(pmu) ((pmu)->num_hw_ctrs + (pmu)->num_fw_ctrs)
> > >
> > > +static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> > > +                       unsigned long *out_val)
> > > +{
> > > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > +       struct kvm_pmc *pmc;
> > > +       u64 enabled, running;
> > > +
> > > +       pmc = &kvpmu->pmc[cidx];
> > > +       if (!pmc->perf_event)
> > > +               return -EINVAL;
> > > +
> > > +       pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> > > +       *out_val = pmc->counter_val;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > > +                               unsigned long *val, unsigned long new_val,
> > > +                               unsigned long wr_mask)
> > > +{
> > > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > +       int cidx, ret = KVM_INSN_CONTINUE_NEXT_SEPC;
> > > +
> > > +       if (!kvpmu || !kvpmu->init_done)
> > > +               return KVM_INSN_EXIT_TO_USER_SPACE;
> >
> > As discussed previously, this should be KVM_INSN_ILLEGAL_TRAP.
> >

Thinking about it more, this results in a panic in guest S-mode which
is probably undesirable.
As per your earlier suggestion, we can return 0 for cycle/instret
counters if accessed.
This is only possible through legacy pmu drivers running in guests or
some other OS that access any hpmcounters
for random reasons.

I think we should return KVM_INSN_ILLEGAL_TRAP for other counters and
make the guest kernel panic.
This does separate the behavior between fixed and programmable
counters when everything is denied access in hcounteren.

The new code will look like this:

if (!kvpmu || !kvpmu->init_done) {
    if (csr_num == CSR_CYCLE || csr_num == CSR_INSTRET) {
        *val = 0;
        return ret;
    } else
         return KVM_INSN_ILLEGAL_TRAP;
}

Let me know if you think otherwise.

>
> Done.
> > > +
> > > +       if (wr_mask)
> > > +               return KVM_INSN_ILLEGAL_TRAP;
> > > +
> > > +       cidx = csr_num - CSR_CYCLE;
> > > +
> > > +       if (pmu_ctr_read(vcpu, cidx, val) < 0)
> > > +               return KVM_INSN_EXIT_TO_USER_SPACE;
> >
> > Same as above.
> >

We can get rid of this as pmu_ctr_read doesn't return errors anyways.

>
> Done.
>
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata)
> > >  {
> > >         struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > @@ -69,7 +107,12 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
> > >  int kvm_riscv_vcpu_pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> > >                                 struct kvm_vcpu_sbi_ext_data *edata)
> > >  {
> > > -       /* TODO */
> > > +       int ret;
> > > +
> > > +       ret = pmu_ctr_read(vcpu, cidx, &edata->out_val);
> > > +       if (ret == -EINVAL)
> > > +               edata->err_val = SBI_ERR_INVALID_PARAM;
> > > +
> > >         return 0;
> > >  }
> > >
> > > --
> > > 2.25.1
> > >
> >
> > Regards,
> > Anup
>
>
>
> --
> Regards,
> Atish
Anup Patel Feb. 1, 2023, 9:09 a.m. UTC | #4 
On Wed, Feb 1, 2023 at 2:29 PM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Tue, Jan 31, 2023 at 2:46 PM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Sun, Jan 29, 2023 at 4:44 AM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Fri, Jan 27, 2023 at 11:56 PM Atish Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > As the KVM guests only see the virtual PMU counters, all hpmcounter
> > > > access should trap and KVM emulates the read access on behalf of guests.
> > > >
> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > >  arch/riscv/include/asm/kvm_vcpu_pmu.h | 16 ++++++++++
> > > >  arch/riscv/kvm/vcpu_insn.c            |  4 ++-
> > > >  arch/riscv/kvm/vcpu_pmu.c             | 45 ++++++++++++++++++++++++++-
> > > >  3 files changed, 63 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > > index 3f43a43..022d45d 100644
> > > > --- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > > +++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
> > > > @@ -43,6 +43,19 @@ struct kvm_pmu {
> > > >  #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
> > > >  #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> > > >
> > > > +#if defined(CONFIG_32BIT)
> > > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > > +{ .base = CSR_CYCLEH,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm }, \
> > > > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > > > +#else
> > > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > > +{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
> > > > +#endif
> > > > +
> > > > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > > > +                               unsigned long *val, unsigned long new_val,
> > > > +                               unsigned long wr_mask);
> > > > +
> > > >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata);
> > > >  int kvm_riscv_vcpu_pmu_ctr_info(struct kvm_vcpu *vcpu, unsigned long cidx,
> > > >                                 struct kvm_vcpu_sbi_ext_data *edata);
> > > > @@ -65,6 +78,9 @@ void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
> > > >  #else
> > > >  struct kvm_pmu {
> > > >  };
> > > > +#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
> > > > +{ .base = 0,      .count = 0, .func = NULL },
> > > > +
> > >
> > > Redundant newline here.
> > >
> >
> > Fixed.
> >
> > > >
> > > >  static inline int kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu)
> > > >  {
> > > > diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> > > > index 0bb5276..f689337 100644
> > > > --- a/arch/riscv/kvm/vcpu_insn.c
> > > > +++ b/arch/riscv/kvm/vcpu_insn.c
> > > > @@ -213,7 +213,9 @@ struct csr_func {
> > > >                     unsigned long wr_mask);
> > > >  };
> > > >
> > > > -static const struct csr_func csr_funcs[] = { };
> > > > +static const struct csr_func csr_funcs[] = {
> > > > +       KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
> > > > +};
> > > >
> > > >  /**
> > > >   * kvm_riscv_vcpu_csr_return -- Handle CSR read/write after user space
> > > > diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> > > > index 7713927..894053a 100644
> > > > --- a/arch/riscv/kvm/vcpu_pmu.c
> > > > +++ b/arch/riscv/kvm/vcpu_pmu.c
> > > > @@ -17,6 +17,44 @@
> > > >
> > > >  #define kvm_pmu_num_counters(pmu) ((pmu)->num_hw_ctrs + (pmu)->num_fw_ctrs)
> > > >
> > > > +static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> > > > +                       unsigned long *out_val)
> > > > +{
> > > > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > > +       struct kvm_pmc *pmc;
> > > > +       u64 enabled, running;
> > > > +
> > > > +       pmc = &kvpmu->pmc[cidx];
> > > > +       if (!pmc->perf_event)
> > > > +               return -EINVAL;
> > > > +
> > > > +       pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
> > > > +       *out_val = pmc->counter_val;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
> > > > +                               unsigned long *val, unsigned long new_val,
> > > > +                               unsigned long wr_mask)
> > > > +{
> > > > +       struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > > +       int cidx, ret = KVM_INSN_CONTINUE_NEXT_SEPC;
> > > > +
> > > > +       if (!kvpmu || !kvpmu->init_done)
> > > > +               return KVM_INSN_EXIT_TO_USER_SPACE;
> > >
> > > As discussed previously, this should be KVM_INSN_ILLEGAL_TRAP.
> > >
>
> Thinking about it more, this results in a panic in guest S-mode which
> is probably undesirable.
> As per your earlier suggestion, we can return 0 for cycle/instret
> counters if accessed.
> This is only possible through legacy pmu drivers running in guests or
> some other OS that access any hpmcounters
> for random reasons.
>
> I think we should return KVM_INSN_ILLEGAL_TRAP for other counters and
> make the guest kernel panic.
> This does separate the behavior between fixed and programmable
> counters when everything is denied access in hcounteren.
>
> The new code will look like this:
>
> if (!kvpmu || !kvpmu->init_done) {
>     if (csr_num == CSR_CYCLE || csr_num == CSR_INSTRET) {
>         *val = 0;
>         return ret;
>     } else
>          return KVM_INSN_ILLEGAL_TRAP;
> }
>
> Let me know if you think otherwise.

Looks good to me. Please also add comment block inside
"if (!kvpmu || !kvpmu->init_done)"

>
> >
> > Done.
> > > > +
> > > > +       if (wr_mask)
> > > > +               return KVM_INSN_ILLEGAL_TRAP;
> > > > +
> > > > +       cidx = csr_num - CSR_CYCLE;
> > > > +
> > > > +       if (pmu_ctr_read(vcpu, cidx, val) < 0)
> > > > +               return KVM_INSN_EXIT_TO_USER_SPACE;
> > >
> > > Same as above.
> > >
>
> We can get rid of this as pmu_ctr_read doesn't return errors anyways.
>
> >
> > Done.
> >
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > >  int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata)
> > > >  {
> > > >         struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
> > > > @@ -69,7 +107,12 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
> > > >  int kvm_riscv_vcpu_pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
> > > >                                 struct kvm_vcpu_sbi_ext_data *edata)
> > > >  {
> > > > -       /* TODO */
> > > > +       int ret;
> > > > +
> > > > +       ret = pmu_ctr_read(vcpu, cidx, &edata->out_val);
> > > > +       if (ret == -EINVAL)
> > > > +               edata->err_val = SBI_ERR_INVALID_PARAM;
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > Regards,
> > > Anup
> >
> >
> >
> > --
> > Regards,
> > Atish
>
>
>
> --
> Regards,
> Atish

Regards,
Anup
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/kvm_vcpu_pmu.h b/arch/riscv/include/asm/kvm_vcpu_pmu.h
index 3f43a43..022d45d 100644
--- a/arch/riscv/include/asm/kvm_vcpu_pmu.h
+++ b/arch/riscv/include/asm/kvm_vcpu_pmu.h
@@ -43,6 +43,19 @@  struct kvm_pmu {
 #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
 #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
 
+#if defined(CONFIG_32BIT)
+#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
+{ .base = CSR_CYCLEH,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm }, \
+{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
+#else
+#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
+{ .base = CSR_CYCLE,      .count = 31, .func = kvm_riscv_vcpu_pmu_read_hpm },
+#endif
+
+int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
+				unsigned long *val, unsigned long new_val,
+				unsigned long wr_mask);
+
 int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata);
 int kvm_riscv_vcpu_pmu_ctr_info(struct kvm_vcpu *vcpu, unsigned long cidx,
 				struct kvm_vcpu_sbi_ext_data *edata);
@@ -65,6 +78,9 @@  void kvm_riscv_vcpu_pmu_reset(struct kvm_vcpu *vcpu);
 #else
 struct kvm_pmu {
 };
+#define KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS \
+{ .base = 0,      .count = 0, .func = NULL },
+
 
 static inline int kvm_riscv_vcpu_pmu_init(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index 0bb5276..f689337 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -213,7 +213,9 @@  struct csr_func {
 		    unsigned long wr_mask);
 };
 
-static const struct csr_func csr_funcs[] = { };
+static const struct csr_func csr_funcs[] = {
+	KVM_RISCV_VCPU_HPMCOUNTER_CSR_FUNCS
+};
 
 /**
  * kvm_riscv_vcpu_csr_return -- Handle CSR read/write after user space
diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index 7713927..894053a 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -17,6 +17,44 @@ 
 
 #define kvm_pmu_num_counters(pmu) ((pmu)->num_hw_ctrs + (pmu)->num_fw_ctrs)
 
+static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
+			unsigned long *out_val)
+{
+	struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
+	struct kvm_pmc *pmc;
+	u64 enabled, running;
+
+	pmc = &kvpmu->pmc[cidx];
+	if (!pmc->perf_event)
+		return -EINVAL;
+
+	pmc->counter_val += perf_event_read_value(pmc->perf_event, &enabled, &running);
+	*out_val = pmc->counter_val;
+
+	return 0;
+}
+
+int kvm_riscv_vcpu_pmu_read_hpm(struct kvm_vcpu *vcpu, unsigned int csr_num,
+				unsigned long *val, unsigned long new_val,
+				unsigned long wr_mask)
+{
+	struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
+	int cidx, ret = KVM_INSN_CONTINUE_NEXT_SEPC;
+
+	if (!kvpmu || !kvpmu->init_done)
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+
+	if (wr_mask)
+		return KVM_INSN_ILLEGAL_TRAP;
+
+	cidx = csr_num - CSR_CYCLE;
+
+	if (pmu_ctr_read(vcpu, cidx, val) < 0)
+		return KVM_INSN_EXIT_TO_USER_SPACE;
+
+	return ret;
+}
+
 int kvm_riscv_vcpu_pmu_num_ctrs(struct kvm_vcpu *vcpu, struct kvm_vcpu_sbi_ext_data *edata)
 {
 	struct kvm_pmu *kvpmu = vcpu_to_pmu(vcpu);
@@ -69,7 +107,12 @@  int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
 int kvm_riscv_vcpu_pmu_ctr_read(struct kvm_vcpu *vcpu, unsigned long cidx,
 				struct kvm_vcpu_sbi_ext_data *edata)
 {
-	/* TODO */
+	int ret;
+
+	ret = pmu_ctr_read(vcpu, cidx, &edata->out_val);
+	if (ret == -EINVAL)
+		edata->err_val = SBI_ERR_INVALID_PARAM;
+
 	return 0;
 }