On Wed, 21 Oct 2020 at 10:51, Greg Kroah-Hartman
<gre...@linuxfoundation.org> 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 <a...@kernel.org> wrote:
> > >
> > > On Wed, 21 Oct 2020 at 07:18, Greg Kroah-Hartman
> > > <gre...@linuxfoundation.org> 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);
