Re: [Qemu-devel] [PATCH v7 20/35] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region

[Vladimir Sementsov-Ogievskiy]

On 03.11.2015 17:47, Xiao Guangrong wrote:
> On 11/03/2015 12:16 AM, Vladimir Sementsov-Ogievskiy wrote:
> > On 02.11.2015 18:06, Xiao Guangrong wrote:
> > > On 11/02/2015 10:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > > On 02.11.2015 16:08, Xiao Guangrong wrote:
> > > > > On 11/02/2015 08:19 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > On 02.11.2015 12:13, Xiao Guangrong wrote:
> > > > > > > Curretly, the memory region of backed memory is directly mapped to
> > > > > > > guest's address space, however, it is not true for nvdimm device
> > > > > > >
> > > > > > > This patch let dimm device realize this fact and use
> > > > > > > DIMMDeviceClass->get_memory_region method to get the mapped memory
> > > > > > > region
> > > > > > >
> > > > > > > Current code did not check the return value of get_memory_region 
> > > > > > > as it
> > > > > > > assumed the backend memory of pc-dimm is always properly 
> > > > > > > initialized,
> > > > > > > we make get_memory_region internally catch the case if something is
> > > > > > > wrong
> > > >
> > > > but here you call not pc-dimm's get_memory_region, but common 
> > > > ddc->get_memory_region, which may be
> > > > nvdimm or possibly other future dimm, so, why not check it here? 
> > > > And than pc_dimm_get_memory_region
> > > > may be left untouched (error_abort is ok, because errp is unused).
> > >
> > > Hmm, because 'here' is not the only place calling 
> > > ->get_memory_region, this method has
> > > multiple callers:
> > >
> > > $ git grep "\->get_memory_region"
> > > hw/i386/pc.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > hw/i386/pc.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > hw/mem/dimm.c:    mr = ddc->get_memory_region(dimm);
> > > hw/mem/nvdimm.c:    ddc->get_memory_region = nvdimm_get_memory_region;
> > > hw/mem/pc-dimm.c:    ddc->get_memory_region = 
> > > pc_dimm_get_memory_region;
> > > hw/ppc/spapr.c:    MemoryRegion *mr = ddc->get_memory_region(dimm);
> > >
> > > memory region validation is also done for NVDIMM in nvdimm device.
> > Ok, then it should be documented by a comment in dimm.h, where 
> > DIMMDeviceClass is defined, that this
> > function should not fail
>
> Okay, how about this comment:
>
>     /*
>      * get the memory region which will be mapped into guest's address
>      * space. It is called after dimm device realized so it is never
>      * failed.
>      */
>     MemoryRegion *(*get_memory_region)(DIMMDevice *dimm);

if you don't mind:
s/it is never failed/it should never fail and assumed to return valid 
not-NULL address

I'll ok with this if others don't mind, but personally I prefer explicit 
error handling for such functions.




--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.