Re: [PATCH 1/2] KVM: Remove arch specific components from the general code

[Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>]

Gregory Haskins wrote:
> Signed-off-by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> ---
>
>  drivers/kvm/kvm.h      |   31 -----
>  drivers/kvm/kvm_main.c |   26 +---
>  drivers/kvm/kvm_svm.h  |    3 
>  drivers/kvm/svm.c      |  322 +++++++++++++++++++++++++-----------------------
>  drivers/kvm/vmx.c      |  236 +++++++++++++++++++++--------------
>  5 files changed, 320 insertions(+), 298 deletions(-)
>  
>  struct kvm_vcpu {
> +	int valid;
>  	struct kvm *kvm;
>  	int vcpu_id;
> -	union {
> -		struct vmcs *vmcs;
> -		struct vcpu_svm *svm;
> -	};
> +	void *_priv;
>   

How are you planning on going about switching to container_of()?  Commit 
this, commit Rusty's stuff, then commit a fix or commit Rusty's stuff, 
then update your patch set?

>  static void svm_inject_gp(struct kvm_vcpu *vcpu, unsigned error_code)
>  {
> -	vcpu->svm->vmcb->control.event_inj = 	SVM_EVTINJ_VALID |
> +	svm(vcpu)->vmcb->control.event_inj = 	SVM_EVTINJ_VALID |
>  						SVM_EVTINJ_VALID_ERR |
>  						SVM_EVTINJ_TYPE_EXEPT |
>  						GP_VECTOR;
> -	vcpu->svm->vmcb->control.event_inj_err = error_code;
> +	svm(vcpu)->vmcb->control.event_inj_err = error_code;
>  }

I'm willing to concede on using the name "svm()" here although I think 
it's a terrible function name but I really think it's important to store 
a reference to this instead of using it as if it's an lvalue.  So I 
would change this to:

struct vcpu_svm *svm = svm(vcpu);

svm->vmcb->control.event_inj =   ....;

I think this is much easier to grok than having svm(vcpu) calls all over 
the place as psuedo-lvalues.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
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/