[SeaBIOS] Re: [PATCHv2 3/6] nvme: Convert nvme_build_prpl() to nvme_prpl_xfer()

[Kevin O'Connor]

On Fri, Jan 21, 2022 at 11:48:45AM -0500, Kevin O'Connor wrote:
> Rename nvme_build_prpl() to nvme_prpl_xfer() and directly invoke
> nvme_io_xfer() or nvme_bounce_xfer() from that function.
> Signed-off-by: Kevin O'Connor
>
> src/hw/nvme-int.h |  1 -
>  src/hw/nvme.c     | 46 ++++++++++++++++++++--------------------------
>  2 files changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/src/hw/nvme-int.h b/src/hw/nvme-int.h
> index a4c1555..9564c17 100644
> --- a/src/hw/nvme-int.h
> +++ b/src/hw/nvme-int.h
> @@ -125,7 +125,6 @@ struct nvme_namespace {
>  /* Page List */
>  u32 prpl_len;
> void *prp1;
> u64 prpl[NVME_MAX_PRPL_ENTRIES];
>
> };
>
> diff --git a/src/hw/nvme.c b/src/hw/nvme.c
> index d656e9b..eee7d17 100644
> --- a/src/hw/nvme.c
> +++ b/src/hw/nvme.c
> @@ -498,10 +498,13 @@ static int nvme_add_prpl(struct nvme_namespace *ns, u64 base)
>      return 0;
>  }
>
> -static int nvme_build_prpl(struct nvme_namespace *ns, void *op_buf, u16 count)
> +// Transfer data using page list (if applicable)
> +static int
> +nvme_prpl_xfer(struct nvme_namespace *ns, u64 lba, void *buf, u16 count,
>           int write)
>
> {
>      int first_page = 1;
> u32 base = (long)op_buf;
>
> u32 base = (long)buf;
> s32 size;
>
> if (count > ns->max_req_size)
>
> @@ -511,31 +514,32 @@ static int nvme_build_prpl(struct nvme_namespace *ns, void *op_buf, u16 count)
>  size = count * ns->block_size;
>  /* Special case for transfers that fit into PRP1, but are unaligned */
> if (((size + (base & ~NVME_PAGE_MASK)) <= NVME_PAGE_SIZE)) {
>    ns->prp1 = op_buf;
>    return count;
> }
> if (((size + (base & ~NVME_PAGE_MASK)) <= NVME_PAGE_SIZE))
>    return nvme_io_xfer(ns, lba, buf, count, write);

Just as a "self review", I think this code path could result in
nvme_io_xfer() being called with a buffer that is not word aligned and
thus result in a DISK_RET_EBADTRACK.  It would seem using the bounce
buffer would be a better result.
>  /* Every request has to be page aligned */
>  if (base & ~NVME_PAGE_MASK)
>    return 0;
>    goto bounce;
>
> /* Make sure a full block fits into the last chunk */
>    if (size & (ns->block_size - 1ULL))
>    return 0;
>    goto bounce;

Also, it's not clear to me how this branch could ever be taken as size
is assigned a few lines up as "size = count * ns->block_size".

However, in this patch series I tried to reproduce the existing
behavior.  Since I don't have a good way for testing nvme, I'm not
planning to make functional changes for the above.

-Kevin