Re: [PATCH v2 16/16] hw/block/nvme: remove explicit qsg/iov parameters

[Maxim Levitsky]

On Thu, 2020-07-30 at 00:06 +0200, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since nvme_map_prp always operate on the request-scoped qsg/iovs, just
> pass a single pointer to the NvmeRequest instead of two for each of the
> qsg and iov.
> 
> Suggested-by: Minwoo Im 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 55b1a68ced8c..aea8a8b6946c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -284,8 +284,8 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList 
> *qsg, QEMUIOVector *iov,
>  return NVME_SUCCESS;
>  }
>  
> -static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t 
> prp1,
> - uint64_t prp2, uint32_t len, NvmeCtrl *n)
> +static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
> + uint32_t len, NvmeRequest *req)
>  {
>  hwaddr trans_len = n->page_size - (prp1 % n->page_size);
>  trans_len = MIN(len, trans_len);
> @@ -293,6 +293,9 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  uint16_t status;
>  bool prp_list_in_cmb = false;
>  
> +QEMUSGList *qsg = >qsg;
> +QEMUIOVector *iov = >iov;
> +
>  trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
>  
>  if (unlikely(!prp1)) {
> @@ -386,7 +389,7 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
> uint32_t len,
>  {
>  uint16_t status = NVME_SUCCESS;
>  
> -status = nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
> +status = nvme_map_prp(n, prp1, prp2, len, req);
>  if (status) {
>  return status;
>  }
> @@ -431,7 +434,7 @@ static uint16_t nvme_map_dptr(NvmeCtrl *n, size_t len, 
> NvmeRequest *req)
>  uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
>  uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>  
> -return nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
> +return nvme_map_prp(n, prp1, prp2, len, req);
>  }
>  
>  static void nvme_post_cqes(void *opaque)
Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH v2 16/16] hw/block/nvme: remove explicit qsg/iov parameters

[Minwoo Im]

On 20-07-30 00:06:38, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Since nvme_map_prp always operate on the request-scoped qsg/iovs, just
> pass a single pointer to the NvmeRequest instead of two for each of the
> qsg and iov.
> 
> Suggested-by: Minwoo Im 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 55b1a68ced8c..aea8a8b6946c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -284,8 +284,8 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList 
> *qsg, QEMUIOVector *iov,
>  return NVME_SUCCESS;
>  }
>  
> -static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t 
> prp1,
> - uint64_t prp2, uint32_t len, NvmeCtrl *n)
> +static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
> + uint32_t len, NvmeRequest *req)
>  {
>  hwaddr trans_len = n->page_size - (prp1 % n->page_size);
>  trans_len = MIN(len, trans_len);
> @@ -293,6 +293,9 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, 
> QEMUIOVector *iov, uint64_t prp1,
>  uint16_t status;
>  bool prp_list_in_cmb = false;
>  
> +QEMUSGList *qsg = >qsg;
> +QEMUIOVector *iov = >iov;
> +
>  trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
>  
>  if (unlikely(!prp1)) {
> @@ -386,7 +389,7 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
> uint32_t len,
>  {
>  uint16_t status = NVME_SUCCESS;
>  
> -status = nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
> +status = nvme_map_prp(n, prp1, prp2, len, req);
>  if (status) {
>  return status;
>  }
> @@ -431,7 +434,7 @@ static uint16_t nvme_map_dptr(NvmeCtrl *n, size_t len, 
> NvmeRequest *req)
>  uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
>  uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>  
> -return nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
> +return nvme_map_prp(n, prp1, prp2, len, req);
>  }
>  
>  static void nvme_post_cqes(void *opaque)


This looks better to have qsg and iov in the NvmeRequest.

Reviewed-by: Minwoo Im 

[PATCH v2 16/16] hw/block/nvme: remove explicit qsg/iov parameters

[Klaus Jensen]

From: Klaus Jensen 

Since nvme_map_prp always operate on the request-scoped qsg/iovs, just
pass a single pointer to the NvmeRequest instead of two for each of the
qsg and iov.

Suggested-by: Minwoo Im 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 55b1a68ced8c..aea8a8b6946c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -284,8 +284,8 @@ static uint16_t nvme_map_addr(NvmeCtrl *n, QEMUSGList *qsg, 
QEMUIOVector *iov,
 return NVME_SUCCESS;
 }
 
-static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
- uint64_t prp2, uint32_t len, NvmeCtrl *n)
+static uint16_t nvme_map_prp(NvmeCtrl *n, uint64_t prp1, uint64_t prp2,
+ uint32_t len, NvmeRequest *req)
 {
 hwaddr trans_len = n->page_size - (prp1 % n->page_size);
 trans_len = MIN(len, trans_len);
@@ -293,6 +293,9 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector 
*iov, uint64_t prp1,
 uint16_t status;
 bool prp_list_in_cmb = false;
 
+QEMUSGList *qsg = >qsg;
+QEMUIOVector *iov = >iov;
+
 trace_pci_nvme_map_prp(trans_len, len, prp1, prp2, num_prps);
 
 if (unlikely(!prp1)) {
@@ -386,7 +389,7 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, 
uint32_t len,
 {
 uint16_t status = NVME_SUCCESS;
 
-status = nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
+status = nvme_map_prp(n, prp1, prp2, len, req);
 if (status) {
 return status;
 }
@@ -431,7 +434,7 @@ static uint16_t nvme_map_dptr(NvmeCtrl *n, size_t len, 
NvmeRequest *req)
 uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
 uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
 
-return nvme_map_prp(>qsg, >iov, prp1, prp2, len, n);
+return nvme_map_prp(n, prp1, prp2, len, req);
 }
 
 static void nvme_post_cqes(void *opaque)
-- 
2.27.0