linux-kernel - Re: [PATCH v7 5/7] PCI: cadence: Add support to configure virtual functions

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]

Date:   Tue, 3 Aug 2021 20:26:42 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Tom Joseph <tjoseph@...ence.com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Marek Vasut <marek.vasut+renesas@...il.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Jonathan Corbet <corbet@....net>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-pci@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-renesas-soc@...r.kernel.org>,
        <linux-rockchip@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Lokesh Vutla <lokeshvutla@...com>
Subject: Re: [PATCH v7 5/7] PCI: cadence: Add support to configure virtual
 functions

Hi Lorenzo,

On 03/08/21 5:15 pm, Lorenzo Pieralisi wrote:
> On Tue, Aug 03, 2021 at 10:33:08AM +0530, Kishon Vijay Abraham I wrote:
>> Now that support for SR-IOV is added in PCIe endpoint core, add support
>> to configure virtual functions in the Cadence PCIe EP driver.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
>> Acked-by: Tom Joseph <tjoseph@...ence.com>
>> ---
>>  .../pci/controller/cadence/pcie-cadence-ep.c  | 241 +++++++++++++++---
>>  drivers/pci/controller/cadence/pcie-cadence.h |   7 +
>>  2 files changed, 217 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> index 912a15be8bfd..791915054ff4 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> @@ -20,7 +20,18 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
>>  				     struct pci_epf_header *hdr)
>>  {
>>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> +	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>>  	struct cdns_pcie *pcie = &ep->pcie;
>> +	u32 reg;
>> +
>> +	if (vfn > 1) {
>> +		dev_dbg(&epc->dev, "Only Virtual Function #1 has deviceID\n");
>> +		return 0;
> 
> Shouldn't this return an error ?

Since the same function driver could be used for physical function and
virtual function, I tried to avoid adding any additional case specific
for virtual function in the function driver.

If we want to return an error here, then the function driver should be
modified to not invoke writeheader for vfn > 1.
> 
>> +	} else if (vfn == 1) {
>> +		reg = cap + PCI_SRIOV_VF_DID;
>> +		cdns_pcie_ep_fn_writew(pcie, fn, reg, hdr->deviceid);
>> +		return 0;
>> +	}
>>  
>>  	cdns_pcie_ep_fn_writew(pcie, fn, PCI_DEVICE_ID, hdr->deviceid);
>>  	cdns_pcie_ep_fn_writeb(pcie, fn, PCI_REVISION_ID, hdr->revid);
>> @@ -51,12 +62,14 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>>  				struct pci_epf_bar *epf_bar)
>>  {
>>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> +	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>>  	struct cdns_pcie_epf *epf = &ep->epf[fn];
>>  	struct cdns_pcie *pcie = &ep->pcie;
>>  	dma_addr_t bar_phys = epf_bar->phys_addr;
>>  	enum pci_barno bar = epf_bar->barno;
>>  	int flags = epf_bar->flags;
>>  	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
>> +	u32 first_vf_offset, stride;
>>  	u64 sz;
>>  
>>  	/* BAR size is 2^(aperture + 7) */
>> @@ -92,26 +105,50 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>>  
>>  	addr0 = lower_32_bits(bar_phys);
>>  	addr1 = upper_32_bits(bar_phys);
>> +
>> +	if (vfn == 1) {
>> +		/* All virtual functions use the same BAR config */
>> +		if (bar < BAR_4) {
>> +			reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG0(fn);
>> +			b = bar;
>> +		} else {
>> +			reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG1(fn);
>> +			b = bar - BAR_4;
>> +		}
>> +	} else if (vfn == 0) {
>> +		/* BAR configuration for physical function */
>> +		if (bar < BAR_4) {
>> +			reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG0(fn);
>> +			b = bar;
>> +		} else {
>> +			reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG1(fn);
>> +			b = bar - BAR_4;
>> +		}
>> +	}
> 
> Code in both branches is almost identical except for what is
> assigned to reg, it is not fundamental but maybe it can be rewritten
> more concisely.

okay.. let me think.

Thanks
Kishon