qemu-kvm vs. qemu: Terminate cpu loop on reset?

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 5 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

Quoted 2 lines

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

If upstream does not need it, we have to understand why and
maybe adopt its pattern (the ultimate goal is unification anyway).

Quoted 34 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 5 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

Quoted 6 lines

Quoted 2 lines

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

It doesn't matter where the code resides as long as it is called on reset.

Quoted 4 lines

If upstream does not need it, we have to understand why and
maybe adopt its pattern (the ultimate goal is unification anyway).

I don't consider kvm upstream as working product. The goal should be moving to qemu-kvm code in upstream preserving all the knowledge we acquired while making it production grade code.

Quoted 38 lines · Gleb Natapov

Quoted 37 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 7 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

Quoted 5 lines

Quoted 3 lines

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

It doesn't matter where the code resides as long as it is called on reset.

It technically matters for the build process (vl.c is built once these days, cpus.c is built per target).

In any case, we apparently need to fix upstream, I'm playing with some approach.

Quoted 4 lines

Quoted 4 lines

If upstream does not need it, we have to understand why and
maybe adopt its pattern (the ultimate goal is unification anyway).

Quoted 3 lines

I don't consider kvm upstream as working product. The goal should be moving to qemu-kvm code in upstream preserving all the knowledge we acquired while making it production grade code.

We had this discussion before. My goal remains to filter the remaining upstream fixes out of the noise, adjust both versions so that they are apparently identical, and then switch to a single version.

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

Quoted 42 lines · Jan Kiszka

Quoted 41 lines · Gleb Natapov

Quoted 40 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 7 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

Quoted 6 lines

Quoted 2 lines

It doesn't matter where the code resides as long as it is called on reset.

It technically matters for the build process (vl.c is built once these days, cpus.c is built per target).

Yes, I understand the build requirement. Runtime behaviour should not change.

Quoted 12 lines

In any case, we apparently need to fix upstream, I'm playing with some approach.

Quoted 7 lines

Quoted 4 lines

If upstream does not need it, we have to understand why and
maybe adopt its pattern (the ultimate goal is unification anyway).

I don't consider kvm upstream as working product. The goal should be moving to qemu-kvm code in upstream preserving all the knowledge we acquired while making it production grade code.

Quoted 4 lines

We had this discussion before. My goal remains to filter the remaining upstream fixes out of the noise, adjust both versions so that they are apparently identical, and then switch to a single version.

I thought there was an agreement to accept qemu-kvm implementation as is into upstream (without some parts like device assignment). If you look at qemu-kvm you'll see that upstream implementation is marked as OBSOLETE_KVM_IMPL.

Quoted 6 lines

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

You are optimistic. My prediction is that it will take at least one major RHEL release until such merged code base will become production-grade. That is when most bugs that were introduced by eliminating subtle differences between working and non-working version will be found :)

BTW Do you have a plan how to move upstream to thread per vcpu?

Quoted 49 lines · Gleb Natapov

Quoted 48 lines · Jan Kiszka

Quoted 35 lines · Gleb Natapov

Quoted 34 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 5 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

It doesn't matter where the code resides as long as it is called on reset.

It technically matters for the build process (vl.c is built once these days, cpus.c is built per target).

Quoted 2 lines

Yes, I understand the build requirement. Runtime behaviour should not change.

Yep, for sure.

BTW, the self-IPI on pending exit request is there for a reason I but. In order to complete half-done string-io or something like that? Would be the next patch for upstream then.

Quoted 49 lines · Gleb Natapov

Quoted 48 lines · Jan Kiszka

Quoted 35 lines · Gleb Natapov

Quoted 34 lines · Jan Kiszka

Quoted 16 lines · Gleb Natapov

Quoted 15 lines · Jan Kiszka

Hi,

does anyone immediately know if this hunk from vl.c

(quoted diff) +4 -0

@@ -1278,6 +1197,10 @@ void qemu_system_reset_request(void)
     } else {
         reset_requested = 1;
     }
+    if (cpu_single_env) {
+        cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
+    }
     qemu_notify_event();
 }
 

Quoted 7 lines

Quoted 4 lines

is (semantically) relevant for upstream as well? IIUC, it ensures that the kvm cpu loop is not continued if an IO access called into qemu_system_reset_request.

I don't know TCG enough to tell. If TCG can continue vcpu execution after io without checking reset_requested then it is relevant for upstream too.

I was first of all thinking about kvm upstream, but their handling differ much less upstream than in current qemu-kvm. Anyway, need to dig into the details.

Quoted 5 lines

Quoted 5 lines

If yes, then it would be a good time to push a patch: these bits will fall to dust on next merge from upstream (vl.c no longer has access to the cpu state).

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

On a next merge cpu state will have to be exposed to vl.c then. This code cannot be dropped in qemu-kvm.

I think a cleaner approach, even if it's only temporarily required, is to move that code to cpus.c. That's likely also the way when we need it upstream.

It doesn't matter where the code resides as long as it is called on reset.

It technically matters for the build process (vl.c is built once these days, cpus.c is built per target).

Quoted 2 lines

Yes, I understand the build requirement. Runtime behaviour should not change.

Yep, for sure.

BTW, the self-IPI on pending exit request is there for a reason I but. In order to complete half-done string-io or something like that? Would be the next patch for upstream then.

Quoted 4 lines

Quoted 4 lines

In any case, we apparently need to fix upstream, I'm playing with some approach.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

If upstream does not need it, we have to understand why and
maybe adopt its pattern (the ultimate goal is unification anyway).

I don't consider kvm upstream as working product. The goal should be moving to qemu-kvm code in upstream preserving all the knowledge we acquired while making it production grade code.

Quoted 8 lines

Quoted 4 lines

We had this discussion before. My goal remains to filter the remaining upstream fixes out of the noise, adjust both versions so that they are apparently identical, and then switch to a single version.

I thought there was an agreement to accept qemu-kvm implementation as is into upstream (without some parts like device assignment). If you look at qemu-kvm you'll see that upstream implementation is marked as OBSOLETE_KVM_IMPL.

You can't merge both trees without introducing regressions, either in the kvm part or some other section that qemu-kvm did not stress. IMO, there is no way around understanding all the nice little "fixes" that piled up over the years and translate them into proper, documented patches.

Quoted 7 lines

Quoted 7 lines

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

Quoted 4 lines

You are optimistic. My prediction is that it will take at least one major RHEL release until such merged code base will become production-grade. That is when most bugs that were introduced by eliminating subtle differences between working and non-working version will be found :)

The more upstream code qemu-kvm stresses, the faster this convergence will become. And there is really not that much left. E.g, I've a qemu-kvm-x86.c here that is <400 LOC.

Quoted 2 lines

BTW Do you have a plan how to move upstream to thread per vcpu?

Upstream has this already, but it's - once again - a different implementation. Understanding those differences is one of the next steps.

In fact, as posted recently, unifying the execution model implementations is the only big problem I see. In-kernel irqchips and device assignment are things that can live in qemu-kvm without much conflicts until they are finally mergable.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

You are optimistic. My prediction is that it will take at least one major RHEL release until such merged code base will become production-grade. That is when most bugs that were introduced by eliminating subtle differences between working and non-working version will be found :)

Quoted 4 lines

The more upstream code qemu-kvm stresses, the faster this convergence will become. And there is really not that much left. E.g, I've a qemu-kvm-x86.c here that is <400 LOC.

That's what I don't get. Why working qemu-kvm should stress non working upstream code? Just remove upstream code and replace it with qemu-kvm version.

Quoted 7 lines

Quoted 3 lines

BTW Do you have a plan how to move upstream to thread per vcpu?

Upstream has this already, but it's - once again - a different implementation. Understanding those differences is one of the next steps.

I see only two threads on upstream no matter how much vcpus I configure.

Quoted 6 lines

In fact, as posted recently, unifying the execution model implementations is the only big problem I see. In-kernel irqchips and device assignment are things that can live in qemu-kvm without much conflicts until they are finally mergable.

Upstream kvm is kinda useless without in-kernel irqchips.

Quoted 10 lines · Gleb Natapov

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

You are optimistic. My prediction is that it will take at least one major RHEL release until such merged code base will become production-grade. That is when most bugs that were introduced by eliminating subtle differences between working and non-working version will be found :)

Quoted 7 lines

Quoted 4 lines

The more upstream code qemu-kvm stresses, the faster this convergence will become. And there is really not that much left. E.g, I've a qemu-kvm-x86.c here that is <400 LOC.

That's what I don't get. Why working qemu-kvm should stress non working upstream code? Just remove upstream code and replace it with qemu-kvm version.

We are 3/4 (if not more) done with refactoring qemu-kvm into a clean state, removing lots of cruft from libkvm days and early kvm modules. We achieved this by creating a "fork of the fork": upstream kvm. We may argue a lot about pros and cons of this approach, but it is a fact now. And a lot of effort would be wasted as well by throwing this away.

Moreover, taking off the x86 glasses: ppc and s390 rely on upstream kvm. So it is impossible to drop those bits without breaking all non-x86 kvm archs.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

BTW Do you have a plan how to move upstream to thread per vcpu?

Quoted 4 lines

Quoted 3 lines

Upstream has this already, but it's - once again - a different implementation. Understanding those differences is one of the next steps.

I see only two threads on upstream no matter how much vcpus I configure.

/me sees a lot of them. Did you enable io-thread support? Otherwise kvm is run just like tcg in single-thread mode.

Quoted 6 lines

Quoted 6 lines

In fact, as posted recently, unifying the execution model implementations is the only big problem I see. In-kernel irqchips and device assignment are things that can live in qemu-kvm without much conflicts until they are finally mergable.

Quoted 1 line

Upstream kvm is kinda useless without in-kernel irqchips.

Not if its code serves the rest of qemu-kvm without further patches (and merge conflicts). And we only need to sort out the execution loop and threading stuff to get there.

Quoted 19 lines · Jan Kiszka

Quoted 10 lines · Gleb Natapov

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

We are on a good track now. I predict that we will be left with only one or two major additional features in qemu-kvm in a few months from now, no more duplications with subtle differences, and production-grade kvm upstream stability.

You are optimistic. My prediction is that it will take at least one major RHEL release until such merged code base will become production-grade. That is when most bugs that were introduced by eliminating subtle differences between working and non-working version will be found :)

Quoted 7 lines

Quoted 4 lines

The more upstream code qemu-kvm stresses, the faster this convergence will become. And there is really not that much left. E.g, I've a qemu-kvm-x86.c here that is <400 LOC.

That's what I don't get. Why working qemu-kvm should stress non working upstream code? Just remove upstream code and replace it with qemu-kvm version.

Quoted 6 lines

We are 3/4 (if not more) done with refactoring qemu-kvm into a clean state, removing lots of cruft from libkvm days and early kvm modules. We achieved this by creating a "fork of the fork": upstream kvm. We may argue a lot about pros and cons of this approach, but it is a fact now. And a lot of effort would be wasted as well by throwing this away.

Upstream kvm was not "fork of the fork". It was something much worse then that. It was (bad) reimplementation of kvm that was unfortunately merged upstream.

This slowed proper kvm inclusion into upstream for more
then 2 years now (and counting). Glauber and you did (and do) a great job trying to sort this mess and nobody propose to throw what was done so far. qemu-kvm and qemu upstream uses a lot of common code. We can either try hard to consolidate even mode code, or at some point just merge qemu-kvm and drop upstream functions that are not used by qemu-kvm (ifdefed as obsolete in qemu-kvm tree).

Quoted 5 lines

Moreover, taking off the x86 glasses: ppc and s390 rely on upstream kvm. So it is impossible to drop those bits without breaking all non-x86 kvm archs.

I do not propose to drop bits from upstream that are used in qemu-kvm obviously.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

BTW Do you have a plan how to move upstream to thread per vcpu?

Upstream has this already, but it's - once again - a different implementation. Understanding those differences is one of the next steps.

Quoted 5 lines

Quoted 1 line

I see only two threads on upstream no matter how much vcpus I configure.

/me sees a lot of them. Did you enable io-thread support? Otherwise kvm is run just like tcg in single-thread mode.

No, I didn't. Does io-thread work properly with TCG? IIRC there were problems with io thread + TCG.

Quoted 9 lines

Quoted 8 lines

Quoted 7 lines

In fact, as posted recently, unifying the execution model implementations is the only big problem I see. In-kernel irqchips and device assignment are things that can live in qemu-kvm without much conflicts until they are finally mergable.

Upstream kvm is kinda useless without in-kernel irqchips.

Quoted 4 lines

Not if its code serves the rest of qemu-kvm without further patches (and merge conflicts). And we only need to sort out the execution loop and threading stuff to get there.

This could have been achieved by not introducing upstream kvm in the first place :). Many if not most merging problems were result of rival kvm implementation in upstream. I thought the goal is to get rid of qemu-kvm fork at all by having fully functional kvm in upstream.