| 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
| ||
|
Message-ID: <20250225102208.GS1615191@kernel.org>
Date: Tue, 25 Feb 2025 10:22:08 +0000
From: Simon Horman <horms@...nel.org>
To: Xin Tian <tianx@...silicon.com>
Cc: netdev@...r.kernel.org, leon@...nel.org, andrew+netdev@...n.ch,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
davem@...emloft.net, jeff.johnson@....qualcomm.com,
przemyslaw.kitszel@...el.com, weihg@...silicon.com,
wanry@...silicon.com, parthiban.veerasooran@...rochip.com,
masahiroy@...nel.org
Subject: Re: [PATCH v4 05/14] net-next/yunsilicon: Add eq and alloc
On Tue, Feb 25, 2025 at 10:34:24AM +0800, Xin Tian wrote:
> On 2025/2/25 2:58, Simon Horman wrote:
> > On Thu, Feb 20, 2025 at 11:35:26PM +0800, tianx wrote:
> >> On 2025/2/19 1:10, Simon Horman wrote:
> >>> On Thu, Feb 13, 2025 at 05:14:14PM +0800, Xin Tian wrote:
> > ...
> >
> >>>> diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/alloc.c b/drivers/net/ethernet/yunsilicon/xsc/pci/alloc.c
> >>> ...
> >>>
> >>>> +/* Handling for queue buffers -- we allocate a bunch of memory and
> >>>> + * register it in a memory region at HCA virtual address 0. If the
> >>>> + * requested size is > max_direct, we split the allocation into
> >>>> + * multiple pages, so we don't require too much contiguous memory.
> >>>> + */
> >>> I can't help but think there is an existing API to handle this.
> >> failed to find one
> > Yes, me neither.
> >
> >>>> +int xsc_buf_alloc(struct xsc_core_device *xdev, int size, int max_direct,
> >>> I think unsigned long would be slightly better types for size and max_direct.
> >> yes, will modify
> >>>> + struct xsc_buf *buf)
> >>>> +{
> >>>> + dma_addr_t t;
> >>>> +
> >>>> + buf->size = size;
> >>>> + if (size <= max_direct) {
> >>>> + buf->nbufs = 1;
> >>>> + buf->npages = 1;
> >>>> + buf->page_shift = get_order(size) + PAGE_SHIFT;
> >>>> + buf->direct.buf = dma_alloc_coherent(&xdev->pdev->dev,
> >>>> + size,
> >>>> + &t,
> >>>> + GFP_KERNEL | __GFP_ZERO);
> >>>> + if (!buf->direct.buf)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + buf->direct.map = t;
> >>>> +
> >>>> + while (t & ((1 << buf->page_shift) - 1)) {
> >>> I think GENMASK() can be used here.
> >> ok
> >>>> + --buf->page_shift;
> >>>> + buf->npages *= 2;
> >>>> + }
> >>>> + } else {
> >>>> + int i;
> >>>> +
> >>>> + buf->direct.buf = NULL;
> >>>> + buf->nbufs = (size + PAGE_SIZE - 1) / PAGE_SIZE;
> >>> I think this is open-coding DIV_ROUND_UP
> >> right, I'll change
> >>>> + buf->npages = buf->nbufs;
> >>>> + buf->page_shift = PAGE_SHIFT;
> >>>> + buf->page_list = kcalloc(buf->nbufs, sizeof(*buf->page_list),
> >>>> + GFP_KERNEL);
> >>>> + if (!buf->page_list)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + for (i = 0; i < buf->nbufs; i++) {
> >>>> + buf->page_list[i].buf =
> >>>> + dma_alloc_coherent(&xdev->pdev->dev, PAGE_SIZE,
> >>>> + &t, GFP_KERNEL | __GFP_ZERO);
> >>>> + if (!buf->page_list[i].buf)
> >>>> + goto err_free;
> >>>> +
> >>>> + buf->page_list[i].map = t;
> >>>> + }
> >>>> +
> >>>> + if (BITS_PER_LONG == 64) {
> >>>> + struct page **pages;
> >>>> +
> >>>> + pages = kmalloc_array(buf->nbufs, sizeof(*pages),
> >>>> + GFP_KERNEL);
> >>>> + if (!pages)
> >>>> + goto err_free;
> >>>> + for (i = 0; i < buf->nbufs; i++) {
> >>>> + void *addr = buf->page_list[i].buf;
> >>>> +
> >>>> + if (is_vmalloc_addr(addr))
> >>>> + pages[i] = vmalloc_to_page(addr);
> >>>> + else
> >>>> + pages[i] = virt_to_page(addr);
> >>>> + }
> >>>> + buf->direct.buf = vmap(pages, buf->nbufs,
> >>>> + VM_MAP, PAGE_KERNEL);
> >>>> + kfree(pages);
> >>>> + if (!buf->direct.buf)
> >>>> + goto err_free;
> >>>> + }
> >>> I think some explanation is warranted of why the above is relevant
> >>> only when BITS_PER_LONG == 64.
> >> Some strange historical reasons, and no need for the check now. I'll
> >> clean this up
> > Thanks.
> >
> > If you do need 64bit only logic, then perhaps it can be moved to a
> > separate function. It could guard code using something like this.
> >
> > int some_func(struct xsc_buf *buf)
> > {
> > if (!IS_ENABLED(CONFIG_64BIT))
> > return 0;
> >
> > ...
> > }
> >
> > Or if that is not possible, something like this:
> >
> > #ifdef CONFIG_64BIT
> > int some_func(struct xsc_buf *buf)
> > {
> > ...
> > }
> > #else /* CONFIG_64BIT */
> > int some_func(struct xsc_buf *buf) { return 0; }
> > #fi /* CONFIG_64BIT */
> >
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +
> >>>> +err_free:
> >>>> + xsc_buf_free(xdev, buf);
> >>>> +
> >>>> + return -ENOMEM;
> >>>> +}
> >>> ...
> >>>
> >>>> +void xsc_fill_page_array(struct xsc_buf *buf, __be64 *pas, int npages)
> >>> As per my comment on unsigned long in my response to another patch,
> >>> I think npages can be unsigned long.
> >> ok
> >>>> +{
> >>>> + int shift = PAGE_SHIFT - PAGE_SHIFT_4K;
> >>>> + int mask = (1 << shift) - 1;
> >>> Likewise, I think that mask should be an unsigned long.
> >>> Or, both shift and mask could be #defines, as they are compile-time
> >>> constants.
> >>>
> >>> Also, mask can be generated using GENMASK, e.g.
> >>>
> >>> #define XSC_PAGE_ARRAY_MASK GENMASK(PAGE_SHIFT, PAGE_SHIFT_4K)
> >>> #define XSC_PAGE_ARRAY_SHIFT (PAGE_SHIFT - PAGE_SHIFT_4K)
> >>>
> >>> And I note, in the (common) case of 4k pages, that both shift and mask are 0.
> >> Thank you for the suggestion, but that's not quite the case here. The
> >> |shift| and |mask| are not used to extract fields from data. Instead,
> >> they are part of a calculation. In |xsc_buf_alloc|, we allocate the
> >> buffer based on the system's page size. However, in this function, we
> >> need to break each page in the |buflist| into 4KB chunks, populate the
> >> |pas| array with the corresponding DMA addresses, and then map them to
> >> hardware.
> >>
> >> The |shift| is calculated as |PAGE_SHIFT - PAGE_SHIFT_4K|, allowing us
> >> to convert the 4KB chunk index (|i|) to the corresponding page index in
> >> |buflist| with |i >> shift|. The |i & mask| gives us the offset of the
> >> current 4KB chunk within the page, and by applying |((i & mask) <<
> >> PAGE_SHIFT_4K)|, we can compute the offset of that chunk within the page.
> >>
> >> I hope this makes things clearer!
> > Thanks, that is clear.
> >
> > I do still think that the shift and mask could
> > be compile-time constants rather than local variables.
> > And it does seem to me that GENMASK can be used to generate the mask.
>
> Hi, Simon,
>
> Assuming we use GENMASK, the mask should be defined as GENMASK(shift -
> 1, 0).
>
> When the system page size is 4K, shift will be 0, which will cause an error.
Understood, so much for that idea.
Powered by blists - more mailing lists