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

[Xiao Guangrong]

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 <address@hidden>
> > ---
> >   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. */:
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.
 */
> > +
> >       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());

> > +
> > +    /*
> > +     * 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
 * its content while we are doing DSM emulation. Avoid it by copying DSM memory
 * to QEMU local memory
 */

> > +     */
> > +    in = g_malloc(memory_region_size(dsm_ram_mr));
> > +    memcpy(in, dsm_ram_addr, memory_region_size(dsm_ram_mr));
> > +
> > +    le32_to_cpus(&in->revision);
> > +    le32_to_cpus(&in->function);
> > +    le32_to_cpus(&in->handle);
> > +
> > +    nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
> > +                 in->handle, in->function);
> > +
> > +    out = g_array_new(false, true /* clear */, 1);
> > +
> > +    if (in->revision != DSM_REVISION) {
> > +        nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
> > +                      in->revision, DSM_REVISION);
> > +        nvdimm_dsm_write_status(out, DSM_STATUS_NOT_SUPPORTED);
> > +        goto exit;
> > +    }
> > +
> > +    /* Handle 0 is reserved for NVDIMM Root Device. */
> > +    if (!in->handle) {
> > +        nvdimm_dsm_write_root(in, out);
> > +        goto exit;
> > +    }
> > +
> > +    nvdimm_dsm_write_nvdimm(in, out);
> > +
> > +exit:
> > +    /* Write our output result to dsm memory. */
> > +    ((dsm_out *)dsm_ram_addr)->len = out->len;
> > +    memcpy(((dsm_out *)dsm_ram_addr)->data, out->data, out->len);
>
> This breaks migration as memory is not dirtied.
>
> address_space_write is generally preferable to change memory.

Good point, will fix.