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

Message-ID: <1jldsiu18d.fsf@starbuckisacylon.baylibre.com>
Date: Wed, 02 Apr 2025 15:44:34 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Frank Li <Frank.li@....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 01 Apr 2025 at 10:55, Frank Li <Frank.li@....com> wrote:

> 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

Ok. Thanks for the feedback.

I think the same type of change should probably be applied to the NTB
endpoint function. It also tries to handle the alignment on its own, but
that's mixed up with msix doorbell things

I don't have the necessary HW to test that function so it would be a bit
risky for me to modify it, but it would be nice for the two endpoint
functions to be somehow aligned, especially since they share the same RC
side driver.

If anyone is able to help on this, that would be greatly appreciated :)

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

-- 
Jerome