Making KVM_GET_ONE_REG/KVM_SET_ONE_REG generic.

Avi has indicated that this is the future. For now, make it dependent on KVM_HAVE_ONE_REG (and define that for PPC and S/390).

Other x86 state:
Control registers: ok. Should userspace be careful to set registers
in legal ways only? i.e. cannot set cr3[0:11] if cr4.pae=0, or vice versa, so need three writes?

Quoted 4 lines · Avi Kivity

Other x86 state:
Control registers: ok. Should userspace be careful to set registers
in legal ways only? i.e. cannot set cr3[0:11] if cr4.pae=0, or vice versa, so need three writes?

The principle I'm hoping we can hold to for ARM is that the kernel only exposes state in such a way that it's always possible for userspace to migrate it with a simple "read everything, send to destination, write everything", ie without needing to know anything of the semantics of any of these registers.

This means that registers which have access controls (eg "can't write this unless you wrote to that other one first") should not enforce those checks for userspace get/set. More significantly, it means that registers with odd behaviour, like "write 1 to clear" or "register A selects which of an array of underlying registers is exposed in register B" are not directly exposed at all. Instead the kernel provides some other (ersatz) register indexes which let userspace do plain get/set on the underlying state.

The idea is that then migration depends only on whether the destination kernel supports all the registers the source kernel does, and we avoid extra dependencies on the source and destination qemu. (Most of the state being transferred is of no interest to userspace at all.) [It also makes write-multiple easier to use.]

Instead of overloading x86's KVM_GET_MSRS/KVM_SET_MSRS. The only basic difference is that the ids are 64 bit.

Signed-off-by Rusty Russell <[email protected]>

arch/arm/include/asm/kvm.h +12 -26

index d040a2a..9838456 100644
@@ -85,35 +85,21 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
-/* Based on x86, but we use KVM_GET_VCPU_MSR_INDEX_LIST. */
-struct kvm_msr_entry {
-       __u32 index;
-       __u32 reserved;
-       __u64 data;
-};
-
-/* for KVM_GET_MSRS and KVM_SET_MSRS */
-struct kvm_msrs {
-       __u32 nmsrs; /* number of msrs in entries */
-       __u32 pad;
-
-       struct kvm_msr_entry entries[0];
-};
-
 /* for KVM_VCPU_GET_MSR_INDEX_LIST */
 struct kvm_msr_list {
-       __u32 nmsrs; /* number of msrs in entries */
-       __u32 indices[0];
+       __u64 nmsrs; /* number of msrs in entries */
+       __u64 indices[0];
 };
 
-/* If you need to interpret the index values, here's the key. */
-#define KVM_ARM_MSR_COPROC_MASK                0xFFFF0000
-#define KVM_ARM_MSR_64_BIT_MASK                0x00008000
-#define KVM_ARM_MSR_64_OPC1_MASK       0x000000F0
-#define KVM_ARM_MSR_64_CRM_MASK                0x0000000F
-#define KVM_ARM_MSR_32_CRM_MASK                0x0000000F
-#define KVM_ARM_MSR_32_OPC2_MASK       0x00000070
-#define KVM_ARM_MSR_32_CRN_MASK                0x00000780
-#define KVM_ARM_MSR_32_OPC1_MASK       0x00003800
+/* If you need to interpret the index values, here are the bit offsets. */
+#define KVM_REG_ARM_COPROC_START       16      /* Mask: 0xFFFF0000 */
+#define KVM_REG_ARM_32_OPC2_START      0       /* Mask: 0x00000007 */
+#define KVM_REG_ARM_32_OPC2_LEN                3
+#define KVM_REG_ARM_OPC1_START         3       /* Mask: 0x00000078 */
+#define KVM_REG_ARM_OPC1_LEN           4
+#define KVM_REG_ARM_CRM_START          7       /* Mask: 0x00000780 */
+#define KVM_REG_ARM_CRM_LEN            4
+#define KVM_REG_ARM_32_CRN_START       11      /* Mask: 0x00007800 */
+#define KVM_REG_ARM_32_CRN_LEN         4
 
 #endif /* __ARM_KVM_H__ */

arch/arm/include/asm/kvm_coproc.h +1 -5

index 894574c..899b3d3 100644
@@ -28,11 +28,7 @@ int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
 
-int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
-                    struct kvm_msr_entry __user *entries, u32 num);
-int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
-                    struct kvm_msr_entry __user *entries, u32 num);
 unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu);
-int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u32 __user *uindices);
+int kvm_arm_copy_msrindices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 void kvm_coproc_table_init(void);
 #endif /* __ARM_KVM_COPROC_H__ */

arch/arm/include/asm/kvm_host.h +1 -0

index 1c0fa75..7548c95 100644
@@ -25,6 +25,7 @@
 #define KVM_MEMORY_SLOTS 32
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
+#define KVM_HAVE_ONE_REG
 
 #define NUM_FEATURES 0
 

arch/arm/kvm/arm.c +0 -14

index 4248aa1..55a2995 100644
@@ -726,20 +726,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
                        return -E2BIG;
                return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
        }
-       case KVM_GET_MSRS: {
-               struct kvm_msrs msrs;
-               struct kvm_msrs __user *umsrs = argp;
-               if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
-                       return -EFAULT;
-               return kvm_arm_get_msrs(vcpu, umsrs->entries, msrs.nmsrs);
-       }
-       case KVM_SET_MSRS: {
-               struct kvm_msrs msrs;
-               struct kvm_msrs __user *umsrs = argp;
-               if (copy_from_user(&msrs, umsrs, sizeof(msrs)) != 0)
-                       return -EFAULT;
-               return kvm_arm_set_msrs(vcpu, umsrs->entries, msrs.nmsrs);
-       }
        default:
                return -EINVAL;
        }

arch/arm/kvm/coproc.c +88 -116

index 824e5a3..99de71b 100644
@@ -568,41 +568,53 @@ int kvm_handle_cp15_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
  *****************************************************************************/
 
 /* Given a simple mask, get those bits. */
-static inline u32 get_bits(u32 index, u32 mask)
+static inline u32 get_bits(u32 index, u32 start, u32 len)
 {
-       return (index & mask) >> (ffs(mask) - 1);
+       return (index >> start) & ((1 << len)-1);
 }
 
-static void index_to_params(u32 index, struct coproc_params *params)
+static bool index_to_params(u64 index, struct coproc_params *params)
 {
-       if (get_bits(index, KVM_ARM_MSR_64_BIT_MASK)) {
+       switch (index & KVM_REG_SIZE_MASK) {
+       case KVM_REG_SIZE_U32:
+               params->is_64bit = false;
+               params->CRn = get_bits(index, KVM_REG_ARM_32_CRN_START,
+                                      KVM_REG_ARM_32_CRN_LEN);
+               params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
+                                      KVM_REG_ARM_CRM_LEN);
+               params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
+                                      KVM_REG_ARM_OPC1_LEN);
+               params->Op2 = get_bits(index, KVM_REG_ARM_32_OPC2_START,
+                                      KVM_REG_ARM_32_OPC2_LEN);
+               return true;
+       case KVM_REG_SIZE_U64:
                params->is_64bit = true;
-               params->CRm = get_bits(index, KVM_ARM_MSR_64_CRM_MASK);
-               params->Op1 = get_bits(index, KVM_ARM_MSR_64_OPC1_MASK);
+               params->CRm = get_bits(index, KVM_REG_ARM_CRM_START,
+                                      KVM_REG_ARM_CRM_LEN);
+               params->Op1 = get_bits(index, KVM_REG_ARM_OPC1_START,
+                                      KVM_REG_ARM_OPC1_LEN);
                params->Op2 = 0;
                params->CRn = 0;
-       } else {
-               params->is_64bit = false;
-               params->CRn = get_bits(index, KVM_ARM_MSR_32_CRN_MASK);
-               params->CRm = get_bits(index, KVM_ARM_MSR_32_CRM_MASK);
-               params->Op1 = get_bits(index, KVM_ARM_MSR_32_OPC1_MASK);
-               params->Op2 = get_bits(index, KVM_ARM_MSR_32_OPC2_MASK);
+               return true;
+       default:
+               return false;
        }
 }
 
 /* Decode an index value, and find the cp15 coproc_reg entry. */
 static const struct coproc_reg *index_to_coproc_reg(struct kvm_vcpu *vcpu,
-                                                   u32 index)
+                                                   u64 index)
 {
        size_t num;
        const struct coproc_reg *table, *r;
        struct coproc_params params;
 
        /* We only do cp15 for now. */
-       if (get_bits(index, KVM_ARM_MSR_COPROC_MASK != 15))
+       if (get_bits(index, KVM_REG_ARM_COPROC_START, 16) != 15)
                return NULL;
 
-       index_to_params(index, &params);
+       if (!index_to_params(index, &params))
+               return NULL;
 
        table = get_target_table(vcpu->arch.target, &num);
        r = find_reg(&params, table, num);

@@ -689,30 +701,54 @@ static struct coproc_reg invariant_cp15[] = {
        { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
 };
 
-static int get_invariant_cp15(u32 index, u64 *val)
+static int reg_from_user(void *val, const void __user *uaddr, u64 index)
+{
+       /* This Just Works because we are little endian. */
+       if (copy_from_user(val, uaddr, KVM_REG_LEN(index)) != 0)
+               return -EFAULT;
+       return 0;
+}
+
+static int reg_to_user(void __user *uaddr, const void *val, u64 index)
+{
+       /* This Just Works because we are little endian. */
+       if (copy_to_user(uaddr, val, KVM_REG_LEN(index)) != 0)
+               return -EFAULT;
+       return 0;
+}
+
+static int get_invariant_cp15(u64 index, void __user *uaddr)
 {
        struct coproc_params params;
        const struct coproc_reg *r;
 
-       index_to_params(index, &params);
+       if (!index_to_params(index, &params))
+               return -ENOENT;
+
        r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
        if (!r)
                return -ENOENT;
 
-       *val = r->val;
-       return 0;
+       return reg_to_user(uaddr, &r->val, index);
 }
 
-static int set_invariant_cp15(u32 index, u64 val)
+static int set_invariant_cp15(u64 index, void __user *uaddr)
 {
        struct coproc_params params;
        const struct coproc_reg *r;
+       int err;
+       u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
 
-       index_to_params(index, &params);
+       if (!index_to_params(index, &params))
+               return -ENOENT;
        r = find_reg(&params, invariant_cp15, ARRAY_SIZE(invariant_cp15));
        if (!r)
                return -ENOENT;
 
+       err = reg_from_user(&val, uaddr, index);
+       if (err)
+               return err;
+
        /* This is what we mean by invariant: you can't change it. */
        if (r->val != val)
                return -EINVAL;

@@ -720,95 +756,36 @@ static int set_invariant_cp15(u32 index, u64 val)
        return 0;
 }
 
-static int get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *val)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        const struct coproc_reg *r;
+       void __user *uaddr = (void __user *)(long)reg->addr;
 
-       r = index_to_coproc_reg(vcpu, index);
-       if (!r)
-               return get_invariant_cp15(index, val);
-
-       *val = vcpu->arch.cp15[r->reg];
-       if (r->is_64)
-               *val |= ((u64)vcpu->arch.cp15[r->reg+1]) << 32;
-       return 0;
-}
-
-static int set_msr(struct kvm_vcpu *vcpu, u32 index, u64 val)
-{
-       const struct coproc_reg *r;
+       if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
+               return -EINVAL;
 
-       r = index_to_coproc_reg(vcpu, index);
+       r = index_to_coproc_reg(vcpu, reg->id);
        if (!r)
-               return set_invariant_cp15(index, val);
+               return get_invariant_cp15(reg->id, uaddr);
 
-       vcpu->arch.cp15[r->reg] = val;
-       if (r->is_64)
-               vcpu->arch.cp15[r->reg+1] = (val >> 32);
-       return 0;
-}
-
-/* Return user adddress to get/set value from. */
-static u64 __user *get_umsr(struct kvm_msr_entry __user *uentry, u32 *idx)
-{
-       struct kvm_msr_entry entry;
-
-       if (copy_from_user(&entry, uentry, sizeof(entry)))
-               return NULL;
-       *idx = entry.index;
-       return &uentry->data;
+       /* Note: copies two regs if size is 64 bit. */
+       return reg_to_user(uaddr, &vcpu->arch.cp15[r->reg], reg->id);
 }
 
-/**
- * kvm_arm_get_msrs - copy one or more special registers to userspace.
- * @vcpu: the vcpu
- * @entries: the array of entries
- * @num: the number of entries
- */
-int kvm_arm_get_msrs(struct kvm_vcpu *vcpu,
-                    struct kvm_msr_entry __user *entries, u32 num)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
-       u32 i, index;
-       u64 val;
-       u64 __user *uval;
-       int ret;
+       const struct coproc_reg *r;
+       void __user *uaddr = (void __user *)(long)reg->addr;
 
-       for (i = 0; i < num; i++) {
-               uval = get_umsr(&entries[i], &index);
-               if (!uval)
-                       return -EFAULT;
-               if ((ret = get_msr(vcpu, index, &val)) != 0)
-                       return ret;
-               if (put_user(val, uval))
-                       return -EFAULT;
-       }
-       return 0;
-}
+       if ((reg->id & KVM_REG_ARCH_MASK) != KVM_REG_ARM)
+               return -EINVAL;
 
-/**
- * kvm_arm_set_msrs - copy one or more special registers from userspace.
- * @vcpu: the vcpu
- * @entries: the array of entries
- * @num: the number of entries
- */
-int kvm_arm_set_msrs(struct kvm_vcpu *vcpu,
-                    struct kvm_msr_entry __user *entries, u32 num)
-{
-       u32 i, index;
-       u64 val;
-       u64 __user *uval;
-       int ret;
+       r = index_to_coproc_reg(vcpu, reg->id);
+       if (!r)
+               return set_invariant_cp15(reg->id, uaddr);
 
-       for (i = 0; i < num; i++) {
-               uval = get_umsr(&entries[i], &index);
-               if (!uval)
-                       return -EFAULT;
-               if (copy_from_user(&val, uval, sizeof(val)) != 0)
-                       return -EFAULT;
-               if ((ret = set_msr(vcpu, index, val)) != 0)
-                       return ret;
-       }
-       return 0;
+       /* Note: copies two regs if size is 64 bit */
+       return copy_from_user(&vcpu->arch.cp15[r->reg], uaddr, reg->id);
 }
 
 static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)

@@ -827,29 +804,24 @@ static int cmp_reg(const struct coproc_reg *i1, const struct coproc_reg *i2)
        return i1->Op2 - i2->Op2;
 }
 
-/* Puts in the position indicated by mask (assumes val fits in mask) */
-static inline u32 set_bits(u32 val, u32 mask)
-{
-       return val << (ffs(mask)-1);
-}
-
-static u32 cp15_to_index(const struct coproc_reg *reg)
+static u64 cp15_to_index(const struct coproc_reg *reg)
 {
-       u32 val = set_bits(15, KVM_ARM_MSR_COPROC_MASK);
+       u64 val = (15 << KVM_REG_ARM_COPROC_START);
        if (reg->is_64) {
-               val |= set_bits(1, KVM_ARM_MSR_64_BIT_MASK);
-               val |= set_bits(reg->Op1, KVM_ARM_MSR_64_OPC1_MASK);
-               val |= set_bits(reg->CRm, KVM_ARM_MSR_64_CRM_MASK);
+               val |= KVM_REG_SIZE_U64;
+               val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
+               val |= (reg->CRm << KVM_REG_ARM_CRM_START);
        } else {
-               val |= set_bits(reg->Op1, KVM_ARM_MSR_32_OPC1_MASK);
-               val |= set_bits(reg->Op2, KVM_ARM_MSR_32_OPC2_MASK);
-               val |= set_bits(reg->CRm, KVM_ARM_MSR_32_CRM_MASK);
-               val |= set_bits(reg->CRn, KVM_ARM_MSR_32_CRN_MASK);
+               val |= KVM_REG_SIZE_U32;
+               val |= (reg->Op1 << KVM_REG_ARM_OPC1_START);
+               val |= (reg->Op2 << KVM_REG_ARM_32_OPC2_START);
+               val |= (reg->CRm << KVM_REG_ARM_CRM_START);
+               val |= (reg->CRn << KVM_REG_ARM_32_CRN_START);
        }
        return val;
 }
 
-static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
+static bool copy_reg_to_user(const struct coproc_reg *reg, u64 __user **uind)
 {
        if (!*uind)
                return true;

@@ -862,7 +834,7 @@ static bool copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
 }
 
 /* Assumed ordered tables, see kvm_coproc_table_init. */
-static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
+static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
 {
        const struct coproc_reg *i1, *i2, *end1, *end2;
        unsigned int total = 0;

@@ -911,7 +883,7 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
  */
 unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
 {
-       return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u32 __user *)NULL);
+       return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
 }
 
 /**

This is a generic interface to find out what you can use KVM_GET_ONE_REG/KVM_SET_ONE_REG on. Arhs need to define KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and kvm_arch_copy_reg_indices() functions.

It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl, and uses 64-bit indices.

Signed-off-by Rusty Russell <[email protected]>

include/linux/kvm.h +6 -0

index 209b432..b607f60 100644
@@ -906,7 +906,7 @@ struct kvm_s390_ucas_mapping {
 #define KVM_KVMCLOCK_CTRL        _IO(KVMIO,   0xad)
 #define KVM_ARM_VCPU_INIT        _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
 #define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
+#define KVM_VCPU_GET_REG_LIST    _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3         (1 << 1)

@@ -958,4 +958,9 @@ struct kvm_assigned_msix_entry {
        __u16 padding[3];
 };
 
+/* For KVM_VCPU_GET_REG_LIST. */
+struct kvm_reg_list {
+       __u64 n; /* number of regs */
+       __u64 reg[0];
+};
 #endif /* __LINUX_KVM_H */

include/linux/kvm_host.h +4 -1

index f3d43c3..daa3838 100644
@@ -572,7 +572,10 @@ int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 #define KVM_REG_LEN(index)                                             \
        (1U << (((index) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
 #endif
-
+#ifdef KVM_HAVE_REG_LIST
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
+#endif
 void kvm_free_physmem(struct kvm *kvm);
 
 void *kvm_kvzalloc(unsigned long size);

virt/kvm/kvm_main.c +10 -0

index ad67cf4..b220c74 100644
@@ -1949,6 +1949,23 @@ out_free2:
                break;
        }
 #endif
+#ifdef KVM_HAVE_REG_LIST
+       case KVM_VCPU_GET_REG_LIST: {
+               struct kvm_reg_list __user *user_list = argp;
+               struct kvm_reg_list reg_list;
+               unsigned n;
+
+               if (copy_from_user(&reg_list, user_list, sizeof reg_list))
+                       return -EFAULT;
+               n = reg_list.n;
+               reg_list.n = kvm_arch_num_regs(vcpu);

This trivially replaces the arm-specific KVM_VCPU_GET_MSR_INDEX_LIST.

Signed-off-by Rusty Russell <[email protected]>

include/linux/kvm.h +0 -1

index 209b432..b607f60 100644
@@ -906,6 +906,5 @@ struct kvm_s390_ucas_mapping {
 #define KVM_KVMCLOCK_CTRL        _IO(KVMIO,   0xad)
 #define KVM_ARM_VCPU_INIT        _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
-#define KVM_VCPU_GET_MSR_INDEX_LIST    _IOWR(KVMIO, 0xaf, struct kvm_msr_list)
 #define KVM_VCPU_GET_REG_LIST    _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)

arch/arm/include/asm/kvm.h +0 -6

index 9838456..a3daa67 100644
@@ -85,12 +85,6 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
-/* for KVM_VCPU_GET_MSR_INDEX_LIST */
-struct kvm_msr_list {
-       __u64 nmsrs; /* number of msrs in entries */
-       __u64 indices[0];
-};
-
 /* If you need to interpret the index values, here are the bit offsets. */
 #define KVM_REG_ARM_COPROC_START       16      /* Mask: 0xFFFF0000 */
 #define KVM_REG_ARM_32_OPC2_START      0       /* Mask: 0x00000007 */

arch/arm/include/asm/kvm_host.h +1 -0

index 7548c95..64fa65f 100644
@@ -26,6 +26,7 @@
 #define KVM_PRIVATE_MEM_SLOTS 4
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 #define KVM_HAVE_ONE_REG
+#define KVM_HAVE_REG_LIST
 
 #define NUM_FEATURES 0
 

arch/arm/kvm/arm.c +0 -15

index 55a2995..fe98b77 100644
@@ -711,21 +711,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
                return kvm_vcpu_set_target(vcpu, &init);
 
        }
-       case KVM_VCPU_GET_MSR_INDEX_LIST: {
-               struct kvm_msr_list __user *user_msr_list = argp;
-               struct kvm_msr_list msr_list;
-               unsigned n;
-
-               if (copy_from_user(&msr_list, user_msr_list, sizeof msr_list))
-                       return -EFAULT;
-               n = msr_list.nmsrs;
-               msr_list.nmsrs = kvm_arm_num_guest_msrs(vcpu);
-               if (copy_to_user(user_msr_list, &msr_list, sizeof msr_list))
-                       return -EFAULT;
-               if (n < msr_list.nmsrs)
-                       return -E2BIG;
-               return kvm_arm_copy_msrindices(vcpu, user_msr_list->indices);
-       }
        default:
                return -EINVAL;
        }

arch/arm/kvm/coproc.c +2 -2

index 99de71b..0b9e8b4 100644
@@ -877,21 +877,21 @@ static int walk_msrs(struct kvm_vcpu *vcpu, u64 __user *uind)
 }
 
 /**
- * kvm_arm_num_guest_msrs - how many registers do we present via KVM_GET_MSR
+ * kvm_arch_num_regs - how many registers do we present via KVM_GET_ONE_REG
  *
  * This is for special registers, particularly cp15.
  */
-unsigned long kvm_arm_num_guest_msrs(struct kvm_vcpu *vcpu)
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu)
 {
        return ARRAY_SIZE(invariant_cp15) + walk_msrs(vcpu, (u64 __user *)NULL);
 }
 
 /**

No structures at all any more.

Quoted 2 lines · Rusty Russell

No structures at all any more.

I fail to see the great benefit of all this. The code is certainly not easier to read and it's certainly not more clear what is going on.

Is this simply so we don't have to copy header files into QEMU when QEMU needs to support a new architecture? We have to do that anyway no? Do core registers really often change and often we need new registers for an existing architecture?

I can see this for cp15 stuff, but core registers?

No structures at all any more.

Quoted 1 line · Rusty Russell

No structures at all any more.

I'm not fussed whether we use structs for the core regs or not; they're not exactly going to change in future so it's purely a question of whether you think it's aesthetically prettier to have everything funneled through the one ioctl.

arch/arm/kvm/guest.c int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) · line 288

Quoted 3 lines

arch/arm/kvm/guest.c +3 -0

+       /* Coprocessor 0 means we want a core register. */
+       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+               return set_core_reg(vcpu, reg);

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

Quoted 4 lines

Quoted 3 lines

arch/arm/kvm/guest.c +3 -0

+       /* Coprocessor 0 means we want a core register. */
+       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+               return set_core_reg(vcpu, reg);

Quoted 4 lines

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

OK, changed that too (16).

Quoted 4 lines · Rusty Russell

Quoted 4 lines

Quoted 3 lines

arch/arm/kvm/guest.c +3 -0

+       /* Coprocessor 0 means we want a core register. */
+       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+               return set_core_reg(vcpu, reg);

Quoted 6 lines

Quoted 4 lines

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

OK, changed that too (16).

And tomorrow they will add 16.

Quoted 11 lines · Avi Kivity

Quoted 10 lines · Rusty Russell

Quoted 8 lines

arch/arm/kvm/guest.c int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) · line 288

Quoted 3 lines

arch/arm/kvm/guest.c +3 -0

+       /* Coprocessor 0 means we want a core register. */
+       if ((u32)reg->id >> KVM_REG_ARM_COPROC_START == 0)
+               return set_core_reg(vcpu, reg);

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

OK, changed that too (16).

Quoted 1 line

And tomorrow they will add 16.

Not possible in the instruction encoding :-) We haven't used anywhere near all the coprocessors (even given we've let the vendors have 0..7, ARM itself uses only 10 and 11 for the FPU, 14 for debug/perf and 15 for system control (and 14 and 15 still have lots of spare space).

Quoted 12 lines · Peter Maydell

Quoted 10 lines · Avi Kivity

Quoted 7 lines · Rusty Russell

Quoted 4 lines · Peter Maydell

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

OK, changed that too (16).

And tomorrow they will add 16.

Quoted 5 lines

Not possible in the instruction encoding :-) We haven't used anywhere near all the coprocessors (even given we've let the vendors have 0..7, ARM itself uses only 10 and 11 for the FPU, 14 for debug/perf and 15 for system control (and 14 and 15 still have lots of spare space).

Yeah, but folding core registers under coprocessors feels just too fishy, so I think we should have a separate field.

Quoted 13 lines · Christoffer Dall

Quoted 12 lines · Peter Maydell

Quoted 10 lines · Avi Kivity

Quoted 7 lines · Rusty Russell

Quoted 4 lines · Peter Maydell

...but if we do go this path, you can't use coprocessor 0 to mean core register -- cp0 could be a valid coprocessor (the ARM ARM reserves cp0..cp7 for "vendor specific features"). Use something outside 0..15.

OK, changed that too (16).

And tomorrow they will add 16.

Quoted 8 lines

Quoted 5 lines

Not possible in the instruction encoding :-) We haven't used anywhere near all the coprocessors (even given we've let the vendors have 0..7, ARM itself uses only 10 and 11 for the FPU, 14 for debug/perf and 15 for system control (and 14 and 15 still have lots of spare space).

Yeah, but folding core registers under coprocessors feels just too fishy, so I think we should have a separate field.

I never really thought of the top half of the index encoding as being particularly a coprocessor-number specific thing in the first place. It's just 16 bits of "what is this thing anyway?", where each coprocessor gets a bit of the space, and so will the GIC, and the VFP regs, and so on. We just happened to use 0..15 of the "what is this?" space for cp0..cp15.

(Incidentally, the term "coprocessor" is now basically just a historical artefact. The bits of the CPU you get at via the "coprocessor" registers and instruction encoding space are not separate functional units, they're part of the core.)

that's fine, but then the #define's shouldn't be called something with COPROC in their names.

No structures at all any more.

Note: the encoding of general registers makes sense, but it's not completely logical so the code is quite messy. It would actually be far neater to expose the struct kvm_vcpu_regs, and use offsets into that as the ABI.

This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.

Mingled in these patches is the conversion of the latest KVM ARM code, which is the first proposed user: by the end, we use these accessors for *every* register and piece of state.

GET_MULTI/SET_MULTI is an obvious extension which is not yet implemented.

Quoted 9 lines · Rusty Russell

This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.

Mingled in these patches is the conversion of the latest KVM ARM code, which is the first proposed user: by the end, we use these accessors for *every* register and piece of state.

GET_MULTI/SET_MULTI is an obvious extension which is not yet implemented.

Hi Rusty.

I don't see any api.txt patches in here, did I miss them? (your cover letter doesn't include a diffstat...)

I don't particularly have comments on the implementation but I would like to see the kernel-userspace ABI clearly described rather than having to infer it from the code. (including the complete set of index mappings for the ARM registers that will use this)

Type: vcpu ioctl

Documentation/virtual/kvm/api.txt +1 -2

-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
 Returns: 0 on success; -1 on error
 Errors:
-  E2BIG:     the msr index list is too big to fit in the array specified by

Documentation/virtual/kvm/api.txt the virtualized real-mode area (VRMA) facility, the kernel will · line 2003

Quoted 3 lines

Documentation/virtual/kvm/api.txt +1 -1

-Capability: basic
+Capability: KVM_CAP_REG_LIST
 Architectures: arm

all

Quoted 7 lines

Type: vcpu ioctl

Documentation/virtual/kvm/api.txt +1 -2

-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
 Returns: 0 on success; -1 on error
 Errors:
-  E2BIG:     the msr index list is too big to fit in the array specified by

Documentation/virtual/kvm/api.txt the virtualized real-mode area (VRMA) facility, the kernel will · line 2014

Quoted 10 lines

Documentation/virtual/kvm/api.txt +4 -3

-             the user.
+  E2BIG:     the reg index list is too big to fit in the array specified by
+             the user (the number required will be written into n).
 
 struct kvm_msr_list {
-	__u32 nmsrs; /* number of msrs in entries */
-	__u32 indices[0];
+	__u64 n; /* number of registers in reg[] */
+	__u64 reg[0];
 };

People complain that this interface is hard to use.

How about supplying the address of the array (in addition to n) so you don't have to deal with variable sized arrays, and dropping E2BIG in favour of always updating n (n changed to something bigger than you had -> reallocate and rerun)

Quoted 5 lines · Avi Kivity

Documentation/virtual/kvm/api.txt the virtualized real-mode area (VRMA) facility, the kernel will · line 2003

Quoted 3 lines

Documentation/virtual/kvm/api.txt +1 -1

-Capability: basic
+Capability: KVM_CAP_REG_LIST
 Architectures: arm

all

OK, I guess that's to be true in future. Fixed.

Quoted 20 lines

Quoted 7 lines

Type: vcpu ioctl

Documentation/virtual/kvm/api.txt +1 -2

-Parameters: struct kvm_msr_list (in/out)
+Parameters: struct kvm_reg_list (in/out)
 Returns: 0 on success; -1 on error
 Errors:
-  E2BIG:     the msr index list is too big to fit in the array specified by

Documentation/virtual/kvm/api.txt the virtualized real-mode area (VRMA) facility, the kernel will · line 2014

Quoted 10 lines

Documentation/virtual/kvm/api.txt +4 -3

-             the user.
+  E2BIG:     the reg index list is too big to fit in the array specified by
+             the user (the number required will be written into n).
 
 struct kvm_msr_list {
-	__u32 nmsrs; /* number of msrs in entries */
-	__u32 indices[0];
+	__u64 n; /* number of registers in reg[] */
+	__u64 reg[0];
 };

People complain that this interface is hard to use.

Quoted 4 lines

How about supplying the address of the array (in addition to n) so you don't have to deal with variable sized arrays, and dropping E2BIG in favour of always updating n (n changed to something bigger than you had -> reallocate and rerun)

We re-write n anyway, *and* return -E2BIG. Not returning an error is asking for trouble.

Passing an address in a struct is pretty bad, since it involves compatibility wrappers.

I don't think that is what makes the API hard
to use.

Quoted 2 lines · Rusty Russell

Passing an address in a struct is pretty bad, since it involves compatibility wrappers.

Right, some s390 thing.

Quoted 2 lines

I don't think that is what makes the API hard
to use.

What is it then? I forgot what the original complaints/complainers were.

I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it.

x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however.

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Rusty Russell

Passing an address in a struct is pretty bad, since it involves compatibility wrappers.

Right, some s390 thing.

Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....

Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this.

Quoted 4 lines

Quoted 2 lines

I don't think that is what makes the API hard
to use.

What is it then? I forgot what the original complaints/complainers were.

I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it.

x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however.

Quoted 7 lines · Rusty Russell

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Rusty Russell

Passing an address in a struct is pretty bad, since it involves compatibility wrappers.

Right, some s390 thing.

Quoted 4 lines

Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....

Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this.

Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

I don't think that is what makes the API hard
to use.

What is it then? I forgot what the original complaints/complainers were.

Quoted 5 lines

I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it.

x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however.

Another option is to use the size parameter from the ioctl. It just sits there doing nothing.

Quoted 3 lines

Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love.

I doubt that anyone wants to run 31-bit user space on an s390x system. In fact, I wouldn't be surprised if exactly that case is broken already.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

I don't think that is what makes the API hard
to use.

What is it then? I forgot what the original complaints/complainers were.

I have no idea, since I didn't hear the complaints. But any non-fixed size array has issues in C; there's not much we can do about it.

Quoted 5 lines

Quoted 2 lines

x86 manages this fine for msrs, and I didn't have a problem using it for my test programs. That's the limit of my experience, however.

Another option is to use the size parameter from the ioctl. It just sits there doing nothing.

It would require quite a bunch of changes throughout the stack. Even in user space, like strace...

Quoted 7 lines · Rusty Russell

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Rusty Russell

Passing an address in a struct is pretty bad, since it involves compatibility wrappers.

Right, some s390 thing.

Quoted 4 lines

Err, no, i386 on x86-64, or ppc32 on ppc64, or arm on arm64....

Any time you put a pointer in a structure which is exposed to userspace, you have to deal with this.

Not is you pack the pointer in a __u64, which is what we do to preserve padding. Then it is only s390 which needs extra love.

Another option is to use the size parameter from the ioctl. It just sits there doing nothing.

Hi all,

This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.

Quoted 4 lines · Rusty Russell

Hi all,

This compiles, completely untested, but it's my attempt to give
Avi (and Alexander) what he asked for in a generic register accessor.

And here's the tested version: see my new "onereg-abi" branch. My next step is to demux CSELR, but that won't be until Tuesday.

Cheers,
Rusty.

The following changes since commit bc9cf74d26786dd2063155f9c703b8cb19d4270d:

KVM: ARM: Add trace and fix prints on guest aborts (2012-08-28 22:35:53 -0700)

are available in the git repository at:

[email protected]:/pub/scm/linux/kernel/git/rusty/linux-kvm-arm.git onereg-abi

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

Quoted 9 lines · Rusty Russell

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t).

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size.

This defeats the idea that userspace should
be able to migrate kernel register state without having to know the semantics of all the registers involved.

Possible solutions:
* switch GET/SET_ONE_REG to just passing data, same as the MSR ioctls
* switch GET/SET_ONE_REG to always writing 64 bits regardless of actual guest register width
* make GET_REG_LIST return register width as well as index

Personally I would really prefer the MSR-style "pass the data".

Otherwise I'm going to end up constructing something like uint64_t actual_values[]
struct kvm_one_reg regs[]

where regs[x].addr = &actual_values[x] for all x. Which seems like unnecessary indirection really :-)

I could live with "always read/write 64 bits". I definitely don't want to have to deal with matching up register widths to accesses in userspace, please.

Quoted 9 lines · Rusty Russell

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t).

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved.

I could live with "always read/write 64 bits". I definitely don't want to have to deal with matching up register widths to accesses in userspace, please.

Quoted 6 lines · Peter Maydell

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved.

It's there. There are bits in the id which indicate the size:

And my patches added a helper:

#define KVM_REG_SIZE(id)                                                \
        (1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))

Quoted 3 lines

I could live with "always read/write 64 bits". I definitely don't want to have to deal with matching up register widths to accesses in userspace, please.

I changed my mind about the old scheme when I realized we have to deal with 128-bit FPU registers.

Quoted 7 lines · Rusty Russell

Quoted 3 lines · Peter Maydell

I could live with "always read/write 64 bits". I definitely don't want to have to deal with matching up register widths to accesses in userspace, please.

I changed my mind about the old scheme when I realized we have to deal with 128-bit FPU registers.

Mmm, ARM might not have any awkward size registers but there's x86 weirdisms to consider for a generic ABI I guess.

Actually, I hadn't realized ARM didn't do 128-bit FP regs already.

But I'd guess they'll arrive one day.

Quoted 18 lines · Peter Maydell

Quoted 9 lines · Rusty Russell

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t).

Quoted 5 lines

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved.

It's there. There are bits in the id which indicate the size:

#define KVM_REG_SIZE_SHIFT 52
#define KVM_REG_SIZE_MASK 0x00f0000000000000ULL
#define KVM_REG_SIZE_U8 0x0000000000000000ULL
#define KVM_REG_SIZE_U16 0x0010000000000000ULL
#define KVM_REG_SIZE_U32 0x0020000000000000ULL
#define KVM_REG_SIZE_U64 0x0030000000000000ULL
#define KVM_REG_SIZE_U128 0x0040000000000000ULL
#define KVM_REG_SIZE_U256 0x0050000000000000ULL
#define KVM_REG_SIZE_U512 0x0060000000000000ULL
#define KVM_REG_SIZE_U1024 0x0070000000000000ULL

Quoted 30 lines · Rusty Russell

Quoted 18 lines · Peter Maydell

Quoted 9 lines · Rusty Russell

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t).

Quoted 5 lines

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved.

It's there. There are bits in the id which indicate the size:

#define KVM_REG_SIZE_SHIFT 52
#define KVM_REG_SIZE_MASK 0x00f0000000000000ULL
#define KVM_REG_SIZE_U8 0x0000000000000000ULL

Quoted 8 lines

#define KVM_REG_SIZE_U16    0x0010000000000000ULL
#define KVM_REG_SIZE_U32    0x0020000000000000ULL
#define KVM_REG_SIZE_U64    0x0030000000000000ULL
#define KVM_REG_SIZE_U128    0x0040000000000000ULL
#define KVM_REG_SIZE_U256    0x0050000000000000ULL
#define KVM_REG_SIZE_U512    0x0060000000000000ULL

#define KVM_REG_SIZE_U1024 0x0070000000000000ULL

Assumes power-of-two registers. On x86 IDTR is 10 bytes long (2 byte limit, 8 byte address). We could split it into two registers, or add padding, but it's unnatural.

Quoted 40 lines · Avi Kivity

Quoted 38 lines · Rusty Russell

Quoted 18 lines · Peter Maydell

Quoted 9 lines · Rusty Russell

Rusty Russell (8):
KVM: ARM: Fix walk_msrs()
KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
KVM: Add KVM_REG_SIZE() helper.
KVM: ARM: use KVM_SET_ONE_REG/KVM_GET_ONE_REG.
KVM: Add KVM_VCPU_GET_REG_LIST.
KVM: ARM: Use KVM_VCPU_GET_REG_LIST.
KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG. KVM ARM: Update api.txt

So I was thinking about this, and I remembered that the SET_ONE_REG/ GET_ONE_REG API has userspace pass a pointer to the variable the kernel should read/write (unlike the _MSR x86 ioctls, where the actual data value is sent back and forth in the struct). Further, the kernel only writes a data value of the size of the register (rather than always reading/writing a uint64_t).

Quoted 5 lines

This is a problem because it means userspace needs to know the size of each register, and the kernel doesn't provide any way to determine the size. This defeats the idea that userspace should be able to migrate kernel register state without having to know the semantics of all the registers involved.

It's there. There are bits in the id which indicate the size:

#define KVM_REG_SIZE_SHIFT 52
#define KVM_REG_SIZE_MASK 0x00f0000000000000ULL
#define KVM_REG_SIZE_U8 0x0000000000000000ULL

#define KVM_REG_SIZE_U16    0x0010000000000000ULL
#define KVM_REG_SIZE_U32    0x0020000000000000ULL
#define KVM_REG_SIZE_U64    0x0030000000000000ULL
#define KVM_REG_SIZE_U128    0x0040000000000000ULL
#define KVM_REG_SIZE_U256    0x0050000000000000ULL
#define KVM_REG_SIZE_U512    0x0060000000000000ULL

#define KVM_REG_SIZE_U1024 0x0070000000000000ULL

Quoted 3 lines

Assumes power-of-two registers. On x86 IDTR is 10 bytes long (2 byte limit, 8 byte address). We could split it into two registers, or add padding, but it's unnatural.

Why is padding bad?

How do you model IDTR throughout the stack today?

How does QEMU's savevm serialize it?

(and the APIC, if treated as one-large-register) is 4k)

Quoted 1 line · Avi Kivity

(and the APIC, if treated as one-large-register) is 4k)

...so don't do that then. Trying to treat the whole APIC as a single "register" means you don't get any of the advantages of "does this kernel support this register?" etc. Is there some reason I'm not seeing why it would make sense to do it that way?

Quoted 8 lines · Peter Maydell

Quoted 1 line · Avi Kivity

(and the APIC, if treated as one-large-register) is 4k)

...so don't do that then. Trying to treat the whole APIC as a single "register" means you don't get any of the advantages of "does this kernel support this register?" etc. Is there some reason I'm not seeing why it would make sense to do it that way?

It's just the easiest path forward.

"one large register" is mainly useful if registers have interdependencies. That doesn't exist in the APIC AFAIR, but it does exist elsewhere. Another way to handle interdependencies is to defer applying the changes until a KVM_RUN, and then evaluate them as a group.