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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Stefan Hajnoczi (stefanha@gmail.com) wrote:
> On Tue, Feb 18, 2020 at 03:48:11PM -0700, Andrzej Jakowski wrote:
> > This patch introduces support for PMR that has been defined as part of NVMe 1.4
> > spec. User can now specify a pmr_file which will be mmap'ed into qemu address
> > space and subsequently in PCI BAR 2. Guest OS can perform mmio read and writes
> > to the PMR region that will stay persistent accross system reboot.
> > 
> > Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
> > ---
> >  hw/block/nvme.c       | 145 ++++++++++++++++++++++++++++++++++-
> >  hw/block/nvme.h       |   5 ++
> >  hw/block/trace-events |   5 ++
> >  include/block/nvme.h  | 172 ++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 326 insertions(+), 1 deletion(-)
> 
> NVDIMM folks, please take a look.  There seems to be commonality here.
> 
> Can this use -object memory-backend-file instead of manually opening and
> mapping a file?
> 
> Also CCing David Gilbert because there is some similarity with the
> vhost-user-fs's DAX Window feature where QEMU mmaps regions of files
> into a BAR.

I guess the biggest difference here is that the read can have the side
effect; in my world I don't have to set read/write/endian ops - I just
map a chunk of memory and use memory_region_add_subregion, so all the
read/writes are native read/writes - I assume that would be a lot
faster - I guess it depends if NVME_PMRCAP_PMRWBM is something constant
you know early on; if you know that you don't need to do side effects in
the read you could do the same trick and avoid the IO ops altogether.


Isn't there also a requirement that BARs are powers of two? Wouldn't
you need to ensure the PMR file is a power of 2 in size?

Dave

> > @@ -1303,6 +1327,38 @@ static const MemoryRegionOps nvme_cmb_ops = {
> >      },
> >  };
> >  
> > +static void nvme_pmr_write(void *opaque, hwaddr addr, uint64_t data,
> > +    unsigned size)
> > +{
> > +    NvmeCtrl *n = (NvmeCtrl *)opaque;
> > +    stn_le_p(&n->pmrbuf[addr], size, data);
> > +}
> > +
> > +static uint64_t nvme_pmr_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    NvmeCtrl *n = (NvmeCtrl *)opaque;
> > +    if (!NVME_PMRCAP_PMRWBM(n->bar.pmrcap)) {
> > +        int ret;
> > +        ret = msync(n->pmrbuf, n->f_pmr_size, MS_SYNC);
> > +        if (!ret) {
> > +            NVME_GUEST_ERR(nvme_ub_mmiowr_pmrread_barrier,
> > +                       "error while persisting data");
> > +        }
> > +    }
> 
> Why is msync(2) done on memory loads instead of stores?


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK