Re: [PATCH v2 15/15] KVM: x86: Move the bulk of register specific code from x86.c to regs.c

[Huang, Kai]

> +void kvm_run_get_regs(struct kvm_vcpu *vcpu)
> +{
> +	BUILD_BUG_ON(sizeof(struct kvm_sync_regs) > SYNC_REGS_SIZE_BYTES);
> +
> +	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_REGS)
> +		__get_regs(vcpu, &vcpu->run->s.regs.regs);
> +
> +	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_SREGS)
> +		__get_sregs(vcpu, &vcpu->run->s.regs.sregs);
> +}
> +
> +int kvm_run_set_regs(struct kvm_vcpu *vcpu)
> +{
> +	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_REGS) {
> +		__set_regs(vcpu, &vcpu->run->s.regs.regs);
> +		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_REGS;
> +	}
> +
> +	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
> +		struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
> +
> +		if (__set_sregs(vcpu, &sregs))
> +			return -EINVAL;
> +
> +		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_SREGS;
> +	}
> +
> +	return 0;
> +}
> 

[...]

> @@ -12699,11 +11904,7 @@ static void store_regs(struct kvm_vcpu *vcpu)
>  {
>  	BUILD_BUG_ON(sizeof(struct kvm_sync_regs) > SYNC_REGS_SIZE_BYTES);
>  
> -	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_REGS)
> -		__get_regs(vcpu, &vcpu->run->s.regs.regs);
> -
> -	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_SREGS)
> -		__get_sregs(vcpu, &vcpu->run->s.regs.sregs);
> +	kvm_run_get_regs(vcpu);
>  
>  	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_EVENTS)
>  		kvm_vcpu_ioctl_x86_get_vcpu_events(
> @@ -12712,19 +11913,8 @@ static void store_regs(struct kvm_vcpu *vcpu)
>  
>  static int sync_regs(struct kvm_vcpu *vcpu)
>  {
> -	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_REGS) {
> -		__set_regs(vcpu, &vcpu->run->s.regs.regs);
> -		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_REGS;
> -	}
> -
> -	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
> -		struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
> -
> -		if (__set_sregs(vcpu, &sregs))
> -			return -EINVAL;
> -
> -		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_SREGS;
> -	}
> +	if (kvm_run_set_regs(vcpu))
> +		return -EINVAL;

Nit:

Do you think 'kvm_run_sync_regs()' is better than 'kvm_run_set_regs()'?

Because I think "sync" reflects better that vcpu->run->kvm_dirty_regs is cleared
after the "set" operation.

>  
>  	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_EVENTS) {
>  		struct kvm_vcpu_events events = vcpu->run->s.regs.events;

Also, I wonder whether it's better to add a helper for events so sync_regs() and
store_regs() can be simplified to:

static int sync_regs(struct kvm_vcpu *vcpu)
{
	if (kvm_run_sync_regs(vcpu))
		return -EINVAL;
	return kvm_run_sync_events(vcpu);
}

static void store_regs(struct kvm_vcpu *vcpu)
{
	kvm_run_get_regs(vcpu);
	kvm_run_get_events(vcpu);
}

And maybe 'kvm_run_get_regs()' could be 'kvm_run_store_regs()' too , so that the
store_regs() could be:

static void store_regs(struct kvm_vcpu *vcpu)
{
	kvm_run_store_regs(vcpu);
	kvm_run_store_events(vcpu);
}

> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 185062a26924..fd55cd031b1c 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -414,6 +414,7 @@ int handle_ud(struct kvm_vcpu *vcpu);
>  
>  void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu,
>  				   struct kvm_queued_exception *ex);
> +void kvm_handle_exception_payload_quirk(struct kvm_vcpu *vcpu);
>  
>  int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data);
>  int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
> @@ -604,6 +605,7 @@ static inline void kvm_machine_check(void)
>  int kvm_spec_ctrl_test_value(u64 value);
>  int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
>  			      struct x86_exception *e);
> +void kvm_invalidate_pcid(struct kvm_vcpu *vcpu, unsigned long pcid);
> 

If I read correct, this is because "regs.c" calls kvm_invalidate_pcid() but you
want to keep it in x86.c.  But it seems the "x86.h" isn't included by "regs.c"
directly but via other headers ("mmu.h" does include "x86.h").

Should the "regs.c" include "x86.h" directly?

Btw, I am a bit confused the relationship between "x86.h" and other headers like
"mmu.h" and the new "regs.h".  That is, headers like "mmu.h" include "x86.h",
but headers like "regs.h" do not (instead, "x86.h" includes them).