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

[Xiao Guangrong]

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