> On Nov. 12, 2015, 3:13 p.m., Steve Reinhardt wrote: > > I didn't look through all the code closely, but it LGTM. Can you highlight > > in the commit message what new functionality this restructuring provides? > > (e.g., are the two bullets at the end that *don't* say "This behavior is > > the same as in the old implementation" a complete summary of the new > > functionality?) > > > > What are examples of devices that might want to override GenericPciHost? > > > > Also, now that we have a dev/pci directory, do you plan to move the other > > dev/pci*.* files there? (I agree that should be a separate changeset > > though.)
The changset currently doesn't affect functional behavior at all. It's mainly a way to make the code clearer and promote code reuse (e.g., PCI config space decoding). Long-term, I'd like to change make interrupt deliver interrupt-controller agnostic so that we can reuse non-PCI devices on multiple platforms. Currently, a lot of devices are ARM specific due to the fact that they need to deliver interrupts using an ARM-specific interrupt controller. After that change, we'll be able to get rid of most of the architecture-specific platform code. There are some cases where you want to override GenericPciHost. Alpha has a specialized class that does memory remapping. I'm also considering implementing a specialized class that does automatic interrupt assignment based on a device's position on the PCI bus. My plan is to move generic PCI stuff into the pci/ directory. I'm not sure about other deivces (e.g., network devices). I'd like to put most of the network-related code in net/, but I it might make sens putting the devicem models in pci/. Similarly, I'd like to put i2c devices in i2c/. > On Nov. 12, 2015, 3:13 p.m., Steve Reinhardt wrote: > > src/dev/pci/host.hh, line 75 > > <http://reviews.gem5.org/r/3204/diff/2/?file=51565#file51565line75> > > > > when did we start putting base classes on a separate line? not a huge > > deal, but an inconsistency wrt the rest of the code base Hmm, I don't actually think this is mentioned anywhere in the style guide. We definitely have it on a separate line in some cases. However, judging from a small sample, having them on the same line is more common. I'll make it consistent with the rest of the code base. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3204/#review7546 ----------------------------------------------------------- On Nov. 12, 2015, 12:22 p.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3204/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2015, 12:22 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11206:c7d8a26d24a5 > --------------------------- > dev: Rewrite PCI host functionality > > The gem5's current PCI host functionality is very ad hoc. The current > implementations require PCI devices to be hooked up to the > configuration space via a separate configuration port. Devices query > the platform to get their config-space address range. Un-mapped parts > of the config space are intercepted using the XBar's default port > mechanism and a magic catch-all device (PciConfigAll). > > This changeset redesigns the PCI host functionality to improve code > reuse and make config-space and interrupt mapping more transparent. > > PCI devices now register themselves with a PCI host controller. The > host controller interface is defined in the abstract base class > PciHost. Registration is done by PciHost::registerDevice() which takes > the device, its bus position (bus/dev/func tuple), and its interrupt > pin (INTA-INTC) as a parameter. The registration interface returns a > PciHost::DeviceInterface that the PCI device can use to query memory > mappings and signal interrupts. > > The host device manages the entire PCI configuration space. Accesses > to devices decoded into the devices bus position and then forwarded to > the correct device. > > Basic PCI host functionality is implemented in the GenericPciHost base > class. Most platforms can use this class as a basic PCI controller. It > provides the following functionality: > > * Configurable configuration space decoding. The number of bits > dedicated to a device is a prameter, making it possible to support > both CAM and ECAM mappings. > > * Basic interrupt mapping using the interruptLine value from a > device's configuration space. This behavior is the same as in the > old implementation. > > * Simple (base + addr) remapping from the PCI bus's address space to > physical addresses for PIO, memory, and DMA. > > > Diffs > ----- > > src/dev/platform.cc 7a9eeecf2b52 > src/dev/x86/Pc.py 7a9eeecf2b52 > src/dev/pcidev.hh 7a9eeecf2b52 > src/dev/pcidev.cc 7a9eeecf2b52 > src/dev/platform.hh 7a9eeecf2b52 > src/dev/alpha/tsunami_pchip.cc 7a9eeecf2b52 > src/dev/arm/RealView.py 7a9eeecf2b52 > src/dev/arm/realview.hh 7a9eeecf2b52 > src/dev/arm/realview.cc 7a9eeecf2b52 > src/dev/pci/PciHost.py PRE-CREATION > src/dev/pci/SConscript PRE-CREATION > src/dev/pci/host.hh PRE-CREATION > src/dev/pci/host.cc PRE-CREATION > src/dev/pci/types.hh PRE-CREATION > src/dev/pciconfigall.hh 7a9eeecf2b52 > src/dev/pciconfigall.cc 7a9eeecf2b52 > configs/common/FSConfig.py 7a9eeecf2b52 > src/dev/Pci.py 7a9eeecf2b52 > src/dev/SConscript 7a9eeecf2b52 > src/dev/alpha/Tsunami.py 7a9eeecf2b52 > src/dev/alpha/tsunami.hh 7a9eeecf2b52 > src/dev/alpha/tsunami.cc 7a9eeecf2b52 > src/dev/alpha/tsunami_pchip.hh 7a9eeecf2b52 > src/dev/x86/SouthBridge.py 7a9eeecf2b52 > src/dev/x86/pc.hh 7a9eeecf2b52 > src/dev/x86/pc.cc 7a9eeecf2b52 > > Diff: http://reviews.gem5.org/r/3204/diff/ > > > Testing > ------- > > Quick regressions pass. Manually verified the diff between two checkpoints of > a booted system. > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
