[PATCH v4 0/4] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2}

[PATCH v4 0/4] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2}

[Jing Zhang]

This patch series enable userspace writable for below idregs:
ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2}_EL1.

It is based on below series [2] which add infrastructure for writable idregs.

---

* v3 -> v4
  - Rebase on v11 of writable idregs series at [2].

* v2 -> v3
  - Rebase on v6 of writable idregs series.
  - Enable writable for ID_AA64PFR0_EL1 and ID_AA64MMFR{0, 1, 2}_EL1.

* v1 -> v2
  - Rebase on latest patch series [1] of enabling writable ID register.

[1] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
[2] https://lore.kernel.org/all/20230602005118.2899664-1-jingzhangos@google.com

[v1] https://lore.kernel.org/all/20230326011950.405749-1-jingzhangos@google.com
[v2] https://lore.kernel.org/all/20230403003723.3199828-1-jingzhangos@google.com
[v3] https://lore.kernel.org/all/20230405172146.297208-1-jingzhangos@google.com

---

Jing Zhang (4):
  KVM: arm64: Enable writable for ID_AA64DFR0_EL1
  KVM: arm64: Enable writable for ID_DFR0_EL1
  KVM: arm64: Enable writable for ID_AA64PFR0_EL1
  KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2}_EL1

 arch/arm64/kvm/sys_regs.c | 78 +++++++++++++++++++++++++++++++++------
 1 file changed, 67 insertions(+), 11 deletions(-)


base-commit: 01b532e41af091a48287dd45f763db4b887bcdfc
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Jing Zhang]

Since number of context-aware breakpoints must be no more than number
of supported breakpoints according to Arm ARM, return an error if
userspace tries to set CTX_CMPS field to such value.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 50d4e25f42d3..a6299c796d03 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
-	u8 pmuver, host_pmuver;
+	u8 pmuver, host_pmuver, brps, ctx_cmps;
 	bool valid_pmu;
 
+	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
+	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
+	if (ctx_cmps > brps)
+		return -EINVAL;
+
 	host_pmuver = kvm_arm_pmu_get_pmuver_limit();
 
 	/*
@@ -2061,7 +2066,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 = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
 	ID_UNALLOCATED(5,3),
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Oliver Upton]

On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
> Since number of context-aware breakpoints must be no more than number
> of supported breakpoints according to Arm ARM, return an error if
> userspace tries to set CTX_CMPS field to such value.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 50d4e25f42d3..a6299c796d03 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  			       const struct sys_reg_desc *rd,
>  			       u64 val)
>  {
> -	u8 pmuver, host_pmuver;
> +	u8 pmuver, host_pmuver, brps, ctx_cmps;
>  	bool valid_pmu;
>  
> +	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
> +	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
> +	if (ctx_cmps > brps)
> +		return -EINVAL;
> +

I'm not fully convinced on the need to do this sort of cross-field
validation... I think it is probably more trouble than it is worth. If
userspace writes something illogical to the register, oh well. All we
should care about is that the advertised feature set is a subset of
what's supported by the host.

The series doesn't even do complete sanity checking, and instead works
on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
special handling depending on how pedantic you're feeling. AArch32
support at a higher exception level implies AArch32 support at all lower
exception levels.

But that isn't a suggestion to implement it, more of a suggestion to
just avoid the problem as a whole.

>  	host_pmuver = kvm_arm_pmu_get_pmuver_limit();
>  
>  	/*
> @@ -2061,7 +2066,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 = GENMASK(63, 0), },

DebugVer requires special handling, as the minimum safe value is 0x6 for
the field. IIUC, as written we would permit userspace to write any value
less than the current register value.

I posted a patch to 'fix' this, but it isn't actually a bug in what's
upstream. Could you pick that patch up and discard the 'Fixes' tag on
it?

https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev/

>  	ID_SANITISED(ID_AA64DFR1_EL1),
>  	ID_UNALLOCATED(5,2),
>  	ID_UNALLOCATED(5,3),
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 
> 

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Cornelia Huck]

On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
>> Since number of context-aware breakpoints must be no more than number
>> of supported breakpoints according to Arm ARM, return an error if
>> userspace tries to set CTX_CMPS field to such value.
>> 
>> Signed-off-by: Jing Zhang <jingzhangos@google.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 50d4e25f42d3..a6299c796d03 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>  			       const struct sys_reg_desc *rd,
>>  			       u64 val)
>>  {
>> -	u8 pmuver, host_pmuver;
>> +	u8 pmuver, host_pmuver, brps, ctx_cmps;
>>  	bool valid_pmu;
>>  
>> +	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
>> +	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
>> +	if (ctx_cmps > brps)
>> +		return -EINVAL;
>> +
>
> I'm not fully convinced on the need to do this sort of cross-field
> validation... I think it is probably more trouble than it is worth. If
> userspace writes something illogical to the register, oh well. All we
> should care about is that the advertised feature set is a subset of
> what's supported by the host.
>
> The series doesn't even do complete sanity checking, and instead works
> on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
> special handling depending on how pedantic you're feeling. AArch32
> support at a higher exception level implies AArch32 support at all lower
> exception levels.
>
> But that isn't a suggestion to implement it, more of a suggestion to
> just avoid the problem as a whole.

Generally speaking, how much effort do we want to invest to prevent
userspace from doing dumb things? "Make sure we advertise a subset of
features of what the host supports" and "disallow writing values that
are not allowed by the architecture in the first place" seem reasonable,
but if userspace wants to create weird frankencpus[1], should it be
allowed to break the guest and get to keep the pieces?

I'd be more in favour to rely on userspace to configure something that
is actually usable; it needs to sanitize any user-provided configuration
anyway.

[1] I think userspace will end up creating frankencpus in any case, but
at least it should be the kind that doesn't look out of place in the
subway if you dress it in proper clothing.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Oliver Upton]

Hi Cornelia,

On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote:
> On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
> >> +	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
> >> +	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
> >> +	if (ctx_cmps > brps)
> >> +		return -EINVAL;
> >> +
> >
> > I'm not fully convinced on the need to do this sort of cross-field
> > validation... I think it is probably more trouble than it is worth. If
> > userspace writes something illogical to the register, oh well. All we
> > should care about is that the advertised feature set is a subset of
> > what's supported by the host.
> >
> > The series doesn't even do complete sanity checking, and instead works
> > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
> > special handling depending on how pedantic you're feeling. AArch32
> > support at a higher exception level implies AArch32 support at all lower
> > exception levels.
> >
> > But that isn't a suggestion to implement it, more of a suggestion to
> > just avoid the problem as a whole.
> 
> Generally speaking, how much effort do we want to invest to prevent
> userspace from doing dumb things? "Make sure we advertise a subset of
> features of what the host supports" and "disallow writing values that
> are not allowed by the architecture in the first place" seem reasonable,
> but if userspace wants to create weird frankencpus[1], should it be
> allowed to break the guest and get to keep the pieces?

What I'm specifically objecting to is having KVM do sanity checks across
multiple fields. That requires explicit, per-field plumbing that will
eventually become a tangled mess that Marc and I will have to maintain.
The context-aware breakpoints is one example, as is ensuring SVE is
exposed iff FP is too. In all likelihood we'll either get some part of
this wrong, or miss a required check altogether.

Modulo a few exceptions to this case, I think per-field validation is
going to cover almost everything we're worried about, and we get that
largely for free from arm64_check_features().

> I'd be more in favour to rely on userspace to configure something that
> is actually usable; it needs to sanitize any user-provided configuration
> anyway.

Just want to make sure I understand your sentiment here, you'd be in
favor of the more robust sanitization?

> [1] I think userspace will end up creating frankencpus in any case, but
> at least it should be the kind that doesn't look out of place in the
> subway if you dress it in proper clothing.

I mean, KVM already advertises a frankencpu in the first place, so we're
off to a good start :)

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Cornelia Huck]

On Tue, Jul 04 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> Hi Cornelia,
>
> On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote:
>> On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
>> 
>> > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
>> >> +	brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
>> >> +	ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
>> >> +	if (ctx_cmps > brps)
>> >> +		return -EINVAL;
>> >> +
>> >
>> > I'm not fully convinced on the need to do this sort of cross-field
>> > validation... I think it is probably more trouble than it is worth. If
>> > userspace writes something illogical to the register, oh well. All we
>> > should care about is that the advertised feature set is a subset of
>> > what's supported by the host.
>> >
>> > The series doesn't even do complete sanity checking, and instead works
>> > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
>> > special handling depending on how pedantic you're feeling. AArch32
>> > support at a higher exception level implies AArch32 support at all lower
>> > exception levels.
>> >
>> > But that isn't a suggestion to implement it, more of a suggestion to
>> > just avoid the problem as a whole.
>> 
>> Generally speaking, how much effort do we want to invest to prevent
>> userspace from doing dumb things? "Make sure we advertise a subset of
>> features of what the host supports" and "disallow writing values that
>> are not allowed by the architecture in the first place" seem reasonable,
>> but if userspace wants to create weird frankencpus[1], should it be
>> allowed to break the guest and get to keep the pieces?
>
> What I'm specifically objecting to is having KVM do sanity checks across
> multiple fields. That requires explicit, per-field plumbing that will
> eventually become a tangled mess that Marc and I will have to maintain.
> The context-aware breakpoints is one example, as is ensuring SVE is
> exposed iff FP is too. In all likelihood we'll either get some part of
> this wrong, or miss a required check altogether.

Nod, this sounds like more trouble than it's worth in the end.

>
> Modulo a few exceptions to this case, I think per-field validation is
> going to cover almost everything we're worried about, and we get that
> largely for free from arm64_check_features().
>
>> I'd be more in favour to rely on userspace to configure something that
>> is actually usable; it needs to sanitize any user-provided configuration
>> anyway.
>
> Just want to make sure I understand your sentiment here, you'd be in
> favor of the more robust sanitization?

In userspace. E.g. QEMU can go ahead and try to implement the
user-exposed knobs in a way that the really broken configurations are
not even possible. I'd also expect userspace to have a more complete
view of what it is trying to instantiate (especially if code is shared
between instantiating a vcpu for use with KVM and a fully emulated
vcpu -- we probably don't want to go all crazy in the latter case,
either.)

>
>> [1] I think userspace will end up creating frankencpus in any case, but
>> at least it should be the kind that doesn't look out of place in the
>> subway if you dress it in proper clothing.
>
> I mean, KVM already advertises a frankencpu in the first place, so we're
> off to a good start :)

Indeed :)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/4] KVM: arm64: Enable writable for ID_AA64DFR0_EL1

[Jing Zhang]

Hi Oliver, Cornelia,

Thanks for the discussion about the cross-field validation. I'm happy
to know that we all agree to avoid that. I'll remove those validations
for later posts.

Thanks,
Jing

On Wed, Jul 5, 2023 at 1:49 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, Jul 04 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
>
> > Hi Cornelia,
> >
> > On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote:
> >> On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
> >>
> >> > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote:
> >> >> + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val);
> >> >> + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val);
> >> >> + if (ctx_cmps > brps)
> >> >> +         return -EINVAL;
> >> >> +
> >> >
> >> > I'm not fully convinced on the need to do this sort of cross-field
> >> > validation... I think it is probably more trouble than it is worth. If
> >> > userspace writes something illogical to the register, oh well. All we
> >> > should care about is that the advertised feature set is a subset of
> >> > what's supported by the host.
> >> >
> >> > The series doesn't even do complete sanity checking, and instead works
> >> > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require
> >> > special handling depending on how pedantic you're feeling. AArch32
> >> > support at a higher exception level implies AArch32 support at all lower
> >> > exception levels.
> >> >
> >> > But that isn't a suggestion to implement it, more of a suggestion to
> >> > just avoid the problem as a whole.
> >>
> >> Generally speaking, how much effort do we want to invest to prevent
> >> userspace from doing dumb things? "Make sure we advertise a subset of
> >> features of what the host supports" and "disallow writing values that
> >> are not allowed by the architecture in the first place" seem reasonable,
> >> but if userspace wants to create weird frankencpus[1], should it be
> >> allowed to break the guest and get to keep the pieces?
> >
> > What I'm specifically objecting to is having KVM do sanity checks across
> > multiple fields. That requires explicit, per-field plumbing that will
> > eventually become a tangled mess that Marc and I will have to maintain.
> > The context-aware breakpoints is one example, as is ensuring SVE is
> > exposed iff FP is too. In all likelihood we'll either get some part of
> > this wrong, or miss a required check altogether.
>
> Nod, this sounds like more trouble than it's worth in the end.
>
> >
> > Modulo a few exceptions to this case, I think per-field validation is
> > going to cover almost everything we're worried about, and we get that
> > largely for free from arm64_check_features().
> >
> >> I'd be more in favour to rely on userspace to configure something that
> >> is actually usable; it needs to sanitize any user-provided configuration
> >> anyway.
> >
> > Just want to make sure I understand your sentiment here, you'd be in
> > favor of the more robust sanitization?
>
> In userspace. E.g. QEMU can go ahead and try to implement the
> user-exposed knobs in a way that the really broken configurations are
> not even possible. I'd also expect userspace to have a more complete
> view of what it is trying to instantiate (especially if code is shared
> between instantiating a vcpu for use with KVM and a fully emulated
> vcpu -- we probably don't want to go all crazy in the latter case,
> either.)
>
> >
> >> [1] I think userspace will end up creating frankencpus in any case, but
> >> at least it should be the kind that doesn't look out of place in the
> >> subway if you dress it in proper clothing.
> >
> > I mean, KVM already advertises a frankencpu in the first place, so we're
> > off to a good start :)
>
> Indeed :)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/4] KVM: arm64: Enable writable for ID_DFR0_EL1

[Jing Zhang]

All valid fields in ID_DFR0_EL1 are writable from usrespace with this
change.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a6299c796d03..3964a85a89fe 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2017,7 +2017,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(63, 0), },
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR1_EL1),
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/4] KVM: arm64: Enable writable for ID_AA64PFR0_EL1

[Jing Zhang]

Return an error if userspace tries to set SVE field of the register
to a value that conflicts with SVE configuration for the guest.
SIMD/FP/SVE fields of the requested value are validated according to
Arm ARM.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3964a85a89fe..8f3ad9c12b27 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1509,9 +1509,36 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 
 	val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
 
+	if (!system_supports_sve())
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
+
 	return val;
 }
 
+static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+			       const struct sys_reg_desc *rd,
+			       u64 val)
+{
+	int fp, simd;
+	bool has_sve = id_aa64pfr0_sve(val);
+
+	simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_AdvSIMD_SHIFT);
+	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_FP_SHIFT);
+	/* AdvSIMD field must have the same value as FP field */
+	if (simd != fp)
+		return -EINVAL;
+
+	/* fp must be supported when sve is supported */
+	if (has_sve && (fp < 0))
+		return -EINVAL;
+
+	/* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */
+	if (vcpu_has_sve(vcpu) ^ has_sve)
+		return -EPERM;
+
+	return set_id_reg(vcpu, rd, val);
+}
+
 static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
@@ -2049,9 +2076,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_ID_AA64PFR0_EL1),
 	  .access = access_id_reg,
 	  .get_user = get_id_reg,
-	  .set_user = set_id_reg,
+	  .set_user = set_id_aa64pfr0_el1,
 	  .reset = read_sanitised_id_aa64pfr0_el1,
-	  .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/4] KVM: arm64: Enable writable for ID_AA64PFR0_EL1

[Oliver Upton]

On Wed, Jun 07, 2023 at 07:45:53PM +0000, Jing Zhang wrote:
> Return an error if userspace tries to set SVE field of the register
> to a value that conflicts with SVE configuration for the guest.
> SIMD/FP/SVE fields of the requested value are validated according to
> Arm ARM.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 3964a85a89fe..8f3ad9c12b27 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1509,9 +1509,36 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>  
>  	val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
>  
> +	if (!system_supports_sve())
> +		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
> +

If the system doesn't support SVE, wouldn't the sanitised system-wide
value hide the feature as well? A few lines up we already mask this
field based on whether or not the vCPU has the feature, which is
actually meaningful.

>  	return val;
>  }
>  
> +static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> +			       const struct sys_reg_desc *rd,
> +			       u64 val)
> +{
> +	int fp, simd;
> +	bool has_sve = id_aa64pfr0_sve(val);
> +
> +	simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_AdvSIMD_SHIFT);
> +	fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_FP_SHIFT);
> +	/* AdvSIMD field must have the same value as FP field */
> +	if (simd != fp)
> +		return -EINVAL;
> +
> +	/* fp must be supported when sve is supported */
> +	if (has_sve && (fp < 0))
> +		return -EINVAL;
> +
> +	/* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */
> +	if (vcpu_has_sve(vcpu) ^ has_sve)
> +		return -EPERM;

Same comment here on cross-field plumbing.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/4] KVM: arm64: Enable writable for ID_AA64PFR0_EL1

[Jing Zhang]

Hi Oliver,

On Mon, Jun 26, 2023 at 9:49 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Jun 07, 2023 at 07:45:53PM +0000, Jing Zhang wrote:
> > Return an error if userspace tries to set SVE field of the register
> > to a value that conflicts with SVE configuration for the guest.
> > SIMD/FP/SVE fields of the requested value are validated according to
> > Arm ARM.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 31 +++++++++++++++++++++++++++++--
> >  1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 3964a85a89fe..8f3ad9c12b27 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1509,9 +1509,36 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> >
> >       val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> >
> > +     if (!system_supports_sve())
> > +             val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
> > +
>
> If the system doesn't support SVE, wouldn't the sanitised system-wide
> value hide the feature as well? A few lines up we already mask this
> field based on whether or not the vCPU has the feature, which is
> actually meaningful.
Yes, you are right. This change is not needed actually.
>
> >       return val;
> >  }
> >
> > +static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > +                            const struct sys_reg_desc *rd,
> > +                            u64 val)
> > +{
> > +     int fp, simd;
> > +     bool has_sve = id_aa64pfr0_sve(val);
> > +
> > +     simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_AdvSIMD_SHIFT);
> > +     fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_EL1_FP_SHIFT);
> > +     /* AdvSIMD field must have the same value as FP field */
> > +     if (simd != fp)
> > +             return -EINVAL;
> > +
> > +     /* fp must be supported when sve is supported */
> > +     if (has_sve && (fp < 0))
> > +             return -EINVAL;
> > +
> > +     /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */
> > +     if (vcpu_has_sve(vcpu) ^ has_sve)
> > +             return -EPERM;
>
> Same comment here on cross-field plumbing.
Will fix.
>
> --
> Thanks,
> Oliver

Thanks,
Jing

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/4] KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2}_EL1

[Jing Zhang]

Enable writable from userspace for ID_AA64MMFR{0, 1, 2}_EL1.
Added a macro for defining general writable idregs.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8f3ad9c12b27..54c762c95983 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1391,9 +1391,6 @@ static u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 encoding)
 		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
 				  pmuver_to_perfmon(vcpu_pmuver(vcpu)));
 		break;
-	case SYS_ID_AA64MMFR2_EL1:
-		val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
-		break;
 	case SYS_ID_MMFR4_EL1:
 		val &= ~ARM64_FEATURE_MASK(ID_MMFR4_EL1_CCIDX);
 		break;
@@ -1663,6 +1660,18 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	return pmuver_update(vcpu, rd, val, perfmon_to_pmuver(perfmon), valid_pmu);
 }
 
+static u64 read_sanitised_id_aa64mmfr2_el1(struct kvm_vcpu *vcpu,
+					   const struct sys_reg_desc *rd)
+{
+	u64 val;
+	u32 id = reg_to_encoding(rd);
+
+	val = read_sanitised_ftr_reg(id);
+	val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
+
+	return val;
+}
+
 /*
  * cpufeature ID register user accessors
  *
@@ -1898,6 +1907,16 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
 	.val = 0,				\
 }
 
+#define ID_SANITISED_WRITABLE(name) {		\
+	SYS_DESC(SYS_##name),			\
+	.access	= access_id_reg,		\
+	.get_user = get_id_reg,			\
+	.set_user = set_id_reg,			\
+	.visibility = id_visibility,		\
+	.reset = general_read_kvm_sanitised_reg,\
+	.val = GENMASK(63, 0),			\
+}
+
 /* sys_reg_desc initialiser for known cpufeature ID registers */
 #define AA32_ID_SANITISED(name) {		\
 	SYS_DESC(SYS_##name),			\
@@ -2113,9 +2132,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_UNALLOCATED(6,7),
 
 	/* CRm=7 */
-	ID_SANITISED(ID_AA64MMFR0_EL1),
-	ID_SANITISED(ID_AA64MMFR1_EL1),
-	ID_SANITISED(ID_AA64MMFR2_EL1),
+	ID_SANITISED_WRITABLE(ID_AA64MMFR0_EL1),
+	ID_SANITISED_WRITABLE(ID_AA64MMFR1_EL1),
+	{ SYS_DESC(SYS_ID_AA64MMFR2_EL1),
+	  .access = access_id_reg,
+	  .get_user = get_id_reg,
+	  .set_user = set_id_reg,
+	  .reset = read_sanitised_id_aa64mmfr2_el1,
+	  .val = GENMASK(63, 0), },
 	ID_UNALLOCATED(7,3),
 	ID_UNALLOCATED(7,4),
 	ID_UNALLOCATED(7,5),
-- 
2.41.0.rc0.172.g3f132b7071-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/4] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2}

[Oliver Upton]

Hi Jing,

On Wed, Jun 07, 2023 at 07:45:50PM +0000, Jing Zhang wrote:
> 
> This patch series enable userspace writable for below idregs:
> ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2}_EL1.
> 
> It is based on below series [2] which add infrastructure for writable idregs.

Could you implement some tests for these changes? We really need to see
that userspace is only allowed to select a subset of features that're
provided by the host, and that the CPU feature set never exceeds what
the host can support.

Additionally, there are places in the kernel where we use host ID
register values for the sake of emulation (DBGDIDR, LORegion). These
both should instead be using the _guest_ ID register values.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/4] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2}

[Jing Zhang]

Hi Oliver,

On Mon, Jun 26, 2023 at 1:52 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Jing,
>
> On Wed, Jun 07, 2023 at 07:45:50PM +0000, Jing Zhang wrote:
> >
> > This patch series enable userspace writable for below idregs:
> > ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2}_EL1.
> >
> > It is based on below series [2] which add infrastructure for writable idregs.
>
> Could you implement some tests for these changes? We really need to see
> that userspace is only allowed to select a subset of features that're
> provided by the host, and that the CPU feature set never exceeds what
> the host can support.
Sure, will add a selftest for these.
>
> Additionally, there are places in the kernel where we use host ID
> register values for the sake of emulation (DBGDIDR, LORegion). These
> both should instead be using the _guest_ ID register values.
Will add a new commit for these change.
>
> --
> Thanks,
> Oliver

Thanks,
Jing

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel