Date:   Thu, 2 Feb 2023 11:40:58 +0000
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Atish Patra <atishp@rivosinc.com>
CC:     <linux-kernel@vger.kernel.org>, Anup Patel <anup@brainfault.org>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Andrew Jones <ajones@ventanamicro.com>,
        Atish Patra <atishp@atishpatra.org>,
        Eric Lin <eric.lin@sifive.com>, Guo Ren <guoren@kernel.org>,
        Heiko Stuebner <heiko@sntech.de>,
        <kvm-riscv@lists.infradead.org>, <kvm@vger.kernel.org>,
        <linux-riscv@lists.infradead.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH v4 13/14] RISC-V: KVM: Support firmware events
Message-ID: <Y9uhSoOvodeQRO6G@wendy>
References: <20230201231250.3806412-1-atishp@rivosinc.com>
 <20230201231250.3806412-14-atishp@rivosinc.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="tlNBWfimPX/S/xVT"
Content-Disposition: inline
In-Reply-To: <20230201231250.3806412-14-atishp@rivosinc.com>
Precedence: bulk

--tlNBWfimPX/S/xVT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey Atish,

On Wed, Feb 01, 2023 at 03:12:49PM -0800, Atish Patra wrote:
> SBI PMU extension defines a set of firmware events which can provide
> useful information to guests about the number of SBI calls. As
> hypervisor implements the SBI PMU extension, these firmware events
> correspond to ecall invocations between VS->HS mode. All other firmware
> events will always report zero if monitored as KVM doesn't implement them.
>=20
> This patch adds all the infrastructure required to support firmware
> events.
>=20
> Reviewed-by: Anup Patel <anup@brainfault.org>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 473ad80..dd16e60 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -202,12 +202,15 @@ static int pmu_ctr_read(struct kvm_vcpu *vcpu, unsi=
gned long cidx,
>  	struct kvm_pmu *kvpmu =3D vcpu_to_pmu(vcpu);
>  	struct kvm_pmc *pmc;
>  	u64 enabled, running;
> +	int fevent_code;
> =20
>  	pmc =3D &kvpmu->pmc[cidx];
> -	if (!pmc->perf_event)
> -		return -EINVAL;
> =20
> -	pmc->counter_val +=3D perf_event_read_value(pmc->perf_event, &enabled, =
&running);
> +	if (pmc->cinfo.type =3D=3D SBI_PMU_CTR_TYPE_FW) {
> +		fevent_code =3D get_event_code(pmc->event_idx);
> +		pmc->counter_val =3D kvpmu->fw_event[fevent_code].value;
> +	} else if (pmc->perf_event)
> +		pmc->counter_val +=3D perf_event_read_value(pmc->perf_event, &enabled,=
 &running);

Here, and elsewhere, all branches of an if/else must use {} if one
branch needs them.
Patches 4 & 12 have similar issues, which checkpatch in the patchwork
CI stuff also complained about.

Thanks,
Conor.


--tlNBWfimPX/S/xVT
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY9uhSQAKCRB4tDGHoIJi
0uqpAP9x4VGfcJGp96G2q7mYM1WaLkLQmijd3P41i9ItrDKGswD/ZmRRoH9XvuNl
IoucvfrlrOXTic9oOkpunlmQVDzl6Aw=
=V4EF
-----END PGP SIGNATURE-----

--tlNBWfimPX/S/xVT--