| Message ID | 20230607194554.87359-4-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable writable for idregs DFR0,PFR0, MMFR{0,1,2} | expand |
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.
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
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),
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(-)