Re: [kvm-devel] rebase hlt emulation
Avi Kivity wrote: > Dong, Eddie wrote: >> Gregory Haskins wrote: >> >>> On Thu, 2007-07-12 at 22:20 +0800, Dong, Eddie wrote: >>> >>> >>> Apply to current kvm.git? Then we need to define false for irqchip_in_kernel() which makes the patch quit stranger. >>> I think what Avi is saying is this can be generalized as a separate >>> feature independent of in-kernel-PIC. Since Avi and I are both >>> advocating a separate capability for in-kernel-HLT, you >>> wouldn't need to >>> have a stubbed irqchip_in_kernel. Rather, you should have a flag >>> that indicates whether userspace enabled the HLT capability or not. >>> >> >> Then Avi need to check in your patch in user level to remove the >> race condition, otherwise the patch does nothing and just some dead >> code. >> >> Avi: >> Can u have a double check to see what you want to do. I am fine >> with either of them. >> > > Like Gregory said: we check in the hlt patch to the kernel, and the > race removal + use the new feature patch for userspace. > > Gregory, can you repost that patch please? Eddie, please test both > with and without the userspace patch. > this is race condition which happens only in certain situation. I am afraid I can reproduce this. Greg said he had done test, right? - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Gregory Haskins wrote: > On Thu, 2007-07-12 at 22:20 +0800, Dong, Eddie wrote: > > >> Apply to current kvm.git? Then we need to define false for >> irqchip_in_kernel() which makes the patch quit stranger. > > I think what Avi is saying is this can be generalized as a separate > feature independent of in-kernel-PIC. Since Avi and I are both > advocating a separate capability for in-kernel-HLT, you > wouldn't need to > have a stubbed irqchip_in_kernel. Rather, you should have a flag that > indicates whether userspace enabled the HLT capability or not. Then Avi need to check in your patch in user level to remove the race condition, otherwise the patch does nothing and just some dead code. Avi: Can u have a double check to see what you want to do. I am fine with either of them. Eddie - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Dong, Eddie wrote: > Gregory Haskins wrote: > >> On Thu, 2007-07-12 at 22:20 +0800, Dong, Eddie wrote: >> >> >> >>> Apply to current kvm.git? Then we need to define false for >>> irqchip_in_kernel() which makes the patch quit stranger. >>> >> I think what Avi is saying is this can be generalized as a separate >> feature independent of in-kernel-PIC. Since Avi and I are both >> advocating a separate capability for in-kernel-HLT, you >> wouldn't need to >> have a stubbed irqchip_in_kernel. Rather, you should have a flag that >> indicates whether userspace enabled the HLT capability or not. >> > > Then Avi need to check in your patch in user level to remove the race > condition, otherwise > the patch does nothing and just some dead code. > > Avi: > Can u have a double check to see what you want to do. I am fine > with either of them. > Like Gregory said: we check in the hlt patch to the kernel, and the race removal + use the new feature patch for userspace. Gregory, can you repost that patch please? Eddie, please test both with and without the userspace patch. -- error compiling committee.c: too many arguments to function - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
On Thu, 2007-07-12 at 22:20 +0800, Dong, Eddie wrote: > Apply to current kvm.git? Then we need to define false for > irqchip_in_kernel() which makes the patch quit stranger. I think what Avi is saying is this can be generalized as a separate feature independent of in-kernel-PIC. Since Avi and I are both advocating a separate capability for in-kernel-HLT, you wouldn't need to have a stubbed irqchip_in_kernel. Rather, you should have a flag that indicates whether userspace enabled the HLT capability or not. > > In theory this is only valid for a case with irqchip in kernel since we > always fall back to user if irqchip is in user. How so? In theory, we can halt in the kernel independent of modeling interrupts in the kernel. The difference is in the wakeup logic. For the patch that goes in pre-PIC, the wakeup is predicated solely on signal delivery. Later, the PIC series can modify the wakeup to include the in-kernel sources as well. -Greg - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Avi Kivity wrote:
> Dong, Eddie wrote:
>> Here is the update with adding HLT cap.
>>
>>
>> diff --git a/drivers/kvm/i8259.c b/drivers/kvm/i8259.c index
>> f4ae5f7..e84e665 100644 --- a/drivers/kvm/i8259.c
>> +++ b/drivers/kvm/i8259.c
>> @@ -411,8 +411,13 @@ static void picdev_read(struct kvm_io_device
>> *this, static void pic_irq_request(void *opaque, int level) {
>> struct kvm *kvm = opaque;
>> +struct kvm_vcpu *vcpu = &kvm->vcpus[0];
>>
>> pic_irqchip(kvm)->output = level;
>> +if (waitqueue_active(&vcpu->wq)) {
>> +wake_up_interruptible(&vcpu->wq);
>> +++vcpu->stat.halt_wakeup;
>> +}
>> }
>>
>
> Please strip out non-hlt related things like this so I can apply on
> current kvm.git.
>
Apply to current kvm.git? Then we need to define false for
irqchip_in_kernel() which makes the patch quit stranger.
In theory this is only valid for a case with irqchip in kernel since we
always fall back to user if irqchip is in user.
Eddie
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Gregory Haskins wrote: > On Thu, 2007-07-12 at 13:33 +0800, Dong, Eddie wrote: > >> Here is the update with adding HLT cap. >> > > Thats a good start, but you probably need to make the decision to use > the halt predicated on a halt-specific boolean set by userspace in order > to actually function as we've discussed. This is in contrast to > overloading the decision with the presence of the in-kernel PIC. A > capability indicator without a way to turn it on/off is fairly useless, > IMHO. > > Conversely I don't think its a big deal to just skip the predicate all > together since your version of the patches wont cause as severe of an > issue as we had in hybrid mode. So I would say either make the full > conversion or drop the CAP altogether. > > I'd like to keep it as a precaution. If we later find out it's unneeded, we can rip it out (keeping the cap but always doing hlt in kernel, regardless of what the user wants). -- error compiling committee.c: too many arguments to function - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Dong, Eddie wrote:
> Here is the update with adding HLT cap.
>
>
> diff --git a/drivers/kvm/i8259.c b/drivers/kvm/i8259.c
> index f4ae5f7..e84e665 100644
> --- a/drivers/kvm/i8259.c
> +++ b/drivers/kvm/i8259.c
> @@ -411,8 +411,13 @@ static void picdev_read(struct kvm_io_device *this,
> static void pic_irq_request(void *opaque, int level)
> {
> struct kvm *kvm = opaque;
> + struct kvm_vcpu *vcpu = &kvm->vcpus[0];
>
> pic_irqchip(kvm)->output = level;
> + if (waitqueue_active(&vcpu->wq)) {
> + wake_up_interruptible(&vcpu->wq);
> + ++vcpu->stat.halt_wakeup;
> + }
> }
>
Please strip out non-hlt related things like this so I can apply on
current kvm.git.
--
error compiling committee.c: too many arguments to function
-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
On Thu, 2007-07-12 at 13:33 +0800, Dong, Eddie wrote: > Here is the update with adding HLT cap. Thats a good start, but you probably need to make the decision to use the halt predicated on a halt-specific boolean set by userspace in order to actually function as we've discussed. This is in contrast to overloading the decision with the presence of the in-kernel PIC. A capability indicator without a way to turn it on/off is fairly useless, IMHO. Conversely I don't think its a big deal to just skip the predicate all together since your version of the patches wont cause as severe of an issue as we had in hybrid mode. So I would say either make the full conversion or drop the CAP altogether. -Greg - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] rebase hlt emulation
Here is the update with adding HLT cap.
diff --git a/drivers/kvm/i8259.c b/drivers/kvm/i8259.c
index f4ae5f7..e84e665 100644
--- a/drivers/kvm/i8259.c
+++ b/drivers/kvm/i8259.c
@@ -411,8 +411,13 @@ static void picdev_read(struct kvm_io_device *this,
static void pic_irq_request(void *opaque, int level)
{
struct kvm *kvm = opaque;
+ struct kvm_vcpu *vcpu = &kvm->vcpus[0];
pic_irqchip(kvm)->output = level;
+ if (waitqueue_active(&vcpu->wq)) {
+ wake_up_interruptible(&vcpu->wq);
+ ++vcpu->stat.halt_wakeup;
+ }
}
struct kvm_pic *kvm_create_pic(struct kvm *kvm)
diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index f1a6773..1d1ee4f 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -260,6 +260,7 @@ struct kvm_stat {
u32 signal_exits;
u32 irq_window_exits;
u32 halt_exits;
+ u32 halt_wakeup;
u32 request_irq_exits;
u32 irq_exits;
u32 light_exits;
@@ -399,6 +400,7 @@ struct kvm_vcpu {
gva_t mmio_fault_cr2;
struct kvm_pio_request pio;
void *pio_data;
+ wait_queue_head_t wq;
int sigset_active;
sigset_t sigset;
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 06e5804..98d9f32 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -74,6 +74,7 @@ static struct kvm_stats_debugfs_item {
{ "signal_exits", STAT_OFFSET(signal_exits) },
{ "irq_window", STAT_OFFSET(irq_window_exits) },
{ "halt_exits", STAT_OFFSET(halt_exits) },
+ { "halt_wakeup", STAT_OFFSET(halt_wakeup) },
{ "request_irq", STAT_OFFSET(request_irq_exits) },
{ "irq_exits", STAT_OFFSET(irq_exits) },
{ "light_exits", STAT_OFFSET(light_exits) },
@@ -326,6 +327,7 @@ static struct kvm *kvm_create_vm(void)
vcpu->cpu = -1;
vcpu->kvm = kvm;
vcpu->mmu.root_hpa = INVALID_PAGE;
+ init_waitqueue_head(&vcpu->wq);
}
return kvm;
}
@@ -1324,15 +1326,41 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
}
EXPORT_SYMBOL_GPL(emulate_instruction);
-int kvm_emulate_halt(struct kvm_vcpu *vcpu)
+/*
+ * The vCPU has executed a HLT instruction with in-kernel mode enabled.
+ */
+static void kvm_vcpu_kernel_halt(struct kvm_vcpu *vcpu)
{
- if (vcpu->irq_summary ||
- (irqchip_in_kernel(vcpu->kvm) &&
kvm_cpu_has_interrupt(vcpu)))
- return 1;
+ DECLARE_WAITQUEUE(wait, current);
+
+ add_wait_queue(&vcpu->wq, &wait);
+
+ /*
+* We will block until either an interrupt or a signal wakes us
up
+*/
+ while(!(irqchip_in_kernel(vcpu->kvm) &&
kvm_cpu_has_interrupt(vcpu))
+ && !vcpu->irq_summary
+ && !signal_pending(current)) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ vcpu_put(vcpu);
+ schedule();
+ vcpu_load(vcpu);
+ }
- vcpu->run->exit_reason = KVM_EXIT_HLT;
+ remove_wait_queue(&vcpu->wq, &wait);
+ set_current_state(TASK_RUNNING);
+}
+
+int kvm_emulate_halt(struct kvm_vcpu *vcpu)
+{
++vcpu->stat.halt_exits;
- return 0;
+ if (irqchip_in_kernel(vcpu->kvm)) {
+ kvm_vcpu_kernel_halt(vcpu);
+ return 1;
+ } else {
+ vcpu->run->exit_reason = KVM_EXIT_HLT;
+ return 0;
+ }
}
EXPORT_SYMBOL_GPL(kvm_emulate_halt);
@@ -2925,6 +2953,7 @@ static long kvm_dev_ioctl(struct file *filp,
switch (ext) {
case KVM_CAP_PIC:
+ case KVM_CAP_HLT:
r = 1;
break;
default:
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index f614800..7150f05 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -1469,8 +1469,12 @@ static void do_interrupt_requests(struct kvm_vcpu
*vcpu,
static void post_kvm_run_save(struct kvm_vcpu *vcpu,
struct kvm_run *kvm_run)
{
- kvm_run->ready_for_interrupt_injection =
(vcpu->interrupt_window_open &&
- vcpu->irq_summary ==
0);
+ if (irqchip_in_kernel(vcpu->kvm))
+ kvm_run->ready_for_interrupt_injection = 1;
+ else
+ kvm_run->ready_for_interrupt_injection =
+(vcpu->interrupt_window_open &&
+ vcpu->irq_summary == 0);
kvm_run->if_flag = (vcpu->svm->vmcb->save.rflags &
X86_EFLAGS_IF) != 0;
kvm_run->cr8 = vcpu->cr8;
kvm_run->apic_base = vcpu->apic_base;
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index 598b2b2..ece7f86 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -1896,8 +1896,12 @@ static void post_kvm_run_save(struct kvm_vcpu
*vcpu,
kvm_run->if_flag = (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) !=
0;
kvm_run->cr8 = vcpu->c
