On Mon, 21 Aug 2023 22:22:37 +0100,
Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
> from userspace with this change.
> RES0 fields and those fields hidden by KVM are not writable.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> arch/arm64/kvm/sys_regs.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index afade7186675..20fc38bad4e8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1931,6 +1931,8 @@ static bool access_spsr(struct kvm_vcpu *vcpu,
> return true;
> }
>
> +#define ID_AA64DFR0_EL1_RES0_MASK (GENMASK(59, 56) | GENMASK(27, 24) | GENMASK(19, 16))
> +
> /*
> * Architected system registers.
> * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
> @@ -2006,7 +2008,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> .set_user = set_id_dfr0_el1,
> .visibility = aa32_id_visibility,
> .reset = read_sanitised_id_dfr0_el1,
> - .val = ID_DFR0_EL1_PerfMon_MASK, },
> + .val = GENMASK(31, 0), },
Can you *please* look at the register and realise that we *don't*
support writing to the whole of the low 32 bits? What does it mean to
allow selecting the M-profile debug? Or the memory-mapped trace?
You are advertising a lot of crap to userspace, and that's definitely
not on.
> ID_HIDDEN(ID_AFR0_EL1),
> AA32_ID_SANITISED(ID_MMFR0_EL1),
> AA32_ID_SANITISED(ID_MMFR1_EL1),
> @@ -2055,7 +2057,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> .get_user = get_id_reg,
> .set_user = set_id_aa64dfr0_el1,
> .reset = read_sanitised_id_aa64dfr0_el1,
> - .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
> + .val = ~(ID_AA64DFR0_EL1_PMSVer_MASK | ID_AA64DFR0_EL1_RES0_MASK), },
And it is the same thing here. Where is the handling code to deal with
variable breakpoint numbers? Oh wait, there is none. Really, the only
thing we support writing to are the PMU and Debug versions. And
nothing else.
What does it mean for userspace? Either the write will be denied
despite being advertised a writable field (remember the first patch of
the series???), or we'll blindly accept the write and further ignore
the requested values. Do you really think any of this is acceptable?
This is the *9th* version of this series, and we're still battling
over some extremely basic userspace issues... I don't think we can
merge this series as is stands.
M.
--
Without deviation from the norm, progress is not possible.
