From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54710)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1ZtGnr-0005jd-12
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:13:50 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1ZtGnm-00023g-27
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:13:46 -0500
Received: from mga02.intel.com ([134.134.136.20]:52811)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1ZtGnl-00021j-Sv
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:13:41 -0500
References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com>
	<1446455617-129562-21-git-send-email-guangrong.xiao@linux.intel.com>
	<563754D5.2060401@virtuozzo.com> <56376042.4090002@linux.intel.com>
	<5637727C.8050205@virtuozzo.com>
From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Message-ID: <56377C10.9080803@linux.intel.com>
Date: Mon, 2 Nov 2015 23:06:56 +0800
MIME-Version: 1.0
In-Reply-To: <5637727C.8050205@virtuozzo.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v7 20/35] dimm: get mapped memory region
 from DIMMDeviceClass->get_memory_region
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>, pbonzini@redhat.com, imammedo@redhat.com
Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net



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.