linux-kernel - Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

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: <Z+v+Uni7PV9Nlstq@lizhi-Precision-Tower-5810>
Date: Tue, 1 Apr 2025 10:55:14 -0400
From: Frank Li <Frank.li@....com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Jon Mason <jdmason@...zu.us>, Dave Jiang <dave.jiang@...el.com>,
	Allen Hubbe <allenbh@...il.com>,
	Marek Vasut <marek.vasut+renesas@...il.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Yuya Hamamachi <yuya.hamamachi.sx@...esas.com>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	ntb@...ts.linux.dev
Subject: Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad
 space allocation

On Tue, Apr 01, 2025 at 09:39:10AM +0200, Jerome Brunet wrote:
> On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@....com> wrote:
>
> > On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
> >> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
> >> should not try to handle the size quirks for the underlying BAR, whether it
> >> is fixed size or alignment. This is already handled by
> >> pci_epf_alloc_space().
> >>
> >> Also, when handling the alignment, this allocate more space than necessary.
> >> For example, with a spad size of 1024B and a ctrl size of 308B, the space
> >> necessary is 1332B. If the alignment is 1MB,
> >> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
> >> been more than enough.
> >>
> >> Just drop all the handling of the BAR size quirks and let
> >> pci_epf_alloc_space() handle that.
> >>
> >> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
> >>  1 file changed, 2 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> >> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
> >>   */
> >>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> >>  {
> >> -	size_t align;
> >>  	enum pci_barno barno;
> >>  	struct epf_ntb_ctrl *ctrl;
> >>  	u32 spad_size, ctrl_size;
> >> -	u64 size;
> >>  	struct pci_epf *epf = ntb->epf;
> >>  	struct device *dev = &epf->dev;
> >>  	u32 spad_count;
> >> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
> >>  								epf->func_no,
> >>  								epf->vfunc_no);
> >>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
> >> -	size = epc_features->bar[barno].fixed_size;
> >> -	align = epc_features->align;
> >> -
> >> -	if ((!IS_ALIGNED(size, align)))
> >> -		return -EINVAL;
> >> -
> >>  	spad_count = ntb->spad_count;
> >>
> >>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
> >
> > I think keep ctrl_size at least align to 4 bytes.
>
> Sure, makes sense
>
> > keep align 2^n is more safe to keep spad area start at align
> > possition.
>
> That's something else. Both region are registers (or the emulation of
> it) so a 32bits aligned is enough, AFAICT.
>
> What purpose would 2^n aligned serve ? If it is safer, what's is the risk
> exactly ?

After second think, it should be fine if 4 bytes align.

Frank

>
> >
> > 	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));
> >
> > Frank
> >
> >>  	spad_size = 2 * spad_count * sizeof(u32);
> >>
> >> -	if (!align) {
> >> -		ctrl_size = roundup_pow_of_two(ctrl_size);
> >> -		spad_size = roundup_pow_of_two(spad_size);
> >> -	} else {
> >> -		ctrl_size = ALIGN(ctrl_size, align);
> >> -		spad_size = ALIGN(spad_size, align);
> >> -	}
> >> -
> >> -	if (!size)
> >> -		size = ctrl_size + spad_size;
> >> -	else if (size < ctrl_size + spad_size)
> >> -		return -EINVAL;
> >> -
> >> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
> >> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
> >> +				   barno, epc_features, 0);
> >>  	if (!base) {
> >>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
> >>  		return -ENOMEM;
> >>
> >> --
> >> 2.47.2
> >>
>
> --
> Jerome