Re: [PATCH RESEND v2] block/nvme: introduce PMR support from NVMe 1.4 spec

[Stefan Hajnoczi]

On Fri, Mar 06, 2020 at 03:38:53PM -0700, Andrzej Jakowski wrote:
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d28335cbf3..ff7e74d765 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -19,10 +19,14 @@
>   *      -drive file=<file>,if=none,id=<drive_id>
>   *      -device nvme,drive=<drive_id>,serial=<serial>,id=<id[optional]>, \
>   *              cmb_size_mb=<cmb_size_mb[optional]>, \
> + *              [pmr_file=<pmr_file_path>,] \
>   *              num_queues=<N[optional]>
>   *
>   * Note cmb_size_mb denotes size of CMB in MB. CMB is assumed to be at
>   * offset 0 in BAR2 and supports only WDS, RDS and SQS for now.
> + *
> + * Either cmb or pmr - due to limitation in avaialbe BAR indexes.

s/avaialbe/available/

> + * pmr_file file needs to be preallocated and power of two in size.

Why does it need to be preallocated?

>   */
>  
>  #include "qemu/osdep.h"
> @@ -1141,6 +1145,28 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, 
> uint64_t data,
>          NVME_GUEST_ERR(nvme_ub_mmiowr_cmbsz_readonly,
>                         "invalid write to read only CMBSZ, ignored");
>          return;
> +#ifndef _WIN32

This ifdef is a hint that the layering is not right.  QEMU device models
usually only implement the "frontend" device registers, interrupts, and
request processing logic.  The platform-specific host "backend"
(mmapping files, sending network packets, audio/graphics APIs, etc) is
implemented separately.

In the previous version I asked NVDIMM folks to review this patch and
suggest how to use the same HostMemoryBackend (see
include/sysemu/hostmem.h) that is already used for NVDIMM emulation.

That seems cleaner than baking platform-specific memory mapped file I/O
into hw/block/nvme.c, and it will also add a few features that this
patch does not have.

If NVDIMM folks don't respond to this email, would you be able to
research backends/hostmem*.c and try to integrate it?  If you feel lost
I can help but it will require me to spend time investigating how that
stuff works again :).

signature.asc
Description: PGP signature