From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52085)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1aiZlL-0000fI-3U
	for qemu-devel@nongnu.org; Tue, 22 Mar 2016 23:47:15 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1aiZlH-0007Ey-3J
	for qemu-devel@nongnu.org; Tue, 22 Mar 2016 23:47:15 -0400
Received: from mga04.intel.com ([192.55.52.120]:5680)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <guangrong.xiao@linux.intel.com>) id 1aiZlG-0007E1-Ta
	for qemu-devel@nongnu.org; Tue, 22 Mar 2016 23:47:11 -0400
References: <1458203581-59143-1-git-send-email-guangrong.xiao@linux.intel.com>
	<1458203581-59143-13-git-send-email-guangrong.xiao@linux.intel.com>
	<20160317105845.GD14062@stefanha-x1.localdomain>
From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Message-ID: <56F2118D.9030200@linux.intel.com>
Date: Wed, 23 Mar 2016 11:46:21 +0800
MIME-Version: 1.0
In-Reply-To: <20160317105845.GD14062@stefanha-x1.localdomain>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 12/15] nvdimm acpi: support Get Namespace
 Label Size function
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: Stefan Hajnoczi <stefanha@gmail.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, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net



On 03/17/2016 06:58 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 17, 2016 at 04:32:58PM +0800, Xiao Guangrong wrote:
>> -    /* No function except function 0 is supported yet. */
>> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
>> +    if (!nvdimm) {
>> +        return nvdimm_dsm_no_payload(1 /* Non-Existing Memory Device */,
>
> "Non-existing Memory Device" is 2 according to the spec.  1 would be
> "Not Supported".

Yes, indeed.

>
> Please define constants for all these magic values instead of putting
> literals into the code:
>
> enum {
>      NVDIMM_DSM_SUCCESS = 0,
>      NVDIMM_DSM_NOT_SUPPORTED = 1,
>      NVDIMM_DSM_NON_EXISTING_MEMORY_DEVICE = 2,
>      NVDIMM_DSM_INVALID_INPUT_PARAMETERS = 3,
>      NVDIMM_DSM_VENDOR_SPECIFIC_ERROR = 4,
> };

Er, it seems Michael much prefers the style which uses raw number which
is followed by a comment. :(

>
>> +                                     dsm_mem_addr);
>> +    }
>> +
>> +    /* Encode DSM function according to DSM Spec Rev1. */
>> +    switch (in->function) {
>> +    case 4 /* Get Namespace Label Size */:
>> +        if (nvdimm->reserve_label) {
>> +            nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
>> +        }
>> +        break;
>
> What is the expected return status code if the device has no labels?
>
> This function must write a return status code, otherwise the guest will
> read the out buffer and attempt to interpret uninitialized memory.
>

Yes, my fault, will fix it. Thanks you for pointing it out.