| Message ID | 1477672540-27952-4-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 29 Oct 2016 00:35:39 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > _FIT is required for hotplug support, guest will inquire the updated > device info from it if a hotplug event is received > > As FIT buffer is not completely mapped into guest address space, so a > new function, Read FIT whose UUID is UUID > 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index > is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer > is concatenated before _FIT return > > Refer to docs/specs/acpi-nvdimm.txt for detailed design > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > docs/specs/acpi_nvdimm.txt | 58 ++++++++++++- > hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 257 insertions(+), 5 deletions(-) > > diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt > index 0fdd251..4aa5e3d 100644 > --- a/docs/specs/acpi_nvdimm.txt > +++ b/docs/specs/acpi_nvdimm.txt > @@ -127,6 +127,58 @@ _DSM process diagram: > | result from the page | | | > +--------------------------+ +--------------+ > > - _FIT implementation > - ------------------- > - TODO (will fill it when nvdimm hotplug is introduced) > +Device Handle Reservation > +------------------------- > +As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device > +handle. The handle is completely QEMU internal thing, the values in range > +[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR), > +other values are reserved by other purpose. > + > +Current reserved handle: > +0x10000 is reserved for QEMU internal DSM function called on the root > +device. Above part should go to section where 'handle' is defined, i.e. earlier in the file: ACPI writes _DSM Input Data (based on the offset in the page): [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM Root device. > +QEMU internal use only _DSM function > +------------------------------------ > +UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal > +DSM function. > + > +There is the function introduced by QEMU and only used by QEMU internal. > + > +1) Read FIT UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function (private QEMU function) > + As we only reserved one page for NVDIMM ACPI it is impossible to map the > + whole FIT data to guest's address space. This function is used by _FIT > + method to read a piece of FIT data from QEMU. _FIT method uses Read_FIT function to fetch NFIT structures blob from QEMU in 1 page sized increments which are then concatenated and returned as _FIT method result. > + > + Input parameters: > + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} > + Arg1 – Revision ID (set to 1) > + Arg2 - Function Index, 0x1 > + Arg3 - A package containing a buffer whose layout is as follows: > + > + +----------+-------------+-------------+-----------------------------------+ > + | Filed | Byte Length | Byte Offset | Description | ^ field, s/Byte//, s/Byte// > + +----------+-------------+-------------+-----------------------------------+ > + | offset | 4 | 0 | the offset of FIT buffer | offset in QEMU's NFIT structures blob to read from > + +----------+-------------+-------------+-----------------------------------+ > + > + Output: > + +----------+-------------+-------------+-----------------------------------+ > + | Filed | Byte Length | Byte Offset | Description | > + +----------+-------------+-------------+-----------------------------------+ > + | | | | return status codes | > + | | | | 0x100 indicates fit has been | > + | status | 4 | 0 | updated | 0x100 - error caused by NFIT update while read by _FIT wasn't completed > + | | | | other follows Chapter 3 in DSM | s/other follows/other codes follow/ > + | | | | Spec Rev1 | > + +----------+-------------+-------------+-----------------------------------+ > + | fit data | Varies | 4 | FIT data | > + | | | | | > + +----------+-------------+-------------+-----------------------------------+ what does "Varies" mean, how would I know reading this how much data Read_FIT should read from shared page? > + > + The FIT offset is maintained by the caller itself, probably is not necessary sentence, or specify a caller (for example OSPM) > current offset plugs ^^^^? > + the length returned by the function is the next offset we should read. > + When all the FIT data has been read out, zero length is returned. > + > + If it returns 0x100, OSPM should restart to read FIT (read from offset 0 > + again). [...] that's all for doc part, I'll do the code part later.
On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote: > +1) Read FIT > + As we only reserved one page for NVDIMM ACPI it is impossible to map the > + whole FIT data to guest's address space. This function is used by _FIT > + method to read a piece of FIT data from QEMU. > + > + Input parameters: > + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} > + Arg1 – Revision ID (set to 1) > + Arg2 - Function Index, 0x1 > + Arg3 - A package containing a buffer whose layout is as follows: > + > + +----------+-------------+-------------+-----------------------------------+ > + | Filed | Byte Length | Byte Offset | Description | s/Filed/Field/ The same applies below too. > + +----------+-------------+-------------+-----------------------------------+ > + | offset | 4 | 0 | the offset of FIT buffer | > + +----------+-------------+-------------+-----------------------------------+ s/offset of FIT buffer/offset into FIT buffer/ > + > + Output: > + +----------+-------------+-------------+-----------------------------------+ > + | Filed | Byte Length | Byte Offset | Description | > + +----------+-------------+-------------+-----------------------------------+ > + | | | | return status codes | > + | | | | 0x100 indicates fit has been | > + | status | 4 | 0 | updated | > + | | | | other follows Chapter 3 in DSM | > + | | | | Spec Rev1 | > + +----------+-------------+-------------+-----------------------------------+ > + | fit data | Varies | 4 | FIT data | > + | | | | | > + +----------+-------------+-------------+-----------------------------------+ > + > + The FIT offset is maintained by the caller itself, current offset plugs s/plugs/plus/ > +struct NvdimmFuncReadFITIn { > + uint32_t offset; /* the offset of FIT buffer. */ s/offset of FIT buffer/offset into FIT buffer/
On Sat, 29 Oct 2016 00:35:39 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > _FIT is required for hotplug support, guest will inquire the updated > device info from it if a hotplug event is received > > As FIT buffer is not completely mapped into guest address space, so a > new function, Read FIT whose UUID is UUID > 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index > is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer > is concatenated before _FIT return > > Refer to docs/specs/acpi-nvdimm.txt for detailed design > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- [...] > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 5f728a6..fc1a012 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn; > QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + > offsetof(NvdimmDsmIn, arg3) > 4096); > > +struct NvdimmFuncReadFITIn { > + uint32_t offset; /* the offset of FIT buffer. */ > +} QEMU_PACKED; > +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn; > +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) + > + offsetof(NvdimmDsmIn, arg3) > 4096); > + > +struct NvdimmFuncReadFITOut { > + /* the size of buffer filled by QEMU. */ > + uint32_t len; > + uint32_t func_ret_status; /* return status code. */ > + uint8_t fit[0]; /* the FIT data. */ > +} QEMU_PACKED; > +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut; > +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096); > + > static void > nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr) > { > @@ -516,6 +532,74 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr) > cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out)); > } > > +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000 > + > +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ > +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, > + hwaddr dsm_mem_addr) > +{ > + NvdimmFitBuffer *fit_buf = &state->fit_buf; > + NvdimmFuncReadFITIn *read_fit; > + NvdimmFuncReadFITOut *read_fit_out; > + GArray *fit; > + uint32_t read_len = 0, func_ret_status; > + int size; > + > + read_fit = (NvdimmFuncReadFITIn *)in->arg3; > + le32_to_cpus(&read_fit->offset); I'd prefer if you'd not do inplace conversion, just do offset = le32_to_cpu(read_fit->offset); > + > + qemu_mutex_lock(&fit_buf->lock); > + fit = fit_buf->fit; > + > + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n", > + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No"); as follow up path replace nvdimm_debug() with trace events > + > + if (read_fit->offset > fit->len) { > + func_ret_status = 3 /* Invalid Input Parameters */; should be macros instead of magic value > + goto exit; > + } > + > + /* It is the first time to read FIT. */ > + if (!read_fit->offset) { > + fit_buf->dirty = false; > + } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */ > + func_ret_status = 0x100 /* fit changed */; should be macros instead of magic value > + goto exit; > + } > + > + func_ret_status = 0 /* Success */; > + read_len = MIN(fit->len - read_fit->offset, > + 4096 - sizeof(NvdimmFuncReadFITOut)); > + > +exit: > + size = sizeof(NvdimmFuncReadFITOut) + read_len; > + read_fit_out = g_malloc(size); > + > + read_fit_out->len = cpu_to_le32(size); > + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status); > + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len); > + > + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size); > + > + g_free(read_fit_out); > + qemu_mutex_unlock(&fit_buf->lock); > +} > + > +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, > + hwaddr dsm_mem_addr) > +{ > + switch (in->function) { > + case 0x0: > + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr); > + return; > + case 0x1 /*Read FIT */: > + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr); > + return; > + } > + > + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr); should be macros instead of magic value > +} > + > static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr) > { > /* > @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > static void > nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > { > + AcpiNVDIMMState *state = opaque; > NvdimmDsmIn *in; > hwaddr dsm_mem_addr = val; > > @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > goto exit; > } > > + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) { > + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr); > + goto exit; > + } > + > /* Handle 0 is reserved for NVDIMM Root Device. */ > if (!in->handle) { > nvdimm_dsm_root(in, dsm_mem_addr); > @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, > #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN" > #define NVDIMM_DSM_OUT_BUF "ODAT" > > +#define NVDIMM_DSM_RFIT_STATUS "RSTA" > + > +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62" > + > static void nvdimm_build_common_dsm(Aml *dev) > { > - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem; > + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2; > Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; > Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size; > uint8_t byte_list[1]; > @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev) > /* UUID for NVDIMM Root Device */, expected_uuid)); > aml_append(method, ifctx); > elsectx = aml_else(); > - aml_append(elsectx, aml_store( > + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT))); > + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID > + /* UUID for QEMU internal use */), expected_uuid)); > + aml_append(elsectx, ifctx); > + elsectx2 = aml_else(); > + aml_append(elsectx2, aml_store( > aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66") > /* UUID for NVDIMM Devices */, expected_uuid)); > + aml_append(elsectx, elsectx2); > aml_append(method, elsectx); > > uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid)); > @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) > aml_append(dev, method); > } > > +static void nvdimm_build_fit(Aml *dev) nvdimm_build_fit_method() > +{ > + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; > + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit; > + > + buf = aml_local(0); > + buf_size = aml_local(1); > + fit = aml_local(2); > + > + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL), > + aml_int(0), NVDIMM_DSM_RFIT_STATUS)); it doesn't have to be buffer as it's internal ASL integer object so it could be just named variable. I'd also move it to _FIT method instead of making it device global and if it could work try to pass it as argument to RFIT RefOf/DerefOf may help here or make return value instead of buffer and pass buffer as reference. Alternatively you can return buffer from RFIT with status field included and check/discard status value there. > + > + /* build helper function, RFIT. */ > + method = aml_method("RFIT", 1, AML_SERIALIZED); > + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), > + aml_int(0), "OFST")); > + > + /* prepare input package. */ > + pkg = aml_package(1); > + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); > + aml_append(pkg, aml_name("OFST")); > + > + /* call Read_FIT function. */ > + call_result = aml_call5(NVDIMM_COMMON_DSM, > + aml_touuid(NVDIMM_QEMU_RSVD_UUID), > + aml_int(1) /* Revision 1 */, > + aml_int(0x1) /* Read FIT */, > + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)); > + aml_append(method, aml_store(call_result, buf)); > + > + /* handle _DSM result. */ > + aml_append(method, aml_create_dword_field(buf, > + aml_int(0) /* offset at byte 0 */, "STAU")); > + > + aml_append(method, aml_store(aml_name("STAU"), > + aml_name(NVDIMM_DSM_RFIT_STATUS))); > + > + /* if something is wrong during _DSM. */ > + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); > + ifctx = aml_if(aml_lnot(ifcond)); > + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); > + aml_append(method, ifctx); > + > + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); > + aml_append(method, aml_subtract(buf_size, > + aml_int(4) /* the size of "STAU" */, > + buf_size)); > + > + /* if we read the end of fit. */ > + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); > + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); > + aml_append(method, ifctx); > + > + aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)), > + buf_size)); there isn't need to convert bytes to bits and store it in the same variable it creates confusion > + aml_append(method, aml_create_field(buf, > + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/ > + buf_size, "BUFF")); just inline conversion in aml_create_field() > + aml_append(method, aml_return(aml_name("BUFF"))); > + aml_append(dev, method); > + > + /* build _FIT. */ > + method = aml_method("_FIT", 0, AML_SERIALIZED); > + offset = aml_local(3); > + > + aml_append(method, aml_store(aml_buffer(0, NULL), fit)); > + aml_append(method, aml_store(aml_int(0), offset)); > + > + whilectx = aml_while(aml_int(1)); > + aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf)); > + aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size)); > + > + /* > + * if fit buffer was changed during RFIT, read from the beginning > + * again. > + */ > + ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS), > + aml_int(0x100 /* fit changed */))); > + aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit)); > + aml_append(ifctx, aml_store(aml_int(0), offset)); > + aml_append(whilectx, ifctx); > + > + elsectx = aml_else(); > + > + /* finish fit read if no data is read out. */ > + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); > + aml_append(ifctx, aml_return(fit)); > + aml_append(elsectx, ifctx); > + > + /* update the offset. */ > + aml_append(elsectx, aml_add(offset, buf_size, offset)); > + /* append the data we read out to the fit buffer. */ > + aml_append(elsectx, aml_concatenate(fit, buf, fit)); > + aml_append(whilectx, elsectx); > + aml_append(method, whilectx); > + > + aml_append(dev, method); > +} > + > static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots) > { > uint32_t slot; > @@ -1052,6 +1251,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data, > > /* 0 is reserved for root device. */ > nvdimm_build_device_dsm(dev, 0); > + nvdimm_build_fit(dev); > > nvdimm_build_nvdimm_devices(dev, ram_slots); >
On 11/02/2016 12:24 AM, Igor Mammedov wrote: > On Sat, 29 Oct 2016 00:35:39 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> _FIT is required for hotplug support, guest will inquire the updated >> device info from it if a hotplug event is received >> >> As FIT buffer is not completely mapped into guest address space, so a >> new function, Read FIT whose UUID is UUID >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer >> is concatenated before _FIT return >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++- >> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 257 insertions(+), 5 deletions(-) >> >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt >> index 0fdd251..4aa5e3d 100644 >> --- a/docs/specs/acpi_nvdimm.txt >> +++ b/docs/specs/acpi_nvdimm.txt >> @@ -127,6 +127,58 @@ _DSM process diagram: >> | result from the page | | | >> +--------------------------+ +--------------+ >> >> - _FIT implementation >> - ------------------- >> - TODO (will fill it when nvdimm hotplug is introduced) >> +Device Handle Reservation >> +------------------------- >> +As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device >> +handle. The handle is completely QEMU internal thing, the values in range >> +[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR), >> +other values are reserved by other purpose. >> + >> +Current reserved handle: >> +0x10000 is reserved for QEMU internal DSM function called on the root >> +device. > Above part should go to section where 'handle' is defined, i.e. earlier in the file: > > ACPI writes _DSM Input Data (based on the offset in the page): > [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM > Root device. > Okay. > >> +QEMU internal use only _DSM function >> +------------------------------------ >> +UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal >> +DSM function. >> + >> +There is the function introduced by QEMU and only used by QEMU internal. >> + >> +1) Read FIT > UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function > (private QEMU function) > okay. >> + As we only reserved one page for NVDIMM ACPI it is impossible to map the >> + whole FIT data to guest's address space. This function is used by _FIT >> + method to read a piece of FIT data from QEMU. > _FIT method uses Read_FIT function to fetch NFIT structures blob from QEMU > in 1 page sized increments which are then concatenated and returned as _FIT method result. > okay. > >> + >> + Input parameters: >> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} >> + Arg1 – Revision ID (set to 1) >> + Arg2 - Function Index, 0x1 >> + Arg3 - A package containing a buffer whose layout is as follows: >> + >> + +----------+-------------+-------------+-----------------------------------+ >> + | Filed | Byte Length | Byte Offset | Description | > ^ field, s/Byte//, s/Byte// >> + +----------+-------------+-------------+-----------------------------------+ >> + | offset | 4 | 0 | the offset of FIT buffer | > offset in QEMU's NFIT structures blob to read from okay. > >> + +----------+-------------+-------------+-----------------------------------+ >> + >> + Output: >> + +----------+-------------+-------------+-----------------------------------+ >> + | Filed | Byte Length | Byte Offset | Description | >> + +----------+-------------+-------------+-----------------------------------+ >> + | | | | return status codes | >> + | | | | 0x100 indicates fit has been | >> + | status | 4 | 0 | updated | > 0x100 - error caused by NFIT update while read by _FIT wasn't completed okay. > >> + | | | | other follows Chapter 3 in DSM | > s/other follows/other codes follow/ okay. > >> + | | | | Spec Rev1 | >> + +----------+-------------+-------------+-----------------------------------+ >> + | fit data | Varies | 4 | FIT data | >> + | | | | | >> + +----------+-------------+-------------+-----------------------------------+ > what does "Varies" mean, how would I know reading this how much data Read_FIT should read > from shared page? I mean other bytes in the buffer returned by this function except the 'status' field will be used as fit data. Maybe it is has a 'length' field? > >> + >> + The FIT offset is maintained by the caller itself, > probably is not necessary sentence, or specify a caller (for example OSPM) > Okay. >> current offset plugs > ^^^^? Typo. should be plus. > >> + the length returned by the function is the next offset we should read. >> + When all the FIT data has been read out, zero length is returned. >> + >> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0 >> + again). > [...] > that's all for doc part, I'll do the code part later. > Thank you, Igor.
On 11/02/2016 12:41 AM, Stefan Hajnoczi wrote: > On Sat, Oct 29, 2016 at 12:35:39AM +0800, Xiao Guangrong wrote: >> +1) Read FIT >> + As we only reserved one page for NVDIMM ACPI it is impossible to map the >> + whole FIT data to guest's address space. This function is used by _FIT >> + method to read a piece of FIT data from QEMU. >> + >> + Input parameters: >> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} >> + Arg1 – Revision ID (set to 1) >> + Arg2 - Function Index, 0x1 >> + Arg3 - A package containing a buffer whose layout is as follows: >> + >> + +----------+-------------+-------------+-----------------------------------+ >> + | Filed | Byte Length | Byte Offset | Description | > > s/Filed/Field/ > > The same applies below too. Will fix. > >> + +----------+-------------+-------------+-----------------------------------+ >> + | offset | 4 | 0 | the offset of FIT buffer | >> + +----------+-------------+-------------+-----------------------------------+ > > s/offset of FIT buffer/offset into FIT buffer/ will fix. > >> + >> + Output: >> + +----------+-------------+-------------+-----------------------------------+ >> + | Filed | Byte Length | Byte Offset | Description | >> + +----------+-------------+-------------+-----------------------------------+ >> + | | | | return status codes | >> + | | | | 0x100 indicates fit has been | >> + | status | 4 | 0 | updated | >> + | | | | other follows Chapter 3 in DSM | >> + | | | | Spec Rev1 | >> + +----------+-------------+-------------+-----------------------------------+ >> + | fit data | Varies | 4 | FIT data | >> + | | | | | >> + +----------+-------------+-------------+-----------------------------------+ >> + >> + The FIT offset is maintained by the caller itself, current offset plugs > > s/plugs/plus/ Yes, indeed. > >> +struct NvdimmFuncReadFITIn { >> + uint32_t offset; /* the offset of FIT buffer. */ > > s/offset of FIT buffer/offset into FIT buffer/ will fix. Thank you, Stefan!
On 11/02/2016 09:56 PM, Igor Mammedov wrote: > On Sat, 29 Oct 2016 00:35:39 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> _FIT is required for hotplug support, guest will inquire the updated >> device info from it if a hotplug event is received >> >> As FIT buffer is not completely mapped into guest address space, so a >> new function, Read FIT whose UUID is UUID >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer >> is concatenated before _FIT return >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- > [...] > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 5f728a6..fc1a012 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn; >> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + >> offsetof(NvdimmDsmIn, arg3) > 4096); >> >> +struct NvdimmFuncReadFITIn { >> + uint32_t offset; /* the offset of FIT buffer. */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn; >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) + >> + offsetof(NvdimmDsmIn, arg3) > 4096); >> + >> +struct NvdimmFuncReadFITOut { >> + /* the size of buffer filled by QEMU. */ >> + uint32_t len; >> + uint32_t func_ret_status; /* return status code. */ >> + uint8_t fit[0]; /* the FIT data. */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut; >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096); >> + >> static void >> nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr) >> { >> @@ -516,6 +532,74 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr) >> cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out)); >> } >> >> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000 >> + >> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ >> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> + hwaddr dsm_mem_addr) >> +{ >> + NvdimmFitBuffer *fit_buf = &state->fit_buf; >> + NvdimmFuncReadFITIn *read_fit; >> + NvdimmFuncReadFITOut *read_fit_out; >> + GArray *fit; >> + uint32_t read_len = 0, func_ret_status; >> + int size; >> + >> + read_fit = (NvdimmFuncReadFITIn *)in->arg3; >> + le32_to_cpus(&read_fit->offset); > I'd prefer if you'd not do inplace conversion, just do > offset = le32_to_cpu(read_fit->offset); okay. > >> + >> + qemu_mutex_lock(&fit_buf->lock); >> + fit = fit_buf->fit; >> + >> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n", >> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No"); > as follow up path replace nvdimm_debug() with trace events I will do it as a separate patch. > >> + >> + if (read_fit->offset > fit->len) { >> + func_ret_status = 3 /* Invalid Input Parameters */; > should be macros instead of magic value Yes. > >> + goto exit; >> + } >> + >> + /* It is the first time to read FIT. */ >> + if (!read_fit->offset) { >> + fit_buf->dirty = false; >> + } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */ >> + func_ret_status = 0x100 /* fit changed */; > should be macros instead of magic value okay. > >> + goto exit; >> + } >> + >> + func_ret_status = 0 /* Success */; >> + read_len = MIN(fit->len - read_fit->offset, >> + 4096 - sizeof(NvdimmFuncReadFITOut)); >> + >> +exit: >> + size = sizeof(NvdimmFuncReadFITOut) + read_len; >> + read_fit_out = g_malloc(size); >> + >> + read_fit_out->len = cpu_to_le32(size); >> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status); >> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len); >> + >> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size); >> + >> + g_free(read_fit_out); >> + qemu_mutex_unlock(&fit_buf->lock); >> +} >> + >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> + hwaddr dsm_mem_addr) >> +{ >> + switch (in->function) { >> + case 0x0: >> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr); >> + return; >> + case 0x1 /*Read FIT */: >> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr); >> + return; >> + } >> + >> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr); > should be macros instead of magic value Okay. > >> +} >> + >> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr) >> { >> /* >> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >> static void >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> { >> + AcpiNVDIMMState *state = opaque; >> NvdimmDsmIn *in; >> hwaddr dsm_mem_addr = val; >> >> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> goto exit; >> } >> >> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) { >> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr); >> + goto exit; >> + } >> + >> /* Handle 0 is reserved for NVDIMM Root Device. */ >> if (!in->handle) { >> nvdimm_dsm_root(in, dsm_mem_addr); >> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, >> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN" >> #define NVDIMM_DSM_OUT_BUF "ODAT" >> >> +#define NVDIMM_DSM_RFIT_STATUS "RSTA" >> + >> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62" >> + >> static void nvdimm_build_common_dsm(Aml *dev) >> { >> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem; >> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2; >> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; >> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size; >> uint8_t byte_list[1]; >> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev) >> /* UUID for NVDIMM Root Device */, expected_uuid)); >> aml_append(method, ifctx); >> elsectx = aml_else(); >> - aml_append(elsectx, aml_store( >> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT))); >> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID >> + /* UUID for QEMU internal use */), expected_uuid)); >> + aml_append(elsectx, ifctx); >> + elsectx2 = aml_else(); >> + aml_append(elsectx2, aml_store( >> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66") >> /* UUID for NVDIMM Devices */, expected_uuid)); >> + aml_append(elsectx, elsectx2); >> aml_append(method, elsectx); >> >> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid)); >> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) >> aml_append(dev, method); >> } >> >> +static void nvdimm_build_fit(Aml *dev) > nvdimm_build_fit_method() okay. > >> +{ >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; >> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit; >> + >> + buf = aml_local(0); >> + buf_size = aml_local(1); >> + fit = aml_local(2); >> + >> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), NVDIMM_DSM_RFIT_STATUS)); > it doesn't have to be buffer as it's internal ASL integer object > so it could be just named variable. Let me try. > > I'd also move it to _FIT method instead of making it device global We can not as it is used both in _FIT method and RFIT method. > > and if it could work try to pass it as argument to RFIT > RefOf/DerefOf may help here > or make return value instead of buffer and pass buffer as reference. > Let me try. > Alternatively you can return buffer from RFIT with status field included > and check/discard status value there. > As we can not create name object in a while-loop, it is not easy to check the status in _FIT. >> + >> + /* build helper function, RFIT. */ >> + method = aml_method("RFIT", 1, AML_SERIALIZED); >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), "OFST")); >> + >> + /* prepare input package. */ >> + pkg = aml_package(1); >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); >> + aml_append(pkg, aml_name("OFST")); >> + >> + /* call Read_FIT function. */ >> + call_result = aml_call5(NVDIMM_COMMON_DSM, >> + aml_touuid(NVDIMM_QEMU_RSVD_UUID), >> + aml_int(1) /* Revision 1 */, >> + aml_int(0x1) /* Read FIT */, >> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)); >> + aml_append(method, aml_store(call_result, buf)); >> + >> + /* handle _DSM result. */ >> + aml_append(method, aml_create_dword_field(buf, >> + aml_int(0) /* offset at byte 0 */, "STAU")); >> + >> + aml_append(method, aml_store(aml_name("STAU"), >> + aml_name(NVDIMM_DSM_RFIT_STATUS))); >> + >> + /* if something is wrong during _DSM. */ >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); >> + ifctx = aml_if(aml_lnot(ifcond)); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); >> + aml_append(method, aml_subtract(buf_size, >> + aml_int(4) /* the size of "STAU" */, >> + buf_size)); >> + >> + /* if we read the end of fit. */ >> + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)), >> + buf_size)); > there isn't need to convert bytes to bits and store it in the same variable > it creates confusion Okay, i will introduce a new variable named buf_size_bits. > >> + aml_append(method, aml_create_field(buf, >> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/ >> + buf_size, "BUFF")); > just inline conversion in aml_create_field() okay.
On Wed, 2 Nov 2016 23:40:56 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 11/02/2016 12:24 AM, Igor Mammedov wrote: > > On Sat, 29 Oct 2016 00:35:39 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> _FIT is required for hotplug support, guest will inquire the updated > >> device info from it if a hotplug event is received > >> > >> As FIT buffer is not completely mapped into guest address space, so a > >> new function, Read FIT whose UUID is UUID > >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index > >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer > >> is concatenated before _FIT return > >> > >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > >> > >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >> --- > >> docs/specs/acpi_nvdimm.txt | 58 ++++++++++++- > >> hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 257 insertions(+), 5 deletions(-) > >> > >> diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt > >> index 0fdd251..4aa5e3d 100644 > >> --- a/docs/specs/acpi_nvdimm.txt > >> +++ b/docs/specs/acpi_nvdimm.txt > >> @@ -127,6 +127,58 @@ _DSM process diagram: > >> | result from the page | | | > >> +--------------------------+ +--------------+ > >> > >> - _FIT implementation > >> - ------------------- > >> - TODO (will fill it when nvdimm hotplug is introduced) > >> +Device Handle Reservation > >> +------------------------- > >> +As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device > >> +handle. The handle is completely QEMU internal thing, the values in range > >> +[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR), > >> +other values are reserved by other purpose. > >> + > >> +Current reserved handle: > >> +0x10000 is reserved for QEMU internal DSM function called on the root > >> +device. > > Above part should go to section where 'handle' is defined, i.e. earlier in the file: > > > > ACPI writes _DSM Input Data (based on the offset in the page): > > [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM > > Root device. > > > > Okay. > > > > >> +QEMU internal use only _DSM function > >> +------------------------------------ > >> +UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal > >> +DSM function. > >> + > >> +There is the function introduced by QEMU and only used by QEMU internal. > >> + > >> +1) Read FIT > > UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function > > (private QEMU function) > > > > okay. > > >> + As we only reserved one page for NVDIMM ACPI it is impossible to map the > >> + whole FIT data to guest's address space. This function is used by _FIT > >> + method to read a piece of FIT data from QEMU. > > _FIT method uses Read_FIT function to fetch NFIT structures blob from QEMU > > in 1 page sized increments which are then concatenated and returned as _FIT method result. > > > > okay. > > > > >> + > >> + Input parameters: > >> + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} > >> + Arg1 – Revision ID (set to 1) > >> + Arg2 - Function Index, 0x1 > >> + Arg3 - A package containing a buffer whose layout is as follows: > >> + > >> + +----------+-------------+-------------+-----------------------------------+ > >> + | Filed | Byte Length | Byte Offset | Description | > > ^ field, s/Byte//, s/Byte// > >> + +----------+-------------+-------------+-----------------------------------+ > >> + | offset | 4 | 0 | the offset of FIT buffer | > > offset in QEMU's NFIT structures blob to read from > > okay. > > > > >> + +----------+-------------+-------------+-----------------------------------+ > >> + > >> + Output: > >> + +----------+-------------+-------------+-----------------------------------+ > >> + | Filed | Byte Length | Byte Offset | Description | > >> + +----------+-------------+-------------+-----------------------------------+ > >> + | | | | return status codes | > >> + | | | | 0x100 indicates fit has been | > >> + | status | 4 | 0 | updated | > > 0x100 - error caused by NFIT update while read by _FIT wasn't completed > > okay. > > > > >> + | | | | other follows Chapter 3 in DSM | > > s/other follows/other codes follow/ > > okay. > > > > >> + | | | | Spec Rev1 | > >> + +----------+-------------+-------------+-----------------------------------+ > >> + | fit data | Varies | 4 | FIT data | > >> + | | | | | > >> + +----------+-------------+-------------+-----------------------------------+ > > what does "Varies" mean, how would I know reading this how much data Read_FIT should read > > from shared page? > > I mean other bytes in the buffer returned by this function except the 'status' field > will be used as fit data. Maybe it is has a 'length' field? you fill in length in C and access it in AML code and it's a part of output buffer format so document it properly here. > > > > >> + > >> + The FIT offset is maintained by the caller itself, > > probably is not necessary sentence, or specify a caller (for example OSPM) > > > > Okay. > > >> current offset plugs > > ^^^^? > > Typo. should be plus. > > > > >> + the length returned by the function is the next offset we should read. > >> + When all the FIT data has been read out, zero length is returned. > >> + > >> + If it returns 0x100, OSPM should restart to read FIT (read from offset 0 > >> + again). > > [...] > > that's all for doc part, I'll do the code part later. > > > > Thank you, Igor. >
On Wed, 2 Nov 2016 23:54:05 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 11/02/2016 09:56 PM, Igor Mammedov wrote: > > On Sat, 29 Oct 2016 00:35:39 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> _FIT is required for hotplug support, guest will inquire the updated > >> device info from it if a hotplug event is received > >> > >> As FIT buffer is not completely mapped into guest address space, so a > >> new function, Read FIT whose UUID is UUID > >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index > >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer > >> is concatenated before _FIT return > >> > >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > >> > >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >> --- > > [...] > > > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >> index 5f728a6..fc1a012 100644 > >> --- a/hw/acpi/nvdimm.c > >> +++ b/hw/acpi/nvdimm.c > >> @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn; > >> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + > >> offsetof(NvdimmDsmIn, arg3) > 4096); > >> > >> +struct NvdimmFuncReadFITIn { > >> + uint32_t offset; /* the offset of FIT buffer. */ > >> +} QEMU_PACKED; > >> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn; > >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) + > >> + offsetof(NvdimmDsmIn, arg3) > 4096); > >> + > >> +struct NvdimmFuncReadFITOut { > >> + /* the size of buffer filled by QEMU. */ > >> + uint32_t len; > >> + uint32_t func_ret_status; /* return status code. */ > >> + uint8_t fit[0]; /* the FIT data. */ > >> +} QEMU_PACKED; > >> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut; > >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096); > >> + > >> static void > >> nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr) > >> { > >> @@ -516,6 +532,74 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr) > >> cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out)); > >> } > >> > >> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000 > >> + > >> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ > >> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, > >> + hwaddr dsm_mem_addr) > >> +{ > >> + NvdimmFitBuffer *fit_buf = &state->fit_buf; > >> + NvdimmFuncReadFITIn *read_fit; > >> + NvdimmFuncReadFITOut *read_fit_out; > >> + GArray *fit; > >> + uint32_t read_len = 0, func_ret_status; > >> + int size; > >> + > >> + read_fit = (NvdimmFuncReadFITIn *)in->arg3; > >> + le32_to_cpus(&read_fit->offset); > > I'd prefer if you'd not do inplace conversion, just do > > offset = le32_to_cpu(read_fit->offset); > > okay. > > > > >> + > >> + qemu_mutex_lock(&fit_buf->lock); > >> + fit = fit_buf->fit; > >> + > >> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n", > >> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No"); > > as follow up path replace nvdimm_debug() with trace events > > I will do it as a separate patch. > > > > >> + > >> + if (read_fit->offset > fit->len) { > >> + func_ret_status = 3 /* Invalid Input Parameters */; > > should be macros instead of magic value > > Yes. > > > > >> + goto exit; > >> + } > >> + > >> + /* It is the first time to read FIT. */ > >> + if (!read_fit->offset) { > >> + fit_buf->dirty = false; > >> + } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */ > >> + func_ret_status = 0x100 /* fit changed */; > > should be macros instead of magic value > > okay. > > > > >> + goto exit; > >> + } > >> + > >> + func_ret_status = 0 /* Success */; > >> + read_len = MIN(fit->len - read_fit->offset, > >> + 4096 - sizeof(NvdimmFuncReadFITOut)); > >> + > >> +exit: > >> + size = sizeof(NvdimmFuncReadFITOut) + read_len; > >> + read_fit_out = g_malloc(size); > >> + > >> + read_fit_out->len = cpu_to_le32(size); > >> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status); > >> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len); > >> + > >> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size); > >> + > >> + g_free(read_fit_out); > >> + qemu_mutex_unlock(&fit_buf->lock); > >> +} > >> + > >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, > >> + hwaddr dsm_mem_addr) > >> +{ > >> + switch (in->function) { > >> + case 0x0: > >> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr); > >> + return; > >> + case 0x1 /*Read FIT */: > >> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr); > >> + return; > >> + } > >> + > >> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr); > > should be macros instead of magic value > > Okay. > > > > >> +} > >> + > >> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr) > >> { > >> /* > >> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > >> static void > >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > >> { > >> + AcpiNVDIMMState *state = opaque; > >> NvdimmDsmIn *in; > >> hwaddr dsm_mem_addr = val; > >> > >> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > >> goto exit; > >> } > >> > >> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) { > >> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr); > >> + goto exit; > >> + } > >> + > >> /* Handle 0 is reserved for NVDIMM Root Device. */ > >> if (!in->handle) { > >> nvdimm_dsm_root(in, dsm_mem_addr); > >> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, > >> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN" > >> #define NVDIMM_DSM_OUT_BUF "ODAT" > >> > >> +#define NVDIMM_DSM_RFIT_STATUS "RSTA" > >> + > >> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62" > >> + > >> static void nvdimm_build_common_dsm(Aml *dev) > >> { > >> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem; > >> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2; > >> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; > >> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size; > >> uint8_t byte_list[1]; > >> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev) > >> /* UUID for NVDIMM Root Device */, expected_uuid)); > >> aml_append(method, ifctx); > >> elsectx = aml_else(); > >> - aml_append(elsectx, aml_store( > >> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT))); > >> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID > >> + /* UUID for QEMU internal use */), expected_uuid)); > >> + aml_append(elsectx, ifctx); > >> + elsectx2 = aml_else(); > >> + aml_append(elsectx2, aml_store( > >> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66") > >> /* UUID for NVDIMM Devices */, expected_uuid)); > >> + aml_append(elsectx, elsectx2); > >> aml_append(method, elsectx); > >> > >> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid)); > >> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) > >> aml_append(dev, method); > >> } > >> > >> +static void nvdimm_build_fit(Aml *dev) > > nvdimm_build_fit_method() > > okay. > > > > >> +{ > >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; > >> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit; > >> + > >> + buf = aml_local(0); > >> + buf_size = aml_local(1); > >> + fit = aml_local(2); > >> + > >> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL), > >> + aml_int(0), NVDIMM_DSM_RFIT_STATUS)); > > it doesn't have to be buffer as it's internal ASL integer object > > so it could be just named variable. > > Let me try. > > > > > I'd also move it to _FIT method instead of making it device global > > We can not as it is used both in _FIT method and RFIT method. > > > > > and if it could work try to pass it as argument to RFIT > > RefOf/DerefOf may help here > > or make return value instead of buffer and pass buffer as reference. > > > > Let me try. > > > Alternatively you can return buffer from RFIT with status field included > > and check/discard status value there. > > > > As we can not create name object in a while-loop, it is not easy to check > the status in _FIT. index() method might help there or even better create a separate method to extract dword from buffer and call it from all other places where you do similar thing instead of creating a bunch of fields. > > >> + > >> + /* build helper function, RFIT. */ > >> + method = aml_method("RFIT", 1, AML_SERIALIZED); > >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), > >> + aml_int(0), "OFST")); > >> + > >> + /* prepare input package. */ > >> + pkg = aml_package(1); > >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); > >> + aml_append(pkg, aml_name("OFST")); > >> + > >> + /* call Read_FIT function. */ > >> + call_result = aml_call5(NVDIMM_COMMON_DSM, > >> + aml_touuid(NVDIMM_QEMU_RSVD_UUID), > >> + aml_int(1) /* Revision 1 */, > >> + aml_int(0x1) /* Read FIT */, > >> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)); > >> + aml_append(method, aml_store(call_result, buf)); > >> + > >> + /* handle _DSM result. */ > >> + aml_append(method, aml_create_dword_field(buf, > >> + aml_int(0) /* offset at byte 0 */, "STAU")); > >> + > >> + aml_append(method, aml_store(aml_name("STAU"), > >> + aml_name(NVDIMM_DSM_RFIT_STATUS))); > >> + > >> + /* if something is wrong during _DSM. */ > >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); > >> + ifctx = aml_if(aml_lnot(ifcond)); > >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); > >> + aml_append(method, ifctx); > >> + > >> + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); > >> + aml_append(method, aml_subtract(buf_size, > >> + aml_int(4) /* the size of "STAU" */, > >> + buf_size)); > >> + > >> + /* if we read the end of fit. */ > >> + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); > >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); > >> + aml_append(method, ifctx); > >> + > >> + aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)), > >> + buf_size)); > > there isn't need to convert bytes to bits and store it in the same variable > > it creates confusion > > Okay, i will introduce a new variable named buf_size_bits. > > > > >> + aml_append(method, aml_create_field(buf, > >> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/ > >> + buf_size, "BUFF")); > > just inline conversion in aml_create_field() > > okay. >
diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt index 0fdd251..4aa5e3d 100644 --- a/docs/specs/acpi_nvdimm.txt +++ b/docs/specs/acpi_nvdimm.txt @@ -127,6 +127,58 @@ _DSM process diagram: | result from the page | | | +--------------------------+ +--------------+ - _FIT implementation - ------------------- - TODO (will fill it when nvdimm hotplug is introduced) +Device Handle Reservation +------------------------- +As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device +handle. The handle is completely QEMU internal thing, the values in range +[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR), +other values are reserved by other purpose. + +Current reserved handle: +0x10000 is reserved for QEMU internal DSM function called on the root +device. + +QEMU internal use only _DSM function +------------------------------------ +UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal +DSM function. + +There is the function introduced by QEMU and only used by QEMU internal. + +1) Read FIT + As we only reserved one page for NVDIMM ACPI it is impossible to map the + whole FIT data to guest's address space. This function is used by _FIT + method to read a piece of FIT data from QEMU. + + Input parameters: + Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} + Arg1 – Revision ID (set to 1) + Arg2 - Function Index, 0x1 + Arg3 - A package containing a buffer whose layout is as follows: + + +----------+-------------+-------------+-----------------------------------+ + | Filed | Byte Length | Byte Offset | Description | + +----------+-------------+-------------+-----------------------------------+ + | offset | 4 | 0 | the offset of FIT buffer | + +----------+-------------+-------------+-----------------------------------+ + + Output: + +----------+-------------+-------------+-----------------------------------+ + | Filed | Byte Length | Byte Offset | Description | + +----------+-------------+-------------+-----------------------------------+ + | | | | return status codes | + | | | | 0x100 indicates fit has been | + | status | 4 | 0 | updated | + | | | | other follows Chapter 3 in DSM | + | | | | Spec Rev1 | + +----------+-------------+-------------+-----------------------------------+ + | fit data | Varies | 4 | FIT data | + | | | | | + +----------+-------------+-------------+-----------------------------------+ + + The FIT offset is maintained by the caller itself, current offset plugs + the length returned by the function is the next offset we should read. + When all the FIT data has been read out, zero length is returned. + + If it returns 0x100, OSPM should restart to read FIT (read from offset 0 + again). diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 5f728a6..fc1a012 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn; QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + offsetof(NvdimmDsmIn, arg3) > 4096); +struct NvdimmFuncReadFITIn { + uint32_t offset; /* the offset of FIT buffer. */ +} QEMU_PACKED; +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn; +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) + + offsetof(NvdimmDsmIn, arg3) > 4096); + +struct NvdimmFuncReadFITOut { + /* the size of buffer filled by QEMU. */ + uint32_t len; + uint32_t func_ret_status; /* return status code. */ + uint8_t fit[0]; /* the FIT data. */ +} QEMU_PACKED; +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut; +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096); + static void nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr) { @@ -516,6 +532,74 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr) cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out)); } +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000 + +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, + hwaddr dsm_mem_addr) +{ + NvdimmFitBuffer *fit_buf = &state->fit_buf; + NvdimmFuncReadFITIn *read_fit; + NvdimmFuncReadFITOut *read_fit_out; + GArray *fit; + uint32_t read_len = 0, func_ret_status; + int size; + + read_fit = (NvdimmFuncReadFITIn *)in->arg3; + le32_to_cpus(&read_fit->offset); + + qemu_mutex_lock(&fit_buf->lock); + fit = fit_buf->fit; + + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n", + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No"); + + if (read_fit->offset > fit->len) { + func_ret_status = 3 /* Invalid Input Parameters */; + goto exit; + } + + /* It is the first time to read FIT. */ + if (!read_fit->offset) { + fit_buf->dirty = false; + } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */ + func_ret_status = 0x100 /* fit changed */; + goto exit; + } + + func_ret_status = 0 /* Success */; + read_len = MIN(fit->len - read_fit->offset, + 4096 - sizeof(NvdimmFuncReadFITOut)); + +exit: + size = sizeof(NvdimmFuncReadFITOut) + read_len; + read_fit_out = g_malloc(size); + + read_fit_out->len = cpu_to_le32(size); + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status); + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len); + + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size); + + g_free(read_fit_out); + qemu_mutex_unlock(&fit_buf->lock); +} + +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, + hwaddr dsm_mem_addr) +{ + switch (in->function) { + case 0x0: + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr); + return; + case 0x1 /*Read FIT */: + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr); + return; + } + + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr); +} + static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr) { /* @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) static void nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { + AcpiNVDIMMState *state = opaque; NvdimmDsmIn *in; hwaddr dsm_mem_addr = val; @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) goto exit; } + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) { + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr); + goto exit; + } + /* Handle 0 is reserved for NVDIMM Root Device. */ if (!in->handle) { nvdimm_dsm_root(in, dsm_mem_addr); @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN" #define NVDIMM_DSM_OUT_BUF "ODAT" +#define NVDIMM_DSM_RFIT_STATUS "RSTA" + +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62" + static void nvdimm_build_common_dsm(Aml *dev) { - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem; + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2; Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size; uint8_t byte_list[1]; @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev) /* UUID for NVDIMM Root Device */, expected_uuid)); aml_append(method, ifctx); elsectx = aml_else(); - aml_append(elsectx, aml_store( + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT))); + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID + /* UUID for QEMU internal use */), expected_uuid)); + aml_append(elsectx, ifctx); + elsectx2 = aml_else(); + aml_append(elsectx2, aml_store( aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66") /* UUID for NVDIMM Devices */, expected_uuid)); + aml_append(elsectx, elsectx2); aml_append(method, elsectx); uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid)); @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) aml_append(dev, method); } +static void nvdimm_build_fit(Aml *dev) +{ + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit; + + buf = aml_local(0); + buf_size = aml_local(1); + fit = aml_local(2); + + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL), + aml_int(0), NVDIMM_DSM_RFIT_STATUS)); + + /* build helper function, RFIT. */ + method = aml_method("RFIT", 1, AML_SERIALIZED); + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), + aml_int(0), "OFST")); + + /* prepare input package. */ + pkg = aml_package(1); + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); + aml_append(pkg, aml_name("OFST")); + + /* call Read_FIT function. */ + call_result = aml_call5(NVDIMM_COMMON_DSM, + aml_touuid(NVDIMM_QEMU_RSVD_UUID), + aml_int(1) /* Revision 1 */, + aml_int(0x1) /* Read FIT */, + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)); + aml_append(method, aml_store(call_result, buf)); + + /* handle _DSM result. */ + aml_append(method, aml_create_dword_field(buf, + aml_int(0) /* offset at byte 0 */, "STAU")); + + aml_append(method, aml_store(aml_name("STAU"), + aml_name(NVDIMM_DSM_RFIT_STATUS))); + + /* if something is wrong during _DSM. */ + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); + ifctx = aml_if(aml_lnot(ifcond)); + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); + aml_append(method, ifctx); + + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); + aml_append(method, aml_subtract(buf_size, + aml_int(4) /* the size of "STAU" */, + buf_size)); + + /* if we read the end of fit. */ + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); + aml_append(method, ifctx); + + aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)), + buf_size)); + aml_append(method, aml_create_field(buf, + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/ + buf_size, "BUFF")); + aml_append(method, aml_return(aml_name("BUFF"))); + aml_append(dev, method); + + /* build _FIT. */ + method = aml_method("_FIT", 0, AML_SERIALIZED); + offset = aml_local(3); + + aml_append(method, aml_store(aml_buffer(0, NULL), fit)); + aml_append(method, aml_store(aml_int(0), offset)); + + whilectx = aml_while(aml_int(1)); + aml_append(whilectx, aml_store(aml_call1("RFIT", offset), buf)); + aml_append(whilectx, aml_store(aml_sizeof(buf), buf_size)); + + /* + * if fit buffer was changed during RFIT, read from the beginning + * again. + */ + ifctx = aml_if(aml_equal(aml_name(NVDIMM_DSM_RFIT_STATUS), + aml_int(0x100 /* fit changed */))); + aml_append(ifctx, aml_store(aml_buffer(0, NULL), fit)); + aml_append(ifctx, aml_store(aml_int(0), offset)); + aml_append(whilectx, ifctx); + + elsectx = aml_else(); + + /* finish fit read if no data is read out. */ + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); + aml_append(ifctx, aml_return(fit)); + aml_append(elsectx, ifctx); + + /* update the offset. */ + aml_append(elsectx, aml_add(offset, buf_size, offset)); + /* append the data we read out to the fit buffer. */ + aml_append(elsectx, aml_concatenate(fit, buf, fit)); + aml_append(whilectx, elsectx); + aml_append(method, whilectx); + + aml_append(dev, method); +} + static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots) { uint32_t slot; @@ -1052,6 +1251,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data, /* 0 is reserved for root device. */ nvdimm_build_device_dsm(dev, 0); + nvdimm_build_fit(dev); nvdimm_build_nvdimm_devices(dev, ram_slots);
_FIT is required for hotplug support, guest will inquire the updated device info from it if a hotplug event is received As FIT buffer is not completely mapped into guest address space, so a new function, Read FIT whose UUID is UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer is concatenated before _FIT return Refer to docs/specs/acpi-nvdimm.txt for detailed design Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- docs/specs/acpi_nvdimm.txt | 58 ++++++++++++- hw/acpi/nvdimm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 257 insertions(+), 5 deletions(-)