Re: [Qemu-devel] [PATCH v6 25/33] nvdimm acpi: build ACPI nvdimm devices

[Xiao Guangrong]

On 11/09/2015 01:38 AM, Michael S. Tsirkin wrote:
> On Fri, Oct 30, 2015 at 01:56:19PM +0800, Xiao Guangrong 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, handle,
> > arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 184 insertions(+)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index dd84e5f..53ed675 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray 
> > *table_offsets,
> >       g_array_free(structures, true);
> >   }
> >
> > +struct NvdimmDsmIn {
> > +    uint32_t handle;
> > +    uint32_t revision;
> > +    uint32_t function;
> > +   /* the remaining size in the page is used by arg3. */
> > +    uint8_t arg3[0];
> > +} QEMU_PACKED;
> > +typedef struct NvdimmDsmIn NvdimmDsmIn;
> > +
> >   static uint64_t
> >   nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> >   {
> > @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
> >   static void
> >   nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> >   {
> > +    fprintf(stderr, "BUG: we never write DSM notification IO Port.\n");
> >   }
> >
> >   static const MemoryRegionOps nvdimm_dsm_ops = {
> > @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, 
> > MemoryRegion *io,
> >       memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_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 BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_)                \
> > +    do {                                                                   \
> > +        Aml *ifctx, *uuid;                                                 \
> > +        _method_ = aml_method("_DSM", 4);                                  \
> > +        /* check UUID if it is we expect, return the errorcode if not.*/   \
>
>         check that UUID is what we expect?

Yes, it is, better English indeed.

> > +        uuid = aml_touuid(_uuid_);                                         \
> > +        ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid)));             \
> > +        aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */)));     \
> > +        aml_append(method, ifctx);                                         \
> > +        aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \
> > +                   aml_arg(1), aml_arg(2), aml_arg(3))));                  \
>
> So name NCAL here matches the name below.
> Pls define a macro for it so we aren't limited
> by silly 4-letter limitations of AML.
> Same applies to all other names you use here and elsewhere.

Okay, it's good to me.

> > +        aml_append(_dev_, _method_);                                       \
> > +    } while (0)
> > +
> > +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_)                     \
> > +    aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE))
> > +
> > +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_)                 \
> > +    BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_)
> > +
>
> why are these macros? Make them functions pls.

Since Igor thought it hidden the details and it was not good to the readers.
I will just drop this macros and inline it in the place where it is used.