Let other objects set up memory like regions in a KVM VM.

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

Description

Changeset 10551:7767dc21318d
---------------------------
Let other objects set up memory like regions in a KVM VM.

Testing Done

Issue Summary

Description	From	Last Updated	Status
The returned slot ID should be typedef:ed, or preferably a struct since that would make type checking more reliable.	Andreas Sandberg	Nov. 19, 2014, 8:39 a.m.	Open
Would it make sense to rename this to mapMemSlot? In my opinion, that'd be more descriptive.	Andreas Sandberg	Nov. 19, 2014, 8:39 a.m.	Open
Indentation is inconsistent with the rest of the file.	Andreas Sandberg	Nov. 23, 2014, 2:39 a.m.	Resolved
What's this group comment doing here and where is it terminated?	Andreas Sandberg	Nov. 23, 2014, 2:39 a.m.	Resolved
How about renaming this to unmapMemSlot?	Andreas Sandberg	Nov. 19, 2014, 8:39 a.m.	Open
Inconsistent indentation.	Andreas Sandberg	Nov. 23, 2014, 2:39 a.m.	Resolved
I think this causes problems with some of the officially supported compilers. It's just a hunch, but please check.	Andreas Hansson	Dec. 8, 2014, 6:36 a.m.	Open

src/cpu/kvm/vm.hh (Diff revision 1)

The returned slot ID should be typedef:ed, or preferably a struct since that would make type checking more reliable.

Show all issues

Gabe Black Nov. 23, 2014, 2:41 a.m. (Nov. 23, 2014, 2:41 a.m.)

I considered it briefly, but then I decided not to worry about it then and forgot about it. It wouldn't be a bad idea.

src/cpu/kvm/vm.hh (Diff revision 1)

Would it make sense to rename this to mapMemSlot? In my opinion, that'd be more descriptive.

Show all issues

Gabe Black Nov. 23, 2014, 2:41 a.m. (Nov. 23, 2014, 2:41 a.m.)

No, I don't think so. A slot isn't mapped or unmapped, it's what keeps track of a mapping between the guest and the host. The guest region is mapped, but I don't want to change this to mapMemRegion because the thing being operated on is the slot, not the region. With that name, the slot parameter seems extraneous.

src/cpu/kvm/vm.hh (Diff revision 1)

Indentation is inconsistent with the rest of the file.

Show all issues

src/cpu/kvm/vm.hh (Diff revision 1)

What's this group comment doing here and where is it terminated?

Show all issues

Gabe Black Nov. 23, 2014, 2:41 a.m. (Nov. 23, 2014, 2:41 a.m.)
```
A left over from earlier edits. I'll get rid of it.
```

src/cpu/kvm/vm.hh (Diff revision 1)

How about renaming this to unmapMemSlot?

Show all issues

Gabe Black Nov. 23, 2014, 2:41 a.m. (Nov. 23, 2014, 2:41 a.m.)
```
See above.
```

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

Inconsistent indentation.

Show all issues

Overall, I'd prefer this to be an internal API and have some way of notifying the VM through the System instead of allowing objects to poke around directly. See my reply in the email thread for RB #2513.

Gabe Black Nov. 23, 2014, 1:59 a.m. (Nov. 23, 2014, 1:59 a.m.)

The device is asking for a specific service from a specific type of VM. Pretending an interface is generic when it really isn't is counterproductive. Not only does it not provide any additional flexibility, it obfuscates what the code is actually doing. What purpose would a device have to declare it has internal memory if it isn't so a VM can indirectly find out about it? If there isn't one, why shouldn't it make it clear what it's trying to do, instead of using a round about approach to get to the same place? If there is, what are the odds that a mechanism designed without any preconception of that use will fit well?

Also, there's no generalized concept of a VM anywhere else in gem5. Making the interface generic (or generic looking) in this one particular place would be a half effort at best. When/if there's a second type of VM, then we can determine what can/should be generalized. If there's ever as second device which would use this mechanism, then we'll know what those requirements look like. If there's ever a second use case for this generic device memory abstraction, then we can know what it's requirements are. In the mean time, we'd be building fancy structures that don't serve much of a purpose, incur a non-trivial complexity/implementation cost, and would likely need to be redone in the future anyway.

Description:

~		Changeset 10546:b4c9aa186307
	~	Changeset 10551:7767dc21318d

		Let other objects set up memory like regions in a KVM VM.

Diff:

Revision 2 (+127 -13)

Show changes

	src/cpu/kvm/vm.hh
	src/cpu/kvm/vm.cc

Ping

As this is quite invasive, how broadly would this be used? Also, is there any chance of rather using devices that do not have memory of their own (and rely on the "normal" system memory that is already mapped)?

Gabe Black Dec. 3, 2014, 3:54 a.m. (Dec. 3, 2014, 3:54 a.m.)

The one example I'm aware of is the cirrus graphics device on x86, and unfortunately the existing driver assumes there's memory on the card. Our team is reluctant to spend a significant amount of effort either writing a new driver or specializing the image to switch to a different video device. QEMU uses cirrus already, and since we use that for VM based testing we already have support for it in place.

It's all a matter of opinion of course, but this doesn't seem that invasive to me. It's making the memory slot management a little bit more robust, and exposing it to external consumers. KVM is already a generalized/standardized interface to hardware virtualization, so it's not reaching behind the curtain all that much. There is some impact, but it seems like a reasonable trade off to me.

I'm still not happy with the setupMemSlot()/disableMemSlot() names, but I can't think of any better names atm, so let's go with them for now.

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

I think this causes problems with some of the officially supported compilers. It's just a hunch, but please check.

Show all issues

Gabe Black Dec. 8, 2014, 6:45 p.m. (Dec. 8, 2014, 6:45 p.m.)

Ugh. Yes, I think you're right. That's standard C but not C++.

Gabe Black Dec. 9, 2014, 9:46 p.m. (Dec. 9, 2014, 9:46 p.m.)
```
I changed it to just initialize the elements one by one.
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10546:b4c9aa186307

Changeset 10551:7767dc21318d

Diff:

Status: Closed (submitted)