Hi Eric,
On Thu, Nov 25, 2021 at 7:31 AM Eric Auger <eauger@xxxxxxxxxx> wrote:
>
>
>
> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
> > This patch adds id_reg_info for ID_AA64MMFR0_EL1 to make it
> > writable by userspace.
> >
> > Since ID_AA64MMFR0_EL1 stage 2 granule size fields don't follow the
> > standard ID scheme, we need a special handling to validate those fields.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/sys_regs.c | 118 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 118 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 5812e39602fe..772e3d3067b2 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -519,6 +519,113 @@ static int validate_id_aa64isar1_el1(struct kvm_vcpu *vcpu,
> > return 0;
> > }
> >
> > +/*
> > + * Check if the requested stage2 translation granule size indicated in
> > + * @mmfr0 is also indicated in @mmfr0_lim. This function assumes that
> > + * the stage1 granule size indicated in @mmfr0 has been validated already.
> I would suggest: relies on the fact TGranX fields are validated before
> through the arm64_check_features lookup
Thank you for the suggestion. I will fix the comment as you suggested.
> > + */
> > +static int aa64mmfr0_tgran2_check(int field, u64 mmfr0, u64 mmfr0_lim)
> > +{
> > + s64 tgran2, lim_tgran2, rtgran1;
> > + int f1;
> > + bool is_signed = true;
> > +
> > + tgran2 = cpuid_feature_extract_unsigned_field(mmfr0, field);
> > + lim_tgran2 = cpuid_feature_extract_unsigned_field(mmfr0_lim, field);
> > + if (tgran2 == lim_tgran2)
> > + return 0;
> > +
> > + if (tgran2 && lim_tgran2)
> > + return (tgran2 > lim_tgran2) ? -E2BIG : 0;
> > +
> > + /*
> > + * Either tgran2 or lim_tgran2 is zero.
> > + * Need stage1 granule size to validate tgran2.
> > + */
> > + switch (field) {
> > + case ID_AA64MMFR0_TGRAN4_2_SHIFT:
> > + f1 = ID_AA64MMFR0_TGRAN4_SHIFT;
> > + break;
> > + case ID_AA64MMFR0_TGRAN64_2_SHIFT:
> > + f1 = ID_AA64MMFR0_TGRAN64_SHIFT;
> > + break;
> > + case ID_AA64MMFR0_TGRAN16_2_SHIFT:
> > + f1 = ID_AA64MMFR0_TGRAN16_SHIFT;
> > + is_signed = false;
> I don't get the is_signed setting. Don't the TGRAN_x have the same
> format? Beside you can get the shift by substracting 12 to @field.
Yes, TGran16 is different from TGran64/TGran4.
https://developer.arm.com/documentation/ddi0595/2021-09/AArch64-Registers/ID-AA64MMFR0-EL1--AArch64-Memory-Model-Feature-Register-0?lang=en
I didn't realize that we could get the shift by substracting 12 from
@field. Thank you for the information. I will use that.
> can't you directly compute if the granule is supported
No, I don't think we can because the value 0 in the TGranx_2 means
that its feature support is identified by another field (TGranx).
> > + break;
> > + default:
> > + /* Should never happen */
> > + WARN_ONCE(1, "Unexpected stage2 granule field (%d)\n", field);
> > + return 0;
> > + }
> > +
> > + /*
> > + * If tgran2 == 0 (&& lim_tgran2 != 0), the requested stage2 granule
> > + * size is indicated in the stage1 granule size field of @mmfr0.
> > + * So, validate the stage1 granule size against the stage2 limit
> > + * granule size.
> > + * If lim_tgran2 == 0 (&& tgran2 != 0), the stage2 limit granule size
> > + * is indicated in the stage1 granule size field of @mmfr0_lim.
> > + * So, validate the requested stage2 granule size against the stage1
> > + * limit granule size.
> > + */
> > +
> > + /* Get the relevant stage1 granule size to validate tgran2 */
> > + if (tgran2 == 0)
> > + /* The requested stage1 granule size */
> > + rtgran1 = cpuid_feature_extract_field(mmfr0, f1, is_signed);
> > + else /* lim_tgran2 == 0 */
> > + /* The stage1 limit granule size */
> > + rtgran1 = cpuid_feature_extract_field(mmfr0_lim, f1, is_signed);
> > +
> > + /*
> > + * Adjust the value of rtgran1 to compare with stage2 granule size,
> > + * which indicates: 1: Not supported, 2: Supported, etc.
> > + */
> > + if (is_signed)
> > + /* For signed, -1: Not supported, 0: Supported, etc. */
> > + rtgran1 += 0x2;
> > + else
> > + /* For unsigned, 0: Not supported, 1: Supported, etc. */
> > + rtgran1 += 0x1;
> > +
> > + if ((tgran2 == 0) && (rtgran1 > lim_tgran2))
> > + /*
> > + * The requested stage1 granule size (== the requested stage2
> > + * granule size) is larger than the stage2 limit granule size.
> > + */
> > + return -E2BIG;
> > + else if ((lim_tgran2 == 0) && (tgran2 > rtgran1))
> > + /*
> > + * The requested stage2 granule size is larger than the stage1
> > + * limit granulze size (== the stage2 limit granule size).
> > + */
> > + return -E2BIG;
> > +
> > + return 0;
> > +}
> > +
> > +static int validate_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu,
> > + const struct id_reg_info *id_reg, u64 val)
> > +{
> > + u64 limit = id_reg->vcpu_limit_val;
> > + int ret;
>
> shouldn't you forbid reserved values for TGran4, 64?
I think what you meant is applied to all signed feature fields on
any ID registers. Considering "Principles of the ID scheme for fields
in ID registers" that is described in Arm ARM, lower value than -1
should not indicate more or higher level of features than KVM on the
host can support. In that sense, it doesn't matter (I would think
it won't cause any problems for guests). So, I rather chose not to do
that check.
> > +
> > + ret = aa64mmfr0_tgran2_check(ID_AA64MMFR0_TGRAN4_2_SHIFT, val, limit);
> > + if (ret)
> > + return ret;
> > +
> > + ret = aa64mmfr0_tgran2_check(ID_AA64MMFR0_TGRAN64_2_SHIFT, val, limit);
> > + if (ret)
> > + return ret;
> > +
> > + ret = aa64mmfr0_tgran2_check(ID_AA64MMFR0_TGRAN16_2_SHIFT, val, limit);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > static void init_id_aa64pfr0_el1_info(struct id_reg_info *id_reg)
> > {
> > u64 limit = id_reg->vcpu_limit_val;
> > @@ -625,6 +732,16 @@ static struct id_reg_info id_aa64isar1_el1_info = {
> > .get_reset_val = get_reset_id_aa64isar1_el1,
> > };
> >
> > +static struct id_reg_info id_aa64mmfr0_el1_info = {
> > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > + .ftr_check_types = S_FCT(ID_AA64MMFR0_TGRAN4_SHIFT, FCT_LOWER_SAFE) |
> > + S_FCT(ID_AA64MMFR0_TGRAN64_SHIFT, FCT_LOWER_SAFE) |
> the default?
They are signed fields, which are not a default.
> > + U_FCT(ID_AA64MMFR0_TGRAN4_2_SHIFT, FCT_IGNORE) |
> > + U_FCT(ID_AA64MMFR0_TGRAN64_2_SHIFT, FCT_IGNORE) |
> > + U_FCT(ID_AA64MMFR0_TGRAN16_2_SHIFT, FCT_IGNORE),
> maybe add comment telling the actual check is handled in the validate cb
That's a good point.
I will add a brief explanation about the validate cb.
Thanks,
Reiji
