| Message ID | 20211012043535.500493-4-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
On Mon, Oct 11, 2021 at 09:35:13PM -0700, Reiji Watanabe wrote: > Introduce arm64_check_features(), which does a basic validity checking > of an ID register value against the register's limit value that KVM > can support. > This function will be used by the following patches to check if an ID > register value that userspace tries to set can be supported by KVM on > the host. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > --- > arch/arm64/include/asm/cpufeature.h | 1 + > arch/arm64/kernel/cpufeature.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index ef6be92b1921..eda7ddbed8cf 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -631,6 +631,7 @@ void check_local_cpu_capabilities(void); > > u64 read_sanitised_ftr_reg(u32 id); > u64 __read_sysreg_by_encoding(u32 sys_id); > +int arm64_check_features(u32 sys_reg, u64 val, u64 limit); > > static inline bool cpu_supports_mixed_endian_el0(void) > { > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 6ec7036ef7e1..d146ef759435 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -3114,3 +3114,29 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, > return sprintf(buf, "Vulnerable\n"); > } > } > + > +/* > + * Check if all features that are indicated in the given ID register value > + * ('val') are also indicated in the 'limit'. Maybe use @<param> syntax to reference the parameters, even though this file doesn't seem to adopt that anywhere else... > + */ > +int arm64_check_features(u32 sys_reg, u64 val, u64 limit) > +{ > + struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > + const struct arm64_ftr_bits *ftrp; > + u64 exposed_mask = 0; > + > + if (!reg) > + return -ENOENT; > + > + for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > + if (arm64_ftr_value(ftrp, val) > arm64_ftr_value(ftrp, limit)) Hmm. Are we sure that '>' is the correct operator for all comparisons? It seems like we need a arm64_ftr_compare() function that takes arm64_ftr_bits.type and .sign into account. > + return -E2BIG; > + > + exposed_mask |= arm64_ftr_mask(ftrp); > + } > + > + if (val & ~exposed_mask) > + return -E2BIG; I'm not sure we want this. I think it implies that any RAO bits need to be cleared before calling this function, which could be inconvenient. Thanks, drew > + > + return 0; > +} > -- > 2.33.0.882.g93a45727a2-goog >
I've just noticed that I mistakenly dropped all the CC-list when I replied to Andrew's first email. So, I've added all of them back. I'm sorry for the mistake. The following email still includes all the discussion with Andrew except for the first comment about using @<param> for the source code comments of arm64_check_features(), which I will fix in v2. On Mon, Oct 18, 2021 at 11:25 PM Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Oct 18, 2021 at 03:59:35PM -0700, Reiji Watanabe wrote: > > > > > > +int arm64_check_features(u32 sys_reg, u64 val, u64 limit) > > > > > > +{ > > > > > > + struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > > > > > > + const struct arm64_ftr_bits *ftrp; > > > > > > + u64 exposed_mask = 0; > > > > > > + > > > > > > + if (!reg) > > > > > > + return -ENOENT; > > > > > > + > > > > > > + for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > > > > > > + if (arm64_ftr_value(ftrp, val) > arm64_ftr_value(ftrp, limit)) > > > > > > > > > > Hmm. Are we sure that '>' is the correct operator for all comparisons? It > > > > > seems like we need a arm64_ftr_compare() function that takes > > > > > arm64_ftr_bits.type and .sign into account. > > > > > > > > Thank you for bringing up this point. > > > > > > > > I agree that '>' is not always the appropriate operator (mostly > > > > inappropriate) for cases where .type is not FTR_LOWER_SAFE. > > > > (The code takes .sign into account though) > > > > > > > > Looking at each field with .type != FTR_LOWER_SAFE, I don't think > > > > we can simply determine how to check the field based on .type > > > > because the appropriate way highly depends on the field. > > > > > > > > - For ID_AA64DFR0_EL1.PMUVER/DEBUGVER (FTR_EXACT), > > > > we can simply use '>'. > > > > - For ID_AA64ISAR1_EL1.APA/API (FTR_EXACT), > > > > it looks like those fields must match between val and limit or > > > > the ones in val can be zero. > > > > - For ID_AA64MMFR0.TGRAN4_2/TGRAN64_2/TGRAN16_2 (FTR_EXACT), > > > > when the field indicates 0x2 (4KB/64KB/16KB granule support at > > > > stage 2) in the limit, 0x1 (4KB/64KB/16KB granule not supported > > > > at stage 2) in val can be allowed, but 0x0 (identified in the > > > > ID_AA64MMFR0_EL1.TGRAN4/TGRAN64/TGRAN16) cannot simply be allowed. > > > > - For ID_AA64MMFR1_EL1.SPECSEI (FTR_HIGHER_SAFE), > > > > it looks like '<' is a safer option (I'm not so sure how it is > > > > actually used though). > > > > > > > > Currently, I am looking at including those fields in ignore_mask of > > > > their id_reg_info (for the function ignore the fields) and updating > > > > its validate function for them (except for ID_AA64DFR0_EL1.PMUVER > > > > and DEBUGVER, which we can use the function for). > > > > > > Yes, in the least I think we need to document the limitations of this > > > arm64_check_features() function in a comment. I still wonder if we > > > shouldn't implement a arm64_ftr_compare() which makes appropriate > > > comparisons, even if it has to key off the register fields to determine > > > what that comparison should be. Either that, or maybe we shouldn't try > > > to introduce a common arm64_check_features() at all and we should just > > > handle each register needed by KVM in its specific way. > > > > I understand your point and I completely agree with your comment above. > > Yes, the function's comparison should be done based on arm64_ftr_bits > > as long as it is in the file using arm64_ftr_bits. > > I will fix this in v2. > > > > > > > > > > + return -E2BIG; > > > > > > + > > > > > > + exposed_mask |= arm64_ftr_mask(ftrp); > > > > > > + } > > > > > > + > > > > > > + if (val & ~exposed_mask) > > > > > > + return -E2BIG; > > > > > > > > > > I'm not sure we want this. I think it implies that any RAO bits need to be > > > > > cleared before calling this function, which could be inconvenient. > > > > > > > > # Does 'RA0' mean RAZ (read as zero) ? > > > > > > RAO (O, not 0) means read-as-one. > > > > Uh, that was O, I see... > > That's why I wasn't able to find the word in ARM Glossary. Thank you ! > > > > > > > > The code is used to check if any feature fields that the host > > > > kernel doesn't recognize (arm64_ftr_bits is not defined for > > > > the field) yet are not exposed to the guest. So, removing the > > > > checking would be inconvenient for the use case (in the current > > > > version of the series at least). > > > > > > > > On the other hand, I agree it might be inconvenient for some > > > > potential use cases. One of the solutions that I could think of > > > > now would be making the checking conditional by adding another > > > > parameter (so its caller can choose the behavior). > > > > I will be happy to make the change when we need it. > > > > > > I agree with wanting to avoid bits that shouldn't be set from being set, > > > but the read-as-one bits will be set, but they won't show up in ftr_bits. > > > > I would guess you meant RES1 rather than RAO (or not ?). > > (I would think RAO is more like for a specific CPU hardware rather > > than for CPU architecture specification) > > > > For every single register in cpufeature.c that has arm64_ftr_bits, > > I checked 'Field descriptions' from the page below. > > https://developer.arm.com/documentation/ddi0595/2021-03/AArch64-Registers > > > > I found one register that has a RES1 field, which is bit 31 of CTR_EL0. > > (I didn't find any field that is defined as RAO though) > > The RES1 field is actually defined in ftr_ctr as follows. > > ------------------------------------------------------------------- > > <arch/arm64/kernel/cpufeature.c> > > > > <...> > > static const struct arm64_ftr_bits ftr_ctr[] = { > > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ > > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, > > CTR_DIC_SHIFT, 1, 1), > > ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, > > CTR_IDC_SHIFT, 1, 1), > > <...> > > ------------------------------------------------------------------- > > > > It looks like RES1 field is okay. > > Or do you have any specific registers/fields that you are > > concerned about ? > > You're right, I meant the RES1 bits that we set ourselves, not bits HW > presents as ones. And, as you've confirmed RES1 is covered by > arm64_ftr_bits, then I apologize for the noise. No, it wasn't noise. It was good for me to confirm that. Thank you so much for the comment. Regards, Reiji
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index ef6be92b1921..eda7ddbed8cf 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -631,6 +631,7 @@ void check_local_cpu_capabilities(void); u64 read_sanitised_ftr_reg(u32 id); u64 __read_sysreg_by_encoding(u32 sys_id); +int arm64_check_features(u32 sys_reg, u64 val, u64 limit); static inline bool cpu_supports_mixed_endian_el0(void) { diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 6ec7036ef7e1..d146ef759435 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -3114,3 +3114,29 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, return sprintf(buf, "Vulnerable\n"); } } + +/* + * Check if all features that are indicated in the given ID register value + * ('val') are also indicated in the 'limit'. + */ +int arm64_check_features(u32 sys_reg, u64 val, u64 limit) +{ + struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); + const struct arm64_ftr_bits *ftrp; + u64 exposed_mask = 0; + + if (!reg) + return -ENOENT; + + for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { + if (arm64_ftr_value(ftrp, val) > arm64_ftr_value(ftrp, limit)) + return -E2BIG; + + exposed_mask |= arm64_ftr_mask(ftrp); + } + + if (val & ~exposed_mask) + return -E2BIG; + + return 0; +}
Introduce arm64_check_features(), which does a basic validity checking of an ID register value against the register's limit value that KVM can support. This function will be used by the following patches to check if an ID register value that userspace tries to set can be supported by KVM on the host. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/cpufeature.h | 1 + arch/arm64/kernel/cpufeature.c | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+)