qemu-kvm: Introduce writeback scope for cpu_synchronize_state

This patch aims at addressing the mp_state writeback issue in a cleaner fashion. By introducing additional information about the scope of the scheduled vcpu state writeback, we can simply skin mp_state (and maybe other specific states in the future) when updating the in-kernel state.

The writeback scope is defined when calling cpu_synchronize_state. It accumulated, ie. once a full writeback was requested, this will stick until it was performed.

This unbreaks --disable-kvm builds of qemu-kvm again.

Signed-off-by Jan Kiszka <[email protected]>

---

A corresponding upstream patch is ready to be posted as well, just waiting for comments on the general direction from KVM POV.

This patch aims at addressing the mp_state writeback issue in a cleaner fashion.

By introducing additional information about the scope of the scheduled vcpu state writeback, we can simply skin mp_state (and maybe other specific states in the future) when updating the in-kernel state.

The writeback scope is defined when calling cpu_synchronize_state. It accumulated, ie. once a full writeback was requested, this will stick until it was performed.

Quoted 2 lines · Jan Kiszka

This patch aims at addressing the mp_state writeback issue in a cleaner fashion.

What's the issue? the fact that mp_state is updated whenever state is synchronized, while it could be simultaneously updated from other vcpus (which latter updates are then lost)?

Quoted 9 lines

By introducing additional information about the scope of the scheduled vcpu state writeback, we can simply skin mp_state (and maybe other specific states in the future) when updating the in-kernel state.

The writeback scope is defined when calling cpu_synchronize_state. It accumulated, ie. once a full writeback was requested, this will stick until it was performed.

Maybe it's just simpler to divorce mp_state from the rest of the state.

Quoted 9 lines · Avi Kivity

Quoted 3 lines · Jan Kiszka

This patch aims at addressing the mp_state writeback issue in a cleaner fashion.

What's the issue? the fact that mp_state is updated whenever state is synchronized, while it could be simultaneously updated from other vcpus (which latter updates are then lost)?

Right, the issue b8a7857071 addressed. But that approach spreads more kvm_* fragments in unrelated qemu code, e.g. the monitor, and fails to update other parts (gdbstub). And it doesn't care about what happens if kvm is off at build or runtime. Such things are better addressed in upstream by encapsulating kvm calls in synchronization points.

Quoted 11 lines · Jan Kiszka

Quoted 9 lines · Avi Kivity

Quoted 3 lines · Jan Kiszka

This patch aims at addressing the mp_state writeback issue in a cleaner fashion.

What's the issue? the fact that mp_state is updated whenever state is synchronized, while it could be simultaneously updated from other vcpus (which latter updates are then lost)?

Quoted 6 lines

Right, the issue b8a7857071 addressed. But that approach spreads more kvm_* fragments in unrelated qemu code, e.g. the monitor, and fails to update other parts (gdbstub). And it doesn't care about what happens if kvm is off at build or runtime. Such things are better addressed in upstream by encapsulating kvm calls in synchronization points.

Note we have the same issue with nmi and the sipi vector - any vcpu

state that is updated outside the vcpu thread. These are particularly bad since we can't exclude them from updates without excluding other state as well.

The whole issue is tricky. I'm inclined to pretend we never meant any vcpu state (outside lapic) to be asynchronous and declare the whole thing a bug. We could fix it by modeling external changes to state (INIT, SIPI, NMI) as messages queued to the vcpu, to be processed in the vcpu thread. The queue would be drained before running the vcpu or before reading state from userspace, so the message queue contents can never be observed and never lost.

Of course, we can't really implement this as a queue (SIGSTOP vcpu thread -> overflow), but a word is sufficient. INIT writes the word, everything else uses compare-and-swap or set_bit to raise events (e.g. SIPI = do { oldq = vcpu->queue; newq = (oldq & ~SIPI_MASK) | sipi_vector | RUNNING; } while (!cas(&vcpu->queue, oldq, newq)))

Quoted 6 lines

state that is updated outside the vcpu thread. These are particularly bad since we can't exclude them from updates without excluding other state as well.

We easily can, using the very same mechanism: No need to overwrite any of the kvm_vcpu_events during runtime, only on reset/vmload).

Quoted 8 lines

The whole issue is tricky. I'm inclined to pretend we never meant any vcpu state (outside lapic) to be asynchronous and declare the whole thing a bug. We could fix it by modeling external changes to state (INIT, SIPI, NMI) as messages queued to the vcpu, to be processed in the vcpu thread. The queue would be drained before running the vcpu or before reading state from userspace, so the message queue contents can never be observed and never lost.

Quoted 7 lines

Of course, we can't really implement this as a queue (SIGSTOP vcpu thread -> overflow), but a word is sufficient. INIT writes the word, everything else uses compare-and-swap or set_bit to raise events (e.g. SIPI = do { oldq = vcpu->queue; newq = (oldq& ~SIPI_MASK) | sipi_vector | RUNNING; } while (!cas(&vcpu->queue, oldq, newq)))

I do not yet see why we need this complication, why the proposed model isn't enough.

Quoted 6 lines · Jan Kiszka

Quoted 6 lines

state that is updated outside the vcpu thread. These are particularly bad since we can't exclude them from updates without excluding other state as well.

Quoted 3 lines

We easily can, using the very same mechanism: No need to overwrite any of the kvm_vcpu_events during runtime, only on reset/vmload).

That's because qemu has no need for this. But kvm is more than just serving qemu, we try to be more general. That said, I can't really see anyone wanting to arbitrarily inject an exception.

Quoted 16 lines

Quoted 16 lines

The whole issue is tricky. I'm inclined to pretend we never meant any vcpu state (outside lapic) to be asynchronous and declare the whole thing a bug. We could fix it by modeling external changes to state (INIT, SIPI, NMI) as messages queued to the vcpu, to be processed in the vcpu thread. The queue would be drained before running the vcpu or before reading state from userspace, so the message queue contents can never be observed and never lost.

Of course, we can't really implement this as a queue (SIGSTOP vcpu thread -> overflow), but a word is sufficient. INIT writes the word, everything else uses compare-and-swap or set_bit to raise events (e.g. SIPI = do { oldq = vcpu->queue; newq = (oldq& ~SIPI_MASK) | sipi_vector | RUNNING; } while (!cas(&vcpu->queue, oldq, newq)))

Quoted 3 lines

I do not yet see why we need this complication, why the proposed model isn't enough.

The current interface is subtly dangerous, you can't run set(get()) as you would expect.

(well you can't with the lapic or the tsc msr either...)

Quoted 5 lines

That's because qemu has no need for this. But kvm is more than just serving qemu, we try to be more general. That said, I can't really see anyone wanting to arbitrarily inject an exception.

Well, the current API comes with millions of ways to shoot yourself into the foot. I don't think we can avoid them all.

Quoted 6 lines

The current interface is subtly dangerous, you can't run set(get()) as you would expect.

(well you can't with the lapic or the tsc msr either...)

We may start documenting such dependency in kvm/api.txt. On the other hand, if you have a get/set interface vs. an inject channel, I think it's obvious that one can overwrite the other.

Quoted 8 lines · Jan Kiszka

Quoted 5 lines

That's because qemu has no need for this. But kvm is more than just serving qemu, we try to be more general. That said, I can't really see anyone wanting to arbitrarily inject an exception.

Well, the current API comes with millions of ways to shoot yourself into the foot. I don't think we can avoid them all.

It would be nice to make the API saner. Do you know of more holes?

Quoted 10 lines

Quoted 6 lines

The current interface is subtly dangerous, you can't run set(get()) as you would expect.

(well you can't with the lapic or the tsc msr either...)

We may start documenting such dependency in kvm/api.txt. On the other hand, if you have a get/set interface vs. an inject channel, I think it's obvious that one can overwrite the other.

Problem is, the inject channels are implied (APIC messages in smp guests). Documentation is good, but if we can avoid it that's better.

Note the only way to rmw vcpu events during smp is pausing the guest, because of this race.

Quoted 7 lines

Problem is, the inject channels are implied (APIC messages in smp guests). Documentation is good, but if we can avoid it that's better.

Note the only way to rmw vcpu events during smp is pausing the guest, because of this race.

That's what qemu does on reset and load.

The alternative would be a complex get&lock/put&unlock + a queue for async events during the lock + an option to ignore what was queued when doing a true reset. Back to square #1: we would still need the proposed high-level interface to communicate the difference between replay and drop queue.

Quoted 4 lines · Jan Kiszka

Quoted 4 lines

Problem is, the inject channels are implied (APIC messages in smp guests). Documentation is good, but if we can avoid it that's better.

Quoted 5 lines

Quoted 3 lines

Note the only way to rmw vcpu events during smp is pausing the guest, because of this race.

That's what qemu does on reset and load.

These aren't rmw.

Quoted 7 lines

The alternative would be a complex get&lock/put&unlock + a queue for async events during the lock + an option to ignore what was queued when doing a true reset. Back to square #1: we would still need the proposed high-level interface to communicate the difference between replay and drop queue.

There's no need for get+lock / put+unlock; a normal get/put with the

addition that get flushes the queue suffices. To make sure queued events don't affect set you need to stop the entire VM before setting state, but you need to do that anyway for non-rmw writes.

Quoted 8 lines

Quoted 8 lines

The alternative would be a complex get&lock/put&unlock + a queue for async events during the lock + an option to ignore what was queued when doing a true reset. Back to square #1: we would still need the proposed high-level interface to communicate the difference between replay and drop queue.

Quoted 2 lines

There's no need for get+lock / put+unlock; a normal get/put with the

You need to track when to queue and when to apply directly. Call it lock or call it something else.

Quoted 5 lines

addition that get flushes the queue suffices. To make sure queued events don't affect set you need to stop the entire VM before setting state, but you need to do that anyway for non-rmw writes.

Well, sounds good, but it will be a non-trivial change in the interface semantics. At bare minimum, we would need a new mp_state interface. If we would count mp_state to our new event structure (hmm...), then we could confine the semantical changes to that new IOCTL pair. But how to deal with existing KVM kernels with their mp_state interface? It's a bit like the vcpu state thing: we are already down a specific road, and it's hard to turn around.

Quoted 7 lines · Jan Kiszka

Quoted 7 lines

Quoted 7 lines

The alternative would be a complex get&lock/put&unlock + a queue for async events during the lock + an option to ignore what was queued when doing a true reset. Back to square #1: we would still need the proposed high-level interface to communicate the difference between replay and drop queue.

Quoted 5 lines

Quoted 2 lines

There's no need for get+lock / put+unlock; a normal get/put with the

You need to track when to queue and when to apply directly. Call it lock or call it something else.

You always queue. When starting vcpu_run() or reading state to userspace you flush the queue.

The hardware equivalent is posting APIC messages, and the core executing them.

Quoted 5 lines

Quoted 5 lines

addition that get flushes the queue suffices. To make sure queued events don't affect set you need to stop the entire VM before setting state, but you need to do that anyway for non-rmw writes.

Quoted 8 lines

Well, sounds good, but it will be a non-trivial change in the interface semantics. At bare minimum, we would need a new mp_state interface. If we would count mp_state to our new event structure (hmm...), then we could confine the semantical changes to that new IOCTL pair. But how to deal with existing KVM kernels with their mp_state interface? It's a bit like the vcpu state thing: we are already down a specific road, and it's hard to turn around.

I think we're not on the same page here. As I see it, no interface change is needed at all.

It's true that existing kernels don't handle this properly, which is why I said I'm willing to treat it as a bug (and thus the -stable treatment etc.). I admit it's a stretch since this is not going to be trivial (though I think less complex that you believe).

Putting mp_state into the events structure is reasonable regardless of this issue (and doable since we haven't pushed it to 2.6.33 yet). But I want to understand why you think it's needed.

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Quoted 7 lines

Quoted 7 lines

The alternative would be a complex get&lock/put&unlock + a queue for async events during the lock + an option to ignore what was queued when doing a true reset. Back to square #1: we would still need the proposed high-level interface to communicate the difference between replay and drop queue.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

There's no need for get+lock / put+unlock; a normal get/put with the

You need to track when to queue and when to apply directly. Call it lock or call it something else.

You always queue. When starting vcpu_run() or reading state to userspace you flush the queue.

Now I finally got your idea.

Quoted 24 lines

The hardware equivalent is posting APIC messages, and the core executing them.

Quoted 5 lines

Quoted 5 lines

addition that get flushes the queue suffices. To make sure queued events don't affect set you need to stop the entire VM before setting state, but you need to do that anyway for non-rmw writes.

Quoted 8 lines

Well, sounds good, but it will be a non-trivial change in the interface semantics. At bare minimum, we would need a new mp_state interface. If we would count mp_state to our new event structure (hmm...), then we could confine the semantical changes to that new IOCTL pair. But how to deal with existing KVM kernels with their mp_state interface? It's a bit like the vcpu state thing: we are already down a specific road, and it's hard to turn around.

I think we're not on the same page here. As I see it, no interface change is needed at all.

It's true that existing kernels don't handle this properly, which is why I said I'm willing to treat it as a bug (and thus the -stable treatment etc.). I admit it's a stretch since this is not going to be trivial (though I think less complex that you believe).

Quoted 4 lines

Putting mp_state into the events structure is reasonable regardless of this issue (and doable since we haven't pushed it to 2.6.33 yet). But I want to understand why you think it's needed.

That wouldn't be required anymore with the "always queue" policy.

But what would you queue at all? Only mp_state, nmi_pending and sipi_vector? Or also all the relevant PIC and LAPIC states that might be changed asynchronously?

Quoted 3 lines

That wouldn't be required anymore with the "always queue" policy.

Hmm, unless we need mp_state manipulation analogously to KVM_NMI vs.
KVM_SET_VCPU_STATE: The former will queue, the latter write, but may be overwritten by anything queued. If you just queue KVM_SET_MP_STATE, you still have a conflict between concurrent manipulations from user space, something we want to resolve automagically.

Quoted 9 lines

I think we're not on the same page here. As I see it, no interface change is needed at all.

It's true that existing kernels don't handle this properly, which is why I said I'm willing to treat it as a bug (and thus the -stable treatment etc.). I admit it's a stretch since this is not going to be trivial (though I think less complex that you believe).

Quoted 5 lines

Putting mp_state into the events structure is reasonable regardless of this issue (and doable since we haven't pushed it to 2.6.33 yet). But I want to understand why you think it's needed.

That wouldn't be required anymore with the "always queue" policy.

But what would you queue at all? Only mp_state, nmi_pending and sipi_vector?

Or also all the relevant PIC and LAPIC states that might be changed asynchronously?

Quoted 9 lines · Jan Kiszka

Quoted 9 lines

I think we're not on the same page here. As I see it, no interface change is needed at all.

It's true that existing kernels don't handle this properly, which is why I said I'm willing to treat it as a bug (and thus the -stable treatment etc.). I admit it's a stretch since this is not going to be trivial (though I think less complex that you believe).

Quoted 7 lines

Quoted 5 lines

Putting mp_state into the events structure is reasonable regardless of this issue (and doable since we haven't pushed it to 2.6.33 yet). But I want to understand why you think it's needed.

That wouldn't be required anymore with the "always queue" policy.

It makes sense from a grouping point of view... maybe.

Quoted 2 lines

But what would you queue at all? Only mp_state, nmi_pending and sipi_vector?

INIT, too.

Quoted 3 lines

INIT, too.

INIT should be handled by queuing up the next mp_state.

BTW, as we do not inject mp_state changes from user space during runtime, the issue I saw with the current interface is not existing. We just need to add that queuing feature to asynchronous in-kernel mp_state changes, and we should be fine.

Let's assume we will have such changes in future kernels: should qemu-kvm and qemu upstream also bother about older kernels and establish workarounds? Because then we need to find a cleaner approach than the current one, and my proposed patch comes into the game again.