[PATCH] nvme: clear the prp2 field of the nvme command.

[PATCH] nvme: clear the prp2 field of the nvme command.

[Lei Rao]

If the prp2 field is not filled in nvme_setup_prp_simple(), the prp2
field is garbage data. According to nvme spec, the prp2 is reserved if
the data transfer does not cross a memory page boundary. Writing a
reserved coded value into a controller property field produces undefined
results, so it needs to be cleared in nvme_setup_rw().

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index da55ce45ac70..332367b66fbe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -891,6 +891,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	cmnd->rw.reftag = 0;
 	cmnd->rw.apptag = 0;
 	cmnd->rw.appmask = 0;
+	cmnd->rw.dptr.prp2 = 0;
 
 	if (ns->ms) {
 		/*
-- 
2.34.1

Re: [PATCH] nvme: clear the prp2 field of the nvme command.

[Chaitanya Kulkarni]

On 11/28/22 17:47, Lei Rao wrote:
> If the prp2 field is not filled in nvme_setup_prp_simple(), the prp2
> field is garbage data. According to nvme spec, the prp2 is reserved if
> the data transfer does not cross a memory page boundary. Writing a
> reserved coded value into a controller property field produces undefined
> results, so it needs to be cleared in nvme_setup_rw().
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>

if it is reserved then controller shoule ignore this field, no ?

not sure if original author wanted to avoid an extra assignment
in the fast path with assumption that reserved fields should be
ignored if it is then we should avoid this, if not then looks good

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme: clear the prp2 field of the nvme command.

[Rao, Lei]

On 11/29/2022 12:16 PM, Chaitanya Kulkarni wrote:
> On 11/28/22 17:47, Lei Rao wrote:
>> If the prp2 field is not filled in nvme_setup_prp_simple(), the prp2
>> field is garbage data. According to nvme spec, the prp2 is reserved if
>> the data transfer does not cross a memory page boundary. Writing a
>> reserved coded value into a controller property field produces undefined
>> results, so it needs to be cleared in nvme_setup_rw().
>>
>> Signed-off-by: Lei Rao <lei.rao@intel.com>
> 
> if it is reserved then controller shoule ignore this field, no ?

It's feasible for the controller to ignore this field. But our controller has
stricter checks, and if prp2 is not used but has a value, some warnings will be
printed. According to the NVMe spec, it seems to write a reserved field produces
an undefined result, so maybe clearing it is better.

Thanks,
Lei

> 
> not sure if original author wanted to avoid an extra assignment
> in the fast path with assumption that reserved fields should be
> ignored if it is then we should avoid this, if not then looks good
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> -ck
> 

Re: [PATCH] nvme: clear the prp2 field of the nvme command.

[hch]

On Tue, Nov 29, 2022 at 12:50:57PM +0800, Rao, Lei wrote:
> stricter checks, and if prp2 is not used but has a value, some warnings will be
> printed. According to the NVMe spec, it seems to write a reserved field produces
> an undefined result, so maybe clearing it is better.

NVMe is a little weird about reserved fields settable by the host.
I don't think your controllers warnings are correct, but given that
the cost of clearing is very low I think we should clear it in Linux.
Please resend a patch with the clearing in the right place as suggested
in my previous mail.

Re: [PATCH] nvme: clear the prp2 field of the nvme command.

[Christoph Hellwig]

On Tue, Nov 29, 2022 at 09:47:11AM +0800, Lei Rao wrote:
> If the prp2 field is not filled in nvme_setup_prp_simple(), the prp2
> field is garbage data. According to nvme spec, the prp2 is reserved if
> the data transfer does not cross a memory page boundary. Writing a
> reserved coded value into a controller property field produces undefined
> results, so it needs to be cleared in nvme_setup_rw().

But this is not the right place to clear it, that needs to be done
in the place that sets up the PRPs, і.e. nvme_setup_prp_simple.

[PATCH] nvme: clear the prp2 field of the nvme command.

[Lei Rao]

If the prp2 field is not filled in nvme_setup_prp_simple(), the prp2
field is garbage data. According to nvme spec, the prp2 is reserved if
the data transfer does not cross a memory page boundary. Writing a
reserved coded value into a controller property field produces undefined
results, so it needs to be cleared in nvme_setup_prp_simple().

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f4335519399d..488ad7dabeb8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -797,6 +797,8 @@ static blk_status_t nvme_setup_prp_simple(struct nvme_dev *dev,
 	cmnd->dptr.prp1 = cpu_to_le64(iod->first_dma);
 	if (bv->bv_len > first_prp_len)
 		cmnd->dptr.prp2 = cpu_to_le64(iod->first_dma + first_prp_len);
+	else
+		cmnd->dptr.prp2 = 0;
 	return BLK_STS_OK;
 }
 
-- 
2.34.1

Re: [PATCH] nvme: clear the prp2 field of the nvme command.

[Christoph Hellwig]

Thanks,

I've applied the patch with a slightly update commit message to the
nvme-6.1 tree.