Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On Tue, Nov 24, 2009 at 08:08:37PM -0200, Marcelo Tosatti wrote: > On Sun, Nov 15, 2009 at 04:41:26PM +0100, Jan Kiszka wrote: > > Avi Kivity wrote: > > > On 11/15/2009 05:02 PM, Jan Kiszka wrote: > > >> > > >> Where should I add "/* next version to use: 13 */", and who will take > > >> care that this comment will also be kept up to date? The CPU vmstate is > > >> already ordered according to logical groups, just look at earlier field. > > >> Only recent KVM additions happened to create some version ordering as > > >> well. > > >> > > > > > > Er, now I'm confused. 11 and 12 indeed do already exist, so how can you > > > update 11 retroactively? > > > > Oh, right, good that we discuss this. My patch dated back before the > > kvmclock addition, which IMHO incorrectly bumped the version numbers. I > > think the current policy in upstream is that we only increment once per > > qemu release, not per bit added. > > > > > > > > Shouldn't you create 13 now? > > > > No, I rather think Glauber's 12 should be downgraded to 11 - unless it > > misses the qemu merge windows for 0.12. My extensions definitely target > > that release, thus will likely carry 11 in upstream. And we should > > really try to avoid diverging again. > > Agree. > > Anthony, > > Are Glauber patches going to make it for 0.12, so we can revert current > qemu-kvm's CPU_SAVE_VERSION from 12 to 11? > > Thanks. Anthony, I still don't see this patch in for qemu 0.12. Please merge it. Date: Thu, 22 Oct 2009 10:26:56 -0200 From: Glauber Costa To: qemu-de...@nongnu.org Subject: [Qemu-devel] [PATCH] v2: properly save kvm system time msr registers Message-Id: <1256214416-20554-1-git-send-email-glom...@redhat.com> TIA -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On Sun, Nov 15, 2009 at 04:41:26PM +0100, Jan Kiszka wrote: > Avi Kivity wrote: > > On 11/15/2009 05:02 PM, Jan Kiszka wrote: > >> > >> Where should I add "/* next version to use: 13 */", and who will take > >> care that this comment will also be kept up to date? The CPU vmstate is > >> already ordered according to logical groups, just look at earlier field. > >> Only recent KVM additions happened to create some version ordering as > >> well. > >> > > > > Er, now I'm confused. 11 and 12 indeed do already exist, so how can you > > update 11 retroactively? > > Oh, right, good that we discuss this. My patch dated back before the > kvmclock addition, which IMHO incorrectly bumped the version numbers. I > think the current policy in upstream is that we only increment once per > qemu release, not per bit added. > > > > > Shouldn't you create 13 now? > > No, I rather think Glauber's 12 should be downgraded to 11 - unless it > misses the qemu merge windows for 0.12. My extensions definitely target > that release, thus will likely carry 11 in upstream. And we should > really try to avoid diverging again. Agree. Anthony, Are Glauber patches going to make it for 0.12, so we can revert current qemu-kvm's CPU_SAVE_VERSION from 12 to 11? Thanks. > > > > > (and I meant: /* The above list is not sorted wrt version, watch out! > > */, but now I feel I'm missing something). > > > > Ok, can add such a note at the end. > > Jan > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/15/2009 05:50 PM, Avi Kivity wrote: Shouldn't you create 13 now? No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again. Agree. Will commit the patches. Ugh, so I did it the hard way by merging upstream and porting it (incorrectly), then going back to my queue to see that you had already nicely prepared this for me. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/15/2009 05:41 PM, Jan Kiszka wrote: Avi Kivity wrote: On 11/15/2009 05:02 PM, Jan Kiszka wrote: Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well. Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively? Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added. What about stable releases? If we need to port a commit which adds a bit, we need to port the entire thing. (well version numbers don't work with nonlinear development anyway). Shouldn't you create 13 now? No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again. Agree. Will commit the patches. (and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something). Ok, can add such a note at the end. In upstream... -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
Avi Kivity wrote: > On 11/15/2009 05:02 PM, Jan Kiszka wrote: >> >> Where should I add "/* next version to use: 13 */", and who will take >> care that this comment will also be kept up to date? The CPU vmstate is >> already ordered according to logical groups, just look at earlier field. >> Only recent KVM additions happened to create some version ordering as >> well. >> > > Er, now I'm confused. 11 and 12 indeed do already exist, so how can you > update 11 retroactively? Oh, right, good that we discuss this. My patch dated back before the kvmclock addition, which IMHO incorrectly bumped the version numbers. I think the current policy in upstream is that we only increment once per qemu release, not per bit added. > > Shouldn't you create 13 now? No, I rather think Glauber's 12 should be downgraded to 11 - unless it misses the qemu merge windows for 0.12. My extensions definitely target that release, thus will likely carry 11 in upstream. And we should really try to avoid diverging again. > > (and I meant: /* The above list is not sorted wrt version, watch out! > */, but now I feel I'm missing something). > Ok, can add such a note at the end. Jan signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/15/2009 05:02 PM, Jan Kiszka wrote: Where should I add "/* next version to use: 13 */", and who will take care that this comment will also be kept up to date? The CPU vmstate is already ordered according to logical groups, just look at earlier field. Only recent KVM additions happened to create some version ordering as well. Er, now I'm confused. 11 and 12 indeed do already exist, so how can you update 11 retroactively? Shouldn't you create 13 now? (and I meant: /* The above list is not sorted wrt version, watch out! */, but now I feel I'm missing something). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
Avi Kivity wrote:
> On 11/15/2009 04:31 PM, Jan Kiszka wrote:
>
> Is there a reason why you add 11 between 9 and 10? We'll probably see
> another 11 when someone else adds the next state.
>
>
>
Logical grouping ("/* KVM-related states */").
>>> These aren't kvm-related, just not implemented in tcg yet. Nothing
>>> kvmish about them - it's all architectural state.
>>>
>> Most of them are KVM-specific. TCG don't have to deal with event
>> re-injection due to host page faults etc. on first try.
>>
>
> Right, tcg can do some things atomically. There's some non-kvm state in
> there.
>
If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be
rejected. I don't think you have to sort VMSTATE entries by their
version number. Am I right, Juan?
>>> I'm worried about something else - someone looking at the end, seeing
>>> version 10, and appending new state with version 11.
>>>
>> Again, that's something proper review should catch (just like checking
>> for the right place when adding a new IOCTL to kvm.h).
>>
>
> And we know how well that works. We should try to make things work by
> default and leave the review to catch the really difficult things (like
> trailing whitespace).
>
> At least add a comment at the end warning people that simple append will
> fail.
>
Where should I add "/* next version to use: 13 */", and who will take
care that this comment will also be kept up to date? The CPU vmstate is
already ordered according to logical groups, just look at earlier field.
Only recent KVM additions happened to create some version ordering as well.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/15/2009 04:31 PM, Jan Kiszka wrote:
Is there a reason why you add 11 between 9 and 10? We'll probably see
another 11 when someone else adds the next state.
Logical grouping ("/* KVM-related states */").
These aren't kvm-related, just not implemented in tcg yet. Nothing
kvmish about them - it's all architectural state.
Most of them are KVM-specific. TCG don't have to deal with event
re-injection due to host page faults etc. on first try.
Right, tcg can do some things atomically. There's some non-kvm state in
there.
If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be
rejected. I don't think you have to sort VMSTATE entries by their
version number. Am I right, Juan?
I'm worried about something else - someone looking at the end, seeing
version 10, and appending new state with version 11.
Again, that's something proper review should catch (just like checking
for the right place when adding a new IOCTL to kvm.h).
And we know how well that works. We should try to make things work by
default and leave the review to catch the really difficult things (like
trailing whitespace).
At least add a comment at the end warning people that simple append will
fail.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
Avi Kivity wrote:
> On 11/15/2009 04:21 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>
>>> On 11/12/2009 02:05 AM, Jan Kiszka wrote:
>>>
This patch extends the qemu-kvm state sync logic with support for
KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception,
interrupt and NMI states.
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..1eda7c5 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
VMSTATE_UINT32_V(mp_state, CPUState, 9),
VMSTATE_UINT64_V(tsc, CPUState, 9),
+VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
/* MCE */
VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
VMSTATE_UINT64_V(mcg_status, CPUState, 10),
>>> Is there a reason why you add 11 between 9 and 10? We'll probably see
>>> another 11 when someone else adds the next state.
>>>
>>>
>> Logical grouping ("/* KVM-related states */").
>
> These aren't kvm-related, just not implemented in tcg yet. Nothing
> kvmish about them - it's all architectural state.
Most of them are KVM-specific. TCG don't have to deal with event
re-injection due to host page faults etc. on first try.
>
>> If anyone once tries to
>> add non-KVM stuff here just because it's version 12, it should be
>> rejected. I don't think you have to sort VMSTATE entries by their
>> version number. Am I right, Juan?
>>
>
> I'm worried about something else - someone looking at the end, seeing
> version 10, and appending new state with version 11.
Again, that's something proper review should catch (just like checking
for the right place when adding a new IOCTL to kvm.h).
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/15/2009 04:21 PM, Jan Kiszka wrote:
Avi Kivity wrote:
On 11/12/2009 02:05 AM, Jan Kiszka wrote:
This patch extends the qemu-kvm state sync logic with support for
KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception,
interrupt and NMI states.
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..1eda7c5 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
VMSTATE_UINT32_V(mp_state, CPUState, 9),
VMSTATE_UINT64_V(tsc, CPUState, 9),
+VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
/* MCE */
VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
VMSTATE_UINT64_V(mcg_status, CPUState, 10),
Is there a reason why you add 11 between 9 and 10? We'll probably see
another 11 when someone else adds the next state.
Logical grouping ("/* KVM-related states */").
These aren't kvm-related, just not implemented in tcg yet. Nothing
kvmish about them - it's all architectural state.
If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be
rejected. I don't think you have to sort VMSTATE entries by their
version number. Am I right, Juan?
I'm worried about something else - someone looking at the end, seeing
version 10, and appending new state with version 11.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
Avi Kivity wrote:
> On 11/12/2009 02:05 AM, Jan Kiszka wrote:
>> This patch extends the qemu-kvm state sync logic with support for
>> KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception,
>> interrupt and NMI states.
>>
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index 6bd447f..1eda7c5 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
>> VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
>> VMSTATE_UINT32_V(mp_state, CPUState, 9),
>> VMSTATE_UINT64_V(tsc, CPUState, 9),
>> +VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
>> +VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
>> +VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
>> +VMSTATE_UINT8_V(has_error_code, CPUState, 11),
>> +VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
>> /* MCE */
>> VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
>> VMSTATE_UINT64_V(mcg_status, CPUState, 10),
>>
>>
>
> Is there a reason why you add 11 between 9 and 10? We'll probably see
> another 11 when someone else adds the next state.
>
Logical grouping ("/* KVM-related states */"). If anyone once tries to
add non-KVM stuff here just because it's version 12, it should be
rejected. I don't think you have to sort VMSTATE entries by their
version number. Am I right, Juan?
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
On 11/12/2009 02:05 AM, Jan Kiszka wrote:
This patch extends the qemu-kvm state sync logic with support for
KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception,
interrupt and NMI states.
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..1eda7c5 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
VMSTATE_UINT32_V(mp_state, CPUState, 9),
VMSTATE_UINT64_V(tsc, CPUState, 9),
+VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
/* MCE */
VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
VMSTATE_UINT64_V(mcg_status, CPUState, 10),
Is there a reason why you add 11 between 9 and 10? We'll probably see
another 11 when someone else adds the next state.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] qemu-kvm: x86: Add support for VCPU event states
This patch extends the qemu-kvm state sync logic with support for
KVM_GET/SET_VCPU_EVENTS, giving access to yet missing exception,
interrupt and NMI states.
Signed-off-by: Jan Kiszka
---
qemu-kvm-x86.c| 64 +
target-i386/cpu.h |5
target-i386/machine.c |5
3 files changed, 74 insertions(+), 0 deletions(-)
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index e03a4ba..577c5db 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -903,6 +903,60 @@ static void get_seg(SegmentCache *lhs, const struct
kvm_segment *rhs)
| (rhs->avl * DESC_AVL_MASK);
}
+static void kvm_get_vcpu_events(CPUState *env)
+{
+#ifdef KVM_CAP_VCPU_EVENTS
+struct kvm_vcpu_events events;
+int r;
+
+r = kvm_vcpu_ioctl(env, KVM_GET_VCPU_EVENTS, &events);
+if (r == 0) {
+env->exception_index =
+events.exception.injected ? events.exception.nr : -1;
+env->has_error_code = events.exception.has_error_code;
+env->error_code = events.exception.error_code;
+
+env->interrupt_injected =
+events.interrupt.injected ? events.interrupt.nr : -1;
+env->soft_interrupt = events.interrupt.soft;
+
+env->nmi_injected = events.nmi.injected;
+env->nmi_pending = events.nmi.pending;
+if (events.nmi.masked) {
+env->hflags2 |= HF2_NMI_MASK;
+} else {
+env->hflags2 &= ~HF2_NMI_MASK;
+}
+
+env->sipi_vector = events.sipi_vector;
+}
+#endif
+}
+
+static void kvm_set_vcpu_events(CPUState *env)
+{
+#ifdef KVM_CAP_VCPU_EVENTS
+struct kvm_vcpu_events events;
+
+events.exception.injected = (env->exception_index >= 0);
+events.exception.nr = env->exception_index;
+events.exception.has_error_code = env->has_error_code;
+events.exception.error_code = env->error_code;
+
+events.interrupt.injected = (env->interrupt_injected >= 0);
+events.interrupt.nr = env->interrupt_injected;
+events.interrupt.soft = env->soft_interrupt;
+
+events.nmi.injected = env->nmi_injected;
+events.nmi.pending = env->nmi_pending;
+events.nmi.masked = !!(env->hflags2 & HF2_NMI_MASK);
+
+events.sipi_vector = env->sipi_vector;
+
+kvm_vcpu_ioctl(env, KVM_SET_VCPU_EVENTS, &events);
+#endif
+}
+
void kvm_arch_load_regs(CPUState *env)
{
struct kvm_regs regs;
@@ -1019,6 +1073,8 @@ void kvm_arch_load_regs(CPUState *env)
rc = kvm_set_msrs(env, msrs, n);
if (rc == -1)
perror("kvm_set_msrs FAILED");
+
+kvm_set_vcpu_events(env);
}
void kvm_load_tsc(CPUState *env)
@@ -1215,6 +1271,8 @@ void kvm_arch_save_regs(CPUState *env)
return;
}
}
+
+kvm_get_vcpu_events(env);
}
static void do_cpuid_ent(struct kvm_cpuid_entry2 *e, uint32_t function,
@@ -1383,7 +1441,10 @@ int kvm_arch_init_vcpu(CPUState *cenv)
kvm_tpr_vcpu_start(cenv);
#endif
+cenv->exception_index = -1;
cenv->interrupt_injected = -1;
+cenv->nmi_injected = 0;
+cenv->nmi_pending = 0;
return 0;
}
@@ -1453,7 +1514,10 @@ void kvm_arch_push_nmi(void *opaque)
void kvm_arch_cpu_reset(CPUState *env)
{
+env->exception_index = -1;
env->interrupt_injected = -1;
+env->nmi_injected = 0;
+env->nmi_pending = 0;
kvm_arch_load_regs(env);
if (!cpu_is_bsp(env)) {
if (kvm_irqchip_in_kernel()) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a638e70..31412a8 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -711,6 +711,11 @@ typedef struct CPUX86State {
/* For KVM */
uint32_t mp_state;
int32_t interrupt_injected;
+uint8_t soft_interrupt;
+uint8_t nmi_injected;
+uint8_t nmi_pending;
+uint8_t has_error_code;
+uint32_t sipi_vector;
/* in order to simplify APIC support, we leave this pointer to the
user */
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..1eda7c5 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -452,6 +452,11 @@ static const VMStateDescription vmstate_cpu = {
VMSTATE_INT32_V(interrupt_injected, CPUState, 9),
VMSTATE_UINT32_V(mp_state, CPUState, 9),
VMSTATE_UINT64_V(tsc, CPUState, 9),
+VMSTATE_UINT8_V(soft_interrupt, CPUState, 11),
+VMSTATE_UINT8_V(nmi_injected, CPUState, 11),
+VMSTATE_UINT8_V(nmi_pending, CPUState, 11),
+VMSTATE_UINT8_V(has_error_code, CPUState, 11),
+VMSTATE_UINT32_V(sipi_vector, CPUState, 11),
/* MCE */
VMSTATE_UINT64_V(mcg_cap, CPUState, 10),
VMSTATE_UINT64_V(mcg_status, CPUState, 10),
signature.asc
Description: OpenPGP digital signature
