x86: kvm: Fix the KVM CPU in SE and FS on Intel CPUs.

Information
Submitter:	Gabe Black
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10606:aa3eb7453246
---------------------------
x86: kvm: Fix the KVM CPU in SE and FS on Intel CPUs.

There were a number of problems with how things were initialized which prevent
VMX from running the simulation as a guest.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Is there a reason the desc.l does not get set for the dataSegDesc()? code sets p,l,d,g,s.	mike upton	Jan. 16, 2015, 3:28 p.m.	Open
when limitHigh and limitLow get set by dataSegDesc(), it seems that limitHigh and limitLow get reversed.	mike upton	Jan. 16, 2015, 3:56 p.m.	Open
For AMD systems, the sys descriptors need to come first. On intel systems they need to come second. I do ...	mike upton	Jan. 21, 2015, 1:22 p.m.	Open
The AMD version only works with the original definition for star... I could not get the new code to work.	mike upton	Jan. 21, 2015, 1:22 p.m.	Open

Posted for reference, don't review yet.

Summary:

-	imported patch wip.patch
+	x86: kvm: Fix the KVM CPU in SE and FS on Intel CPUs.

Description:

~		Changeset 10605:edee8f168abd
	~	Changeset 10606:aa3eb7453246

~		imported patch wip.patch
	~	x86: kvm: Fix the KVM CPU in SE and FS on Intel CPUs.
	+
	+	There were a number of problems with how things were initialized which prevent
	+	VMX from running the simulation as a guest.

Diff:

Revision 2 (+294 -317)

Show changes

	src/arch/x86/process.cc
	src/arch/x86/system.hh
	src/arch/x86/system.cc
	src/arch/x86/utility.hh
	src/arch/x86/utility.cc
	src/arch/x86/regs/misc.hh
	src/cpu/kvm/x86_cpu.cc

Ok, now it's for real. Review away.

Amit Gaikwad Jan. 13, 2015, 3:52 p.m. (Jan. 13, 2015, 3:52 p.m.)

Thank you Gabe. I am new to gem5. Can you please let me know where can I find the wip.patch and also what will be command for patching gem5 ?

There is some issue with AMD platforms. A test that used to run in 30 sec is not finishing.

mike upton Dec. 10, 2014, 2:32 p.m. (Dec. 10, 2014, 2:32 p.m.)
```
hello world passes. SPEC apps hang.
```

Gabe Black Dec. 10, 2014, 2:41 p.m. (Dec. 10, 2014, 2:41 p.m.)

Can you identify where it's getting stuck? It could be there's something wrong with how exception handling is set up, and it gets stuck faulting over and over when a page fault happens, for instance. You could have the KVM CPU print what the IP is each time it regains control and see if they cluster somewhere interesting.

Gabe Black Dec. 11, 2014, 12:12 a.m. (Dec. 11, 2014, 12:12 a.m.)

Any luck? Unfortunately the time I have to work on gem5 is pretty limited, so I probably won't be able to dig into this for a while.

mike upton Jan. 5, 2015, 3:12 p.m. (Jan. 5, 2015, 3:12 p.m.)

on the intel platform, with the test input set, all of cpu2006 integer tests pass, except for astar.
astar dies with a panic and page fault.
[m5PageFault:build/X86/arch/x86/pseudo_inst.cc, line 85]

on the amd platform, all of the tests I tried fail to complete.
time continues to advance, but instructions are not completing.

I am not sure what to do, I need the Intel platform code, but we should not break the AMD side.

Are there any AMD folks I can work with to figure out how to get both sides working?

Alexandru Dutu Jan. 6, 2015, 10:39 a.m. (Jan. 6, 2015, 10:39 a.m.)

Sorry about the late reply, I remember replying on a different mailing list thread about this. I have tried this patch on my AMD system with some short running applications and things worked fine. So, I will run cpu2006 and see if there are any issues.

Alexandru Dutu Jan. 11, 2015, 12:48 a.m. (Jan. 11, 2015, 12:48 a.m.)

When running SPEC CPU2006 with latest gem5 on a 6 core Bulldozer, Ubuntu 14.04, linux 3.13.0-34, most of them call exit_group syscall except gromacs which fails because of unimplemented clock_gettime syscall. This means they ran until completion, however I have not yet checked if the output is actually correct.

However, after applying this patch things break and there are unexpected exits in the middle of the execution. Looking more into it and coming up with more details.

Amit Gaikwad Jan. 15, 2015, 9:27 a.m. (Jan. 15, 2015, 9:27 a.m.)

I am new to gem5. Can someone please let me know where can I find the wip.patch and also what will be command for patching gem5 ?

mike upton Jan. 15, 2015, 2:55 p.m. (Jan. 15, 2015, 2:55 p.m.)

I am also having trouble running this patch on a different intel x86 system.
It looks like there are both segmentation changes, and KVM changes merged together.

Can we split it into 2 separate patches?

I think some of the style cleanup in the segmentation stuff changed the behavior.

If no one else does it, I will attempt a separate minimal patch that just fixes the KVM stuff.

Amit Gaikwad Jan. 15, 2015, 4:36 p.m. (Jan. 15, 2015, 4:36 p.m.)

@ Mike Upton,

I am not even able to find the patch and I am actively looking for it.
Did you manually change the below 6 files ? Can you please let me know ? 

src/arch/x86/process.cc: 8 changes 
src/arch/x86/system.hh: 1 change
src/arch/x86/system.cc: 3 changes 
src/arch/x86/utility.hh: 2 changes
src/arch/x86/utility.cc: 1 change
src/cpu/kvm/x86_cpu.cc: 2 changes

The changes listed on this link : http://reviews.gem5.org/r/2557/diff/1-2/

src/arch/x86/regs/misc.hh (Diff revision 2)

really <32, 0> in the new code?

Gabe Black Jan. 15, 2015, 10:27 p.m. (Jan. 15, 2015, 10:27 p.m.)
```
Yeah, I think that should be 31, 0
```

src/arch/x86/utility.hh (Diff revision 2)

Is there a reason the desc.l does not get set for the dataSegDesc()? code sets p,l,d,g,s.

Show all issues

Gabe Black Jan. 18, 2015, 11:59 p.m. (Jan. 18, 2015, 11:59 p.m.)

Yes. The long mode bit only means something for code segment descriptors.

src/arch/x86/process.cc (Diff revision 2)

when limitHigh and limitLow get set by dataSegDesc(), it seems that limitHigh and limitLow get reversed.

Show all issues

Gabe Black Jan. 18, 2015, 11:59 p.m. (Jan. 18, 2015, 11:59 p.m.)

This patch reverses them because they started out the wrong way around.

src/arch/x86/process.cc (Diff revision 2)

For AMD systems, the sys descriptors need to come first. On intel systems they need to come second.

I do not know how to resolve...

Show all issues

mike upton Jan. 21, 2015, 1:23 p.m. (Jan. 21, 2015, 1:23 p.m.)

I have been debugging why patch rb2557 breaks AMD KVM functionality.




I was hoping to get to code that would work on both intel and AMD platforms, but am not there yet.




This patch is to be applied on top of rb2557.patch.




There are 2 main issues, neither of which I understand well enough to take much further.




The first issue is that the order that the segment descriptors get instantiated in the GDT table seems to matter between AMD and Intel, and they seem to be mutually incompatible.




AMD wants:

csSys

dsSys

ds

cs




Intel wants:

ds

cs

dsSys

csSys




I am not sure the relative ordering of ds and cs within a class matters, only that AMD wants the Sys ones first, and Intel wants them second.




There is also an issue with how 'star' gets defined.

I can not make the Intel code work for AMD.




Both issues are addressed in this patch.




The patch makes the AMD system work, but breaks Intel functionality.




I am also not sure how to upload this into review board. Do I create a separate patch from TOT, or can I somehow attach this to rb2557.




Hopefully Gabe or Alexandru can weigh in. I am happy to help, but I am at my 'Peter Principal Limit' as far as my understanding goes.




I think it would be really ugly to have a machine-type test to version the code...

Gabe Black Jan. 21, 2015, 1:48 p.m. (Jan. 21, 2015, 1:48 p.m.)

You should grab a copy of the architecture manual. From there:


STAR—The STAR register has the following fields (unless otherwise noted, all bits are
read/write):
- SYSRET CS and SS Selectors—Bits 63:48. This field is used to specify both the CS and SS
selectors loaded into CS and SS during SYSRET. If SYSRET is returning to 32-bit mode
(either legacy or compatibility), this field is copied directly into the CS selector field. If
SYSRET is returning to 64-bit mode, the CS selector is set to this field + 16. SS.Sel is set to
this field + 8, regardless of the target mode. Because SYSRET always returns to CPL 3, the
RPL bits 49:48 should be initialized to 11b.
- SYSCALL CS and SS Selectors—Bits 47:32. This field is used to specify both the CS and SS
selectors loaded into CS and SS during SYSCALL. This field is copied directly into CS.Sel.
SS.Sel is set to this field + 8. Because SYSCALL always switches to CPL 0, the RPL bits
33:32 should be initialized to 00b.


That's why the order matters and is what it is.

mike upton Jan. 21, 2015, 3:15 p.m. (Jan. 21, 2015, 3:15 p.m.)

AMD and Intel use different solutions, right?

AMD: Syscall, sysret
Intel: Sysenter, sysexit

Do we need independent code streams for each?

The original code worked for AMD, but not intel.
The current 2557 patch works for Intel, but not AMD.

Gabe Black Jan. 21, 2015, 3:35 p.m. (Jan. 21, 2015, 3:35 p.m.)

Yeah, there are some differences between the two. I think both support both pairs of instructions, but I think one or the other only works in 32 bit mode on for one of the vendors, or something along those lines. At one point I could have told you exactly what the difference was, but now I'd have to check the manuals. My expectation/hope is that a single GDT layout would work for both. I doubt the kernel, for instance, specializes its layout based on who's CPU it's running on.

mike upton Jan. 22, 2015, 11:32 a.m. (Jan. 22, 2015, 11:32 a.m.)

OK, it seems like the feedback is that we do need to runtime test the CPU we are running on and do CPU specific code.
I certainly can code this up.

Any pointers to what state is available in the simulator to test?
Or should I just add a cpuHostType() routine that will return Intel, AMD, or UNKNOWN?

src/arch/x86/process.cc (Diff revision 2)

The AMD version only works with the original definition for star...

I could not get the new code to work.

Show all issues

Hi folks. I put together this patch a long time ago, there was some discussion about it, and then I lost track of what was going on with it. Have the issues it causes with AMD cpus been tracked down? Glancing through this again, I think there are a couple things which are genuinely wrong which should fix things regardless of the underlying CPU (like reversed bits, things that shouldn't be set but are), but I wouldn't want to push it if it would break some other things which are currently working. Please update me on the current state of things.

mike upton Jan. 30, 2017, 9:23 p.m. (Jan. 30, 2017, 9:23 p.m.)

I developed another patch on top that had both AMD and intel working. I cant remember the number, and it should certainly be retested before merging.
I will need to see if I can fins a current AMD box.

So the answer to your question is: yes there were issues, the issues were tracked down and there is a additional patch that gets both intel and AMD SE virtualization working. The code has not been tested in 2 years though.

Jason Lowe-Power Jan. 31, 2017, 8:05 a.m. (Jan. 31, 2017, 8:05 a.m.)

I've been using some version of these patches in my local repository for a while now. I think I may have some patches on top of these to fix other issues, too.

I've added this as a possible project for a Sprint this Sunday (http://gem5.org/Sprint_Ideas#Push_in_fixes_to_x86_KVM). Any chance you're going to be at HPCA, and want to work on this, Mike or Gabe?

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Summary:

Description:

Changeset 10605:edee8f168abd

Changeset 10606:aa3eb7453246

Diff: