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

[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


On Aug. 21, 2016, 3:19 a.m., Michael LeBeane wrote:
> 
> ---
> 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
> -
> 
>   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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [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/#review8694
---


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

- Michael LeBeane


On Aug. 21, 2016, 3:19 a.m., Michael LeBeane wrote:
> 
> ---
> 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
> -
> 
>   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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Michael LeBeane]

> On Aug. 21, 2016, 3:29 a.m., Michael LeBeane wrote:
> > 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 wrote:
> 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: 
> 
>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.
> 
> 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:
> 
> ```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.
> 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.

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


On Aug. 21, 2016, 3:19 a.m., Michael LeBeane wrote:
> 
> ---
> 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
> -
> 
>   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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Sandberg]

> On Aug. 21, 2016, 4:29 a.m., Michael LeBeane wrote:
> > 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?

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: 

   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.

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:

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


- Andreas


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


On Aug. 21, 2016, 4:19 a.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[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


On Aug. 21, 2016, 4:19 a.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [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/#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


On Aug. 21, 2016, 3:19 a.m., Michael LeBeane wrote:
> 
> ---
> 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
> -
> 
>   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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [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/
---

(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

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Hansson]

> On Aug. 17, 2016, 10:05 p.m., Andreas Hansson wrote:
> > src/cpu/kvm/base.cc, line 228
> > 
> >
> > 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.
> 
> Michael LeBeane wrote:
> 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 wrote:
> 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 wrote:
> 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 wrote:
> 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
---


On Aug. 17, 2016, 8:07 p.m., Michael LeBeane wrote:
> 
> ---
> 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.
> 
> 
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Sandberg]

> On Aug. 18, 2016, 9:50 a.m., Andreas Sandberg wrote:
> > src/cpu/kvm/base.cc, line 505
> > 
> >
> > 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.
> 
> Michael LeBeane wrote:
> 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 wrote:
> 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
---


On Aug. 17, 2016, 9:07 p.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Michael LeBeane]

> On Aug. 18, 2016, 8:50 a.m., Andreas Sandberg wrote:
> > src/cpu/kvm/base.cc, line 505
> > 
> >
> > 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
---


On Aug. 17, 2016, 8:07 p.m., Michael LeBeane wrote:
> 
> ---
> 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.
> 
> 
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Sandberg]

> On Aug. 17, 2016, 11:05 p.m., Andreas Hansson wrote:
> > src/cpu/kvm/base.cc, line 228
> > 
> >
> > 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.
> 
> Michael LeBeane wrote:
> 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 wrote:
> 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 wrote:
> 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
---


On Aug. 17, 2016, 9:07 p.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Sandberg]

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



src/cpu/kvm/base.cc 


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)


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


On Aug. 17, 2016, 9:07 p.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Andreas Sandberg]

> On Aug. 17, 2016, 11:05 p.m., Andreas Hansson wrote:
> > src/cpu/kvm/base.cc, line 228
> > 
> >
> > 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.
> 
> Michael LeBeane wrote:
> 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 wrote:
> 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.


- Andreas


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


On Aug. 17, 2016, 9:07 p.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[Michael LeBeane]

> On Aug. 17, 2016, 10:05 p.m., Andreas Hansson wrote:
> > src/cpu/kvm/base.cc, line 228
> > 
> >
> > 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
---


On Aug. 17, 2016, 8:07 p.m., Michael LeBeane wrote:
> 
> ---
> 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.
> 
> 
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[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)


a stack?



src/cpu/kvm/base.hh (line 592)


unsigned int?



src/cpu/kvm/base.cc (line 189)


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)


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)


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


On Aug. 17, 2016, 8:07 p.m., Michael LeBeane wrote:
> 
> ---
> 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.
> 
> 
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [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/
---

(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

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

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

[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)


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



src/cpu/kvm/base.cc (lines 1097 - )


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)


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


- Andreas Sandberg


On Aug. 16, 2016, 11:28 p.m., Michael LeBeane wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 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
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev