Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
Christoffer Dall c.d...@virtualopensystems.com writes: that's fine, but then the #define's shouldn't be called something with COPROC in their names. Sure, feel free to rename it. I failed to come up with a concise, clear name, so coproc stuck. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On 1 September 2012 20:40, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sep 1, 2012, at 6:25 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 1 September 2012 10:16, Avi Kivity a...@redhat.com wrote: On 08/29/2012 11:21 AM, Rusty Russell wrote: Peter Maydell wrote: ...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. 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.) -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On Tue, Sep 4, 2012 at 9:09 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 1 September 2012 20:40, Christoffer Dall c.d...@virtualopensystems.com wrote: On Sep 1, 2012, at 6:25 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 1 September 2012 10:16, Avi Kivity a...@redhat.com wrote: On 08/29/2012 11:21 AM, Rusty Russell wrote: Peter Maydell wrote: ...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. 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. -Christoffer -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On 08/29/2012 08:29 AM, Christoffer Dall wrote: On Tue, Aug 28, 2012 at 4:48 PM, Rusty Russell rusty.russ...@linaro.org wrote: 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? The nice thing about it is that the hardware vendors can keep adding stuff, and we don't need new ioctls. Just new encodings for register numbers. x86 needed 6-7 updates (some due to our missing some hidden state). -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On 08/29/2012 11:21 AM, Rusty Russell wrote: + /* 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). And tomorrow they will add 16. Have the first byte say where the state belongs (core or coprocessor) second byte refer to the register family (if applicable), the rest identifies the register within that family. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On 1 September 2012 10:16, Avi Kivity a...@redhat.com wrote: On 08/29/2012 11:21 AM, Rusty Russell wrote: + /* 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). 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). -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On Sep 1, 2012, at 6:25 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 1 September 2012 10:16, Avi Kivity a...@redhat.com wrote: On 08/29/2012 11:21 AM, Rusty Russell wrote: + /* 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). 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). Yeah, but folding core registers under coprocessors feels just too fishy, so I think we should have a separate field. -Christoffer-- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On Tue, Aug 28, 2012 at 4:48 PM, Rusty Russell rusty.russ...@linaro.org wrote: 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? -Christoffer -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
On 29 August 2012 00:48, Rusty Russell rusty.russ...@linaro.org wrote: 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. + /* 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. -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
Peter Maydell peter.mayd...@linaro.org writes: On 29 August 2012 00:48, Rusty Russell rusty.russ...@linaro.org wrote: 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. Avi wanted to get rid of the structures. But I thought it will never change was already proven untrue, for your psuedo-secure mode proposal? + /* 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). Thanks, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 5/5] KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
Rusty Russell rusty.russ...@linaro.org writes:
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.
That approach is much nicer. This version just exposes the internal
kvm_vcpu_regs (as kvm_regs) and uses the offsets in that as our register
ids.
It will have to become a mapping table if we change our internal
representation, but that's still straightforward.
Cheers,
Rusty.
PS. Also use 16, not 0 as coprocessor number.
Subject: KVM: ARM: Access all registers via KVM_GET_ONE_REG/KVM_SET_ONE_REG.
We use the offset within the structure (which, conveniently, is used
internally as well) to supply unique ids for the core registers.
This makes it trivially extensible by appending fields to the struct.
Signed-off-by: Rusty Russell rusty.russ...@linaro.org
diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
index a3daa67..ba35a15 100644
--- a/arch/arm/include/asm/kvm.h
+++ b/arch/arm/include/asm/kvm.h
@@ -32,31 +32,15 @@ enum KVM_ARM_IRQ_LINE_TYPE {
KVM_ARM_FIQ_LINE = 1,
};
-/*
- * Modes used for short-hand mode determinition in the world-switch code and
- * in emulation code.
- *
- * Note: These indices do NOT correspond to the value of the CPSR mode bits!
- */
-enum vcpu_mode {
- MODE_FIQ = 0,
- MODE_IRQ,
- MODE_SVC,
- MODE_ABT,
- MODE_UND,
- MODE_USR,
- MODE_SYS
-};
-
struct kvm_regs {
- __u32 regs0_7[8]; /* Unbanked regs. (r0 - r7)*/
- __u32 fiq_regs8_12[5]; /* Banked fiq regs. (r8 - r12) */
- __u32 usr_regs8_12[5]; /* Banked usr registers (r8 - r12) */
- __u32 reg13[6]; /* Banked r13, indexed by MODE_*/
- __u32 reg14[6]; /* Banked r13, indexed by MODE_*/
- __u32 reg15;
- __u32 cpsr;
- __u32 spsr[5]; /* Banked SPSR, indexed by MODE_ */
+ __u32 usr_regs[15]; /* R0_usr - R14_usr */
+ __u32 svc_regs[3]; /* SP_svc, LR_svc, SPSR_svc */
+ __u32 abt_regs[3]; /* SP_abt, LR_abt, SPSR_abt */
+ __u32 und_regs[3]; /* SP_und, LR_und, SPSR_und */
+ __u32 irq_regs[3]; /* SP_irq, LR_irq, SPSR_irq */
+ __u32 fiq_regs[8]; /* R8_fiq - R14_fiq, SPSR_fiq */
+ __u32 pc; /* The program counter (r15) */
+ __u32 cpsr; /* The guest CPSR */
};
/* Supported Processor Types */
@@ -96,4 +80,8 @@ struct kvm_arch_memory_slot {
#define KVM_REG_ARM_32_CRN_START 11 /* Mask: 0x7800 */
#define KVM_REG_ARM_32_CRN_LEN 4
+/* Normal registers are mapped as coprocessor 16. */
+#define KVM_REG_ARM_CORE (0x0010 KVM_REG_ARM_COPROC_START)
+#define KVM_REG_ARM_CORE_REG(name) (offsetof(struct kvm_regs, name) / 4)
+
#endif /* __ARM_KVM_H__ */
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 64fa65f..4e67f94 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -20,6 +20,7 @@
#define __ARM_KVM_HOST_H__
#include asm/fpstate.h
+#include asm/kvm.h
#define KVM_MAX_VCPUS 4
#define KVM_MEMORY_SLOTS 32
@@ -75,16 +76,21 @@ struct kvm_mmu_memory_cache {
void *objects[KVM_NR_MEM_OBJS];
};
-struct kvm_vcpu_regs {
- u32 usr_regs[15]; /* R0_usr - R14_usr */
- u32 svc_regs[3];/* SP_svc, LR_svc, SPSR_svc */
- u32 abt_regs[3];/* SP_abt, LR_abt, SPSR_abt */
- u32 und_regs[3];/* SP_und, LR_und, SPSR_und */
- u32 irq_regs[3];/* SP_irq, LR_irq, SPSR_irq */
- u32 fiq_regs[8];/* R8_fiq - R14_fiq, SPSR_fiq */
- u32 pc; /* The program counter (r15) */
- u32 cpsr; /* The guest CPSR */
-} __packed;
+/*
+ * Modes used for short-hand mode determinition in the world-switch code and
+ * in emulation code.
+ *
+ * Note: These indices do NOT correspond to the value of the CPSR mode bits!
+ */
+enum vcpu_mode {
+ MODE_FIQ = 0,
+ MODE_IRQ,
+ MODE_SVC,
+ MODE_ABT,
+ MODE_UND,
+ MODE_USR,
+ MODE_SYS
+};
/* 0 is reserved as an invalid value. */
enum cp15_regs {
@@ -117,7 +123,7 @@ enum cp15_regs {
};
struct kvm_vcpu_arch {
- struct kvm_vcpu_regs regs;
+ struct kvm_regs regs;
u32 target; /* Currently KVM_ARM_TARGET_CORTEX_A15 */
DECLARE_BITMAP(features, NUM_FEATURES);
@@ -192,4 +198,10 @@ static inline int kvm_test_age_hva(struct kvm *kvm,
unsigned long hva)
{
return 0;
}
+
+int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
+unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
+struct kvm_one_reg;
+int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
+int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const
