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

[Andrzej Jakowski]

On 3/10/20 2:51 AM, Stefan Hajnoczi wrote:
> 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?

PMR file is mmaped into address space. If memory accesses are made outside of 
file then SIGBUS signal is raised. Preallocation requirement was introduced
to prevent this situation.

> 
>>   */
>>  
>>  #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.

Agree. I couldn't find QEMU backend ensuring persistence - thus decided to
go with mmap.

> 
> 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 :).
> 

Yes I can research this topic. Does HostMemoryBacked provide persistence?