On 05/14/15 15:00, Andrew Jones wrote: > On Thu, May 14, 2015 at 01:38:11PM +0100, Peter Maydell wrote: >> On 14 May 2015 at 13:28, Paolo Bonzini <pbonz...@redhat.com> wrote: >>> Well, PCI BARs are generally MMIO resources, and hence should not be cached. >>> >>> As an optimization, OS drivers can mark them as cacheable or >>> write-combining or something like that, but in general it's a safe >>> default to leave them uncached---one would think. >> >> Isn't this handled by the OS mapping them in the 'prefetchable' >> MMIO window rather than the 'non-prefetchable' one? (QEMU's >> generic-PCIe device doesn't yet support the prefetchable window.) > > I was thinking (with my limited PCI knowledge) the same thing, and > was planning on experimenting with that.
This could be supported in UEFI as well, with the following steps: - the DTB that QEMU provides UEFI with should advertise such a prefetchable window. - The driver in UEFI that parses the DTB should understand that DTB node (well, record type), and store the appropriate base & size into some new dynamic PCDs (= basically, firmware wide global variables; PCD = platform configuration database) - The entry point of the host bridge driver would call gDS->AddMemorySpace() twice, separately for the two different windows, with their appropriate caching attributes. - The host bridge driver needs to be extended so that TypePMem32 requests are not rejected (like now); they should be handled similarly to TypeMem32. Except, the gDS->AllocateMemorySpace() call should allocate from the prefetchable range (determined by the new PCDs above). - QEMU's emulated devices should then expose their BARs as prefetchable (so that the above branch would be taken in the host bridge driver). (Of course, if QEMU intends to emulate PCI devices somewhat realistically, then QEMU should claim "non-prefetchable" for BARs that would not be prefetchable on physical hardware either, and then the hypervisor should accommodate the firmware's UC mapping and say "hey I know better, we're virtual in fact", and override the attribute (-> use WB instead of UC). With which we'd be back to square one...) Thanks Laszlo
