Re: [RFC PATCH 6/6] hw/block/nvme: Make device target agnostic

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]