Re: [PATCH v2 02/18] KVM/MIPS32: Arch specific KVM data structures.

[David Daney <ddaney.cavm@gmail.com>]

On 11/21/2012 06:34 PM, Sanjay Lal wrote:
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> ---
>   arch/mips/include/asm/kvm.h      |  55 ++++

asm/kvm.h defines the user space ABI, and thus should be placed in 
arch/mips/include/uapi/asm instead.



>   arch/mips/include/asm/kvm_host.h | 669 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 724 insertions(+)
>   create mode 100644 arch/mips/include/asm/kvm.h
>   create mode 100644 arch/mips/include/asm/kvm_host.h
>
> diff --git a/arch/mips/include/asm/kvm.h b/arch/mips/include/asm/kvm.h
> new file mode 100644
> index 0000000..85789ea
> --- /dev/null
> +++ b/arch/mips/include/asm/kvm.h
> @@ -0,0 +1,55 @@
> +/*
> +* This file is subject to the terms and conditions of the GNU General Public
> +* License.  See the file "COPYING" in the main directory of this archive
> +* for more details.
> +*
> +* Copyright (C) 2012  MIPS Technologies, Inc.  All rights reserved.
> +* Authors: Sanjay Lal <sanjayl@kymasys.com>
> +*/
> +
> +#ifndef __LINUX_KVM_MIPS_H
> +#define __LINUX_KVM_MIPS_H
> +
> +#include <linux/types.h>
> +
> +#define __KVM_MIPS
> +
> +#define N_MIPS_COPROC_REGS      32
> +#define N_MIPS_COPROC_SEL   	8
> +
> +/* for KVM_GET_REGS and KVM_SET_REGS */
> +struct kvm_regs {
> +	__u32 gprs[32];

MIPS64 registers are 64 bits wide.  How is this going to work for MIPS64?

It seems a little important to answer this question as this is a 
userspace ABI that really cannot be changed once it is published.


> +	__u32 hi;
> +	__u32 lo;
> +	__u32 pc;
> +
> +	__u32 cp0reg[N_MIPS_COPROC_REGS][N_MIPS_COPROC_SEL];

Do we really want CP0 regs in here?  Other architectures don't have 
things like this.  They use things like KVM_GET_MSRS and KVM_SET_MSRS 
for this.

> +};
> +
> +/* for KVM_GET_SREGS and KVM_SET_SREGS */
> +struct kvm_sregs {
> +};
> +
> +/* for KVM_GET_FPU and KVM_SET_FPU */
> +struct kvm_fpu {

This is a userspace ABI, and MIPS definitely has a FPU.  That means that 
we cannot change the definition after it is merged, but we know this 
must have the FPU registers in it.

So it cannot be both present and empty.


> +};
> +
> +struct kvm_debug_exit_arch {
> +};
> +
> +/* for KVM_SET_GUEST_DEBUG */
> +struct kvm_guest_debug_arch {
> +};
> +
> +struct kvm_mips_interrupt {
> +	/* in */
> +	__u32 cpu;
> +	__u32 irq;
> +};
> +
> +/* definition of registers in kvm_run */
> +struct kvm_sync_regs {
> +};
> +
> +#endif /* __LINUX_KVM_MIPS_H */
[...]