From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch] KVM: add MSR based hypercall API Date: Tue, 09 Jan 2007 13:24:42 +0200 Message-ID: <45A37B7A.8020709@qumranet.com> References: <20070109092705.GA8300@elte.hu> <45A36758.1000808@qumranet.com> <20070109103809.GA24515@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel Return-path: To: Ingo Molnar In-Reply-To: <20070109103809.GA24515-X9Un+BFzKDI@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Ingo Molnar wrote: > * Avi Kivity wrote: > > >>> @@ -237,6 +238,8 @@ struct kvm_vcpu { >>> unsigned long cr0; >>> unsigned long cr2; >>> unsigned long cr3; >>> + struct kvm_vcpu_para_state *para_state; >>> >>> >> Do we want this as part of kvm_vcpu or kvm? I can see arguments for >> both views. >> > > definitely needs to be a property of the vcpu. For example the cr3 cache > is attached to the physical CPU. SMP scalability necessiates this too - > we want to use the para_state to pass information between the guest and > the host without any hypercall. > > Ok, agreed. It makes guest-side registration a bit more icky (need execute the code on all vcpus). >>> +struct kvm_cr3_cache { >>> + struct kvm_cr3_cache_entry entry[KVM_CR3_CACHE_SIZE]; >>> + u32 max_idx; >>> +}; >>> >>> >> This will require an api version bump whenever KVM_CR3_CACHE_SIZE >> changes. >> >> Better to advertise the gpa of the cache, so it can be unlimited. >> > > the gpa of the cache, and its guest-side size, right? > > Yes (can use max_idx, no?). BTW, max_idx is ambiguous: is it the last valid entry or one past the end? entry_count is more explicit IMO. >>> + >>> + struct kvm_cr3_cache cr3_cache; >>> + >>> +} __attribute__ ((aligned(PAGE_SIZE))); >>> >>> >> Perhaps packed too, to avoid 32/64 ambiguity. Or even better, pad it >> explicitly to avoid unaligned fields. >> > > it should already be padded - i layed it out that way. (if it's not then > let me know where it's not padded) > > Right, I was confused by the cr3 cache, but it's the last field. > + > + if (kvm_arch_ops->patch_hypercall) { > It's safe to assume that the arch op exists. > +EXPORT_SYMBOL_GPL(gpa_to_hpa); > Is this needed now? If so, it needs a kvm_ prefix. > @@ -1448,6 +1448,17 @@ static int handle_io(struct kvm_vcpu *vc > return 0; > } > > +static void > +vmx_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall) > +{ > + /* > + * Patch in the VMCALL instruction: > + */ > + hypercall[0] = 0x0f; > + hypercall[1] = 0x01; > + hypercall[2] = 0xc1; > +} > + > static int handle_cr(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > { > u64 exit_qualification; > @@ -2042,6 +2053,7 @@ static struct kvm_arch_ops vmx_arch_ops > .run = vmx_vcpu_run, > .skip_emulated_instruction = skip_emulated_instruction, > .vcpu_setup = vmx_vcpu_setup, > + .patch_hypercall = vmx_patch_hypercall, > }; > Where is the vmcall exit handler? Please add the svm code too. I can test it if you lack amd hardware. -- error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV