diff mbox series

[RFC,v4,20/26] KVM: arm64: Trap disabled features of ID_AA64PFR0_EL1

Message ID 20220106042708.2869332-21-reijiw@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Make CPU ID registers writable by userspace | expand

Commit Message

Reiji Watanabe Jan. 6, 2022, 4:27 a.m. UTC 
Add feature_config_ctrl for RAS and AMU, which are indicated in
ID_AA64PFR0_EL1, to program configuration registers to trap
guest's using those features when they are not exposed to the guest.

Introduce trap_ras_regs() to change a behavior of guest's access to
the registers, which is currently raz/wi, depending on the feature's
availability for the guest (and inject undefined instruction
exception when guest's RAS register access are trapped and RAS is
not exposed to the guest).  In order to keep the current visibility
of the RAS registers from userspace (always visible), a visibility
function for RAS registers is not added.

No code is added for AMU's access/visibility handler because the
current code already injects the exception for Guest's AMU register
access unconditionally because AMU is never exposed to the guest.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/sys_regs.c | 90 +++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 8 deletions(-)

Comments

Fuad Tabba Jan. 24, 2022, 5:16 p.m. UTC | #1 
Hi Reiji,

On Thu, Jan 6, 2022 at 4:29 AM Reiji Watanabe <reijiw@google.com> wrote:
>
> Add feature_config_ctrl for RAS and AMU, which are indicated in
> ID_AA64PFR0_EL1, to program configuration registers to trap
> guest's using those features when they are not exposed to the guest.
>
> Introduce trap_ras_regs() to change a behavior of guest's access to
> the registers, which is currently raz/wi, depending on the feature's
> availability for the guest (and inject undefined instruction
> exception when guest's RAS register access are trapped and RAS is
> not exposed to the guest).  In order to keep the current visibility
> of the RAS registers from userspace (always visible), a visibility
> function for RAS registers is not added.
>
> No code is added for AMU's access/visibility handler because the
> current code already injects the exception for Guest's AMU register
> access unconditionally because AMU is never exposed to the guest.

I think it might be code to trap it anyway, in case AMU guest support
is added in the future.

>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 90 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 33893a501475..015d67092d5e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -304,6 +304,63 @@ struct feature_config_ctrl {
>         void    (*trap_activate)(struct kvm_vcpu *vcpu);
>  };
>
> +enum vcpu_config_reg {
> +       VCPU_HCR_EL2 = 1,
> +       VCPU_MDCR_EL2,
> +       VCPU_CPTR_EL2,
> +};
> +
> +static void feature_trap_activate(struct kvm_vcpu *vcpu,
> +                                 enum vcpu_config_reg cfg_reg,
> +                                 u64 cfg_set, u64 cfg_clear)
> +{
> +       u64 *reg_ptr, reg_val;
> +
> +       switch (cfg_reg) {
> +       case VCPU_HCR_EL2:
> +               reg_ptr = &vcpu->arch.hcr_el2;
> +               break;
> +       case VCPU_MDCR_EL2:
> +               reg_ptr = &vcpu->arch.mdcr_el2;
> +               break;
> +       case VCPU_CPTR_EL2:
> +               reg_ptr = &vcpu->arch.cptr_el2;
> +               break;
> +       }
> +
> +       /* Clear/Set fields that are indicated by cfg_clear/cfg_set. */
> +       reg_val = (*reg_ptr & ~cfg_clear);
> +       reg_val |= cfg_set;
> +       *reg_ptr = reg_val;
> +}
> +
> +static void feature_ras_trap_activate(struct kvm_vcpu *vcpu)
> +{
> +       feature_trap_activate(vcpu, VCPU_HCR_EL2, HCR_TERR | HCR_TEA, HCR_FIEN);

Covers all the flags for ras.

> +}
> +
> +static void feature_amu_trap_activate(struct kvm_vcpu *vcpu)
> +{
> +       feature_trap_activate(vcpu, VCPU_CPTR_EL2, CPTR_EL2_TAM, 0);

Covers the CPTR flags for AMU, but as you mentioned, does not
explicitly clear HCR_AMVOFFEN.

Cheers,
/fuad


> +}
> +
> +/* For ID_AA64PFR0_EL1 */
> +static struct feature_config_ctrl ftr_ctrl_ras = {
> +       .ftr_reg = SYS_ID_AA64PFR0_EL1,
> +       .ftr_shift = ID_AA64PFR0_RAS_SHIFT,
> +       .ftr_min = ID_AA64PFR0_RAS_V1,
> +       .ftr_signed = FTR_UNSIGNED,
> +       .trap_activate = feature_ras_trap_activate,
> +};
> +
> +static struct feature_config_ctrl ftr_ctrl_amu = {
> +       .ftr_reg = SYS_ID_AA64PFR0_EL1,
> +       .ftr_shift = ID_AA64PFR0_AMU_SHIFT,
> +       .ftr_min = ID_AA64PFR0_AMU,
> +       .ftr_signed = FTR_UNSIGNED,
> +       .trap_activate = feature_amu_trap_activate,
> +};
> +
>  struct id_reg_info {
>         u32     sys_reg;        /* Register ID */
>         u64     sys_val;        /* Sanitized system value */
> @@ -778,6 +835,11 @@ static struct id_reg_info id_aa64pfr0_el1_info = {
>         .init = init_id_aa64pfr0_el1_info,
>         .validate = validate_id_aa64pfr0_el1,
>         .vcpu_mask = vcpu_mask_id_aa64pfr0_el1,
> +       .trap_features = &(const struct feature_config_ctrl *[]) {
> +               &ftr_ctrl_ras,
> +               &ftr_ctrl_amu,
> +               NULL,
> +       },
>  };
>
>  static struct id_reg_info id_aa64pfr1_el1_info = {
> @@ -901,6 +963,18 @@ static inline bool vcpu_feature_is_available(struct kvm_vcpu *vcpu,
>         return feature_avail(ctrl, val);
>  }
>
> +static bool trap_ras_regs(struct kvm_vcpu *vcpu,
> +                         struct sys_reg_params *p,
> +                         const struct sys_reg_desc *r)
> +{
> +       if (!vcpu_feature_is_available(vcpu, &ftr_ctrl_ras)) {
> +               kvm_inject_undefined(vcpu);
> +               return false;
> +       }
> +
> +       return trap_raz_wi(vcpu, p, r);
> +}
> +
>  /*
>   * ARMv8.1 mandates at least a trivial LORegion implementation, where all the
>   * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0
> @@ -2265,14 +2339,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>         { SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
>         { SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
>
> -       { SYS_DESC(SYS_ERRIDR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERRSELR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXFR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXCTLR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXSTATUS_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> +       { SYS_DESC(SYS_ERRIDR_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERRSELR_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXFR_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXCTLR_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXSTATUS_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXADDR_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXMISC0_EL1), trap_ras_regs },
> +       { SYS_DESC(SYS_ERXMISC1_EL1), trap_ras_regs },
>
>         MTE_REG(TFSR_EL1),
>         MTE_REG(TFSRE0_EL1),
> --
> 2.34.1.448.ga2b2bfdf31-goog
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Reiji Watanabe Jan. 27, 2022, 7:19 a.m. UTC | #2 
Hi Fuad,

On Mon, Jan 24, 2022 at 9:17 AM Fuad Tabba <tabba@google.com> wrote:
>
> Hi Reiji,
>
> On Thu, Jan 6, 2022 at 4:29 AM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Add feature_config_ctrl for RAS and AMU, which are indicated in
> > ID_AA64PFR0_EL1, to program configuration registers to trap
> > guest's using those features when they are not exposed to the guest.
> >
> > Introduce trap_ras_regs() to change a behavior of guest's access to
> > the registers, which is currently raz/wi, depending on the feature's
> > availability for the guest (and inject undefined instruction
> > exception when guest's RAS register access are trapped and RAS is
> > not exposed to the guest).  In order to keep the current visibility
> > of the RAS registers from userspace (always visible), a visibility
> > function for RAS registers is not added.
> >
> > No code is added for AMU's access/visibility handler because the
> > current code already injects the exception for Guest's AMU register
> > access unconditionally because AMU is never exposed to the guest.
>
> I think it might be code to trap it anyway, in case AMU guest support
> is added in the future.

Yes, I will fix it.
(I forgot to update the comment above...)


> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 90 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 82 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 33893a501475..015d67092d5e 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -304,6 +304,63 @@ struct feature_config_ctrl {
> >         void    (*trap_activate)(struct kvm_vcpu *vcpu);
> >  };
> >
> > +enum vcpu_config_reg {
> > +       VCPU_HCR_EL2 = 1,
> > +       VCPU_MDCR_EL2,
> > +       VCPU_CPTR_EL2,
> > +};
> > +
> > +static void feature_trap_activate(struct kvm_vcpu *vcpu,
> > +                                 enum vcpu_config_reg cfg_reg,
> > +                                 u64 cfg_set, u64 cfg_clear)
> > +{
> > +       u64 *reg_ptr, reg_val;
> > +
> > +       switch (cfg_reg) {
> > +       case VCPU_HCR_EL2:
> > +               reg_ptr = &vcpu->arch.hcr_el2;
> > +               break;
> > +       case VCPU_MDCR_EL2:
> > +               reg_ptr = &vcpu->arch.mdcr_el2;
> > +               break;
> > +       case VCPU_CPTR_EL2:
> > +               reg_ptr = &vcpu->arch.cptr_el2;
> > +               break;
> > +       }
> > +
> > +       /* Clear/Set fields that are indicated by cfg_clear/cfg_set. */
> > +       reg_val = (*reg_ptr & ~cfg_clear);
> > +       reg_val |= cfg_set;
> > +       *reg_ptr = reg_val;
> > +}
> > +
> > +static void feature_ras_trap_activate(struct kvm_vcpu *vcpu)
> > +{
> > +       feature_trap_activate(vcpu, VCPU_HCR_EL2, HCR_TERR | HCR_TEA, HCR_FIEN);
>
> Covers all the flags for ras.
>
> > +}
> > +
> > +static void feature_amu_trap_activate(struct kvm_vcpu *vcpu)
> > +{
> > +       feature_trap_activate(vcpu, VCPU_CPTR_EL2, CPTR_EL2_TAM, 0);
>
> Covers the CPTR flags for AMU, but as you mentioned, does not
> explicitly clear HCR_AMVOFFEN.

In my understanding, clearing HCR_EL2.AMVOFFEN is not necessary as
CPTR_EL2.TAM == 1 traps the guest's accessing AMEVCNTR0<n>_EL0 and
AMEVCNTR1<n>_EL0 anyway (HCR_EL2.AMVOFFEN doesn't matter).
(Or is my understanding wrong ??)

Thanks,
Reiji
Fuad Tabba Feb. 1, 2022, 2:14 p.m. UTC | #3 
Hi Reiji,

...

> > > +static void feature_amu_trap_activate(struct kvm_vcpu *vcpu)
> > > +{
> > > +       feature_trap_activate(vcpu, VCPU_CPTR_EL2, CPTR_EL2_TAM, 0);
> >
> > Covers the CPTR flags for AMU, but as you mentioned, does not
> > explicitly clear HCR_AMVOFFEN.
>
> In my understanding, clearing HCR_EL2.AMVOFFEN is not necessary as
> CPTR_EL2.TAM == 1 traps the guest's accessing AMEVCNTR0<n>_EL0 and
> AMEVCNTR1<n>_EL0 anyway (HCR_EL2.AMVOFFEN doesn't matter).
> (Or is my understanding wrong ??)

You're right. However, I think they should be cleared first for
completeness. Also, if I understand correctly, AMVOFFEN is about
enabling and disabling virtualization of the registers, making
indirect reads of the virtual offset registers as zero, so it's not
just about trapping.

Thanks,
/fuad

> Thanks,
> Reiji
Reiji Watanabe Feb. 10, 2022, 4:15 a.m. UTC | #4 
Hi Fuad,

On Tue, Feb 1, 2022 at 6:14 AM Fuad Tabba <tabba@google.com> wrote:
>
> Hi Reiji,
>
> ...
>
> > > > +static void feature_amu_trap_activate(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       feature_trap_activate(vcpu, VCPU_CPTR_EL2, CPTR_EL2_TAM, 0);
> > >
> > > Covers the CPTR flags for AMU, but as you mentioned, does not
> > > explicitly clear HCR_AMVOFFEN.
> >
> > In my understanding, clearing HCR_EL2.AMVOFFEN is not necessary as
> > CPTR_EL2.TAM == 1 traps the guest's accessing AMEVCNTR0<n>_EL0 and
> > AMEVCNTR1<n>_EL0 anyway (HCR_EL2.AMVOFFEN doesn't matter).
> > (Or is my understanding wrong ??)
>
> You're right. However, I think they should be cleared first for
> completeness. Also, if I understand correctly, AMVOFFEN is about
> enabling and disabling virtualization of the registers, making
> indirect reads of the virtual offset registers as zero, so it's not
> just about trapping.

I understand that AMVOFFEN is making indirect reads of the
virtual offset registers as zero.  But, in my understanding,
enabling and disabling virtualization of the registers doesn't
matter as long as CPTR_EL2.TAM == 1 (a value of HCR_EL2.AMVOFFEN
doesn't change any behavior because the virtual offset registers
won't be used). So, I'm not too keen on adding that so far.

Thanks,
Reiji
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 33893a501475..015d67092d5e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -304,6 +304,63 @@  struct feature_config_ctrl {
 	void	(*trap_activate)(struct kvm_vcpu *vcpu);
 };
 
+enum vcpu_config_reg {
+	VCPU_HCR_EL2 = 1,
+	VCPU_MDCR_EL2,
+	VCPU_CPTR_EL2,
+};
+
+static void feature_trap_activate(struct kvm_vcpu *vcpu,
+				  enum vcpu_config_reg cfg_reg,
+				  u64 cfg_set, u64 cfg_clear)
+{
+	u64 *reg_ptr, reg_val;
+
+	switch (cfg_reg) {
+	case VCPU_HCR_EL2:
+		reg_ptr = &vcpu->arch.hcr_el2;
+		break;
+	case VCPU_MDCR_EL2:
+		reg_ptr = &vcpu->arch.mdcr_el2;
+		break;
+	case VCPU_CPTR_EL2:
+		reg_ptr = &vcpu->arch.cptr_el2;
+		break;
+	}
+
+	/* Clear/Set fields that are indicated by cfg_clear/cfg_set. */
+	reg_val = (*reg_ptr & ~cfg_clear);
+	reg_val |= cfg_set;
+	*reg_ptr = reg_val;
+}
+
+static void feature_ras_trap_activate(struct kvm_vcpu *vcpu)
+{
+	feature_trap_activate(vcpu, VCPU_HCR_EL2, HCR_TERR | HCR_TEA, HCR_FIEN);
+}
+
+static void feature_amu_trap_activate(struct kvm_vcpu *vcpu)
+{
+	feature_trap_activate(vcpu, VCPU_CPTR_EL2, CPTR_EL2_TAM, 0);
+}
+
+/* For ID_AA64PFR0_EL1 */
+static struct feature_config_ctrl ftr_ctrl_ras = {
+	.ftr_reg = SYS_ID_AA64PFR0_EL1,
+	.ftr_shift = ID_AA64PFR0_RAS_SHIFT,
+	.ftr_min = ID_AA64PFR0_RAS_V1,
+	.ftr_signed = FTR_UNSIGNED,
+	.trap_activate = feature_ras_trap_activate,
+};
+
+static struct feature_config_ctrl ftr_ctrl_amu = {
+	.ftr_reg = SYS_ID_AA64PFR0_EL1,
+	.ftr_shift = ID_AA64PFR0_AMU_SHIFT,
+	.ftr_min = ID_AA64PFR0_AMU,
+	.ftr_signed = FTR_UNSIGNED,
+	.trap_activate = feature_amu_trap_activate,
+};
+
 struct id_reg_info {
 	u32	sys_reg;	/* Register ID */
 	u64	sys_val;	/* Sanitized system value */
@@ -778,6 +835,11 @@  static struct id_reg_info id_aa64pfr0_el1_info = {
 	.init = init_id_aa64pfr0_el1_info,
 	.validate = validate_id_aa64pfr0_el1,
 	.vcpu_mask = vcpu_mask_id_aa64pfr0_el1,
+	.trap_features = &(const struct feature_config_ctrl *[]) {
+		&ftr_ctrl_ras,
+		&ftr_ctrl_amu,
+		NULL,
+	},
 };
 
 static struct id_reg_info id_aa64pfr1_el1_info = {
@@ -901,6 +963,18 @@  static inline bool vcpu_feature_is_available(struct kvm_vcpu *vcpu,
 	return feature_avail(ctrl, val);
 }
 
+static bool trap_ras_regs(struct kvm_vcpu *vcpu,
+			  struct sys_reg_params *p,
+			  const struct sys_reg_desc *r)
+{
+	if (!vcpu_feature_is_available(vcpu, &ftr_ctrl_ras)) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}
+
+	return trap_raz_wi(vcpu, p, r);
+}
+
 /*
  * ARMv8.1 mandates at least a trivial LORegion implementation, where all the
  * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0
@@ -2265,14 +2339,14 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
 	{ SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
 
-	{ SYS_DESC(SYS_ERRIDR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERRSELR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXFR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXCTLR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXSTATUS_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERRIDR_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERRSELR_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXFR_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXCTLR_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXSTATUS_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXADDR_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXMISC0_EL1), trap_ras_regs },
+	{ SYS_DESC(SYS_ERXMISC1_EL1), trap_ras_regs },
 
 	MTE_REG(TFSR_EL1),
 	MTE_REG(TFSRE0_EL1),