kvm: Support timing accesses for KVM cpu

Information
Submitter:	Michael LeBeane
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

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.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I think you need to submit the next deferred packet here.	Andreas Sandberg	Aug. 17, 2016, 10:23 a.m.	Resolved
Refactor this into a helper function that takes a packet and returns a delay. You're using the exact same code ...	Andreas Sandberg	Aug. 17, 2016, 10:23 a.m.	Resolved
Since you potentially submit multiple timing packets, you can't reuse the req without ending up in a double free situation.	Andreas Sandberg	Aug. 17, 2016, 10:23 a.m.	Resolved
a stack?	Andreas Hansson	Aug. 18, 2016, 10:50 a.m.	Resolved
unsigned int?	Andreas Hansson	Aug. 18, 2016, 10:50 a.m.	Resolved
if we already have queued packets, should this not be added FIFO? also, if we are waiting for a retry, ...	Andreas Hansson	Aug. 18, 2016, 10:50 a.m.	Resolved
This is odd. We did not receive a retry, we received a response. We should not call sendTiming again until ...	Andreas Hansson	Aug. 18, 2016, 10:50 a.m.	Resolved
Conceptually this is not very nice, as it assumes infinite throughput. I see how we save an even this way, ...	Andreas Hansson	Aug. 18, 2016, 10:50 a.m.	Resolved
I would prefer if we could keep the bypass caches check. Classic memory will definitely break if it isn't executing ...	Andreas Sandberg	Aug. 18, 2016, 3:47 p.m.	Resolved
This is a bit of a nit, but would it be possible to move the state transitions to this switch ...	Andreas Sandberg	Aug. 18, 2016, 10:51 a.m.	Resolved

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 (Diff revision 1)

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

Show all issues

src/cpu/kvm/base.cc (Diff revision 1)

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.

Show all issues

src/cpu/kvm/x86_cpu.cc (Diff revision 1)

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

Show all issues

Change Summary:

Addresses Andreas S.'s review comments.

Diff:

Revision 2 (+153 -52)

Show changes

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

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

src/cpu/kvm/base.hh (Diff revision 2)

a stack?

Show all issues

src/cpu/kvm/base.hh (Diff revision 2)

unsigned int?

Show all issues

src/cpu/kvm/base.cc (Diff revision 2)

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

Show all issues

src/cpu/kvm/base.cc (Diff revision 2)

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.

Show all issues

src/cpu/kvm/base.cc (Diff revision 2)

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.

Show all issues

Michael LeBeane Aug. 17, 2016, 3:58 p.m. (Aug. 17, 2016, 3:58 p.m.)

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.

Andreas Hansson Aug. 17, 2016, 5:25 p.m. (Aug. 17, 2016, 5:25 p.m.)

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 Sandberg Aug. 18, 2016, 1:48 a.m. (Aug. 18, 2016, 1:48 a.m.)

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.

Andreas Sandberg Aug. 18, 2016, 1:52 a.m. (Aug. 18, 2016, 1:52 a.m.)

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 Hansson Aug. 18, 2016, 4:38 p.m. (Aug. 18, 2016, 4:38 p.m.)

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.

src/cpu/kvm/base.cc (Diff revision 2)

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.

Show all issues

Michael LeBeane Aug. 18, 2016, 8:34 a.m. (Aug. 18, 2016, 8:34 a.m.)

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 LeBeane Aug. 18, 2016, 9:19 a.m. (Aug. 18, 2016, 9:19 a.m.)

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.

Andreas Sandberg Aug. 18, 2016, 9:24 a.m. (Aug. 18, 2016, 9:24 a.m.)

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.

src/cpu/kvm/base.cc (Diff revision 2)

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.

Show all issues

Diff:

Revision 3 (+159 -40)

Show changes

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

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?

Andreas Sandberg Aug. 22, 2016, 7:15 a.m. (Aug. 22, 2016, 7:15 a.m.)

This is an unfortunate side-effect of using KVM. A display processor would cause the same type of issues (you'd get events at least once per refresh, but possibly once per N pixels). There are basically two high-level solutions:

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.
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.

In this particular case, I think 2 sounds like a reasonable solution since you presumably want good timing fidelity for the GPU. Synchronisation is going to be "interesting", but the KVM CPU should be able to cope with being in its own thread. Communication should only really happen when handling MMIOs and interrupts, which already support synchronisation. I have something along these lines in my KVM script to map CPUs to threads:

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.
for idx, cpu in enumerate(system.cpu):
    # Child objects usually inherit the parent's event
    # queue. Override that and use queue 0 instead.
    for obj in cpu.descendants():
        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.

Michael LeBeane Aug. 22, 2016, 8:41 a.m. (Aug. 22, 2016, 8:41 a.m.)

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.

Ship It!

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

Looks great. Thanks for addressing all the comments.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Diff:

Diff:

Status: Closed (submitted)