> On Nov. 12, 2015, 7:13 a.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.) > > Andreas Sandberg wrote: > 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/.
Sounds good. I agree the organization is much better and more realistic, so I support this change even if it doesn't have any immediate functional benefits. It would be nice if the commit message was clearer about that though. I'd be fine with putting actual devices in subdirs based on the device type, with dev/pci only for things that are specifically related to PCI. BTW, the style guide says that names with acronyms should use upper case for the acronym, e.g., PCIHost. I agree it's awkward to add PCIHost when we still have PciDevice; probably the right thing to do is just have a big rename patch at some point (or wimp out and modify the style guide). It's probably one of our most violated rules, all due to historical inconsistencies that were never addressed. > On Nov. 12, 2015, 7:13 a.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 > > Andreas Sandberg wrote: > 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. It's not in the style guide, true, but if there are other classes where they're not on the same line, it's only because someone else was inconsistent and I hadn't noticed yet. Further investigation indicates that "more common" seems to be an understatement: % find . -name '*.hh' | xargs grep ': public' | wc -l 1151 % find . -name '*.hh' | xargs grep '^ *: public' | wc -l 8 and on looking at those 8 exceptions, 6 of them are you, and the other two are heinous templates that required the line break to stay under 80 chars... If it helps to add it to the style guide we can do that. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3204/#review7546 ----------------------------------------------------------- On Nov. 12, 2015, 4:22 a.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3204/ > ----------------------------------------------------------- > > (Updated Nov. 12, 2015, 4:22 a.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
