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

[Avi Kivity <avi@redhat.com>]

On 10/31/2012 05:18 PM, Sanjay Lal wrote:
> 
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> ---
>  arch/mips/include/asm/kvm.h      |  58 ++++
>  arch/mips/include/asm/kvm_host.h | 672 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 730 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..39bb715
> --- /dev/null
> +++ b/arch/mips/include/asm/kvm.h
> @@ -0,0 +1,58 @@
> +/*
> +* 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];
> +    __u32 hi;
> +    __u32 lo;
> +    __u32 pc;
> +
> +    ulong cp0reg[N_MIPS_COPROC_REGS][N_MIPS_COPROC_SEL];
> +};

ulong changes size in 64-bit archs, requiring compat translations when
issuing 32-bit syscalls on a 64-bit kernel.  I don't know MIPS enough to
know whether that's a useful scenario.

> +
> +#define KVM_MAX_VCPUS 8

Set to 1 until smp is supported.

> +#define KVM_MEMORY_SLOTS 32
> +/* memory slots that does not exposed to userspace */
> +#define KVM_PRIVATE_MEM_SLOTS 4

Do you really need those?

> +
> +#define ENTER_CRITICAL(flags)   local_irq_save(flags)
> +#define EXIT_CRITICAL(flags)    local_irq_restore(flags)

Why wrap?

> +
> +
> +#define KVM_GUEST_KERNEL_ASID  ((vcpu->arch.guest_kernel_asid[smp_processor_id()]) & ASID_MASK)
> +
> +#define KVM_GUEST_USER_ASID		((vcpu->arch.guest_user_asid[smp_processor_id()]) & ASID_MASK)
> +
> +
> +#define KVM_GUEST_WIRED_TLBS    (current_cpu_data.tlbsize)
> +#define KVM_GUEST_COMMPAGE_TLB  (vcpu->kvm->arch.commpage_tlb)
> +#define KVM_GUEST_TLBS          KVM_GUEST_WIRED_TLBS

Don't use defines for variable expressions.  Use inline functions for
those, or just open-code when it's more readable.

> +
> +/* Special address that contains the comm page, used for reducing # of traps */
> +#define KVM_GUEST_COMMPAGE_ADDR     0x0
> +
> +struct kvm_arch
> +{
> +    /* Guest GVA->HPA page table */
> +    ulong *guest_pmap;
> +    ulong guest_pmap_npages;
> +
> +    /* Wired host TLB used for the commpage */
> +    int commpage_tlb;
> +
> +    pfn_t (*gfn_to_pfn) (struct kvm *kvm, gfn_t gfn);
> +    void (*release_pfn_clean) (pfn_t pfn);
> +    bool (*is_error_pfn) (pfn_t pfn);

Why this indirection?  Do those functions change at runtime?

> +
> +    /* Stats for exit reasons */
> +    ulong exit_reason_stats[MAX_KVM_MIPS_EXIT_TYPES];
> +};

Please use tracepoints for statistics instead of manual collection +
debugfs.

> +
> +struct kvm_mips_callbacks {
> +    int (*handle_cop_unusable)(struct kvm_vcpu *vcpu);
> +    int (*handle_tlb_mod)(struct kvm_vcpu *vcpu);
> +    int (*handle_tlb_ld_miss)(struct kvm_vcpu *vcpu);
> +    int (*handle_tlb_st_miss)(struct kvm_vcpu *vcpu);
> +    int (*handle_addr_err_st)(struct kvm_vcpu *vcpu);
> +    int (*handle_addr_err_ld)(struct kvm_vcpu *vcpu);
> +    int (*handle_syscall)(struct kvm_vcpu *vcpu);
> +    int (*handle_res_inst)(struct kvm_vcpu *vcpu);
> +    int (*handle_break)(struct kvm_vcpu *vcpu);
> +    gpa_t (*gva_to_gpa)(gva_t gva);
> +    void (*queue_timer_int)(struct kvm_vcpu *vcpu);
> +    void (*dequeue_timer_int)(struct kvm_vcpu *vcpu);
> +    void (*queue_io_int)(struct kvm_vcpu *vcpu, struct kvm_mips_interrupt *irq);
> +    void (*dequeue_io_int)(struct kvm_vcpu *vcpu, struct kvm_mips_interrupt *irq);
> +    int (*irq_deliver)(struct kvm_vcpu *vcpu, unsigned int priority, uint32_t cause);
> +    int (*irq_clear)(struct kvm_vcpu *vcpu, unsigned int priority, uint32_t cause);
> +    int (*vcpu_ioctl_get_regs)(struct kvm_vcpu *vcpu, struct kvm_regs *regs);
> +    int (*vcpu_ioctl_set_regs)(struct kvm_vcpu *vcpu, struct kvm_regs *regs);
> +    int (*vcpu_init)(struct kvm_vcpu *vcpu);
> +};

We use callbacks on x86 because we have two separate implementations
(svm and vmx).  Will that be the case on MIPS? If not, use direct calls.



-- 
error compiling committee.c: too many arguments to function