Re: [Qemu-devel] [PATCH v3 25/32] nvdimm: build ACPI nvdimm devices

[Xiao Guangrong]

On 10/13/2015 10:39 PM, Igor Mammedov wrote:
> On Sun, 11 Oct 2015 11:52:57 +0800
> Xiao Guangrong <address@hidden> wrote:
>
> > NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices
> >
> > There is a root device under \_SB and specified NVDIMM devices are under the
> > root device. Each NVDIMM device has _ADR which returns its handle used to
> > associate MEMDEV structure in NFIT
> >
> > We reserve handle 0 for root device. In this patch, we save handle, arg0,
> > arg1 and arg2. Arg3 is conditionally saved in later patch
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   hw/mem/nvdimm/acpi.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 203 insertions(+)
> >
> > diff --git a/hw/mem/nvdimm/acpi.c b/hw/mem/nvdimm/acpi.c
> I'd suggest to put ACPI parts to hw/acpi/nvdimm.c file so that ACPI
> maintainers won't miss changes to this files.

Sounds reasonable to me.

> > index 1450a6a..d9fa0fd 100644
> > --- a/hw/mem/nvdimm/acpi.c
> > +++ b/hw/mem/nvdimm/acpi.c
> > @@ -308,15 +308,38 @@ static void build_nfit(void *fit, GSList *device_list, 
> > GArray *table_offsets,
> >                    "NFIT", table_data->len - nfit_start, 1);
> >   }
> >
> > +#define NOTIFY_VALUE      0x99
> > +
> > +struct dsm_in {
> > +    uint32_t handle;
> > +    uint8_t arg0[16];
> > +    uint32_t arg1;
> > +    uint32_t arg2;
> > +   /* the remaining size in the page is used by arg3. */
> > +    uint8_t arg3[0];
> > +} QEMU_PACKED;
> > +typedef struct dsm_in dsm_in;
> > +
> > +struct dsm_out {
> > +    /* the size of buffer filled by QEMU. */
> > +    uint16_t len;
> > +    uint8_t data[0];
> > +} QEMU_PACKED;
> > +typedef struct dsm_out dsm_out;
> > +
> >   static uint64_t dsm_read(void *opaque, hwaddr addr,
> >                            unsigned size)
> >   {
> > +    fprintf(stderr, "BUG: we never read DSM notification MMIO.\n");
> >       return 0;
> >   }
> >
> >   static void dsm_write(void *opaque, hwaddr addr,
> >                         uint64_t val, unsigned size)
> >   {
> > +    if (val != NOTIFY_VALUE) {
> > +        fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val);
> > +    }
> >   }
> >
> >   static const MemoryRegionOps dsm_ops = {
> > @@ -372,6 +395,183 @@ static MemoryRegion *build_dsm_memory(NVDIMMState *state)
> >       return dsm_fit_mr;
> >   }
> >
> > +#define BUILD_STA_METHOD(_dev_, _method_)                                  \
> > +    do {                                                                   \
> > +        _method_ = aml_method("_STA", 0);                                  \
> > +        aml_append(_method_, aml_return(aml_int(0x0f)));                   \
> > +        aml_append(_dev_, _method_);                                       \
> > +    } while (0)
> > +
> > +#define SAVE_ARG012_HANDLE_LOCK(_method_, _handle_)                        \
> > +    do {                                                                   \
> > +        aml_append(_method_, aml_acquire(aml_name("NLCK"), 0xFFFF));       \
> how about making method serialized, then you could drop explicit lock/unlock 
> logic
> for that you'd need to extend existing aml_method() to something like this:
>
>    aml_method("FOO", 3/*count*/, AML_SERIALIZED, 0 /* sync_level */)

I am not sure if multiple methods under different namespace objects can be
serialized, for example:
Device("__D0") {
        Method("FOO", 3, AML_SERIALIZED, 0) {
                BUF = Arg0
        }
}

Device("__D1") {
        Method("FOO", 3, AML_SERIALIZED, 0) {
                BUF = Arg0
        }
}

__D0.FOO and __D1.FOO can be serialized?

Your suggestion definitely valuable to me, i will abstract the access of
shared-memory into one method as your comment below.

> > +        aml_append(_method_, aml_store(_handle_, aml_name("HDLE")));       \
> > +        aml_append(_method_, aml_store(aml_arg(0), aml_name("ARG0")));     \
> Could you describe QEMU<->ASL interface in a separate spec
> file (for example like: docs/specs/acpi_mem_hotplug.txt),
> it will help to with review process as there will be something to compare
> patches with.
> Once that is finalized/agreed upon, it should be easy to review and probably
> to write corresponding patches.

Sure, i considered it too and was planing to make this kind of spec after this
patchset is merged... I will document the interface in the next version.

> Also I'd try to minimize QEMU<->ASL interface and implement as much as possible
> of ASL logic in AML instead of pushing it in hardware (QEMU).

Okay, i agree.

Since ACPI ASL/AML is new knowledge to me, i did it using the opposite way - 
move
the control to QEMU side as possible ... :)

> For example there isn't really any need to tell QEMU ARG0 (UUID), _DSM method
> could just compare UUIDs itself and execute a corresponding branch.
> Probably something else could be optimized as well but that we can find out
> during discussion over QEMU<->ASL interface spec.

Okay.

> > +        aml_append(_method_, aml_store(aml_arg(1), aml_name("ARG1")));     \
> > +        aml_append(_method_, aml_store(aml_arg(2), aml_name("ARG2")));     \
> > +    } while (0)
> > +
> > +#define NOTIFY_AND_RETURN_UNLOCK(_method_)                           \
> > +    do {                                                                   \
> > +        aml_append(_method_, aml_store(aml_int(NOTIFY_VALUE),              \
> > +                   aml_name("NOTI")));                                     \
> > +        aml_append(_method_, aml_store(aml_name("RLEN"), aml_local(6)));   \
> > +        aml_append(_method_, aml_store(aml_shiftleft(aml_local(6),         \
> > +                      aml_int(3)), aml_local(6)));                         \
> > +        aml_append(_method_, aml_create_field(aml_name("ODAT"), aml_int(0),\
> > +                                              aml_local(6) , "OBUF"));     \
> > +        aml_append(_method_, aml_name_decl("ZBUF", aml_buffer(0, NULL)));  \
> > +        aml_append(_method_, aml_concatenate(aml_name("ZBUF"),             \
> > +                                          aml_name("OBUF"), aml_arg(6)));  \
> > +        aml_append(_method_, aml_release(aml_name("NLCK")));               \
> > +        aml_append(_method_, aml_return(aml_arg(6)));                      \
> > +    } while (0)
> > +
> > +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_)                 \
> > +    aml_append(_field_, aml_named_field(_name_,                            \
> > +               sizeof(typeof_field(_s_, _f_)) * BITS_PER_BYTE))
> > +
> > +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_)                     \
> > +    aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE))
> > +
> > +static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list,
> > +                                 Aml *root_dev)
> > +{
> > +    for (; device_list; device_list = device_list->next) {
> > +        NVDIMMDevice *nvdimm = device_list->data;
> > +        int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
> > +                                           NULL);
> > +        uint32_t handle = nvdimm_slot_to_handle(slot);
> > +        Aml *dev, *method;
> > +
> > +        dev = aml_device("NV%02X", slot);
> > +        aml_append(dev, aml_name_decl("_ADR", aml_int(handle)));
> > +
> > +        BUILD_STA_METHOD(dev, method);
> > +
> > +        method = aml_method("_DSM", 4);
> That will create the same method per each device which increases
> ACPI table size unnecessarily.
> I'd suggest to make per nvdimm device method a wrapper over common
> NVDR._DSM method and make the later handle all the logic.

Good to me.

Really appropriate for your review, Igor!