Message ID | 20240813104400.1956132-9-maz@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: arm64: Add support for FP8 | expand |
Hello! On Tue, Aug 13, 2024 at 11:44:00AM +0100, Marc Zyngier wrote: > Everything is now in place for a guest to "enjoy" FP8 support. > Expose ID_AA64PFR2_EL1 to both userspace and guests, with the > explicit restriction of only being able to clear FPMR. > > All other features (MTE* at the time of writing) are hidden > and not writable. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 51627add0a72..da6d017f24a1 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1722,6 +1722,15 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > return val; > } > > +static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, > + const struct sys_reg_desc *rd) > +{ > + u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); > + > + /* We only expose FPMR */ > + return val & ID_AA64PFR2_EL1_FPMR; > +} Wondering why you're adding this function instead of extending __kvm_read_sanitised_id_reg()? > + > #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ > ({ \ > u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ > @@ -2381,7 +2390,12 @@ static const struct sys_reg_desc sys_reg_descs[] = { > ID_AA64PFR0_EL1_AdvSIMD | > ID_AA64PFR0_EL1_FP), }, > ID_SANITISED(ID_AA64PFR1_EL1), > - ID_UNALLOCATED(4,2), > + { SYS_DESC(SYS_ID_AA64PFR2_EL1), > + .access = access_id_reg, > + .get_user = get_id_reg, > + .set_user = set_id_reg, > + .reset = read_sanitised_id_aa64pfr2_el1, > + .val = ID_AA64PFR2_EL1_FPMR, }, Then I think this would just be ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR). > ID_UNALLOCATED(4,3), > ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), > ID_HIDDEN(ID_AA64SMFR0_EL1), Thanks, Joey
On Tue, 13 Aug 2024 11:57:10 +0100, Joey Gouly <joey.gouly@arm.com> wrote: > > Hello! > > On Tue, Aug 13, 2024 at 11:44:00AM +0100, Marc Zyngier wrote: > > Everything is now in place for a guest to "enjoy" FP8 support. > > Expose ID_AA64PFR2_EL1 to both userspace and guests, with the > > explicit restriction of only being able to clear FPMR. > > > > All other features (MTE* at the time of writing) are hidden > > and not writable. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 51627add0a72..da6d017f24a1 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -1722,6 +1722,15 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > return val; > > } > > > > +static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, > > + const struct sys_reg_desc *rd) > > +{ > > + u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); > > + > > + /* We only expose FPMR */ > > + return val & ID_AA64PFR2_EL1_FPMR; > > +} > > Wondering why you're adding this function instead of extending __kvm_read_sanitised_id_reg()? > > > + > > #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ > > ({ \ > > u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ > > @@ -2381,7 +2390,12 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > ID_AA64PFR0_EL1_AdvSIMD | > > ID_AA64PFR0_EL1_FP), }, > > ID_SANITISED(ID_AA64PFR1_EL1), > > - ID_UNALLOCATED(4,2), > > + { SYS_DESC(SYS_ID_AA64PFR2_EL1), > > + .access = access_id_reg, > > + .get_user = get_id_reg, > > + .set_user = set_id_reg, > > + .reset = read_sanitised_id_aa64pfr2_el1, > > + .val = ID_AA64PFR2_EL1_FPMR, }, > > Then I think this would just be ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR). Yeah, that's an interesting point. I'm afraid I have lost track of the many helpers that have been added over time. Something like this? diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index da6d017f24a1..2d1e45178422 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1539,6 +1539,10 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME); break; + case SYS_ID_AA64PFR2_EL1: + /* We only expose FPMR */ + val &= ID_AA64PFR2_EL1_FPMR; + break; case SYS_ID_AA64ISAR1_EL1: if (!vcpu_has_ptrauth(vcpu)) val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_APA) | @@ -1722,15 +1726,6 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, return val; } -static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, - const struct sys_reg_desc *rd) -{ - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); - - /* We only expose FPMR */ - return val & ID_AA64PFR2_EL1_FPMR; -} - #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ ({ \ u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ @@ -2390,12 +2385,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_AA64PFR0_EL1_AdvSIMD | ID_AA64PFR0_EL1_FP), }, ID_SANITISED(ID_AA64PFR1_EL1), - { SYS_DESC(SYS_ID_AA64PFR2_EL1), - .access = access_id_reg, - .get_user = get_id_reg, - .set_user = set_id_reg, - .reset = read_sanitised_id_aa64pfr2_el1, - .val = ID_AA64PFR2_EL1_FPMR, }, + ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR), ID_UNALLOCATED(4,3), ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), ID_HIDDEN(ID_AA64SMFR0_EL1), Thanks, M.
On Tue, Aug 13, 2024 at 01:47:48PM +0100, Marc Zyngier wrote: > On Tue, 13 Aug 2024 11:57:10 +0100, > Joey Gouly <joey.gouly@arm.com> wrote: > > > > Hello! > > > > On Tue, Aug 13, 2024 at 11:44:00AM +0100, Marc Zyngier wrote: > > > Everything is now in place for a guest to "enjoy" FP8 support. > > > Expose ID_AA64PFR2_EL1 to both userspace and guests, with the > > > explicit restriction of only being able to clear FPMR. > > > > > > All other features (MTE* at the time of writing) are hidden > > > and not writable. > > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > --- > > > arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 51627add0a72..da6d017f24a1 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -1722,6 +1722,15 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > > > return val; > > > } > > > > > > +static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, > > > + const struct sys_reg_desc *rd) > > > +{ > > > + u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); > > > + > > > + /* We only expose FPMR */ > > > + return val & ID_AA64PFR2_EL1_FPMR; > > > +} > > > > Wondering why you're adding this function instead of extending __kvm_read_sanitised_id_reg()? > > > > > + > > > #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ > > > ({ \ > > > u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ > > > @@ -2381,7 +2390,12 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > > ID_AA64PFR0_EL1_AdvSIMD | > > > ID_AA64PFR0_EL1_FP), }, > > > ID_SANITISED(ID_AA64PFR1_EL1), > > > - ID_UNALLOCATED(4,2), > > > + { SYS_DESC(SYS_ID_AA64PFR2_EL1), > > > + .access = access_id_reg, > > > + .get_user = get_id_reg, > > > + .set_user = set_id_reg, > > > + .reset = read_sanitised_id_aa64pfr2_el1, > > > + .val = ID_AA64PFR2_EL1_FPMR, }, > > > > Then I think this would just be ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR). > > Yeah, that's an interesting point. I'm afraid I have lost track of the > many helpers that have been added over time. > > Something like this? LGTM! > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index da6d017f24a1..2d1e45178422 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1539,6 +1539,10 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, > > val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME); > break; > + case SYS_ID_AA64PFR2_EL1: > + /* We only expose FPMR */ > + val &= ID_AA64PFR2_EL1_FPMR; > + break; > case SYS_ID_AA64ISAR1_EL1: > if (!vcpu_has_ptrauth(vcpu)) > val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_APA) | > @@ -1722,15 +1726,6 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, > return val; > } > > -static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, > - const struct sys_reg_desc *rd) > -{ > - u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); > - > - /* We only expose FPMR */ > - return val & ID_AA64PFR2_EL1_FPMR; > -} > - > #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ > ({ \ > u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ > @@ -2390,12 +2385,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > ID_AA64PFR0_EL1_AdvSIMD | > ID_AA64PFR0_EL1_FP), }, > ID_SANITISED(ID_AA64PFR1_EL1), > - { SYS_DESC(SYS_ID_AA64PFR2_EL1), > - .access = access_id_reg, > - .get_user = get_id_reg, > - .set_user = set_id_reg, > - .reset = read_sanitised_id_aa64pfr2_el1, > - .val = ID_AA64PFR2_EL1_FPMR, }, > + ID_WRITABLE(ID_AA64PFR2_EL1, ID_AA64PFR2_EL1_FPMR), > ID_UNALLOCATED(4,3), > ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), > ID_HIDDEN(ID_AA64SMFR0_EL1), > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 51627add0a72..da6d017f24a1 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1722,6 +1722,15 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu, return val; } +static u64 read_sanitised_id_aa64pfr2_el1(struct kvm_vcpu *vcpu, + const struct sys_reg_desc *rd) +{ + u64 val = read_sanitised_ftr_reg(SYS_ID_AA64PFR2_EL1); + + /* We only expose FPMR */ + return val & ID_AA64PFR2_EL1_FPMR; +} + #define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit) \ ({ \ u64 __f_val = FIELD_GET(reg##_##field##_MASK, val); \ @@ -2381,7 +2390,12 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_AA64PFR0_EL1_AdvSIMD | ID_AA64PFR0_EL1_FP), }, ID_SANITISED(ID_AA64PFR1_EL1), - ID_UNALLOCATED(4,2), + { SYS_DESC(SYS_ID_AA64PFR2_EL1), + .access = access_id_reg, + .get_user = get_id_reg, + .set_user = set_id_reg, + .reset = read_sanitised_id_aa64pfr2_el1, + .val = ID_AA64PFR2_EL1_FPMR, }, ID_UNALLOCATED(4,3), ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0), ID_HIDDEN(ID_AA64SMFR0_EL1),
Everything is now in place for a guest to "enjoy" FP8 support. Expose ID_AA64PFR2_EL1 to both userspace and guests, with the explicit restriction of only being able to clear FPMR. All other features (MTE* at the time of writing) are hidden and not writable. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/sys_regs.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)