[gem5-dev] Review Request 3619: kvm: Support timing accesses for KVM cpu

[Michael LeBeane]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------

Review request for Default.


Repository: gem5


Description
-------

Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.


Diffs
-----

src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c

Diff: http://reviews.gem5.org/r/3619/diff/


Testing
-------


Thanks,

Michael LeBeane

[Andreas Sandberg]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8645
-----------------------------------------------------------


Thanks for implementing this! Overall, I think this is the right approach. However, I think you could refactor it a bit to make it cleaner. I think you could move most of the new packet handling logic to the KVMCpuPort, this would largely hide the fact that it support both timing and atomic from the rest of the CPU. I'd suggest something along these lines:

* Tick submitIO(pkt): Send the packet (and cleanup storage in atomic). Return the delay (0 for timing).
* Statis nextIOState(): Return RunningMMIOPending if there are pending timing accesses, RunningServiceCompletion otherwise.
* recvReqRetry(): Resubmit the deferred packet.
* recvTimingResp(pkt): Clean up pkt and try to submit the next deferred packet. When the last packet has received a response, call cpu->finishTiming() which updates the state of the CPU.

Using these helper functions, the MMIO handlers would be almost identical to before. Instead of calling port->submitAtomic(pkt) you'd call port->submitIO(pkt). Before exiting the IO handler, you set the state to port->nextIOState().


src/cpu/kvm/base.cc (line 192)
<http://reviews.gem5.org/r/3619/#comment7498>

I think you need to submit the next deferred packet here.



src/cpu/kvm/base.cc (lines 1097 - 1111)
<http://reviews.gem5.org/r/3619/#comment7496>

Refactor this into a helper function that takes a packet and returns a delay. You're using the exact same code when handling an IO instruction on x86.



src/cpu/kvm/x86_cpu.cc (lines 1347 - 1349)
<http://reviews.gem5.org/r/3619/#comment7497>

Since you potentially submit multiple timing packets, you can't reuse the req without ending up in a double free situation.


- Andreas Sandberg

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 16, 2016, 11:28 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------

(Updated Aug. 17, 2016, 8:07 p.m.)


Review request for Default.


Changes
-------

Addresses Andreas S.'s review comments.


Repository: gem5


Description
-------

Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.


Diffs (updated)
-----

src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c

Diff: http://reviews.gem5.org/r/3619/diff/


Testing
-------


Thanks,

Michael LeBeane

[Andreas Hansson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8652
-----------------------------------------------------------


Overall it looks great. Some concerns regarding packet ordering and flow-control handling.


src/cpu/kvm/base.hh (line 589)
<http://reviews.gem5.org/r/3619/#comment7500>

a stack?



src/cpu/kvm/base.hh (line 592)
<http://reviews.gem5.org/r/3619/#comment7499>

unsigned int?



src/cpu/kvm/base.cc (line 189)
<http://reviews.gem5.org/r/3619/#comment7501>

if we already have queued packets, should this not be added FIFO?

also, if we are waiting for a retry, we should not attempt to send a new packet



src/cpu/kvm/base.cc (line 210)
<http://reviews.gem5.org/r/3619/#comment7502>

This is odd. We did not receive a retry, we received a response.

We should not call sendTiming again until we get a retry. It only wastes cycles as we will make no progress.



src/cpu/kvm/base.cc (line 228)
<http://reviews.gem5.org/r/3619/#comment7503>

Conceptually this is not very nice, as it assumes infinite throughput.

I see how we save an even this way, but I am tempted to suggest the inclusion of an even rather.


- Andreas Hansson

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 228
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line228>
Conceptually this is not very nice, as it assumes infinite throughput.
I see how we save an even this way, but I am tempted to suggest the inclusion of an even rather.

I'm not sure it's worth modeling this in KVM, but if you feel strongly about it I can implement a fixed number of requests per cycle.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8652
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Hansson]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 228
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line228>
Conceptually this is not very nice, as it assumes infinite throughput.
I see how we save an even this way, but I am tempted to suggest the inclusion of an even rather.

I'm not sure it's worth modeling this in KVM, but if you feel strongly about it I can implement a fixed number of requests per cycle.

I am inclinced to agree. It's probably not worth changing. Perhaps add a comment to make it clear that this is unrealistic.

My worry is mostly that we are drifting further away from SystemC TLM2, and the fact that we don't have a proper 4-phase handshake where we are told when a request is accepted. If we have the "ack" for the request then it would be clear that we must not send a second request until the first one is accepted. That's a much bigger topic though, but just so you know the background.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8652
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Sandberg]

This post might be inappropriate. Click to display it.

[Andreas Sandberg]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 228
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line228>
Conceptually this is not very nice, as it assumes infinite throughput.
I see how we save an even this way, but I am tempted to suggest the inclusion of an even rather.

I'm not sure it's worth modeling this in KVM, but if you feel strongly about it I can implement a fixed number of requests per cycle.
I am inclinced to agree. It's probably not worth changing. Perhaps add a comment to make it clear that this is unrealistic.
My worry is mostly that we are drifting further away from SystemC TLM2, and the fact that we don't have a proper 4-phase handshake where we are told when a request is accepted. If we have the "ack" for the request then it would be clear that we must not send a second request until the first one is accepted. That's a much bigger topic though, but just so you know the background.
KVM actually requires this. The only case you'll get multiple requests in one tick is if the CPU executes an IO instruction with a rep prefix on x86. In this case, the model MUST accept all of the data before returning to KVM. The whole point of this routine is to buffer such requests until the interconnect accepts them. Once we get a return for KVM to handle IO, we won't hand back control until that batch of MMIOs have been handled.

Sorry, I'm being confused. Anyway, I agree with Michael. There is really no reason to model this. The KVM CPU already makes a lot of other assumptions that are several orders of magnitude worse when it comes to timing fidelity. Issueing a handful of memory accesses in one cycle seems like a small issue (especially considering it's extremely uncommon since it only happens for ioport operations on x86 with the rep prefix) in the grand scheme of things.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8652
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 9:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Hansson]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 228
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line228>
Conceptually this is not very nice, as it assumes infinite throughput.
I see how we save an even this way, but I am tempted to suggest the inclusion of an even rather.

I'm not sure it's worth modeling this in KVM, but if you feel strongly about it I can implement a fixed number of requests per cycle.
I am inclinced to agree. It's probably not worth changing. Perhaps add a comment to make it clear that this is unrealistic.
My worry is mostly that we are drifting further away from SystemC TLM2, and the fact that we don't have a proper 4-phase handshake where we are told when a request is accepted. If we have the "ack" for the request then it would be clear that we must not send a second request until the first one is accepted. That's a much bigger topic though, but just so you know the background.
KVM actually requires this. The only case you'll get multiple requests in one tick is if the CPU executes an IO instruction with a rep prefix on x86. In this case, the model MUST accept all of the data before returning to KVM. The whole point of this routine is to buffer such requests until the interconnect accepts them. Once we get a return for KVM to handle IO, we won't hand back control until that batch of MMIOs have been handled.
Sorry, I'm being confused. Anyway, I agree with Michael. There is really no reason to model this. The KVM CPU already makes a lot of other assumptions that are several orders of magnitude worse when it comes to timing fidelity. Issueing a handful of memory accesses in one cycle seems like a small issue (especially considering it's extremely uncommon since it only happens for ioport operations on x86 with the rep prefix) in the grand scheme of things.

As I said, I am less worried about the fidelity, and more about API compabitility. By allowing and using this construct, we are moving further away from TLM2 semantics.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8652
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Sandberg]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8656
-----------------------------------------------------------



src/cpu/kvm/base.cc
<http://reviews.gem5.org/r/3619/#comment7506>

I would prefer if we could keep the bypass caches check. Classic memory will definitely break if it isn't executing in cache bypass mode. I don't know if it is worth fixing, but I also don't like the fact that we can get really weird errors if this is removed.

I'm not sure how this would interact with your Ruby patches though. You might need to add a timing cache bypass mode.



src/cpu/kvm/base.cc (line 994)
<http://reviews.gem5.org/r/3619/#comment7507>

This is a bit of a nit, but would it be possible to move the state transitions to this switch statement instead of distributing them? I think that'll keep the state machine more maintainable in the future.

You'll probably need to call the handler before updating the state in that case.


- Andreas Sandberg

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 9:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 505
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line505>
I would prefer if we could keep the bypass caches check. Classic memory will definitely break if it isn't executing in cache bypass mode. I don't know if it is worth fixing, but I also don't like the fact that we can get really weird errors if this is removed.
I'm not sure how this would interact with your Ruby patches though. You might need to add a timing cache bypass mode.

Just so I understand better, what issues are you concerned about if we don't operate in bypass cache mode? It looks like caches won't be flushed when entering KVM without being in bypass cache mode, so that's a concern. Are you also concerned that other devices can write to the cache and KVM won't see it? KVM itself only issues uncacheable IO requests, which shouldn't matter whether bypass cache is enabled.

For Ruby, we definately timing cache bypass mode, if only to make sure caches are properly flushed when switching into KVM from Ruby-land. But that's probably a seperate patch.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8656
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 505
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line505>
I would prefer if we could keep the bypass caches check. Classic memory will definitely break if it isn't executing in cache bypass mode. I don't know if it is worth fixing, but I also don't like the fact that we can get really weird errors if this is removed.
I'm not sure how this would interact with your Ruby patches though. You might need to add a timing cache bypass mode.

Just so I understand better, what issues are you concerned about if we don't operate in bypass cache mode? It looks like caches won't be flushed when entering KVM without being in bypass cache mode, so that's a concern. Are you also concerned that other devices can write to the cache and KVM won't see it? KVM itself only issues uncacheable IO requests, which shouldn't matter whether bypass cache is enabled.
For Ruby, we definately timing cache bypass mode, if only to make sure caches are properly flushed when switching into KVM from Ruby-land. But that's probably a seperate patch.

Turns out I was wrong, we don't need to flush Ruby caches because of the backing store. But I will still need to introduce a timing_noncacheable mode if you would like to keep this assertion, else I can't actually use this in Ruby.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8656
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 8:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Sandberg]

Post by Michael LeBeane

src/cpu/kvm/base.cc, line 505
<http://reviews.gem5.org/r/3619/diff/2/?file=57574#file57574line505>
I would prefer if we could keep the bypass caches check. Classic memory will definitely break if it isn't executing in cache bypass mode. I don't know if it is worth fixing, but I also don't like the fact that we can get really weird errors if this is removed.
I'm not sure how this would interact with your Ruby patches though. You might need to add a timing cache bypass mode.

Just so I understand better, what issues are you concerned about if we don't operate in bypass cache mode? It looks like caches won't be flushed when entering KVM without being in bypass cache mode, so that's a concern. Are you also concerned that other devices can write to the cache and KVM won't see it? KVM itself only issues uncacheable IO requests, which shouldn't matter whether bypass cache is enabled.
For Ruby, we definately timing cache bypass mode, if only to make sure caches are properly flushed when switching into KVM from Ruby-land. But that's probably a seperate patch.
Turns out I was wrong, we don't need to flush Ruby caches because of the backing store. But I will still need to introduce a timing_noncacheable mode if you would like to keep this assertion, else I can't actually use this in Ruby.

Devices (mainly DMAs) would definitely cause issues in classic since they typically sit behind an IO cache (and could potentially issue caching accesses). IIRC, there are also things we do differently in the interconnects.


- Andreas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8656
-----------------------------------------------------------

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 17, 2016, 9:07 p.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------

(Updated Aug. 21, 2016, 3:19 a.m.)


Review request for Default.


Repository: gem5


Description
-------

Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.


Diffs (updated)
-----

src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c

Diff: http://reviews.gem5.org/r/3619/diff/


Testing
-------


Thanks,

Michael LeBeane

[Michael LeBeane]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8668
-----------------------------------------------------------


Well, having played around with this solution for KVM MMIO + Ruby a bit it does work (with the addition of a timing_noncacheable state as Andreas S. noted), but not very well. If you put the system in timing mode you get events like DRAM refresh that make it so you can't stay in KVM very long, which kinda defeats the purpose. Any non-hacky ideas how to get around this?

- Michael LeBeane

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 21, 2016, 3:19 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Sandberg]

This post might be inappropriate. Click to display it.

[Michael LeBeane]

Post by Andreas Sandberg

Post by Michael LeBeane
Well, having played around with this solution for KVM MMIO + Ruby a bit it does work (with the addition of a timing_noncacheable state as Andreas S. noted), but not very well. If you put the system in timing mode you get events like DRAM refresh that make it so you can't stay in KVM very long, which kinda defeats the purpose. Any non-hacky ideas how to get around this?

1. Don't issue frequent events when running in KVM mode. I have been considering this for the HDLCD. If running in *_noncacheable, we'd just reduce simulation fidelity to get events down to something manageable.
2. Run KVM in a separate thread similar to when simulating a multi-core system using KVM. This allows you to put devices in one event queue and each of the simulated KVM cores in separate event queues and control when the queues are synchronised.
```python
root.sim_quantum=m5.ticks.fromSeconds(options.quantum * 1E-3)
# Assign independent event queues (threads) to the KVM CPUs,
# event queue 0 is reserved for simulated devices.
# Child objects usually inherit the parent's event
# queue. Override that and use queue 0 instead.
obj.eventq_index = 0
cpu.eventq_index = idx + 1
```
You might want to test the timing changes on their own in a multi-core system in timing_noncacheable mode to make sure that they synchronise correctly. I have a sneaking suspicion that they don't at the moment.

Thanks for the good suggestions! Yeah, solution 2) seems like the right way to go. I don't exactly need to run KVM and the GPU at the same time (right now I switch CPU models), but I can see how 2) would be very useful for those out there just studying GPU performance.

Looks like the dram refresh event is turned off in atomic mode specifically for KVM. Making it do the same in timing with KVM running for solution 1) sounds a bit hacky if not impossible.

Having played with multiple event queues a bit for other projects, I know that it sometimes fails to work as intended the first time around :-). I'm a bit too busy with other stuff to get sucked up in this right now, but will hopefully get some free cycles to explore this later.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8668
-----------------------------------------------------------

Post by Andreas Sandberg
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 21, 2016, 3:19 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Sandberg]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8669
-----------------------------------------------------------

Ship it!


Ship It!

- Andreas Sandberg

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 21, 2016, 4:19 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Michael LeBeane]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8694
-----------------------------------------------------------


Andreas H., were your concerns resolved on this patch? Thanks!

- Michael LeBeane

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 21, 2016, 3:19 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane

[Andreas Hansson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3619/#review8695
-----------------------------------------------------------

Ship it!


Looks great. Thanks for addressing all the comments.

- Andreas Hansson

Post by Michael LeBeane
-----------------------------------------------------------
http://reviews.gem5.org/r/3619/
-----------------------------------------------------------
(Updated Aug. 21, 2016, 3:19 a.m.)
Review request for Default.
Repository: gem5
Description
-------
Changeset 11561:4595cc3848fc
---------------------------
kvm: Support timing accesses for KVM cpu
This patch enables timing accesses for KVM cpu. A new state,
RunningMMIOPending, is added to indicate that there are outstanding timing
requests generated by KVM in the system. KVM's tick() is disabled and the
simulation does not enter into KVM until all outstanding timing requests have
completed. The main motivation for this is to allow KVM CPU to perform MMIO
in Ruby, since Ruby does not support atomic accesses.
Diffs
-----
src/cpu/kvm/x86_cpu.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c
src/cpu/kvm/base.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c
Diff: http://reviews.gem5.org/r/3619/diff/
Testing
-------
Thanks,
Michael LeBeane