From: Marc Zyngier <maz@kernel.org>
To: Sebastian Ott <sebott@redhat.com>
Cc: linux-arm-kernel@lists.infradead.org,
Oliver Upton <oliver.upton@linux.dev>,
James Morse <james.morse@arm.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
kvmarm <kvmarm@lists.linux.dev>
Subject: Re: [PATCH 1/4] KVM: arm64: add emulation for CTR_EL0 register
Date: Mon, 18 Mar 2024 11:45:14 +0000 [thread overview]
Message-ID: <8634sn227p.wl-maz@kernel.org> (raw)
In-Reply-To: <20240318111636.10613-2-sebott@redhat.com>
Please add all the reviewers and the relevant mailing lists.
On Mon, 18 Mar 2024 11:16:33 +0000,
Sebastian Ott <sebott@redhat.com> wrote:
>
> CTR_EL0 is currently handled as an invariant register, thus
> guests will be presented with the host value of that register.
> Add emulation for CTR_EL0 and maintain a per vcpu value. The
> only thing that is allowed to be changed compared to the host
> value is to switch off the DIC bit which describes Icache
> invalidation requirements.
>
> Signed-off-by: Sebastian Ott <sebott@redhat.com>
> ---
> arch/arm64/kvm/sys_regs.c | 44 +++++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..b2019faa9d73 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1871,10 +1871,42 @@ static bool access_ctr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> if (p->is_write)
> return write_to_read_only(vcpu, p, r);
>
> - p->regval = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + p->regval = __vcpu_sys_reg(vcpu, r->reg);
> return true;
> }
>
> +static u64 reset_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
> +{
> + u64 val = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> + __vcpu_sys_reg(vcpu, rd->reg) = val;
> + return __vcpu_sys_reg(vcpu, rd->reg);
I really don't think we should make this a per-CPU value, and instead
keep it a VM-wide value, just like any other ID register.
Also, rd->reg isn't set to anything useful, leading to memory
corruption (hint, there is no CTR_EL0 in the vcpu sysreg file).
> +}
> +
> +static int get_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> + u64 *val)
> +{
> + *val = __vcpu_sys_reg(vcpu, rd->reg);
> + return 0;
> +}
> +
> +static int set_ctr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> + u64 val)
> +{
> + u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> +
> + if (kvm_vm_has_ran_once(vcpu->kvm) &&
> + val != __vcpu_sys_reg(vcpu, rd->reg))
> + return -EBUSY;
> +
> + if (((ctr_el0 & ~CTR_EL0_DIC_MASK) != (val & ~CTR_EL0_DIC_MASK)) ||
> + ((ctr_el0 & CTR_EL0_DIC_MASK) < (val & CTR_EL0_DIC_MASK)))
> + return -EINVAL;
Why limit this to DIC only? Anything that advertises something
"weaker" than what the HW has, such as a smaller cache line size, is
equally valid.
> +
> + __vcpu_sys_reg(vcpu, rd->reg) = val;
> + return 0;
> +}
> +
> static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> {
> @@ -2461,7 +2493,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
> { SYS_DESC(SYS_SMIDR_EL1), undef_access },
> { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
> - { SYS_DESC(SYS_CTR_EL0), access_ctr },
> + { SYS_DESC(SYS_CTR_EL0), access_ctr, .reset = reset_ctr,
> + .get_user = get_ctr, .set_user = set_ctr},
Now, who traps this? Since c876c3f182a5 ("KVM: arm64: Relax trapping
of CTR_EL0 when FEAT_EVT is available"), we don't trap it anymore when
FEAT_EVT is present. Surely you should account for this.
Also, we really shouldn't trap it unless the guest view is different,
as this has a very visible impact on any userspace.
M.
--
Without deviation from the norm, progress is not possible.
parent reply other threads:[~2024-03-18 11:45 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <20240318111636.10613-2-sebott@redhat.com>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8634sn227p.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=james.morse@arm.com \
--cc=kvmarm@lists.linux.dev \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=oliver.upton@linux.dev \
--cc=sebott@redhat.com \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).