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

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

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 <guangrong.xiao@linux.intel.com>
>> ---
>>   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.