On Fri, Jan 07, 2022 at 11:28:26PM +0100, Pali Rohár wrote: > On Friday 07 January 2022 15:55:04 Bjorn Helgaas wrote: > > On Thu, Nov 25, 2021 at 01:45:58PM +0100, Pali Rohár wrote: > > > Properly propagate failure from mvebu_pcie_add_windows() function back to > > > the caller mvebu_pci_bridge_emul_base_conf_write() and correctly updates > > > PCI_IO_BASE, PCI_MEM_BASE and PCI_IO_BASE_UPPER16 registers on error. > > > On error set base value higher than limit value which indicates that > > > address range is disabled. > > > > Does the spec say that if software programs something invalid, > > hardware should proactively set the base and limit registers to > > disable the window? > > No. But this patch address something totally different. Software can do > fully valid operation, e.g. try to set forwarding memory window as large > as possible. But because this driver "emulates" pci bridge by calling > software/kernel function (mvebu_pcie_add_windows), some operations which > in real HW cannot happen, are possible in software. > > For example there are limitations in sizes of forwarding memory windows, > because it is done by mvebu-mbus driver, which is responsible for > configuring mapping and forwarding of PCIe I/O and MEM windows. And due > to Marvell HW, there are restrictions which are not in PCIe HW. > > Currently if such error happens, obviously kernel is not able to set > PCIe windows and it just print warnings to dmesg. Trying to access these > windows would result in the worst case in crashes. > > With this change when mvebu_pcie_add_windows() function fails then into > emulated config space is put information that particular forwarding > window is disabled. I think that it is better to indicate it in config > space what is the current "reality" of hardware configuration. If window > is disabled in real-HW (meaning in mvebu-mbus driver) then show it also > in emulated config space of pci bridge. > > Do you have better idea what should emulated pci bridge do, if software > try to set fully valid configuration of forwarding window, but it is not > possible to achieve it (even compliant PCI bridge must be able to do > it)? On an ACPI system, the host bridge window sizes are constrained by the host bridge _CRS method. I assume there's a similar constraint in DT. Is the fact that mvebu_pcie_add_windows() can fail a symptom of a DT that describes more available space than mvebu-bus can map? > > I'm not sure I've seen hardware that does this, and it seems ... maybe > > a little aggressive. > > > > What happens if software writes the base and limit in the wrong order, > > so the window is invalid after the first write but valid after the > > second? That actually sounds like it could be a sensible strategy to > > prevent a partially-configured window from being active. > > Invalid window (limit < base) means that window is disabled. And > pci-mvebu.c in its callbacks from pci-bridge-emul.c should correctly > handle it and propagates information about disablement to mvebu-mbus > driver. > > After window is valid again (limit > base) then pci-mvebu.c call > mvebu-mbus to setup new mapping. Not sure I'm understanding the code correctly. Here's the sort of thing I'm worried about, but maybe this is actually impossible: Let's say software writes (0x00, 0xff) to the I/O (base, limit), which describes the [io 0x0000-0xffff] window. If mvebu-mbus can't handle that, it looks like you set the (base, limit) to (0xf0, 0x0f), which would describe [io 0xf000-0x0fff], which is invalid. The software writes 0x40 to the limit, so now we have (0xf0, 0x40), or [io 0xf000-0x40ff]. That's still invalid, but software thinks the 0x00 it wrote to the base is still there. Bjorn
