Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-04-08 Thread Kevin Wolf 
Am 30.03.2020 um 18:55 hat Keith Busch geschrieben:
> On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> > HostMemoryBackend.
> > pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> > NVMe
> > device. Guest OS can perform mmio read and writes to the PMR region that 
> > will stay
> > persistent across system reboot.
> 
> Looks pretty good to me.
> 
> Reviewed-by: Keith Busch 

Thanks, applied to the block-next branch for 5.1.

Kevin

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-31 Thread Andrzej Jakowski 
On 3/30/20 12:31 PM, Klaus Birkelund Jensen wrote:
> On Mar 31 03:13, Keith Busch wrote:
>> On Mon, Mar 30, 2020 at 08:07:32PM +0200, Klaus Birkelund Jensen wrote:
>>> On Mar 31 01:55, Keith Busch wrote:
 On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> HostMemoryBackend.
> pmrdev memory region will subsequently be exposed as PCI BAR 2 in 
> emulated NVMe
> device. Guest OS can perform mmio read and writes to the PMR region that 
> will stay
> persistent across system reboot.

 Looks pretty good to me.

 Reviewed-by: Keith Busch 

 For a possible future extention, it could be interesting to select the
 BAR for PMR dynamically, so that you could have CMB and PMR enabled on
 the same device.

>>>
>>> I thought about the same thing, but this would require disabling MSI-X
>>> right? Otherwise there is not enough room. AFAIU, the iomem takes up
>>> BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.
>>
>> That's the way the emulated device is currently set, but there's no
>> reason for CMB or MSIx to use an exlusive bar. Both of those can be
>> be offsets into BAR 0/1, for example. PMR is the only one that can't
>> share.
>>
> 
> Oh, I did not consider that at all. Thanks!
> 
Makes sense - I didn't realize that it is possible. Thx Keith! 
Let me propose to introduce it in a separate patch.

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-30 Thread Klaus Birkelund Jensen 
On Mar 31 03:13, Keith Busch wrote:
> On Mon, Mar 30, 2020 at 08:07:32PM +0200, Klaus Birkelund Jensen wrote:
> > On Mar 31 01:55, Keith Busch wrote:
> > > On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> > > > HostMemoryBackend.
> > > > pmrdev memory region will subsequently be exposed as PCI BAR 2 in 
> > > > emulated NVMe
> > > > device. Guest OS can perform mmio read and writes to the PMR region 
> > > > that will stay
> > > > persistent across system reboot.
> > > 
> > > Looks pretty good to me.
> > > 
> > > Reviewed-by: Keith Busch 
> > > 
> > > For a possible future extention, it could be interesting to select the
> > > BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> > > the same device.
> > > 
> > 
> > I thought about the same thing, but this would require disabling MSI-X
> > right? Otherwise there is not enough room. AFAIU, the iomem takes up
> > BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.
> 
> That's the way the emulated device is currently set, but there's no
> reason for CMB or MSIx to use an exlusive bar. Both of those can be
> be offsets into BAR 0/1, for example. PMR is the only one that can't
> share.
> 

Oh, I did not consider that at all. Thanks!

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-30 Thread Keith Busch 
On Mon, Mar 30, 2020 at 08:07:32PM +0200, Klaus Birkelund Jensen wrote:
> On Mar 31 01:55, Keith Busch wrote:
> > On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> > > HostMemoryBackend.
> > > pmrdev memory region will subsequently be exposed as PCI BAR 2 in 
> > > emulated NVMe
> > > device. Guest OS can perform mmio read and writes to the PMR region that 
> > > will stay
> > > persistent across system reboot.
> > 
> > Looks pretty good to me.
> > 
> > Reviewed-by: Keith Busch 
> > 
> > For a possible future extention, it could be interesting to select the
> > BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> > the same device.
> > 
> 
> I thought about the same thing, but this would require disabling MSI-X
> right? Otherwise there is not enough room. AFAIU, the iomem takes up
> BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.

That's the way the emulated device is currently set, but there's no
reason for CMB or MSIx to use an exlusive bar. Both of those can be
be offsets into BAR 0/1, for example. PMR is the only one that can't
share.

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-30 Thread Klaus Birkelund Jensen 
On Mar 31 01:55, Keith Busch wrote:
> On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> > HostMemoryBackend.
> > pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> > NVMe
> > device. Guest OS can perform mmio read and writes to the PMR region that 
> > will stay
> > persistent across system reboot.
> 
> Looks pretty good to me.
> 
> Reviewed-by: Keith Busch 
> 
> For a possible future extention, it could be interesting to select the
> BAR for PMR dynamically, so that you could have CMB and PMR enabled on
> the same device.
> 

I thought about the same thing, but this would require disabling MSI-X
right? Otherwise there is not enough room. AFAIU, the iomem takes up
BAR0 and BAR1, CMB (or PMR) uses BAR2 and BAR3 and MSI-X uses BAR4 or 5.

[PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-30 Thread Andrzej Jakowski 
This patch introduces support for PMR that has been defined as part of NVMe 1.4
spec. User can now specify a pmrdev option that should point to 
HostMemoryBackend.
pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated NVMe
device. Guest OS can perform mmio read and writes to the PMR region that will 
stay
persistent across system reboot.

Signed-off-by: Andrzej Jakowski 
Reviewed-by: Klaus Jensen 
Reviewed-by: Stefan Hajnoczi 
---
Changelog:

v4:
 - replaced qemu_msync() use with qemu_ram_writeback() to allow pmem_persist()
   or qemu_msync() be called depending on configuration [4] (Stefan)

 - rephrased comments to improve clarity and fixed code style issues [4]
   (Stefan, Klaus)

v3:
 - reworked PMR to use HostMemoryBackend instead of directly mapping PMR
   backend file into qemu [1] (Stefan)

v2:
 - provided support for Bit 1 from PMRWBM register instead of Bit 0 to ensure
   improved performance in virtualized environment [2] (Stefan)

 - added check if pmr size is power of two in size [3] (David)

 - addressed cross compilation build problems reported by CI environment

v1:
 - inital push of PMR emulation

[1]: 
https://lore.kernel.org/qemu-devel/20200306223853.37958-1-andrzej.jakow...@linux.intel.com/
[2]: 
https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf
[3]: 
https://lore.kernel.org/qemu-devel/20200218224811.30050-1-andrzej.jakow...@linux.intel.com/
[4]: 
https://lore.kernel.org/qemu-devel/20200318200303.11322-1-andrzej.jakow...@linux.intel.com/
 
---
Persistent Memory Region (PMR) is a new optional feature provided in NVMe 1.4
specification. This patch implements initial support for it in NVMe driver.
---
 hw/block/Makefile.objs |   2 +-
 hw/block/nvme.c| 109 ++
 hw/block/nvme.h|   2 +
 hw/block/trace-events  |   4 +
 include/block/nvme.h   | 172 +
 5 files changed, 288 insertions(+), 1 deletion(-)

diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index 4b4a2b338d..47960b5f0d 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -7,12 +7,12 @@ common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o
 common-obj-$(CONFIG_XEN) += xen-block.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
-common-obj-$(CONFIG_NVME_PCI) += nvme.o
 common-obj-$(CONFIG_SWIM) += swim.o
 
 common-obj-$(CONFIG_SH4) += tc58128.o
 
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
 obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
+obj-$(CONFIG_NVME_PCI) += nvme.o
 
 obj-y += dataplane/
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d28335cbf3..9b453423cf 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -19,10 +19,19 @@
  *  -drive file=,if=none,id=
  *  -device nvme,drive=,serial=,id=, \
  *  cmb_size_mb=, \
+ *  [pmrdev=,] \
  *  num_queues=
  *
  * 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.
+ *
+ * cmb_size_mb= and pmrdev= options are mutually exclusive due to limitation
+ * in available BAR's. cmb_size_mb= will take precedence over pmrdev= when
+ * both provided.
+ * Enabling pmr emulation can be achieved by pointing to memory-backend-file.
+ * For example:
+ * -object memory-backend-file,id=,share=on,mem-path=, \
+ *  size=  -device nvme,...,pmrdev=
  */
 
 #include "qemu/osdep.h"
@@ -35,7 +44,9 @@
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "sysemu/hostmem.h"
 #include "sysemu/block-backend.h"
+#include "exec/ram_addr.h"
 
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -1141,6 +1152,26 @@ 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;
+case 0xE00: /* PMRCAP */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrcap_readonly,
+   "invalid write to PMRCAP register, ignored");
+return;
+case 0xE04: /* TODO PMRCTL */
+break;
+case 0xE08: /* PMRSTS */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrsts_readonly,
+   "invalid write to PMRSTS register, ignored");
+return;
+case 0xE0C: /* PMREBS */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrebs_readonly,
+   "invalid write to PMREBS register, ignored");
+return;
+case 0xE10: /* PMRSWTP */
+NVME_GUEST_ERR(nvme_ub_mmiowr_pmrswtp_readonly,
+   "invalid write to PMRSWTP register, ignored");
+return;
+case 0xE14: /* TODO PMRMSC */
+ break;
 default:
 NVME_GUEST_ERR(nvme_ub_mmiowr_invalid,
"invalid MMIO write,"
@@ -1169,6 +1200,16 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr 
addr, unsigned size)
 }
 
 if (addr < sizeof(n->bar)) {
+/*
+

Re: [PATCH RESEND v4] nvme: introduce PMR support from NVMe 1.4 spec

2020-03-30 Thread Keith Busch 
On Mon, Mar 30, 2020 at 09:46:56AM -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 pmrdev option that should point to 
> HostMemoryBackend.
> pmrdev memory region will subsequently be exposed as PCI BAR 2 in emulated 
> NVMe
> device. Guest OS can perform mmio read and writes to the PMR region that will 
> stay
> persistent across system reboot.

Looks pretty good to me.

Reviewed-by: Keith Busch 

For a possible future extention, it could be interesting to select the
BAR for PMR dynamically, so that you could have CMB and PMR enabled on
the same device.