From: Zhang, X. <xia...@in...> - 2007-11-08 07:22:17
|
>From cba4340cef2217343c540ca5de9b67cc8826b63f Mon Sep 17 00:00:00 2001 From: Zhang Xiantao <xia...@in...> Date: Thu, 8 Nov 2007 13:07:23 +0800 Subject: [PATCH] Using kvm_arch prefix to define functions, and replace kvm_x86_ops callback. Signed-off-by: Zhang Xiantao <xia...@in...> --- drivers/kvm/kvm.h | 15 +++++++++++++++ drivers/kvm/kvm_main.c | 26 +++++++++++++------------- drivers/kvm/x86.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h index 091f9b7..4b2421a 100644 --- a/drivers/kvm/kvm.h +++ b/drivers/kvm/kvm.h @@ -649,6 +649,21 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run); =20 __init void kvm_arch_init(void); =20 + +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id); + +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); +void kvm_arch_hardware_enable(void *garbage); +void kvm_arch_hardware_disable(void *garbage); +int kvm_arch_hardware_setup(void); +void kvm_arch_hardware_unsetup(void); +void kvm_arch_check_processor_compat(void *rtn); + + static inline void kvm_guest_enter(void) { account_system_vtime(current); diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c index f1cf8f0..da7fb22 100644 --- a/drivers/kvm/kvm_main.c +++ b/drivers/kvm/kvm_main.c @@ -240,7 +240,7 @@ static void kvm_free_vcpus(struct kvm *kvm) kvm_unload_vcpu_mmu(kvm->vcpus[i]); for (i =3D 0; i < KVM_MAX_VCPUS; ++i) { if (kvm->vcpus[i]) { - kvm_x86_ops->vcpu_free(kvm->vcpus[i]); + kvm_arch_vcpu_free(kvm->vcpus[i]); kvm->vcpus[i] =3D NULL; } } @@ -890,7 +890,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n) if (!valid_vcpu(n)) return -EINVAL; =20 - vcpu =3D kvm_x86_ops->vcpu_create(kvm, n); + vcpu =3D kvm_arch_vcpu_create(kvm, n); if (IS_ERR(vcpu)) return PTR_ERR(vcpu); =20 @@ -900,7 +900,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n) BUG_ON((unsigned long)&vcpu->host_fx_image & 0xF); =20 vcpu_load(vcpu); - r =3D kvm_x86_ops->vcpu_reset(vcpu); + r =3D kvm_arch_vcpu_reset(vcpu); if (r =3D=3D 0) r =3D kvm_mmu_setup(vcpu); vcpu_put(vcpu); @@ -933,7 +933,7 @@ mmu_unload: vcpu_put(vcpu); =20 free_vcpu: - kvm_x86_ops->vcpu_free(vcpu); + kvm_arch_vcpu_free(vcpu); return r; } =20 @@ -1297,7 +1297,7 @@ static void decache_vcpus_on_cpu(int cpu) */ if (mutex_trylock(&vcpu->mutex)) { if (vcpu->cpu =3D=3D cpu) { - kvm_x86_ops->vcpu_decache(vcpu); + kvm_arch_vcpu_decache(vcpu); vcpu->cpu =3D -1; } mutex_unlock(&vcpu->mutex); @@ -1313,7 +1313,7 @@ static void hardware_enable(void *junk) if (cpu_isset(cpu, cpus_hardware_enabled)) return; cpu_set(cpu, cpus_hardware_enabled); - kvm_x86_ops->hardware_enable(NULL); + kvm_arch_hardware_enable(NULL); } =20 static void hardware_disable(void *junk) @@ -1324,7 +1324,7 @@ static void hardware_disable(void *junk) return; cpu_clear(cpu, cpus_hardware_enabled); decache_vcpus_on_cpu(cpu); - kvm_x86_ops->hardware_disable(NULL); + kvm_arch_hardware_disable(NULL); } =20 static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val, @@ -1492,7 +1492,7 @@ static void kvm_sched_in(struct preempt_notifier *pn, int cpu) { struct kvm_vcpu *vcpu =3D preempt_notifier_to_vcpu(pn); =20 - kvm_x86_ops->vcpu_load(vcpu, cpu); + kvm_arch_vcpu_load(vcpu, cpu); } =20 static void kvm_sched_out(struct preempt_notifier *pn, @@ -1500,7 +1500,7 @@ static void kvm_sched_out(struct preempt_notifier *pn, { struct kvm_vcpu *vcpu =3D preempt_notifier_to_vcpu(pn); =20 - kvm_x86_ops->vcpu_put(vcpu); + kvm_arch_vcpu_put(vcpu); } =20 int kvm_init_x86(struct kvm_x86_ops *ops, unsigned int vcpu_size, @@ -1525,13 +1525,13 @@ int kvm_init_x86(struct kvm_x86_ops *ops, unsigned int vcpu_size, =20 kvm_x86_ops =3D ops; =20 - r =3D kvm_x86_ops->hardware_setup(); + r =3D kvm_arch_hardware_setup(); if (r < 0) goto out; =20 for_each_online_cpu(cpu) { smp_call_function_single(cpu, - kvm_x86_ops->check_processor_compatibility, + kvm_arch_check_processor_compat, &r, 0, 1); if (r < 0) goto out_free_0; @@ -1586,7 +1586,7 @@ out_free_2: out_free_1: on_each_cpu(hardware_disable, NULL, 0, 1); out_free_0: - kvm_x86_ops->hardware_unsetup(); + kvm_arch_hardware_unsetup(); out: kvm_x86_ops =3D NULL; return r; @@ -1602,7 +1602,7 @@ void kvm_exit_x86(void) unregister_reboot_notifier(&kvm_reboot_notifier); unregister_cpu_notifier(&kvm_cpu_notifier); on_each_cpu(hardware_disable, NULL, 0, 1); - kvm_x86_ops->hardware_unsetup(); + kvm_arch_hardware_unsetup(); kvm_x86_ops =3D NULL; } EXPORT_SYMBOL_GPL(kvm_exit_x86); diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c index e26e46a..8aea240 100644 --- a/drivers/kvm/x86.c +++ b/drivers/kvm/x86.c @@ -2327,3 +2327,50 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) fx_restore(&vcpu->host_fx_image); } EXPORT_SYMBOL_GPL(kvm_put_guest_fpu); + +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) +{ + kvm_x86_ops->vcpu_free(vcpu); +} + +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu) +{ + kvm_x86_ops->vcpu_decache(vcpu); +} + +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, + unsigned int id) +{ + return kvm_x86_ops->vcpu_create(kvm, id); +} + +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) +{ + return kvm_x86_ops->vcpu_reset(vcpu); +} + +void kvm_arch_hardware_enable(void *garbage) +{ + kvm_x86_ops->hardware_enable(garbage); +} + +void kvm_arch_hardware_disable(void *garbage) +{ + kvm_x86_ops->hardware_disable(garbage); +} + +int kvm_arch_hardware_setup(void) +{ + return kvm_x86_ops->hardware_setup(); +} + +void kvm_arch_hardware_unsetup(void) +{ + kvm_x86_ops->hardware_unsetup(); +} + +void kvm_arch_check_processor_compat(void *rtn) +{ + kvm_x86_ops->check_processor_compatibility(rtn); +} + --=20 1.5.1.2 |
From: Carsten O. <co...@de...> - 2007-11-08 13:49:21
|
Zhang, Xiantao wrote: > +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); > +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); > +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); > +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); > +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int > id); > + > +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); > +void kvm_arch_hardware_enable(void *garbage); > +void kvm_arch_hardware_disable(void *garbage); > +int kvm_arch_hardware_setup(void); > +void kvm_arch_hardware_unsetup(void); > +void kvm_arch_check_processor_compat(void *rtn); I don't like the generic introduction of all x86_ops wrappers into the arch callbacks. I would rather prefer to work out a different split between common and arch specifics - at least in the following cases: - unloading the mmu needs to be moved out of kvm_free_vcpus into the arch part, because we don't have a shaddow mmu on s390 - decache_vcpus_on_cpu should be arch-dependent alltogether, rather than having a per cpu callback. We've got nothing to decache, so the entire thing is a nop for us. - vcpu_reset works very different for our architecture, we'd need an initial processor status word. I'd prefer to keep the existence of this callback arch dependent. - hardware enable/disable/setup/unsetup/check_processor_compat does not make any sense for us: all CPUs that have been sold since the 1970s have proper hardware virtualization, and there's nothing to enable - it just works. |
From: Zhang, X. <xia...@in...> - 2007-11-08 15:07:06
|
Carsten Otte wrote: > Zhang, Xiantao wrote: >> +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); >> +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); >> +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); >> +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); >> +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int >> id); + >> +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); >> +void kvm_arch_hardware_enable(void *garbage); >> +void kvm_arch_hardware_disable(void *garbage); >> +int kvm_arch_hardware_setup(void); >> +void kvm_arch_hardware_unsetup(void); >> +void kvm_arch_check_processor_compat(void *rtn); > I don't like the generic introduction of all x86_ops wrappers into the > arch callbacks. I would rather prefer to work out a different split > between common and arch specifics - at least in the following cases: > - unloading the mmu needs to be moved out of kvm_free_vcpus into the > arch part, because we don't have a shaddow mmu on s390 I think we have an old thread related to mmu code in kvm_main.c. In IA64, we haven't shadow mmu as well, but we still need mmu interfaces, such as mmu load, reload, unload to notify arch-specific VMM module about the memory changes.=20 > - decache_vcpus_on_cpu should be arch-dependent alltogether, rather > than having a per cpu callback. We've got nothing to decache, so the > entire thing is a nop for us. Actually, IA64 doesn't require it. As you said, decache_vcpus_on_cpu is only needed by x86. > - vcpu_reset works very different for our architecture, we'd need an > initial processor status word. I'd prefer to keep the existence of > this callback arch dependent. Yes, I think kvm_arch_vcpu_reset in this patch should meet your requirement. > - hardware enable/disable/setup/unsetup/check_processor_compat does > not make any sense for us: all CPUs that have been sold since the > 1970s have proper hardware virtualization, and there's nothing to > enable - it just works. OK, Maybe we can move them to arch-specific. =20 Avi, How about your ideas? Thanks=20 Xiantao |
From: Avi K. <av...@qu...> - 2007-11-11 10:50:14
|
Zhang, Xiantao wrote: > >> - hardware enable/disable/setup/unsetup/check_processor_compat does >> not make any sense for us: all CPUs that have been sold since the >> 1970s have proper hardware virtualization, and there's nothing to >> enable - it just works. >> > OK, Maybe we can move them to arch-specific. > > Avi, How about your ideas? I'm actually least qualified to comment since I only know about x86. Since this is all preparations before actual code is seen, it's hard to see where things are flowing. I don't mind if a few mistakes are made along the way that are corrected later, as long as x86 continues to build and run fine. It's okay to start working on this problem with a 10-pound hammer and move to atomic force microscopy once things are merged. So, I will be very much inclined to accept anything that has gained the consensus of the arch maintainers. -- error compiling committee.c: too many arguments to function |
From: Hollis B. <ho...@us...> - 2007-11-08 20:19:00
|
On Thu, 2007-11-08 at 14:49 +0100, Carsten Otte wrote: > Zhang, Xiantao wrote: > > +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); > > +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); > > +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); > > +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); > > +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int > > id); > > + > > +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); > > +void kvm_arch_hardware_enable(void *garbage); > > +void kvm_arch_hardware_disable(void *garbage); > > +int kvm_arch_hardware_setup(void); > > +void kvm_arch_hardware_unsetup(void); > > +void kvm_arch_check_processor_compat(void *rtn); > I don't like the generic introduction of all x86_ops wrappers into the > arch callbacks. I would rather prefer to work out a different split > between common and arch specifics - at least in the following cases: > - unloading the mmu needs to be moved out of kvm_free_vcpus into the > arch part, because we don't have a shaddow mmu on s390 > - decache_vcpus_on_cpu should be arch-dependent alltogether, rather > than having a per cpu callback. We've got nothing to decache, so the > entire thing is a nop for us. > - vcpu_reset works very different for our architecture, we'd need an > initial processor status word. I'd prefer to keep the existence of > this callback arch dependent. > - hardware enable/disable/setup/unsetup/check_processor_compat does not > make any sense for us: all CPUs that have been sold since the 1970s have > proper hardware virtualization, and there's nothing to enable - it just > works. Sounds fine to me: you're just proposing to move the abstraction one level higher in some places. -- Hollis Blanchard IBM Linux Technology Center |
From: Carsten O. <co...@de...> - 2007-11-09 09:01:46
|
Hollis Blanchard wrote: > On Thu, 2007-11-08 at 14:49 +0100, Carsten Otte wrote: >> Zhang, Xiantao wrote: >>> +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); >>> +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); >>> +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); >>> +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); >>> +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int >>> id); >>> + >>> +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); >>> +void kvm_arch_hardware_enable(void *garbage); >>> +void kvm_arch_hardware_disable(void *garbage); >>> +int kvm_arch_hardware_setup(void); >>> +void kvm_arch_hardware_unsetup(void); >>> +void kvm_arch_check_processor_compat(void *rtn); >> I don't like the generic introduction of all x86_ops wrappers into the >> arch callbacks. I would rather prefer to work out a different split >> between common and arch specifics - at least in the following cases: >> - unloading the mmu needs to be moved out of kvm_free_vcpus into the >> arch part, because we don't have a shaddow mmu on s390 >> - decache_vcpus_on_cpu should be arch-dependent alltogether, rather >> than having a per cpu callback. We've got nothing to decache, so the >> entire thing is a nop for us. >> - vcpu_reset works very different for our architecture, we'd need an >> initial processor status word. I'd prefer to keep the existence of >> this callback arch dependent. >> - hardware enable/disable/setup/unsetup/check_processor_compat does not >> make any sense for us: all CPUs that have been sold since the 1970s have >> proper hardware virtualization, and there's nothing to enable - it just >> works. > > Sounds fine to me: you're just proposing to move the abstraction one > level higher in some places. That's right, I'd like to drag the bar a little where the common code does something just to call a callback that is nop for us. |
From: Carsten O. <co...@de...> - 2007-11-12 09:16:43
|
Avi Kivity wrote: > For the present discussion, I agree, but in general we should be > prepared to accept some no-op callouts. Oh sure, I don't mind those. We'll have plenty of them, where other architectures will need to take action and s390 won't. It's just that in the current location, the common code would do common_func() { loop { arch_callback(); } } And I suggested to make the whole thing a callback. |
From: Avi K. <av...@qu...> - 2007-11-12 09:51:58
|
Carsten Otte wrote: > Avi Kivity wrote: > >> For the present discussion, I agree, but in general we should be >> prepared to accept some no-op callouts. >> > > Oh sure, I don't mind those. We'll have plenty of them, where other > architectures will need to take action and s390 won't. It's just that > in the current location, the common code would do > > common_func() { > loop { > arch_callback(); > } > } > And I suggested to make the whole thing a callback. > Ah, agreed. -- error compiling committee.c: too many arguments to function |
From: Avi K. <av...@qu...> - 2007-11-11 10:51:26
|
Carsten Otte wrote: > Hollis Blanchard wrote: > >> On Thu, 2007-11-08 at 14:49 +0100, Carsten Otte wrote: >> >>> Zhang, Xiantao wrote: >>> >>>> +void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu); >>>> +void kvm_arch_vcpu_decache(struct kvm_vcpu *vcpu); >>>> +void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu); >>>> +void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu); >>>> +struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int >>>> id); >>>> + >>>> +int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu); >>>> +void kvm_arch_hardware_enable(void *garbage); >>>> +void kvm_arch_hardware_disable(void *garbage); >>>> +int kvm_arch_hardware_setup(void); >>>> +void kvm_arch_hardware_unsetup(void); >>>> +void kvm_arch_check_processor_compat(void *rtn); >>>> >>> I don't like the generic introduction of all x86_ops wrappers into the >>> arch callbacks. I would rather prefer to work out a different split >>> between common and arch specifics - at least in the following cases: >>> - unloading the mmu needs to be moved out of kvm_free_vcpus into the >>> arch part, because we don't have a shaddow mmu on s390 >>> - decache_vcpus_on_cpu should be arch-dependent alltogether, rather >>> than having a per cpu callback. We've got nothing to decache, so the >>> entire thing is a nop for us. >>> - vcpu_reset works very different for our architecture, we'd need an >>> initial processor status word. I'd prefer to keep the existence of >>> this callback arch dependent. >>> - hardware enable/disable/setup/unsetup/check_processor_compat does not >>> make any sense for us: all CPUs that have been sold since the 1970s have >>> proper hardware virtualization, and there's nothing to enable - it just >>> works. >>> >> Sounds fine to me: you're just proposing to move the abstraction one >> level higher in some places. >> > That's right, I'd like to drag the bar a little where the common code > does something just to call a callback that is nop for us. > For the present discussion, I agree, but in general we should be prepared to accept some no-op callouts. -- error compiling committee.c: too many arguments to function |
From: Hollis B. <ho...@us...> - 2007-11-08 20:58:14
|
On Thu, 2007-11-08 at 15:21 +0800, Zhang, Xiantao wrote: > @@ -890,7 +890,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm > *kvm, > int n) > if (!valid_vcpu(n)) > return -EINVAL; > > - vcpu = kvm_x86_ops->vcpu_create(kvm, n); > + vcpu = kvm_arch_vcpu_create(kvm, n); > if (IS_ERR(vcpu)) > return PTR_ERR(vcpu); > > @@ -900,7 +900,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm > *kvm, > int n) > BUG_ON((unsigned long)&vcpu->host_fx_image & 0xF); > > vcpu_load(vcpu); > - r = kvm_x86_ops->vcpu_reset(vcpu); > + r = kvm_arch_vcpu_reset(vcpu); > if (r == 0) > r = kvm_mmu_setup(vcpu); > vcpu_put(vcpu); > @@ -933,7 +933,7 @@ mmu_unload: > vcpu_put(vcpu); > > free_vcpu: > - kvm_x86_ops->vcpu_free(vcpu); > + kvm_arch_vcpu_free(vcpu); > return r; > } Have a look at the patch I posted on Wednesday: "[PATCH 2 of 2] RFC: Create kvm_arch_vcpu_create()". kvm_arch_vcpu_create() will actually encompass more logic from kvm_vm_ioctl_create_vcpu(), and once you do that, kvm_arch_vcpu_reset() doesn't need to exist (which will also make Carsten happy). -- Hollis Blanchard IBM Linux Technology Center |
From: Zhang, X. <xia...@in...> - 2007-11-09 07:25:40
|
Hollis Blanchard wrote: >> - kvm_x86_ops->vcpu_free(vcpu); >> + kvm_arch_vcpu_free(vcpu); >> return r; >> } >=20 > Have a look at the patch I posted on Wednesday: "[PATCH 2 of 2] RFC: > Create kvm_arch_vcpu_create()". kvm_arch_vcpu_create() will actually > encompass more logic from kvm_vm_ioctl_create_vcpu(), and once you do > that, kvm_arch_vcpu_reset() doesn't need to exist (which will also > make Carsten happy). Hi, Hollis Thanks for your response. If we need to move out the code related to mmu, sure to keep more logic in kvm_arch_create_vcpu. But I don't know the decision about the mmu part putting in common or arch. As Avi said before, maybe we need to keep mmu interfaces in common for future possible memory hotplug of guest and so on. =20 Best Wishes Xiantao |