Re: [Qemu-devel] [PATCH 12/15] nvdimm acpi: support Get Namespace Label Size function

[Xiao Guangrong]

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.