Re: [Qemu-devel] [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED function

[Eduardo Habkost]

On Wed, Oct 14, 2015 at 10:50:40PM +0800, Xiao Guangrong wrote:
> On 10/14/2015 05:40 PM, Stefan Hajnoczi wrote:
> >On Sun, Oct 11, 2015 at 11:52:59AM +0800, Xiao Guangrong wrote:
> >>  static void dsm_write(void *opaque, hwaddr addr,
> >>                        uint64_t val, unsigned size)
> >>  {
> >>+    NVDIMMState *state = opaque;
> >>+    MemoryRegion *dsm_ram_mr;
> >>+    dsm_in *in;
> >>+    dsm_out *out;
> >>+    uint32_t revision, function, handle;
> >>+
> >>      if (val != NOTIFY_VALUE) {
> >>          fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val);
> >>      }
> >>+
> >>+    dsm_ram_mr = memory_region_find(&state->mr, state->page_size,
> >>+                                    state->page_size).mr;
> >>+    memory_region_unref(dsm_ram_mr);
> >>+    in = memory_region_get_ram_ptr(dsm_ram_mr);
> >
> >This looks suspicious.  Shouldn't the memory_region_unref(dsm_ram_mr)
> >happen after we're done using it?
> 
> This region is keep-alive during QEMU's running, it is okay.  The same
> style is applied to other codes, for example: line 208 in
> hw/s390x/sclp.c.

In sclp.c (assign_storage()), the memory region is never used after
memory_region_unref() is called. In unassign_storage(), sclp.c owns an
additional reference, grabbed by assign_storage().

-- 
Eduardo