Re: [PATCH] habanalabs: fix build warning

[Oded Gabbay]

On Fri, Apr 1, 2022 at 9:40 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Fri, Apr 1, 2022 at 6:16 AM Max Filippov <jcmvbkbc@xxxxxxxxx> wrote:
> >
> > allmodconfig build fails on ARCH=xtensa with the following message:
> >
> >   drivers/misc/habanalabs/common/memory.c:153:49: error: cast from pointer
> >         to integer of different size [-Werror=pointer-to-int-cast]
> >         (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
> >
> > Fix it by adding intermediate conversion to uintptr_t as in other places
> > in that driver.
> >
> > Fixes: e8458e20e0a3 ("habanalabs: make sure device mem alloc is page aligned")
> > Signed-off-by: Max Filippov <jcmvbkbc@xxxxxxxxx>
> > ---
> >  drivers/misc/habanalabs/common/memory.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/misc/habanalabs/common/memory.c b/drivers/misc/habanalabs/common/memory.c
> > index e008d82e4ba3..f0d373171d2a 100644
> > --- a/drivers/misc/habanalabs/common/memory.c
> > +++ b/drivers/misc/habanalabs/common/memory.c
> > @@ -150,9 +150,9 @@ static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args,
> >                 for (i = 0 ; i < num_pgs ; i++) {
> >                         if (is_power_of_2(page_size))
> >                                 phys_pg_pack->pages[i] =
> > -                                               (u64) gen_pool_dma_alloc_align(vm->dram_pg_pool,
> > -                                                                               page_size, NULL,
> > -                                                                               page_size);
> > +                                       (u64)(uintptr_t)gen_pool_dma_alloc_align(vm->dram_pg_pool,
> > +                                                                                page_size, NULL,
> > +                                                                                page_size);
> >                         else
> >                                 phys_pg_pack->pages[i] = (u64) gen_pool_alloc(vm->dram_pg_pool,
> >                                                                                 page_size);
>
> This addresses the warning, but I suspect there is still a problem in the code:
> The description of that member lists it as '@pages: the physical page array',
> but it is actually a kernel virtual address that gets passed to it. Since this
> is a 'u64' member, it is hard to tell what type it actually is.
>
> gen_pool_dma_alloc_align() returns both a virtual address and a dma (bus)
> address. The dma address is ignored here, which makes me wonder why
> this interface is used in the first place.
>
> I can see four possible things that may be going on here:
>
> - if the pages[] array is meant to be a kernel virtual address, it should be
>   changed from a 'u64' to a normal pointer, with the cast removed.
>
> - if the pages[] array is meant to be a physical address, as documented,
>   it should be assigned using virt_to_phys() on the pointer, with a warning
>   that this must not be used a as a dma address (which can easily get
>   confused with a phys address as the binary representation is often the
>   same in the absence of an iommu). In this case, it should also be
>   changed to a phys_addr_t.
>
> - if the pages[] array is meant to be a dma address, it should be changed
>   to a dma_addr_t, and passed as the third argument to
>   gen_pool_dma_alloc_align() in order to return the correct address.
>
> - if there is a 'u64' member that is used for two (or all three) of the above
>   depending on context, it should be replaced with either multiple
>   struct members or a union.
>
> Looking at other uses of the pages[] array, I see a dma_addr_t assigned
> to it in init_phys_pg_pack_from_userptr(), but map_phys_pg_pack() and
> alloc_sgt_from_device_pages appear to treat it as a cpu-physical phys_addr_t
> rather than a device address again.
>
>         Arnd

Hi,
We use gen_pool in this function to manage our device memory
allocations (this is why it is called alloc_device_memory).

Basically, we initialize the genpool with the total size of the device memory,
and each bit represents a page according to a fixed page size, which
is dependent on asic type.
The addresses represent the physical address of the device memory, as
our device sees them.
As these addresses are not accessible from the host, it is appropriate
to hold them in u64, imo.

For future asics which will support multiple page sizes, we need to
use the gen_pool_dma_alloc_align() variant,
because then we need the allocation to be aligned to the page size as
requested by the user per allocation.

We ignore the DMA address because this is device memory, not host memory.
Therefore, our device's dma engine addresses the memory using the
virtual memory addresses we assign to it in our device's MMU.

Having said that, I'm wondering whether gen_pool_first_fit_align() can
also work here, which might be less confusing.

Oded