Bus: Split the bus into a non-coherent and coherent bus

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9034:b5b92099f4c9
---------------------------
Bus: Split the bus into a non-coherent and coherent bus

This patch introduces a class hierarchy of buses, a non-coherent one,
and a coherent one, splitting the existing bus functionality. By doing
so it also enables further specialisation of the two types of buses.

A non-coherent bus connects a number of non-snooping masters and
slaves, and routes the request and response packets based on the
address. The request packets issued by the master connected to a
non-coherent bus could still snoop in caches attached to a coherent
bus, as is the case with the I/O bus and memory bus in most system
configurations. No snoops will, however, reach any master on the
non-coherent bus itself. The non-coherent bus can be used as a
template for modelling PCI, PCIe, and non-coherent AMBA and OCP buses,
and is typically used for the I/O buses.

A coherent bus connects a number of (potentially) snooping masters and
slaves, and routes the request and response packets based on the
address, and also forwards all requests to the snoopers and deals with
the snoop responses. The coherent bus can be used as a template for
modelling QPI, HyperTransport, ACE and coherent OCP buses, and is
typically used for the L1-to-L2 buses and as the main system
interconnect.

The configuration scripts are updated to use a NoncoherentBus for all
peripheral and I/O buses.

A bit of minor tidying up has also been done.

Testing Done

util/regress all passing (disregarding t1000 and eio)

Seems reasonable, although there doesn't seem to be  much code shared between the two buses. Mabye there just isn't much similar functionality?

I'd like steve to sign off before committing.

Nilay Vaish May 27, 2012, 2:25 p.m. (May 27, 2012, 2:25 p.m.)

The coherent bus class inherits from the non-coherent bus class. So the functionality is being
shared between the two.

Andreas, from the posted patch it is not clear how much of the code is new, what code has been
retained from the Bus class. Do you think there is a better way to display the patch? It seems
reviewboard does not handle renaming of files that well.

Andreas Hansson May 28, 2012, 1:35 a.m. (May 28, 2012, 1:35 a.m.)

Essentially non of the code is new. As mentioned in the comments, the two files are copies of bus.{hh,cc}, and the only larger change are the simplifications of the non-coherent bus which now does not have to deal with any snoopers and forwarding. All I can suggest is download the patch and look at it with hg diff. Other ideas?

In terms of Ali's question, most of the bus functionality is indeed shared through the inheritance. I did consider having an abstract "BaseBus" and then implement the transport functions in NoncoherentBus and CoherentBus, but at this point it felt like a bit too much object orientation. I'd be happy to change though.

Looks OK to me.  I can see where a common BaseBus class would clean up the constructor situation a bit, and I think would allow some of the NoncoherentBus-only bits like the ports to be more cleanly separated.  If you're up for it, Andreas, you might want to give it a try.  It's not unreasonable as it stands though.

Comments below are orthogonal to the patch, just because I noticed this code for the first time.

Andreas Hansson May 29, 2012, 4:07 a.m. (May 29, 2012, 4:07 a.m.)
```
The split is done. The BaseBus resides in bus.{hh,cc}.
```

src/mem/noncoherent_bus.cc (Diff revision 1)

I see this code isn't new, but I never noticed it before... does anything actually work if connected devices don't all have the same block size?  I'm just concerned that this code gives the impression that there's more flexibility than actually exists.

Andreas Hansson May 29, 2012, 4:06 a.m. (May 29, 2012, 4:06 a.m.)
```
Agreed. This should be addressed in a future patch.
```

src/mem/noncoherent_bus.cc (Diff revision 1)

On the other hand, this warning seems overly conservative... other block sizes should work fine as long as *everyone* uses the same size.

Description:

~		Changeset 9028:cd897893780f
	~	Changeset 9034:b5b92099f4c9

		Bus: Split the bus into a non-coherent and coherent bus

		This patch introduces a class hierarchy of buses, a non-coherent one,
		and a coherent one, splitting the existing bus functionality. By doing
		so it also enables further specialisation of the two types of buses.

		A non-coherent bus connects a number of non-snooping masters and
		slaves, and routes the request and response packets based on the
		address. The request packets issued by the master connected to a
		non-coherent bus could still snoop in caches attached to a coherent
		bus, as is the case with the I/O bus and memory bus in most system
		configurations. No snoops will, however, reach any master on the
		non-coherent bus itself. The non-coherent bus can be used as a
		template for modelling PCI, PCIe, and non-coherent AMBA and OCP buses,
		and is typically used for the I/O buses.

		A coherent bus connects a number of (potentially) snooping masters and
		slaves, and routes the request and response packets based on the
		address, and also forwards all requests to the snoopers and deals with
		the snoop responses. The coherent bus can be used as a template for
		modelling QPI, HyperTransport, ACE and coherent OCP buses, and is
		typically used for the L1-to-L2 buses and as the main system
		interconnect.

		The configuration scripts are updated to use a NoncoherentBus for all
		peripheral and I/O buses.

		A bit of minor tidying up has also been done.

Diff:

Revision 2 (+1353 -755)

Show changes

	configs/common/CacheConfig.py
	configs/common/FSConfig.py
	configs/example/memtest.py
	configs/example/se.py
	configs/splash2/cluster.py
	configs/splash2/run.py
	src/cpu/BaseCPU.py
	src/mem/Bus.py
	37 more

Thanks!  I didn't have a chance to go over it in detail, but it looks good to me.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 9028:cd897893780f

Changeset 9034:b5b92099f4c9

Diff:

Status: Closed (submitted)