Re: [Qemu-devel] [PATCH v5 3/5] nvdimm acpi: let qemu handle _DSM method

[Xiao Guangrong]

On 03/03/2016 09:23 PM, Michael S. Tsirkin wrote:

> > +struct NvdimmDsmIn {
> > +    uint32_t handle;
> > +    uint32_t revision;
> > +    uint32_t function;
> > +   /* the remaining size in the page is used by arg3. */
>
> Pls fix indentation so /* aligns with union.

Good eyes.

Surprise me that checkpatch did not report error/warning. Sorry for the
careless typo and will fix it.

> > +    union {
> > +        uint8_t arg3[0];
> > +    };
> > +} QEMU_PACKED;
> > +typedef struct NvdimmDsmIn NvdimmDsmIn;
> > +
> > +struct NvdimmDsmOut {
> > +    /* the size of buffer filled by QEMU. */
> > +    uint32_t len;
> > +    uint8_t data[0];
> > +} QEMU_PACKED;
> > +typedef struct NvdimmDsmOut NvdimmDsmOut;
> > +
> >   static uint64_t
> >   nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> >   {
> > @@ -411,11 +429,18 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, 
> > MemoryRegion *io,
> >
> >   static void nvdimm_build_common_dsm(Aml *dev)
> >   {
> > -    Aml *method, *ifctx, *function;
> > +    Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
> >       uint8_t byte_list[1];
> >
> > -    method = aml_method(NVDIMM_COMMON_DSM, 4, AML_NOTSERIALIZED);
> > +    method = aml_method(NVDIMM_COMMON_DSM, 4, AML_SERIALIZED);
> >       function = aml_arg(2);
> > +    dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
> > +
> > +    /*
> > +     * do not support any method if DSM memory address has not been
> > +     * patched.
> > +     */
>
> I am confused. When can this happen?

If BIOS can not allocate memory due to deficiency memory?

It's possible if the same approach is used by other device in the future...

> > +    unpatched = aml_if(aml_equal(dsm_mem, aml_int(0x0)));
> >
> >       /*
> >        * function 0 is called to inquire what functions are supported by
> > @@ -424,12 +449,36 @@ static void nvdimm_build_common_dsm(Aml *dev)
> >       ifctx = aml_if(aml_equal(function, aml_int(0)));
> >       byte_list[0] = 0 /* No function Supported */;
> >       aml_append(ifctx, aml_return(aml_buffer(1, byte_list)));
> > -    aml_append(method, ifctx);
> > +    aml_append(unpatched, ifctx);
> >
> >       /* No function is supported yet. */
> >       byte_list[0] = 1 /* Not Supported */;
> > -    aml_append(method, aml_return(aml_buffer(1, byte_list)));
> > +    aml_append(unpatched, aml_return(aml_buffer(1, byte_list)));
> > +    aml_append(method, unpatched);
> > +
> > +    /*
> > +     * Currently no function is supported for both root device and NVDIMM
> > +     * devices, let's temporarily set handle to 0x0 at this time.
>
> This comment confuses me. What does temporarily mean here?
> At what time?
> And when is it set to another value?

The HDLE indicates the DSM function is issued from which device; It is
0 if it comes from root device otherwise it is the handle_id of nvdimm
device.

As we do not support any real function now, i made it always be 0x0 for
all the devices. It will be set to the appropriate value when we implement
the label support in the last part of the long patch serial.

> Pls explain that instead of "set handle to 0" - that can
> be seen at a glance.

How about change the comment to this:

The HDLE indicates the DSM function is issued from which device, it
is not used at this time as no function is supported yet. Currently
we make it always be 0 for all the device and will set it the
appropriate value once real function is implemented.

> > +     */
> > +    aml_append(method, aml_store(aml_int(0x0), aml_name("HDLE")));
> > +    aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
> > +    aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
> >
> > +    /*
> > +     * tell QEMU about the real address of DSM memory, then QEMU begins
> > +     * to emulate the method
>
> emulate the method? I'd just drop this.

Okay, will drop "then QEMU begins to emulate the method".

> > and fills the result to DSM memory.
>
> in DSM memory

Okay.

> > +     */
> > +    aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
> > +
> > +    result_size = aml_local(1);
> > +    aml_append(method, aml_store(aml_name("RLEN"), result_size));
> > +    aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
> > +                                 result_size));
> > +    aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
> > +                                        result_size, "OBUF"));
> > +    aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
> > +                                       aml_arg(6)));
> > +    aml_append(method, aml_return(aml_arg(6)));
> >       aml_append(dev, method);
> >   }
> >
> > @@ -472,7 +521,7 @@ static void nvdimm_build_nvdimm_devices(GSList 
> > *device_list, Aml *root_dev)
> >   static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> >                                 GArray *table_data, GArray *linker)
> >   {
> > -    Aml *ssdt, *sb_scope, *dev;
> > +    Aml *ssdt, *sb_scope, *dev, *field;
> >       int mem_addr_offset, nvdimm_ssdt;
> >
> >       acpi_add_table(table_offsets, table_data);
> > @@ -497,6 +546,64 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray 
> > *table_offsets,
> >        */
> >       aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
> >
> > +    /* map DSM memory and IO into ACPI namespace. */
> > +    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
> > +               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
> > +    aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
> > +               aml_name(NVDIMM_ACPI_MEM_ADDR), TARGET_PAGE_SIZE));
> > +
> > +    /*
> > +     * DSM notifier:
> > +     * NTFI: write the address of DSM memory and notify QEMU to emulate
> > +     *       the access.
> > +     *
> > +     * It is the IO port so that accessing them will cause VM-exit, the
> > +     * control will be transferred to QEMU.
> > +     */
> > +    field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
> > +    aml_append(field, aml_named_field("NTFI",
> > +               sizeof(uint32_t) * BITS_PER_BYTE));
> > +    aml_append(dev, field);
> > +
> > +    /*
> > +     * DSM input:
> > +     * @HDLE: store device's handle, it's zero if the _DSM call happens
> > +     *        on NVDIMM Root Device.
> > +     * @REVS: store the Arg1 of _DSM call.
> > +     * @FUNC: store the Arg2 of _DSM call.
> > +     * @ARG3: store the Arg3 of _DSM call.
>
> Don't use @ prefixes - there are for function arguments.

Okay.

> > +     *
> > +     * They are RAM mapping on host so that these accesses never cause
> > +     * VM-EXIT.
> > +     */
> > +    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
> > +    aml_append(field, aml_named_field("HDLE",
> > +               sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
> > +    aml_append(field, aml_named_field("REVS",
> > +               sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
> > +    aml_append(field, aml_named_field("FUNC",
> > +               sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
> > +    aml_append(field, aml_named_field("ARG3",
> > +               (TARGET_PAGE_SIZE - offsetof(NvdimmDsmIn, arg3)) *
> > +                     BITS_PER_BYTE));
>
> drop the extra () here please, and align BITS_PER_BYTE with
> TARGET_PAGE_SIZE.

Okay.

> > +    aml_append(dev, field);
> > +
> > +    /*
> > +     * DSM output:
> > +     * @RLEN: the size of the buffer filled by QEMU.
> > +     * @ODAT: the buffer QEMU uses to store the result.
> > +     *
> > +     * Since the page is reused by both input and out, the input data
> > +     * will be lost after storing new result into @ODAT.
>
> And so?


So we should fetch all the input data before writing the result.

I will append it to the end of the comment.