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] [day] [month] [year] [list]

Message-ID: <20210804105318.GA31443@lpieralisi>
Date:   Wed, 4 Aug 2021 11:53:18 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Kishon Vijay Abraham I <kishon@...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

On Tue, Aug 03, 2021 at 08:26:42PM +0530, Kishon Vijay Abraham I wrote:
> 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.

Well, I see it the other way around. If writing the header for vfn > 1
is an error it must be reported as such and handled accordingly.

As it stands - it looks like we do nothing and everything is just
fine, which is weird.

Thanks,
Lorenzo

> >> +	} 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