From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method Date: Wed, 21 Sep 2016 13:30:47 +0800 Message-ID: <56c532d1-f6d0-b64c-3468-4401fa37a0ae@linux.intel.com> References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-2-git-send-email-guangrong.xiao@linux.intel.com> <20160920160757.3fdc2ce8@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org To: Igor Mammedov Return-path: Received: from mga11.intel.com ([192.55.52.93]:55006 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751550AbcIUFgs (ORCPT ); Wed, 21 Sep 2016 01:36:48 -0400 In-Reply-To: <20160920160757.3fdc2ce8@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/20/2016 10:07 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:03 +0800 > Xiao Guangrong wrote: > >> Currently, 'RLEN' is the totally buffer size written by QEMU and it is >> ACPI internally used only. The buffer size returned to guest should >> not include 'RLEN' itself > Do you see any errors in guest with this bug present? > It would be nice to put error messages here so that fix could be found > later just by searching git log and qemu-devel for errors user sees > in guest. > No, i did not see any error log in vm. I guess kernel nvdimm driver uses the buffer based on the 'length' field. I will improve the code to check whether the buffer size is matched with this field in vm. > >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/acpi/nvdimm.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index e486128..5454c0f 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev) >> >> result_size = aml_local(1); >> aml_append(method, aml_store(aml_name("RLEN"), result_size)); >> + /* RLEN is not included in the payload returned to guest. */ >> + aml_append(method, aml_subtract(result_size, aml_int(4), result_size)); > you can merge above store with subtract like this: > aml_subtract(aml_name("RLEN"), foo, result_size) Yes, it is better indeed. > > Style nit: try not to use magic numbers, > look at how RLEN is defined earlier, extract it into macro and reuse in both places Okay. > > >> aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)), > instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE > so it would obvious what's going on and rewrite following without intermediate store. > I agree. However, qemu does not implement multiply primitive, i'd make a separate patchset for these cleanups you suggested. >> result_size)); >> aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > aml_create_field(aml_name("ODAT"), > aml_int(0), > aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL), > "OBUF")) > > BTW: > dsm_out_buf_size is more descriptive than result_size Yes, indeed. > > also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right > instead of arg6 you should make/use local variable 'dsm_out_buf' Sorry, my typo. Will fix. > > As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back. > Currently I can't compile it back which mean it's really broken. > Good suggestion, i will try it. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40397) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmaDH-00014S-EW for qemu-devel@nongnu.org; Wed, 21 Sep 2016 01:36:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmaDD-0000cO-V0 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 01:36:55 -0400 Received: from mga06.intel.com ([134.134.136.31]:40033) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmaDD-0000bY-Ol for qemu-devel@nongnu.org; Wed, 21 Sep 2016 01:36:51 -0400 References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-2-git-send-email-guangrong.xiao@linux.intel.com> <20160920160757.3fdc2ce8@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <56c532d1-f6d0-b64c-3468-4401fa37a0ae@linux.intel.com> Date: Wed, 21 Sep 2016 13:30:47 +0800 MIME-Version: 1.0 In-Reply-To: <20160920160757.3fdc2ce8@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org On 09/20/2016 10:07 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:03 +0800 > Xiao Guangrong wrote: > >> Currently, 'RLEN' is the totally buffer size written by QEMU and it is >> ACPI internally used only. The buffer size returned to guest should >> not include 'RLEN' itself > Do you see any errors in guest with this bug present? > It would be nice to put error messages here so that fix could be found > later just by searching git log and qemu-devel for errors user sees > in guest. > No, i did not see any error log in vm. I guess kernel nvdimm driver uses the buffer based on the 'length' field. I will improve the code to check whether the buffer size is matched with this field in vm. > >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/acpi/nvdimm.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index e486128..5454c0f 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -863,6 +863,8 @@ static void nvdimm_build_common_dsm(Aml *dev) >> >> result_size = aml_local(1); >> aml_append(method, aml_store(aml_name("RLEN"), result_size)); >> + /* RLEN is not included in the payload returned to guest. */ >> + aml_append(method, aml_subtract(result_size, aml_int(4), result_size)); > you can merge above store with subtract like this: > aml_subtract(aml_name("RLEN"), foo, result_size) Yes, it is better indeed. > > Style nit: try not to use magic numbers, > look at how RLEN is defined earlier, extract it into macro and reuse in both places Okay. > > >> aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)), > instead of shiftleft, I'd suggest use here multiply operator and BITS_PER_BYTE > so it would obvious what's going on and rewrite following without intermediate store. > I agree. However, qemu does not implement multiply primitive, i'd make a separate patchset for these cleanups you suggested. >> result_size)); >> aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > aml_create_field(aml_name("ODAT"), > aml_int(0), > aml_multiply(result_size, aml_int(BITS_PER_BYTE), NULL), > "OBUF")) > > BTW: > dsm_out_buf_size is more descriptive than result_size Yes, indeed. > > also NCAL later uses Arg6 when method has only 5 arguments which doesn't seem right > instead of arg6 you should make/use local variable 'dsm_out_buf' Sorry, my typo. Will fix. > > As sanity check I'd suggest to extract nvdimm ssdt in guest, decompile and compile it back. > Currently I can't compile it back which mean it's really broken. > Good suggestion, i will try it.