| 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: <9b96cab4-e433-4752-a668-1d8ff262be2a@yunsilicon.com>
Date: Tue, 25 Feb 2025 10:34:24 +0800
From: "Xin Tian" <tianx@...silicon.com>
To: "Simon Horman" <horms@...nel.org>
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 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.
>
> ...
Powered by blists - more mailing lists