Re: [PATCH] Enable memory mapped TPR shadow(FlexPriority)

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Yang, Sheng wrote:
> >From ac4dd1782b9f0f51e0c366a1b8db4515d6828df8 Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Tue, 23 Oct 2007 12:34:42 +0800
> Subject: [PATCH] Enable memory mapped TPR shadow(FlexPriority)
>
> This patch based on CR8/TPR patch before, and enable the TPR
> shadow(FlexPriority) for 32bit Windows. Since TPR is accessed
> very frequently by 32bit Windows, especially SMP guest, with
> FlexPriority enabled, we saw significant performance gain.
>   

For the benefit of users, can you detail which processors have this feature?

>  
>  static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
> @@ -503,6 +504,11 @@ void kvm_mmu_slot_remove_write_access(struct kvm
> *kvm, int slot);
>  void kvm_mmu_zap_all(struct kvm *kvm);
>  void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int
> kvm_nr_mmu_pages);
>  
> +int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> +				  struct
> +				  kvm_userspace_memory_region *mem,
> +				  int user_alloc);
> +
>   

Exporting an ioctl handler isn't very pretty.  Izik is working on a 
patch that also requires an internal memory slot, so he will add an API 
for it.

> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -643,10 +643,10 @@ EXPORT_SYMBOL_GPL(fx_init);
>   *
>   * Discontiguous memory is allowed, mostly for framebuffers.
>   */
> -static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> -					  struct
> -					  kvm_userspace_memory_region
> *mem,
> -					  int user_alloc)
> +int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
> +				  struct
> +				  kvm_userspace_memory_region *mem,
> +				  int user_alloc)
>  {
>  	int r;
>  	gfn_t base_gfn;
> @@ -776,6 +776,7 @@ out_unlock:
>  out:
>  	return r;
>  }
> +EXPORT_SYMBOL_GPL(kvm_vm_ioctl_set_memory_region);
>   

The internal memory slot needs to be hidden from userspace.  Hopefully 
Izik's API will add that too.

> +
> +	/* For APIC access vmexit */
> +	if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
> +		goto mmio;
>   

What if the guest changes the apic base address?

> @@ -1610,13 +1675,15 @@ static int vmx_vcpu_reset(struct kvm_vcpu *vcpu)
>  
>  	vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, 0);  /* 22.2.1 */
>  
> -#ifdef CONFIG_X86_64
>  	vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, 0);
>   

I think the #ifdef is for a reason... probably Core processors which 
don't support x86_64 (and therefore cr8) don't have this.

Please check.

>  /*
>   * Interruption-information format
> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
> index f858c01..d24166e 100644
> --- a/drivers/kvm/x86_emulate.c
> +++ b/drivers/kvm/x86_emulate.c
> @@ -845,6 +845,11 @@ modrm_done:
>  			c->src.type = OP_REG;
>  			break;
>  		}
> +		if (((ctxt->cmd_type & EMULCMD_DECODE_ADDR) != 0) &&
> +		    (c->modrm_ea == 0)) {
> +			ctxt->cr2 = insn_fetch(u32, c->src.bytes,
> c->eip);
> +			c->eip -= c->src.bytes;
> +		}
>   

Please explain this.  Is this for mov eax, abs?  If so, it's much 
needed, and should be in a separate patch.  I don't think 
EMULCMD_DECODE_ADDR is necessary.

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/