Hi Oliver,
On 3/23/22 12:22 PM, Oliver Upton wrote:
Hi Reiji,
On Thu, Mar 10, 2022 at 08:47:48PM -0800, Reiji Watanabe wrote:
Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
and save ID registers' sanitized value in the array at KVM_CREATE_VM.
Use the saved ones when ID registers are read by the guest or
userspace (via KVM_GET_ONE_REG).
Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
---
arch/arm64/include/asm/kvm_host.h | 12 ++++++
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/sys_regs.c | 65 ++++++++++++++++++++++++-------
3 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2869259e10c0..c041e5afe3d2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -101,6 +101,13 @@ struct kvm_s2_mmu {
struct kvm_arch_memory_slot {
};
+/*
+ * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
+ * where 0<=crm<8, 0<=op2<8.
Doesn't the Feature ID register scheme only apply to CRm={1-7},
op2={0-7}? I believe CRm=0, op2={1-4,7} are in fact UNDEFINED, not RAZ
like the other ranges. Furthermore, the registers that are defined in
that range do not go through the read_id_reg() plumbing.
Will fix this.
+ */
+#define KVM_ARM_ID_REG_MAX_NUM 64
+#define IDREG_IDX(id) ((sys_reg_CRm(id) << 3) | sys_reg_Op2(id))
+
struct kvm_arch {
struct kvm_s2_mmu mmu;
@@ -137,6 +144,9 @@ struct kvm_arch {
/* Memory Tagging Extension enabled for the guest */
bool mte_enabled;
bool ran_once;
+
+ /* ID registers for the guest. */
+ u64 id_regs[KVM_ARM_ID_REG_MAX_NUM];
This is a decently large array. Should we embed it in kvm_arch or
allocate at init?
What is the reason why you think you might want to allocate it at init ?
[...]
+
+/*
+ * Set the guest's ID registers that are defined in sys_reg_descs[]
+ * with ID_SANITISED() to the host's sanitized value.
+ */
+void set_default_id_regs(struct kvm *kvm)
nit, more relevant if you take the above suggestion: maybe call it
kvm_init_id_regs()?
+{
+ int i;
+ u32 id;
+ const struct sys_reg_desc *rd;
+ u64 val;
+
+ for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
You could avoid walking the entire system register table, since we
already know the start and end values for the Feature ID register range.>
maybe:
#define FEATURE_ID_RANGE_START SYS_ID_PFR0_EL1
#define FEATURE_ID_RANGE_END sys_reg(3, 0, 0, 7, 7)
u32 sys_reg;
for (sys_reg = FEATURE_ID_RANGE_START; sys_reg <= FEATURE_ID_RANGE_END; sys_reg++)
But, it depends on if this check is necessary:
+ rd = &sys_reg_descs[i];
+ if (rd->access != access_id_reg)
+ /* Not ID register, or hidden/reserved ID register */
+ continue;
Which itself is dependent on whether KVM is going to sparsely or
verbosely define its feature filtering tables per the other thread. So
really only bother with this if that is the direction you're going.
Even just going through for ID register ranges, we should do the check
to skip hidden/reserved ID registers (not to call read_sanitised_ftr_reg).
Yes, it's certainly possible to avoid walking the entire system register,
and I will fix it. The reason why I didn't care it so much was just
because the code (walking the entire system register) will be removed by
the following patches:)
Thanks,
Reiji
