[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 9 Sep 2022 23:53:28 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Keith Busch <kbusch@...nel.org>
Cc: Serge Semin <Sergey.Semin@...kalelectronics.ru>,
Jonathan Derrick <jonathan.derrick@...el.com>,
Revanth Rajashekar <revanth.rajashekar@...el.com>,
Jens Axboe <axboe@...nel.dk>, Jens Axboe <axboe@...com>,
Christoph Hellwig <hch@....de>,
Sagi Grimberg <sagi@...mberg.me>,
Guenter Roeck <linux@...ck-us.net>,
Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
linux-nvme@...ts.infradead.org, linux-block@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] nvme-hwmon: Cache-line-align the NVME SMART
log-buffer
On Fri, Sep 09, 2022 at 01:42:34PM -0600, Keith Busch wrote:
> On Fri, Sep 09, 2022 at 10:19:15PM +0300, Serge Semin wrote:
> > Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
> >
> > Folks, I've thoroughly studied the whole NVME subsystem looking for
> > similar problems. Turned out there is one more place which may cause the
> > same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
> > the nvme_sec_submit() method. The rest of the buffers involved in the NVME
> > DMA are either allocated by kmalloc (must be cache-line-aligned by design)
> > or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
> > method implementation).
>
> What about user space addresses?
Reasonable question. Alas I haven't researched the user-space part as
much thorough. What I can say for sure that we haven't detected any
unaligned buffers passed to the DMA-mapping procedure other than the
ones denoted in this patch and in the next one. So to speak so far
none of the NVME-involved user-space buffers have had unaligned offset
in the physical address space. I have merged in the next patch in our
local kernel tree:
https://patchwork.linux-mips.org/project/linux-mips/patch/20161125184611.28396-3-paul.burton@imgtec.com/
So if an unaligned buffer was passed we would have immediately got it
detected.
> We can map those with cacheline offsets.
If we could do that easily it would have been great. But I don't see
an easy way out. AFAICS we'll need to fix the blk_rq_map_user_iov()
method so one would CPU-based copy the unaligned part of the buffer
and perform the DMA-required operations with the rest of it. Do you
have any better suggestion in mind?
>
> > ---
> > drivers/nvme/host/hwmon.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> > index 0a586d712920..94192ab7a02d 100644
> > --- a/drivers/nvme/host/hwmon.c
> > +++ b/drivers/nvme/host/hwmon.c
> > @@ -10,9 +10,10 @@
> >
> > #include "nvme.h"
> >
> > +/* DMA-noncoherent platforms require the cache-aligned buffers */
> > struct nvme_hwmon_data {
> > + struct nvme_smart_log log ____cacheline_aligned;
> > struct nvme_ctrl *ctrl;
> > - struct nvme_smart_log log;
>
> So this by chance happened to work before 52fde2c07da6 because the field
> started at a 4-byte offset on your arch?
Correct. The offset is 4-bytes indeed so the log-field base address is
4-bytes aligned. Due to that the bounce-buffer used to be used for the
NVME SMART log getting. Since the denoted commit the log-buffer have
been directly used for DMA, which has revealed the problem caused by the
cache-invalidation on the buffer mapping.
>
> The change looks good.
>
> Reviewed-by: Keith Busch <kbusch@...nel.org>
Thanks.
-Sergey
Powered by blists - more mailing lists