linux-kernel - Re: [PATCH 3/3] PCI: qcom: Enable ECAM feature based on config size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5bb0d96d-0d11-99c2-a569-7c928e0ae4fe@quicinc.com>
Date: Wed, 4 Dec 2024 07:56:54 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: <cros-qcom-dts-watchers@...omium.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
        "Manivannan
 Sadhasivam" <manivannan.sadhasivam@...aro.org>,
        Lorenzo Pieralisi
	<lpieralisi@...nel.org>,
        Krzysztof Wilczyński
	<kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, <quic_vbadigan@...cinc.com>,
        <quic_ramkri@...cinc.com>, <quic_nitegupt@...cinc.com>,
        <quic_skananth@...cinc.com>, <quic_vpernami@...cinc.com>,
        <quic_mrana@...cinc.com>, <mmareddy@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [PATCH 3/3] PCI: qcom: Enable ECAM feature based on config size



On 12/4/2024 12:29 AM, Bjorn Helgaas wrote:
> On Sun, Nov 17, 2024 at 03:30:20AM +0530, Krishna chaitanya chundru wrote:
>> Enable the ECAM feature if the config space size is equal to size required
>> to represent number of buses in the bus range property.
>>
>> The ELBI registers falls after the DBI space, so use the cfg win returned
>> from the ecam init to map these regions instead of doing the ioremap again.
>> ELBI starts at offset 0xf20 from dbi.
>>
>> On bus 0, we have only the root complex. Any access other than that should
>> not go out of the link and should return all F's. Since the IATU is
>> configured for bus 1 onwards, block the transactions for bus 0:0:1 to
>> 0:31:7 (i.e., from dbi_base + 4KB to dbi_base + 1MB) from going outside the
>> link through ecam blocker through parf registers.
> 
> s/ecam/ECAM/
> s/dbi/DBI/
> s/IATU/iATU/ (Seems to be the convention?  Also below)
> s/parf/PARF/ (I assume an initialism?)
> 
> Use conventional format for PCI bus addresses ... I assume "0:0:1"
> means bus 0, device 0, function 1, which would normally be formatted
> as "00:00.1" (also below).
> 
>> +static int qcom_pci_config_ecam_blocker(struct dw_pcie_rp *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>> +	u64 addr, addr_end;
>> +	u32 val;
>> +
>> +	/* Set the ECAM base */
>> +	writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE);
>> +	writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf + PARF_ECAM_BASE_HI);
>> +
>> +	/*
>> +	 * On bus 0, we have only the root complex. Any access other than that
>> +	 * should not go out of the link and should return all F's. Since the
>> +	 * IATU is configured for bus 1 onwards, block the transactions for
>> +	 * bus 0:0:1 to 0:31:7 (i.e from dbi_base + 4kb to dbi_base + 1MB) from
>> +	 * going outside the link.
> 
> s/IATU/iATU/ to match other usage.
> 
> Use conventional formatting of PCI bus/device/function addresses.
> 
> Unless the root bus number is hard-wired to be zero, maybe this should
> say "root bus" instead of "bus 0"?
> 
> There is no architected presence of a PCIe Root Complex as a PCI
> device.  Maybe this should say "the only device on bus 0 is the *Root
> Port*"?
> 
> Or maybe there's a PCI device with some sort of device-specific
> interface to Root Complex registers?  But if that were the *only*
> device on bus 0, there would be no Root Port to reach other devices,
> so this doesn't seem likely.
> 
>> +static bool qcom_pcie_check_ecam_support(struct device *dev)
> 
> Rename to be an assertion that can be either true or false, e.g.,
> "ecam_supported".  "Check" doesn't hint about what true/false mean.
> 
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct resource bus_range, *config_res;
>> +	u64 bus_config_space_count;
>> +	int ret;
>> +
>> +	/* If bus range is not present, keep the bus range as maximum value */
>> +	ret = of_pci_parse_bus_range(dev->of_node, &bus_range);
>> +	if (ret) {
>> +		bus_range.start = 0x0;
>> +		bus_range.end = 0xff;
>> +	}
> 
> I would have thought the generic OF parsing would already default to
> [bus 00-ff]?
> 
if there is no bus-range of_pci_parse_bus_range is not updating it[1],
the bus ranges is being updated to default value in
devm_of_pci_get_host_bridge_resources()[2]

[1]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L193
[2]https://elixir.bootlin.com/linux/v6.12.1/source/drivers/pci/of.c#L347

- Krishna Chaitanya.
>> +	config_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
>> +	if (!config_res)
>> +		return false;
> 
> Move of_pci_parse_bus_range() (if it's needed) down here so it's
> together with the use of the results.  No point in calling it before
> looking for "config".
> 
>> +	bus_config_space_count = resource_size(config_res) >> PCIE_ECAM_BUS_SHIFT;
>> +	if (resource_size(&bus_range) > bus_config_space_count)
>> +		return false;
>> +
>> +	return true;
>> +}