Re: [Qemu-devel] [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

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

On 10/19/2015 03:06 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote:
>>> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
>>>> __DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)
>>>>
>>>> Function 0 is a query function. We do not support any function on root
>>>> device and only 3 functions are support for NVDIMM device,
>>>> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
>>>> DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
>>>> access device's Label Namespace
>>>>
>>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>>> ---
>>>>   hw/acpi/nvdimm.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 182 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>>>> index b211b8b..37fea1c 100644
>>>> --- a/hw/acpi/nvdimm.c
>>>> +++ b/hw/acpi/nvdimm.c
>>>> @@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
>>>>       return nvdimm_slot_to_spa_index(slot) + 1;
>>>>   }
>>>>
>>>> +static NVDIMMDevice
>>>> +*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
>>>> +{
>>>> +    for (; list; list = list->next) {
>>>> +        NVDIMMDevice *nvdimm = list->data;
>>>> +        int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
>>>> +                                           NULL);
>>>> +
>>>> +        if (nvdimm_slot_to_handle(slot) == handle) {
>>>> +            return nvdimm;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>>   /*
>>>>    * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
>>>>    * Structure
>>>> @@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray *table_offsets,
>>>>   /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
>>>>   #define NOTIFY_VALUE      0x99
>>>
>>> Again, please prefix everything consistently.
>>
>> Okay, will do. Sorry for i missed it.
>>
>>>
>>>>
>>>> +enum {
>>>> +    DSM_FUN_IMPLEMENTED = 0,
>>>> +
>>>> +    /* NVDIMM Root Device Functions */
>>>> +    DSM_ROOT_DEV_FUN_ARS_CAP = 1,
>>>> +    DSM_ROOT_DEV_FUN_ARS_START = 2,
>>>> +    DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
>>>> +
>>>> +    /* NVDIMM Device (non-root) Functions */
>>>> +    DSM_DEV_FUN_SMART = 1,
>>>> +    DSM_DEV_FUN_SMART_THRESHOLD = 2,
>>>> +    DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
>>>> +    DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
>>>> +    DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
>>>> +    DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
>>>> +    DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
>>>> +    DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
>>>> +    DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
>>>> +};
>>>
>>> Does FUN stand for "function"? FUNC or FN is probably better.
>>>
>>
>> Yes.
>>
>>> Please list exact names as they appear in spec so
>>> they can be searched for.
>>
>> The spec reference was at where this _FUN_ is used, eg:
>>
>> /*
>>   * Please refer to DSM specification 4.4.1 Get Namespace Label Size
>>   * (Function Index 4).
>>   *
>>   * It gets the size of Namespace Label data area and the max data size
>>   * that Get/Set Namespace Label Data functions can transfer.
>>   */
>> static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *out)
>>
>> I will follow your ‘single use’ comments below, these definitions will
>> be dropped, the code will be like this:
>>
>> switch (function) {
>> case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:
>
> If it's the same spec, you don't have to repeat it:
>
> /* Encode function according to DSM Spec rev 1.0 */
>> switch (function) {
>>      case 4 /* 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:
>
> same for chapter etc.

Okay.

>
>> nvdimm_dsm_func_label_size();
>> case ...
>> ...
>> };
>>
>>>
>>>
>>>
>>>> +
>>>> +enum {
>>>> +    /* Common return status codes. */
>>>> +    DSM_STATUS_SUCCESS = 0,                   /* Success */
>>>> +    DSM_STATUS_NOT_SUPPORTED = 1,             /* Not Supported */
>>>> +
>>>> +    /* NVDIMM Root Device _DSM function return status codes*/
>>>> +    DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,    /* Invalid Input Parameters */
>>>> +    DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
>>>> +                                                        Error */
>>>> +
>>>> +    /* NVDIMM Device (non-root) _DSM function return status codes*/
>>>> +    DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory Device */
>>>> +    DSM_DEV_STATUS_INVALID_PARAS = 3,         /* Invalid Input Parameters */
>>>> +    DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
>>>> +};
>>>> +
>>>> +/* Current revision supported by DSM specification is 1. */
>>>> +#define DSM_REVISION        (1)
>>>> +
>>>> +/*
>>>> + * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
>>>> + * Value Information:
>>>
>>> Drop "please refer to".
>>
>> Okay.
>>
>>>
>>>> + *   if set to zero, no functions are supported (other than function zero)
>>>> + *   for the specified UUID and Revision ID. If set to one, at least one
>>>> + *   additional function is supported.
>>>> + */
>>>> +
>>>> +/* do not support any function on root. */
>>>> +#define ROOT_SUPPORT_FUN     (0ULL)
>>>
>>> Needs a name that implies the comment somehow.
>>>
>>>> +#define DIMM_SUPPORT_FUN    ((1 << DSM_FUN_IMPLEMENTED)                   \
>>>> +                           | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)      \
>>>> +                           | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA)  \
>>>> +                           | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA))
>>>> +
>>>
>>> I think it's best to just drop these macros.
>>> There's a single point of use - just add a comment there
>>> explaining what does it mean.
>>
>> Okay. Good to me.
>>
>>> You will be able to drop all _FUN_ macros too.
>>
>> Yes, it's good for code reduction.
>>
>>>
>>>
>>>>   struct dsm_in {
>>>>       uint32_t handle;
>>>>       uint32_t revision;
>>>> @@ -420,6 +490,11 @@ struct dsm_in {
>>>>   } QEMU_PACKED;
>>>>   typedef struct dsm_in dsm_in;
>>>>
>>>> +struct cmd_out_implemented {
>>>> +    uint64_t cmd_list;
>>>> +};
>>>> +typedef struct cmd_out_implemented cmd_out_implemented;
>>>> +
>>>>   struct dsm_out {
>>>>       /* the size of buffer filled by QEMU. */
>>>>       uint32_t len;
>>>> @@ -434,12 +509,115 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
>>>>       return 0;
>>>>   }
>>>>
>>>> +static void nvdimm_dsm_write_status(GArray *out, uint32_t status)
>>>> +{
>>>> +    /* status locates in the first 4 bytes in the dsm memory. */
>>>
>>> located?
>>
>> Yes...
>>>
>>>> +    assert(!out->len);
>>>
>>>
>>> But dsm itself can be part of a bigger table.
>>> So don't do it.
>>
>> Okay, will drop it.
>>
>>>
>>>> +
>>>> +    status = cpu_to_le32(status);
>>>> +    g_array_append_vals(out, &status, sizeof(status));
>>>
>>> I think this should just use the (unfortunately named)
>>> build_append_int_noprefix. Same applied everywhere
>>> where you add single values.
>>
>> Okay, will use it instead.
>>
>>>
>>>> +}
>>>> +
>>>> +static void nvdimm_dsm_write_root(dsm_in *in, GArray *out)
>>>> +{
>>>> +    uint32_t status = DSM_STATUS_NOT_SUPPORTED;
>>>> +
>>>> +    /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */
>>>> +    if (in->function == DSM_FUN_IMPLEMENTED) {
>>>> +        uint64_t cmd_list = cpu_to_le64(ROOT_SUPPORT_FUN);
>>>
>>> see about about single use values.
>>>
>>
>> Yes, it is good to me, will follow it.
>>
>>>
>>>> +
>>>> +        g_array_append_vals(out, &cmd_list, sizeof(cmd_list));
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    nvdimm_debug("Return status %#x.\n", status);
>>>> +    nvdimm_dsm_write_status(out, status);
>>>> +}
>>>> +
>>>> +static void nvdimm_dsm_write_nvdimm(dsm_in *in, GArray *out)
>>>> +{
>>>> +    GSList *list = nvdimm_get_plugged_device_list();
>>>> +    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(list, in->handle);
>>>> +    uint32_t status = DSM_DEV_STATUS_NON_EXISTING_MEM_DEV;
>>>> +    uint64_t cmd_list;
>>>> +
>>>> +    if (!nvdimm) {
>>>> +        goto set_status_free;
>>>> +    }
>>>> +
>>>> +    switch (in->function) {
>>>> +    /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */
>>>> +    case DSM_FUN_IMPLEMENTED:
>>>> +        cmd_list = cpu_to_le64(DIMM_SUPPORT_FUN);
>>>> +        g_array_append_vals(out, &cmd_list, sizeof(cmd_list));
>>>> +        goto free;
>>>> +    default:
>>>> +        status = DSM_STATUS_NOT_SUPPORTED;
>>>> +    };
>>>> +
>>>> +set_status_free:
>>>> +    nvdimm_debug("Return status %#x.\n", status);
>>>> +    nvdimm_dsm_write_status(out, status);
>>>> +free:
>>>> +    g_slist_free(list);
>>>> +}
>>>> +
>>>>   static void
>>>>   nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>>>>   {
>>>> +    NVDIMMState *state = opaque;
>>>> +    MemoryRegion *dsm_ram_mr;
>>>> +    dsm_in *in;
>>>> +    GArray *out;
>>>> +    void *dsm_ram_addr;
>>>
>>>
>>> Why don't you give this the correct type? Will avoid need for casts.
>>
>> If it's defined as "dsm_out *", that will make "copy(in, dsm_ram_addr..)"
>> little strange.
>>
>> I will do it as your suggestion, and make a comment for the copy operation:
>> /*
>>   * The DSM memory is used for both OSPM saves its input parameter and QEMU
>>   * saves its output result.
>>   */
>
> Fix up the english here pls:
>>   * The DSM memory has two uses: OSPM saves its input parameter there, QEMU
>>   * uses it to save its output result.
>

Sorry for my English, will fix.

>
>>>
>>>> +
>>>>       if (val != NOTIFY_VALUE) {
>>>>           fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val);
>>>>       }
>>>> +
>>>> +    dsm_ram_mr = memory_region_find(&state->mr, getpagesize(),
>>>> +                                    getpagesize()).mr;
>>>> +    dsm_ram_addr = memory_region_get_ram_ptr(dsm_ram_mr);
>>>
>>>
>>> This needs a validity check for size.
>>
>> Okay, will add this:
>>
>> assert(memory_region_size(dsm_ram_mr) == getpagesize());
>
> No, the point is to make sure getpagesize is big enough to hold
> the structure.

Got it, will change it to:

/*
  * The DSM memory should be big enough to contain Input parameters and
  * output result.
  */
assert(memory_region_size(dsm_ram_mr) >= sizeof(dsm_in) &&
        memory_region_size(dsm_ram_mr) >= sizeof(dsm_out));

>
>>>
>>>> +
>>>> +    /*
>>>> +     * copy all input data to our local memory to avoid potential issue
>>>> +     * as the dsm memory is visible to guest.
>>>
>>> this comment doesn't help.
>>> pls replace "potential issue" with an explanation.
>>
>> Okay, will change the comment to:
>> /* As DSM memory is mapped to guest address space so that evil guest can change
>
> s/As/The/
> s/that evil/an evil/
>
>>   * its content while we are doing DSM emulation. Avoid it by copying DSM memory
>
> s/Avoid it/Avoid this/
>
>>   * to QEMU local memory
>>   */

Thanks for your fix.