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

[Avi Kivity <avi@redhat.com>]

On 11/02/2012 07:11 PM, Sanjay Lal wrote:
> On Nov 1, 2012, at 11:04 AM, Avi Kivity wrote:
>
> > On 10/31/2012 05:18 PM, Sanjay Lal wrote:
> > 
> >> +
> >> +/* 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?
>
> On MIPS, kernel modules are executed from "mapped space", which requires TLBs.  The TLB handling code is statically linked with the rest of the kernel (kvm_tlb.c) to avoid the possibility of double faulting. The problem is that the code references routines that are part of the the KVM module, which are only available once the module is loaded, hence the indirection.

Ok.  These should be global function pointers then, assigned when the
module is loaded, and cleared when it is unloaded, with a comment
explaining why.

>
> > 
> >> +
> >> +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.
>
> We will eventually have separate implementations based on the features supported by H/W.

It's probably better to add the indirection when the need arrives,
unless you already have hardware specs and know exactly the best points
for a split.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.