diff mbox

[14/29] arm64: KVM: guest one-reg interface

Message ID 1362455265-24165-15-git-send-email-marc.zyngier@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc Zyngier March 5, 2013, 3:47 a.m. UTC
Let userspace play with the guest registers.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)
 create mode 100644 arch/arm64/kvm/guest.c

Comments

Christopher Covington March 12, 2013, 5:31 p.m. UTC | #1
Hi Marc,

On 03/04/2013 10:47 PM, Marc Zyngier wrote:
> Let userspace play with the guest registers.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 240 insertions(+)
>  create mode 100644 arch/arm64/kvm/guest.c

[...]

> +int __attribute_const__ kvm_target_cpu(void)
> +{
> +	unsigned long implementor = read_cpuid_implementor();
> +	unsigned long part_number = read_cpuid_part_number();
> +
> +	if (implementor != ARM_CPU_IMP_ARM)
> +		return -EINVAL;
> +
> +	switch (part_number) {
> +	case ARM_CPU_PART_AEM_V8:
> +	case ARM_CPU_PART_FOUNDATION:
> +		/* Treat the models just as an A57 for the time being */
> +	case ARM_CPU_PART_CORTEX_A57:
> +		return KVM_ARM_TARGET_CORTEX_A57;
> +	default:
> +		return -EINVAL;
> +	}
> +}

What is the motivation behind these checks? Why not let any ARMv8 system that
has EL2 host a virtualized Cortex A57 guest?

[...]

Thanks,
Christopher
Marc Zyngier March 12, 2013, 6:05 p.m. UTC | #2
On 12/03/13 17:31, Christopher Covington wrote:
> Hi Marc,
> 
> On 03/04/2013 10:47 PM, Marc Zyngier wrote:
>> Let userspace play with the guest registers.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 240 insertions(+)
>>  create mode 100644 arch/arm64/kvm/guest.c
> 
> [...]
> 
>> +int __attribute_const__ kvm_target_cpu(void)
>> +{
>> +	unsigned long implementor = read_cpuid_implementor();
>> +	unsigned long part_number = read_cpuid_part_number();
>> +
>> +	if (implementor != ARM_CPU_IMP_ARM)
>> +		return -EINVAL;
>> +
>> +	switch (part_number) {
>> +	case ARM_CPU_PART_AEM_V8:
>> +	case ARM_CPU_PART_FOUNDATION:
>> +		/* Treat the models just as an A57 for the time being */
>> +	case ARM_CPU_PART_CORTEX_A57:
>> +		return KVM_ARM_TARGET_CORTEX_A57;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
> What is the motivation behind these checks? Why not let any ARMv8 system that
> has EL2 host a virtualized Cortex A57 guest?

The main reason is errata management. How do you deal with errata in the
guest when you hide the underlying host CPU? I don't have an answer to
that. So for the time being, we only allow the guest to see the same CPU
as the host, and require that new CPUs are added to this function.

We went the same way for KVM/ARM.

	M.
Christopher Covington March 12, 2013, 10:07 p.m. UTC | #3
Hi Marc,

On 03/12/2013 02:05 PM, Marc Zyngier wrote:
> On 12/03/13 17:31, Christopher Covington wrote:
>> Hi Marc,
>>
>> On 03/04/2013 10:47 PM, Marc Zyngier wrote:
>>> Let userspace play with the guest registers.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 240 insertions(+)
>>>  create mode 100644 arch/arm64/kvm/guest.c
>>
>> [...]
>>
>>> +int __attribute_const__ kvm_target_cpu(void)
>>> +{
>>> +	unsigned long implementor = read_cpuid_implementor();
>>> +	unsigned long part_number = read_cpuid_part_number();
>>> +
>>> +	if (implementor != ARM_CPU_IMP_ARM)
>>> +		return -EINVAL;
>>> +
>>> +	switch (part_number) {
>>> +	case ARM_CPU_PART_AEM_V8:
>>> +	case ARM_CPU_PART_FOUNDATION:
>>> +		/* Treat the models just as an A57 for the time being */
>>> +	case ARM_CPU_PART_CORTEX_A57:
>>> +		return KVM_ARM_TARGET_CORTEX_A57;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>
>> What is the motivation behind these checks? Why not let any ARMv8 system that
>> has EL2 host a virtualized Cortex A57 guest?
> 
> The main reason is errata management. How do you deal with errata in the
> guest when you hide the underlying host CPU? I don't have an answer to
> that. So for the time being, we only allow the guest to see the same CPU
> as the host, and require that new CPUs are added to this function.

Can you please elaborate on how this code ensures the guest is seeing the same
CPU as the host? It looks rather unlike VPIDR = MIDR.

Thanks,
Christopher
Marc Zyngier March 13, 2013, 7:48 a.m. UTC | #4
On 12/03/13 22:07, Christopher Covington wrote:

Hi Christopher,

> On 03/12/2013 02:05 PM, Marc Zyngier wrote:
>> On 12/03/13 17:31, Christopher Covington wrote:
>>> Hi Marc,
>>>
>>> On 03/04/2013 10:47 PM, Marc Zyngier wrote:
>>>> Let userspace play with the guest registers.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 240 insertions(+)
>>>>  create mode 100644 arch/arm64/kvm/guest.c
>>>
>>> [...]
>>>
>>>> +int __attribute_const__ kvm_target_cpu(void)
>>>> +{
>>>> +	unsigned long implementor = read_cpuid_implementor();
>>>> +	unsigned long part_number = read_cpuid_part_number();
>>>> +
>>>> +	if (implementor != ARM_CPU_IMP_ARM)
>>>> +		return -EINVAL;
>>>> +
>>>> +	switch (part_number) {
>>>> +	case ARM_CPU_PART_AEM_V8:
>>>> +	case ARM_CPU_PART_FOUNDATION:
>>>> +		/* Treat the models just as an A57 for the time being */
>>>> +	case ARM_CPU_PART_CORTEX_A57:
>>>> +		return KVM_ARM_TARGET_CORTEX_A57;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +}
>>>
>>> What is the motivation behind these checks? Why not let any ARMv8 system that
>>> has EL2 host a virtualized Cortex A57 guest?
>>
>> The main reason is errata management. How do you deal with errata in the
>> guest when you hide the underlying host CPU? I don't have an answer to
>> that. So for the time being, we only allow the guest to see the same CPU
>> as the host, and require that new CPUs are added to this function.
> 
> Can you please elaborate on how this code ensures the guest is seeing the same
> CPU as the host? It looks rather unlike VPIDR = MIDR.

I was merely elaborating on the "why". For the how:
- vmidr_el2 is set in arch/arm64/kernel/head.S and never changed,
ensuring the guest sees the same thing as the kernel.
- Some additional code in guest.c ensures that both the host and the CPU
requested by userspace for the guest are the same
- KVM_ARM_TARGET_CORTEX_A57 is used in sys_regs_a57.c to register the
sys_reg/cp15 handlers.

Cheers,

	M.
Christopher Covington March 13, 2013, 8:34 p.m. UTC | #5
Hi Marc,

On 03/13/2013 03:48 AM, Marc Zyngier wrote:
> On 12/03/13 22:07, Christopher Covington wrote:
> 
> Hi Christopher,
> 
>> On 03/12/2013 02:05 PM, Marc Zyngier wrote:
>>> On 12/03/13 17:31, Christopher Covington wrote:
>>>> Hi Marc,
>>>>
>>>> On 03/04/2013 10:47 PM, Marc Zyngier wrote:
>>>>> Let userspace play with the guest registers.
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  arch/arm64/kvm/guest.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 240 insertions(+)
>>>>>  create mode 100644 arch/arm64/kvm/guest.c
>>>>
>>>> [...]
>>>>
>>>>> +int __attribute_const__ kvm_target_cpu(void)
>>>>> +{
>>>>> +	unsigned long implementor = read_cpuid_implementor();
>>>>> +	unsigned long part_number = read_cpuid_part_number();
>>>>> +
>>>>> +	if (implementor != ARM_CPU_IMP_ARM)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	switch (part_number) {
>>>>> +	case ARM_CPU_PART_AEM_V8:
>>>>> +	case ARM_CPU_PART_FOUNDATION:
>>>>> +		/* Treat the models just as an A57 for the time being */
>>>>> +	case ARM_CPU_PART_CORTEX_A57:
>>>>> +		return KVM_ARM_TARGET_CORTEX_A57;
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +}
>>>>
>>>> What is the motivation behind these checks? Why not let any ARMv8 system that
>>>> has EL2 host a virtualized Cortex A57 guest?
>>>
>>> The main reason is errata management. How do you deal with errata in the
>>> guest when you hide the underlying host CPU? I don't have an answer to
>>> that. So for the time being, we only allow the guest to see the same CPU
>>> as the host, and require that new CPUs are added to this function.
>>
>> Can you please elaborate on how this code ensures the guest is seeing the same
>> CPU as the host? It looks rather unlike VPIDR = MIDR.
> 
> I was merely elaborating on the "why". For the how:
> - vmidr_el2 is set in arch/arm64/kernel/head.S and never changed,
> ensuring the guest sees the same thing as the kernel.
> - Some additional code in guest.c ensures that both the host and the CPU
> requested by userspace for the guest are the same
> - KVM_ARM_TARGET_CORTEX_A57 is used in sys_regs_a57.c to register the
> sys_reg/cp15 handlers.

As I believe your response indicates, the code cited above in this email isn't
ensuring that the host is the same as the guest. That's done elsewhere. The
code here is doing something different, something that seems to me is going to
make building upon the ARM64 KVM infrastructure more of a hassle than it
should be.

My guess at the goal of the code cited above in this email is that it's trying
to sanity check that virtualization will work. Rather than taking a default
deny approach with a hand-maintained white list of virtualization-supporting
machine identifiers, why not check that EL2 is implemented on the current
system and if it's not implied by that, that the timer and interrupt
controller are suitable as well?

Thanks,
Christopher
Peter Maydell March 14, 2013, 8:57 a.m. UTC | #6
On 13 March 2013 20:34, Christopher Covington <cov@codeaurora.org> wrote:
> My guess at the goal of the code cited above in this email is that it's trying
> to sanity check that virtualization will work. Rather than taking a default
> deny approach with a hand-maintained white list of virtualization-supporting
> machine identifiers, why not check that EL2 is implemented on the current
> system and if it's not implied by that, that the timer and interrupt
> controller are suitable as well?

I think the question of "how much do we need to virtualize to allow
us to expose a different CPU to the guest than the host" is not yet
one that's been answered; so for the moment we support only
"guest == host == A57" [or == A15 on armv7]. When somebody has added
support for a second type of host/guest CPU then I think the process
of going through that work will make it much clearer how much
'new cpu' support is needed and what can be coded to work with any
virt-capable cpu. Until then I think it's safer to simply lock out
the unsupported combinations. That way anybody trying to run KVM on
a different CPU will be able to see that they have work to do and it's
not expected to work out of the box just yet.

We don't support other guests than A57 currently because you need to
implement emulation code for the imp-def registers for a guest CPU.
Again, I suspect the process of adding support for a 2nd guest CPU
will make it obvious what can be done generically and what really
does need to be per-CPU.

One thing I'm a bit worried about is the possibility that we accidentally
by-default allow the guest access to some new sysreg that didn't
exist on the A57 [or A15 for 32 bit] that turns out to be a security
hole (since both guest and host kernel run at EL1). But I think
trapping the whole of the impdef sysreg space should cover that.

-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Covington March 20, 2013, 8:06 p.m. UTC | #7
Hi Marc, Peter,

On 03/14/2013 04:57 AM, Peter Maydell wrote:
> On 13 March 2013 20:34, Christopher Covington <cov@codeaurora.org> wrote:
>> My guess at the goal of the code cited above in this email is that it's trying
>> to sanity check that virtualization will work. Rather than taking a default
>> deny approach with a hand-maintained white list of virtualization-supporting
>> machine identifiers, why not check that EL2 is implemented on the current
>> system and if it's not implied by that, that the timer and interrupt
>> controller are suitable as well?

[...]

> ...you need to implement emulation code for the imp-def registers for a
> guest CPU.

[...]

This is reasonable. In this light the code I was picking out above is simply
converting MIDRs to KVM_ARM_TARGET_* constants. Because the mapping isn't
one-to-one, the hand-maintained list is an acceptable approach.

In the long term, I wonder if some kind of KVM_TARGET_CURRENT_CPU might be handy.

Thanks,
Christopher
diff mbox

Patch

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
new file mode 100644
index 0000000..2a8aaf8
--- /dev/null
+++ b/arch/arm64/kvm/guest.c
@@ -0,0 +1,240 @@ 
+/*
+ * Copyright (C) 2012 - ARM Ltd
+ * Author: Marc Zyngier <marc.zyngier@arm.com>
+ *
+ * Derived from arch/arm/kvm/guest.c:
+ * Copyright (C) 2012 - Virtual Open Systems and Columbia University
+ * Author: Christoffer Dall <c.dall@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/kvm_host.h>
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/fs.h>
+#include <asm/cputype.h>
+#include <asm/uaccess.h>
+#include <asm/kvm.h>
+#include <asm/kvm_asm.h>
+#include <asm/kvm_emulate.h>
+#include <asm/kvm_coproc.h>
+
+struct kvm_stats_debugfs_item debugfs_entries[] = {
+	{ NULL }
+};
+
+int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS;
+	return 0;
+}
+
+static u64 core_reg_offset_from_id(u64 id)
+{
+	return id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_CORE);
+}
+
+static int get_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr;
+	struct kvm_regs *regs = &vcpu->arch.regs;
+	u64 off;
+
+	if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
+		return -ENOENT;
+
+	/* Our ID is an index into the kvm_regs struct. */
+	off = core_reg_offset_from_id(reg->id);
+	if (off >= sizeof(*regs) / KVM_REG_SIZE(reg->id))
+		return -ENOENT;
+
+	return put_user(((unsigned long *)regs)[off], uaddr);
+}
+
+static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	unsigned long __user *uaddr = (unsigned long __user *)(unsigned long)reg->addr;
+	struct kvm_regs *regs = &vcpu->arch.regs;
+	u64 off, val;
+
+	if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long))
+		return -ENOENT;
+
+	/* Our ID is an index into the kvm_regs struct. */
+	off = core_reg_offset_from_id(reg->id);
+	if (off >= sizeof(*regs) / KVM_REG_SIZE(reg->id))
+		return -ENOENT;
+
+	if (get_user(val, uaddr) != 0)
+		return -EFAULT;
+
+	if (off == KVM_REG_ARM_CORE_REG(regs.pstate)) {
+		unsigned long mode = val & COMPAT_PSR_MODE_MASK;
+		switch (mode) {
+		case PSR_MODE_EL0t:
+		case PSR_MODE_EL1t:
+		case PSR_MODE_EL1h:
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	((unsigned long *)regs)[off] = val;
+	return 0;
+}
+
+int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
+{
+	return -EINVAL;
+}
+
+int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
+{
+	return -EINVAL;
+}
+
+static unsigned long num_core_regs(void)
+{
+	return sizeof(struct kvm_regs) / sizeof(unsigned long);
+}
+
+/**
+ * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG
+ *
+ * This is for all registers.
+ */
+unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
+{
+	return num_core_regs() + kvm_arm_num_sys_reg_descs(vcpu);
+}
+
+/**
+ * kvm_arm_copy_reg_indices - get indices of all registers.
+ *
+ * We do core registers right here, then we apppend system regs.
+ */
+int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
+{
+	unsigned int i;
+	const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE;
+
+	for (i = 0; i < sizeof(struct kvm_regs)/sizeof(unsigned long); i++) {
+		if (put_user(core_reg | i, uindices))
+			return -EFAULT;
+		uindices++;
+	}
+
+	return kvm_arm_copy_sys_reg_indices(vcpu, uindices);
+}
+
+int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	/* We currently use nothing arch-specific in upper 32 bits */
+	if ((reg->id & ~KVM_REG_SIZE_MASK) >> 32 != KVM_REG_ARM64 >> 32)
+		return -EINVAL;
+
+	/* Register group 16 means we want a core register. */
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
+		return get_core_reg(vcpu, reg);
+
+	return kvm_arm_sys_reg_get_reg(vcpu, reg);
+}
+
+int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
+{
+	/* We currently use nothing arch-specific in upper 32 bits */
+	if ((reg->id & ~KVM_REG_SIZE_MASK) >> 32 != KVM_REG_ARM64 >> 32)
+		return -EINVAL;
+
+	/* Register group 16 means we set a core register. */
+	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
+		return set_core_reg(vcpu, reg);
+
+	return kvm_arm_sys_reg_set_reg(vcpu, reg);
+}
+
+int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
+				  struct kvm_sregs *sregs)
+{
+	return -EINVAL;
+}
+
+int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
+				  struct kvm_sregs *sregs)
+{
+	return -EINVAL;
+}
+
+int __attribute_const__ kvm_target_cpu(void)
+{
+	unsigned long implementor = read_cpuid_implementor();
+	unsigned long part_number = read_cpuid_part_number();
+
+	if (implementor != ARM_CPU_IMP_ARM)
+		return -EINVAL;
+
+	switch (part_number) {
+	case ARM_CPU_PART_AEM_V8:
+	case ARM_CPU_PART_FOUNDATION:
+		/* Treat the models just as an A57 for the time being */
+	case ARM_CPU_PART_CORTEX_A57:
+		return KVM_ARM_TARGET_CORTEX_A57;
+	default:
+		return -EINVAL;
+	}
+}
+
+int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
+			const struct kvm_vcpu_init *init)
+{
+	unsigned int i;
+
+	/* We can only do a cortex A57 for now. */
+	if (init->target != kvm_target_cpu())
+		return -EINVAL;
+
+	vcpu->arch.target = init->target;
+	bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
+
+	/* -ENOENT for unknown features, -EINVAL for invalid combinations. */
+	for (i = 0; i < sizeof(init->features)*8; i++) {
+		if (init->features[i / 32] & (1 << (i % 32))) {
+			if (i >= KVM_VCPU_MAX_FEATURES)
+				return -ENOENT;
+			set_bit(i, vcpu->arch.features);
+		}
+	}
+
+	/* Now we know what it is, we can reset it. */
+	return kvm_reset_vcpu(vcpu);
+}
+
+int kvm_arch_vcpu_ioctl_get_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
+{
+	return -EINVAL;
+}
+
+int kvm_arch_vcpu_ioctl_set_fpu(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu)
+{
+	return -EINVAL;
+}
+
+int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
+				  struct kvm_translation *tr)
+{
+	return -EINVAL;
+}