| Message ID | 20200504094641.4963-7-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Align block pages queue to host page size | expand |
+Keith new email On 5/4/20 11:46 AM, Philippe Mathieu-Daudé wrote: > The NVMe device should not use target specific API. > Use memory_region_do_writeback() (which was introduced > in commit 61c490e25e0, after the NVMe emulated device > was added) to replace qemu_ram_writeback(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > RFC because I have no clue how dirty_log_mask works. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Beata Michalska <beata.michalska@linaro.org> > --- > hw/block/nvme.c | 4 +--- > hw/block/Makefile.objs | 2 +- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 9b453423cf..9b0ac0ea2a 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -46,7 +46,6 @@ > #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" > @@ -1207,8 +1206,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) > */ > if (addr == 0xE08 && > (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) { > - qemu_ram_writeback(n->pmrdev->mr.ram_block, > - 0, n->pmrdev->size); > + memory_region_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size); > } > memcpy(&val, ptr + addr, size); > } else { > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs > index 47960b5f0d..8855c22656 100644 > --- a/hw/block/Makefile.objs > +++ b/hw/block/Makefile.objs > @@ -13,6 +13,6 @@ 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 > +common-obj-$(CONFIG_NVME_PCI) += nvme.o > > obj-y += dataplane/ >
On Mon, May 04, 2020 at 05:36:22PM +0200, Philippe Mathieu-Daudé wrote: > +Keith new email > > On 5/4/20 11:46 AM, Philippe Mathieu-Daudé wrote: > > The NVMe device should not use target specific API. > > Use memory_region_do_writeback() (which was introduced > > in commit 61c490e25e0, after the NVMe emulated device > > was added) to replace qemu_ram_writeback(). > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > RFC because I have no clue how dirty_log_mask works. > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Beata Michalska <beata.michalska@linaro.org> > > --- > > hw/block/nvme.c | 4 +--- > > hw/block/Makefile.objs | 2 +- > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index 9b453423cf..9b0ac0ea2a 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -46,7 +46,6 @@ > > #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" > > @@ -1207,8 +1206,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) > > */ > > if (addr == 0xE08 && > > (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) { > > - qemu_ram_writeback(n->pmrdev->mr.ram_block, > > - 0, n->pmrdev->size); > > + memory_region_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size); qemu_ram_write() is being called because we need to msync or persist pmem here. The memory_region_do_writeback() API is not equivalent, its purpose is for dirty write logging (which we don't care about here because the writes themselves will already have been logged when the guest performed them). I think qemu_ram_writeback() should just be made common so that this code isn't target-specific. Maybe it should be renamed to qemu_ram_msync() to avoid confusion with dirty write APIs. Stefan
On 07/05/20 12:04, Stefan Hajnoczi wrote: >>> (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) { >>> - qemu_ram_writeback(n->pmrdev->mr.ram_block, >>> - 0, n->pmrdev->size); >>> + memory_region_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size); > qemu_ram_write() is being called because we need to msync or persist > pmem here. > > The memory_region_do_writeback() API is not equivalent, its purpose is > for dirty write logging (which we don't care about here because the > writes themselves will already have been logged when the guest performed > them). > > I think qemu_ram_writeback() should just be made common so that this > code isn't target-specific. Maybe it should be renamed to > qemu_ram_msync() to avoid confusion with dirty write APIs. Yes, we can add qemu_ram_msync and memory_region_msync. Paolo
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 9b453423cf..9b0ac0ea2a 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -46,7 +46,6 @@ #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" @@ -1207,8 +1206,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) */ if (addr == 0xE08 && (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) { - qemu_ram_writeback(n->pmrdev->mr.ram_block, - 0, n->pmrdev->size); + memory_region_do_writeback(&n->pmrdev->mr, 0, n->pmrdev->size); } memcpy(&val, ptr + addr, size); } else { diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs index 47960b5f0d..8855c22656 100644 --- a/hw/block/Makefile.objs +++ b/hw/block/Makefile.objs @@ -13,6 +13,6 @@ 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 +common-obj-$(CONFIG_NVME_PCI) += nvme.o obj-y += dataplane/
The NVMe device should not use target specific API. Use memory_region_do_writeback() (which was introduced in commit 61c490e25e0, after the NVMe emulated device was added) to replace qemu_ram_writeback(). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- RFC because I have no clue how dirty_log_mask works. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Beata Michalska <beata.michalska@linaro.org> --- hw/block/nvme.c | 4 +--- hw/block/Makefile.objs | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-)