Re: [PATCH v2 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 09/20/2016 10:07 PM, Igor Mammedov wrote:
> On Fri, 12 Aug 2016 14:54:03 +0800
> Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 <guangrong.xiao@linux.intel.com>
>> ---
>>  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.