* [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset
[not found] <CGME20210408162750epcas5p1f093ab42779ab250fbcb672a41455a30@epcas5p1.samsung.com>
@ 2021-04-08 16:23 ` Padmakar Kalghatgi
2021-04-08 19:20 ` Klaus Jensen
2021-04-08 21:38 ` Keith Busch
0 siblings, 2 replies; 4+ messages in thread
From: Padmakar Kalghatgi @ 2021-04-08 16:23 UTC (permalink / raw)
To: qemu-devel
Cc: fam, kwolf, padmakar, qemu-block, mreitz, its, stefanha, kbusch
From: padmakar <p.kalghatgi@samsung.com>
nvme_map_prp needs to calculate the number of list entries based on the
offset value. For the subsequent PRP2 list, need to ensure the number of
entries is within the MAX number of PRP entries for a page.
Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
---
hw/block/nvme.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d439e44..efb7368 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -577,7 +577,12 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
uint32_t nents, prp_trans;
int i = 0;
- nents = (len + n->page_size - 1) >> n->page_bits;
+ /*
+ * The first PRP list entry, pointed by PRP2 can contain
+ * offsets. Hence, we need calculate the no of entries in
+ * prp2 based on the offset it has.
+ */
+ nents = (n->page_size - (prp2 % n->page_size)) >> 3;
prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
if (ret) {
@@ -588,7 +593,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
while (len != 0) {
uint64_t prp_ent = le64_to_cpu(prp_list[i]);
- if (i == n->max_prp_ents - 1 && len > n->page_size) {
+ if (i == nents - 1 && len > n->page_size) {
if (unlikely(prp_ent & (n->page_size - 1))) {
trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
status = NVME_INVALID_PRP_OFFSET | NVME_DNR;
@@ -597,7 +602,8 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
i = 0;
nents = (len + n->page_size - 1) >> n->page_bits;
- prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
+ nents = MIN(nents, n->max_prp_ents);
+ prp_trans = nents * sizeof(uint64_t);
ret = nvme_addr_read(n, prp_ent, (void *)prp_list,
prp_trans);
if (ret) {
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset
2021-04-08 16:23 ` [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset Padmakar Kalghatgi
@ 2021-04-08 19:20 ` Klaus Jensen
2021-04-08 21:38 ` Keith Busch
1 sibling, 0 replies; 4+ messages in thread
From: Klaus Jensen @ 2021-04-08 19:20 UTC (permalink / raw)
To: Padmakar Kalghatgi
Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch
[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]
On Apr 8 21:53, Padmakar Kalghatgi wrote:
>From: padmakar <p.kalghatgi@samsung.com>
>
>nvme_map_prp needs to calculate the number of list entries based on the
>offset value. For the subsequent PRP2 list, need to ensure the number of
>entries is within the MAX number of PRP entries for a page.
>
>Signed-off-by: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
>---
> hw/block/nvme.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index d439e44..efb7368 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -577,7 +577,12 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
> uint32_t nents, prp_trans;
> int i = 0;
>
>- nents = (len + n->page_size - 1) >> n->page_bits;
>+ /*
>+ * The first PRP list entry, pointed by PRP2 can contain
>+ * offsets. Hence, we need calculate the no of entries in
>+ * prp2 based on the offset it has.
>+ */
>+ nents = (n->page_size - (prp2 % n->page_size)) >> 3;
> prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
> ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
> if (ret) {
>@@ -588,7 +593,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
> while (len != 0) {
> uint64_t prp_ent = le64_to_cpu(prp_list[i]);
>
>- if (i == n->max_prp_ents - 1 && len > n->page_size) {
>+ if (i == nents - 1 && len > n->page_size) {
> if (unlikely(prp_ent & (n->page_size - 1))) {
> trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
> status = NVME_INVALID_PRP_OFFSET | NVME_DNR;
>@@ -597,7 +602,8 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
>
> i = 0;
> nents = (len + n->page_size - 1) >> n->page_bits;
>- prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
>+ nents = MIN(nents, n->max_prp_ents);
>+ prp_trans = nents * sizeof(uint64_t);
> ret = nvme_addr_read(n, prp_ent, (void *)prp_list,
> prp_trans);
> if (ret) {
>--
>2.7.0.windows.1
>
>
LGTM.
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset
2021-04-08 16:23 ` [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset Padmakar Kalghatgi
2021-04-08 19:20 ` Klaus Jensen
@ 2021-04-08 21:38 ` Keith Busch
2021-04-09 7:00 ` Klaus Jensen
1 sibling, 1 reply; 4+ messages in thread
From: Keith Busch @ 2021-04-08 21:38 UTC (permalink / raw)
To: Padmakar Kalghatgi
Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, its
On Thu, Apr 08, 2021 at 09:53:13PM +0530, Padmakar Kalghatgi wrote:
> + /*
> + * The first PRP list entry, pointed by PRP2 can contain
> + * offsets. Hence, we need calculate the no of entries in
> + * prp2 based on the offset it has.
> + */
This comment has some unnecessary spacing at the beginning.
> + nents = (n->page_size - (prp2 % n->page_size)) >> 3;
page_size is a always a power of two, so let's replace the costly modulo
with:
nents = (n->page_size - (prp2 & (n->page_size - 1))) >> 3;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset
2021-04-08 21:38 ` Keith Busch
@ 2021-04-09 7:00 ` Klaus Jensen
0 siblings, 0 replies; 4+ messages in thread
From: Klaus Jensen @ 2021-04-09 7:00 UTC (permalink / raw)
To: Keith Busch
Cc: fam, kwolf, stefanha, qemu-block, qemu-devel, mreitz, Padmakar Kalghatgi
[-- Attachment #1: Type: text/plain, Size: 687 bytes --]
On Apr 9 06:38, Keith Busch wrote:
>On Thu, Apr 08, 2021 at 09:53:13PM +0530, Padmakar Kalghatgi wrote:
>> + /*
>> + * The first PRP list entry, pointed by PRP2 can contain
>> + * offsets. Hence, we need calculate the no of entries in
>> + * prp2 based on the offset it has.
>> + */
>
>This comment has some unnecessary spacing at the beginning.
>
>> + nents = (n->page_size - (prp2 % n->page_size)) >> 3;
>
>page_size is a always a power of two, so let's replace the costly modulo
>with:
>
> nents = (n->page_size - (prp2 & (n->page_size - 1))) >> 3;
>
Good point.
I'll fix both issues and queue the patch.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-09 7:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20210408162750epcas5p1f093ab42779ab250fbcb672a41455a30@epcas5p1.samsung.com>
2021-04-08 16:23 ` [PATCH] hw/block/nvme: map prp fix if prp2 contains non-zero offset Padmakar Kalghatgi
2021-04-08 19:20 ` Klaus Jensen
2021-04-08 21:38 ` Keith Busch
2021-04-09 7:00 ` Klaus Jensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.