On 19/12/17 14:56, Alex Williamson wrote: > On Tue, 19 Dec 2017 14:44:51 +1100 > Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > >> On 18/12/17 16:02, Alex Williamson wrote: >>> Add one more layer to our stack of MemoryRegions, this base region >>> allows us to register BARs independently of the vfio region or to >>> extend the size of BARs which do map to a region. This will be >>> useful when we want hypervisor defined BARs or sections of BARs, >>> for purposes such as relocating MSI-X emulation. We therefore call >>> msix_init() based on this new base MemoryRegion, while the quirks, >>> which only modify regions still operate on those sub-MemoryRegions. >> >> >> Looks ok, one worry though - the default config produces this: >> >> >> memory-region: p...@800000020000000.mmio >> 0000000000000000-ffffffffffffffff (prio 0, i/o): p...@800000020000000.mmio >> 0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 base BAR 1 >> 0000210000000000-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 >> 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table >> 000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] >> 0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 base BAR 3 >> 0000210000040000-000021000007ffff (prio 0, i/o): 0001:03:00.0 BAR 3 >> 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR >> 3 mmaps[0] >> >> >> Where "BAR 1" and "msix-table" overlap. It resolves correctly: >> >> >> FlatView #1 >> AS "memory", root: system >> AS "cpu-memory", root: system >> Root memory region: system >> 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram >> 0000210000000000-000021000000dfff (prio 0, i/o): 0001:03:00.0 BAR 1 >> 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table >> 000021000000e600-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 >> @000000000000e600 >> 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 >> mmaps[0] >> >> >> but this looks like an accident - should not we raise the msix-table >> priority or lower the BAR1 priority (to -1)? > > I was worried about this too, but the way I read the code is that the > last registered subregion with the same priority wins. Therefore so > long as msix_init() is called after we layer anything else onto the base > BAR MemoryRegion that might interfere with it, everything should work > correctly. It certainly might make sense for msix_init() to add > subregions with a higher priority, but then you immediately get into > the road block of what priority should it use, which is a bit of a rat > hole since we can make it work as-is with proper ordering.
The order is not obvious from any variant of "info mtree" (I may post a patch with a "-u" switch to disable sorting). And docs/devel/memory.txt allows using negative priorities so the "BAR 1" region could have -1 to make things predictable and more self-documented imho. > Thanks, > > Alex > -- Alexey
