|
From: | Xiao Guangrong |
Subject: | Re: [Qemu-devel] [PATCH v3 25/32] nvdimm: build ACPI nvdimm devices |
Date: | Wed, 14 Oct 2015 01:24:54 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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.cI'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!
[Prev in Thread] | Current Thread | [Next in Thread] |