(CC'ing Will Deacon, author of "drivers/pci/host/pci-host-generic.c")
On 02/21/15 18:19, Laszlo Ersek wrote:
> Alex,
[...]
> I'm having second thoughts if the translation in this patch covers
> everything. I'm wondering if the translation should also be done for the
> gDS->XxxxMemorySpace() calls as well -- IOW if the GCD memory operations
> should also get translated (== cpu absolute) addresses. This is
> especially important for the KVM hack in patch v2 16/28, see
> gDS->SetMemorySpaceAttributes().
>
> Given that the MMIO translation offset that QEMU currently exports is
> zero, there's no way to tell *by way of testing* if this patch covers
> everything related. Forgetting to add zero is identical to adding zero.
>
> So, before I try to reason more about code that's potentially not
> covered by this patch, let me ask this -- is it conceivable that in the
> following:
>
> //
> // child base cpu base
> // type address address size
> // --------- -------------- -------------- --------------
> ranges = <0x1000000 0x0 0x0 0x0 0x3eff0000 0x0 0x10000
> 0x2000000 0x0 0x10000000 0x0 0x10000000 0x0 0x2eff0000>;
>
> the "child base address" would *ever* be different from the "cpu base
> address" for the PCI MMIO32 type range (0x2000000)?
>
> Because, if that's inconceivable in practice, then I'd much prefer
> killing the MMIO translation in this patchset -- including the PCD
> (platform config database) entry that carries the translation offset --
> over investigating (without a nonzero test case!) if everything has been
> covered. I'd check (childbase == cpubase) nonetheless, but I wouldn't
> try to support the nonzero offset case, just reject bringing up the host
> bridge.
>
> If the nonzero offset case is valid and should be handled by the
> firmware, then can you please give me a small qemu patch that sets up
> stuff like that? I have some ideas as to what additions would be
> necessary for this patchset, but without a test, it's all just
> speculation.
I'm completely unfamiliar with "drivers/pci/host/" in the kernel tree,
but I did try to look at the underlying types, like "struct resource"
and "struct resource_entry".
"resource_entry.offset" is the quantity whose non-nullity we're
investigating here, judging from:
gen_pci_parse_request_of_pci_ranges()
of_pci_get_host_bridge_resources()
of_pci_range_to_resource()
pci_add_resource_offset()
where ultimately we get
resource_entry.offset = of_pci_range.cpu_addr - of_pci_range.pci_addr;
(Compare, in patch v2 06/28:
+ MmioBase = fdt64_to_cpu (Record->ChildBase);
+ MmioSize = fdt64_to_cpu (Record->Size);
+ MmioTranslation = fdt64_to_cpu (Record->CpuBase) - MmioBase;
)
So, the question becomes, I think, if "pci-host-generic.c" in the kernel
puts "resource_entry.offset" to any use.
I think it doesn't. gen_pci_parse_request_of_pci_ranges() iterates over
the parsed ranges, with "win" being the loop variable. "win->offset"
would be the translation offset discussed above, but that field is never
accessed.
This is unlike, say, "pci-xgene.c", where "resource_entry.offset" *is*
used. See xgene_pcie_map_ranges().
Hence I think the "pci-host-generic.c" kernel driver assumes
of_pci_range.cpu_addr == of_pci_range.pci_addr
ie.
resource_entry.offset == 0
(This holds for your temporary patch "arm64: Enable generic PHB driver"
too.)
If that's the case, I should do the same in the firmware. (Ie. only
check the above equality, and if it doesn't hold, just give up.) There's
no point in writing untestable, speculatively generic code.
What do you guys think?
Thanks,
Laszlo
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
