Re: [PATCH] firmware: gsmi: Drop the use of dma_pool_* API functions

[Ard Biesheuvel]

On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Oct 21, 2020 at 12:37:52AM -0700, Furquan Shaikh wrote:
> > On Tue, Oct 20, 2020 at 11:37 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 10:01:41PM -0700, Furquan Shaikh wrote:
> > > > > GSMI driver uses dma_pool_* API functions for buffer allocation
> > > > > because it requires that the SMI buffers are allocated within 32-bit
> > > > > physical address space. However, this does not work well with IOMMU
> > > > > since there is no real device and hence no domain associated with the
> > > > > device.
> > > > >
> > > > > Since this is not a real device, it does not require any device
> > > > > address(IOVA) for the buffer allocations. The only requirement is to
> > > > > ensure that the physical address allocated to the buffer is within
> > > > > 32-bit physical address space. This change allocates a page using
> > > > > `get_zeroed_page()` and passes in GFP_DMA32 flag to ensure that the
> > > > > page allocation is done in the DMA32 zone. All the buffer allocation
> > > > > requests for gsmi_buf are then satisfed using this pre-allocated page
> > > > > for the device.
> > > >
> > > > Are you sure that "GFP_DMA32" really does what you think it does?  A
> > > > "normal" call with GFP_KERNEL" will give you memory that is properly
> > > > dma-able.
> > > >
> > > > We should not be adding new GFP_DMA* users in the kernel in these days,
> > > > just call dma_alloc*() and you should be fine.
> > > >
> > >
> > > The point seems to be that this is not about DMA at all, and so there
> > > is no device associated with the DMA allocation.
> > >
> > > The other 'master' is the CPU running firmware in an execution mode
> > > where it can only access the bottom 4 GB of memory, and GFP_DMA32
> > > happens to allocate from a zone which is guaranteed to be accessible
> > > to the firmware.
> >
> > Ard captured the context and requirement perfectly. GFP_DMA32
> > satisfies the requirement for allocating memory from a zone which is
> > accessible to the firmware in SMI mode. This seems to be one of the
> > common ways  how other drivers and common code in the kernel currently
> > allocate physical memory below the 4G boundary. Hence, I used the same
> > mechanism in GSMI driver.
>
> Then can you please document this a bit better in the changelog,
> explaining why this is ok to use this obsolete api, and also in the code
> itself so that no one tries to clean it up in the future?
>

Wouldn't it be simpler to switch to a SLAB cache created with SLAB_CACHE_DMA32?


I.e., something like the below (whitespace mangling courtesy of gmail)

diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index 7d9367b22010..d932284f970c 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -85,7 +85,6 @@
 struct gsmi_buf {
        u8 *start;                      /* start of buffer */
        size_t length;                  /* length of buffer */
-       dma_addr_t handle;              /* dma allocation handle */
        u32 address;                    /* physical address of buffer */
 };

@@ -97,7 +96,7 @@ static struct gsmi_device {
        spinlock_t lock;                /* serialize access to SMIs */
        u16 smi_cmd;                    /* SMI command port */
        int handshake_type;             /* firmware handler interlock type */
-       struct dma_pool *dma_pool;      /* DMA buffer pool */
+       struct kmem_cache *mem_pool;    /* buffer pool */
 } gsmi_dev;

 /* Packed structures for communicating with the firmware */
@@ -157,8 +156,7 @@ static struct gsmi_buf *gsmi_buf_alloc(void)
        }

        /* allocate buffer in 32bit address space */
-       smibuf->start = dma_pool_alloc(gsmi_dev.dma_pool, GFP_KERNEL,
-                                      &smibuf->handle);
+       smibuf->start = kmem_cache_zalloc(gsmi_dev.mem_pool, GFP_KERNEL);
        if (!smibuf->start) {
                printk(KERN_ERR "gsmi: failed to allocate name buffer\n");
                kfree(smibuf);
@@ -176,8 +174,7 @@ static void gsmi_buf_free(struct gsmi_buf *smibuf)
 {
        if (smibuf) {
                if (smibuf->start)
-                       dma_pool_free(gsmi_dev.dma_pool, smibuf->start,
-                                     smibuf->handle);
+                       kmem_cache_free(gsmi_dev.mem_pool, smibuf->start);
                kfree(smibuf);
        }
 }
@@ -914,9 +911,10 @@ static __init int gsmi_init(void)
        spin_lock_init(&gsmi_dev.lock);

        ret = -ENOMEM;
-       gsmi_dev.dma_pool = dma_pool_create("gsmi", &gsmi_dev.pdev->dev,
-                                            GSMI_BUF_SIZE, GSMI_BUF_ALIGN, 0);
-       if (!gsmi_dev.dma_pool)
+       gsmi_dev.mem_pool = kmem_cache_create("gsmi", GSMI_BUF_SIZE,
+                                             GSMI_BUF_ALIGN, SLAB_CACHE_DMA32,
+                                             NULL);
+       if (!gsmi_dev.mem_pool)
                goto out_err;

        /*
@@ -1032,7 +1030,7 @@ static __init int gsmi_init(void)
        gsmi_buf_free(gsmi_dev.param_buf);
        gsmi_buf_free(gsmi_dev.data_buf);
        gsmi_buf_free(gsmi_dev.name_buf);
-       dma_pool_destroy(gsmi_dev.dma_pool);
+       kmem_cache_destroy(gsmi_dev.mem_pool);
        platform_device_unregister(gsmi_dev.pdev);
        pr_info("gsmi: failed to load: %d\n", ret);
 #ifdef CONFIG_PM
@@ -1057,7 +1055,7 @@ static void __exit gsmi_exit(void)
        gsmi_buf_free(gsmi_dev.param_buf);
        gsmi_buf_free(gsmi_dev.data_buf);
        gsmi_buf_free(gsmi_dev.name_buf);
-       dma_pool_destroy(gsmi_dev.dma_pool);
+       kmem_cache_destroy(gsmi_dev.mem_pool);
        platform_device_unregister(gsmi_dev.pdev);
 #ifdef CONFIG_PM
        platform_driver_unregister(&gsmi_driver_info);