| Message ID | 1470984850-66891-4-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved > by QEMU to read the piece of FIT buffer. The buffer is concatenated > before _FIT return Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve 0xFFFFFFFF for some purposes. So spec should be amended first or custom generated UUID should be used. > > Refer to docs/specs/acpi-nvdimm.txt for detailed design and amend docs to reflect that. > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 82 insertions(+) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 0e2b9f0..4bbd1e7 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -886,6 +886,87 @@ 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, *fit; > + > + buf = aml_local(0); > + buf_size = aml_local(1); > + fit = aml_local(2); > + > + /* build helper function, RFIT. */ > + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); since you create named fields (global variable) in method scope, you should make method serialized. Same goes for _FIT method. > + 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("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" > + /* UUID for NVDIMM Root Device */), > + aml_int(1) /* Revision 1 */, > + aml_int(0xFFFFFFFF) /* Read FIT. */, > + pkg, aml_int(0) /* for root device. */); > + 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")); > + > + /* 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)); Since you handle error case the same as EOF case you could replace it with EOF case here and on qemu side of interface as well. That should simplify code a bit as you won't need to strip out func_ret_status. > + > + /* 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_NOTSERIALIZED); > + 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)); > + > + /* 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(whilectx, ifctx); > + > + /* update the offset. */ > + aml_append(whilectx, aml_add(offset, buf_size, offset)); > + /* append the data we read out to the fit buffer. */ > + aml_append(whilectx, aml_concatenate(fit, buf, fit)); > + aml_append(method, whilectx); > + > + aml_append(dev, method); > +} > + > static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots) > { > uint32_t slot; > @@ -1001,6 +1082,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 09/30/2016 09:14 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved >> by QEMU to read the piece of FIT buffer. The buffer is concatenated >> before _FIT return > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > 0xFFFFFFFF for some purposes. > So spec should be amended first or custom generated UUID should be used. Okay. I will change the changelog to reflect this fact and move the spec update to this patch. > >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > and amend docs to reflect that. Already done in the spec, i will merge the spec changes into this patch as you suggested later. > >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 82 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 0e2b9f0..4bbd1e7 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -886,6 +886,87 @@ 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, *fit; >> + >> + buf = aml_local(0); >> + buf_size = aml_local(1); >> + fit = aml_local(2); >> + >> + /* build helper function, RFIT. */ >> + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); > since you create named fields (global variable) in method scope, > you should make method serialized. Same goes for _FIT method. Indeed, will fix. > > >> + 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("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> + /* UUID for NVDIMM Root Device */), >> + aml_int(1) /* Revision 1 */, >> + aml_int(0xFFFFFFFF) /* Read FIT. */, >> + pkg, aml_int(0) /* for root device. */); >> + 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")); >> + >> + /* 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)); > > Since you handle error case the same as EOF case you could replace > it with EOF case here and on qemu side of interface as well. That should > simplify code a bit as you won't need to strip out func_ret_status. > You mean returning NULL buffer if errors happen? However, the buffer is generated by NVDIMM_COMMON_DSM function which is also used by _DSM based on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway no matter is successful or failed. Or i missed your idea?
On Sat, 8 Oct 2016 15:17:14 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 09/30/2016 09:14 PM, Igor Mammedov wrote: > > On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved > >> by QEMU to read the piece of FIT buffer. The buffer is concatenated > >> before _FIT return > > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > > 0xFFFFFFFF for some purposes. > > So spec should be amended first or custom generated UUID should be used. > > Okay. > > I will change the changelog to reflect this fact and move the spec update > to this patch. under spec, I've meant ACPI spec where this UUID is declared > > > > >> > >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > > and amend docs to reflect that. > > Already done in the spec, i will merge the spec changes into > this patch as you suggested later. > > > > >> > >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >> --- > >> hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 82 insertions(+) > >> > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >> index 0e2b9f0..4bbd1e7 100644 > >> --- a/hw/acpi/nvdimm.c > >> +++ b/hw/acpi/nvdimm.c > >> @@ -886,6 +886,87 @@ 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, *fit; > >> + > >> + buf = aml_local(0); > >> + buf_size = aml_local(1); > >> + fit = aml_local(2); > >> + > >> + /* build helper function, RFIT. */ > >> + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); > > since you create named fields (global variable) in method scope, > > you should make method serialized. Same goes for _FIT method. > > Indeed, will fix. > > > > > > >> + 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("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" > >> + /* UUID for NVDIMM Root Device */), > >> + aml_int(1) /* Revision 1 */, > >> + aml_int(0xFFFFFFFF) /* Read FIT. */, > >> + pkg, aml_int(0) /* for root device. */); > >> + 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")); > >> + > >> + /* 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)); > > > > Since you handle error case the same as EOF case you could replace > > it with EOF case here and on qemu side of interface as well. That should > > simplify code a bit as you won't need to strip out func_ret_status. > > > > You mean returning NULL buffer if errors happen? However, the buffer is > generated by NVDIMM_COMMON_DSM function which is also used by _DSM based > on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway > no matter is successful or failed. > > Or i missed your idea? ok, leave it as is. > > > >
On 10/10/2016 08:51 PM, Igor Mammedov wrote: > On Sat, 8 Oct 2016 15:17:14 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> On 09/30/2016 09:14 PM, Igor Mammedov wrote: >>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved >>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated >>>> before _FIT return >>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve >>> 0xFFFFFFFF for some purposes. >>> So spec should be amended first or custom generated UUID should be used. >> >> Okay. >> >> I will change the changelog to reflect this fact and move the spec update >> to this patch. > under spec, I've meant ACPI spec where this UUID is declared Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used in the future. I'd prefer to custom-generated UUID, however, currently the UUID is checked in OSPM, i.e, QEMU is not able to distinguish other UUIDs, so how about drop the UUID check in ACPI and pass the UUID info to QEMU?
On Mon, 10 Oct 2016 21:09:30 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 10/10/2016 08:51 PM, Igor Mammedov wrote: > > On Sat, 8 Oct 2016 15:17:14 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> On 09/30/2016 09:14 PM, Igor Mammedov wrote: > >>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved > >>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated > >>>> before _FIT return > >>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > >>> 0xFFFFFFFF for some purposes. > >>> So spec should be amended first or custom generated UUID should be used. > >> > >> Okay. > >> > >> I will change the changelog to reflect this fact and move the spec update > >> to this patch. > > under spec, I've meant ACPI spec where this UUID is declared > > Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used > in the future. > > I'd prefer to custom-generated UUID, however, currently the UUID is checked > in OSPM, i.e, QEMU is not able to distinguish other UUIDs, I'd go with custom-generated UUID > so how about > drop the UUID check in ACPI and pass the UUID info to QEMU? It's a bit late to do so as it would be qemu-guest ABI change and one would need to maintain old and new protocol.
On 10/11/2016 07:49 PM, Igor Mammedov wrote: > On Mon, 10 Oct 2016 21:09:30 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> On 10/10/2016 08:51 PM, Igor Mammedov wrote: >>> On Sat, 8 Oct 2016 15:17:14 +0800 >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: >>> >>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote: >>>>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved >>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated >>>>>> before _FIT return >>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve >>>>> 0xFFFFFFFF for some purposes. >>>>> So spec should be amended first or custom generated UUID should be used. >>>> >>>> Okay. >>>> >>>> I will change the changelog to reflect this fact and move the spec update >>>> to this patch. >>> under spec, I've meant ACPI spec where this UUID is declared >> >> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used >> in the future. >> >> I'd prefer to custom-generated UUID, however, currently the UUID is checked >> in OSPM, i.e, QEMU is not able to distinguish other UUIDs, > I'd go with custom-generated UUID > >> so how about >> drop the UUID check in ACPI and pass the UUID info to QEMU? > It's a bit late to do so as it would be qemu-guest ABI change and > one would need to maintain old and new protocol. > Okay. How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use it to indicate different UUIDs, i,e: struct NvdimmDsmIn { uint16_t handle; uint16_t uuid_type; uint32_t revision; uint32_t function; /* the remaining size in the page is used by arg3. */ union { uint8_t arg3[4084]; }; } QEMU_PACKED; typedef struct NvdimmDsmIn NvdimmDsmIn; For this case, we set uuid_type = 1 to indicate the UUID is the one used for RFIT. u16 for 'handle' is large enough as QEMU supports 255 memory devices.
On Wed, 12 Oct 2016 16:20:07 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 10/11/2016 07:49 PM, Igor Mammedov wrote: > > On Mon, 10 Oct 2016 21:09:30 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> On 10/10/2016 08:51 PM, Igor Mammedov wrote: > >>> On Sat, 8 Oct 2016 15:17:14 +0800 > >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >>> > >>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote: > >>>>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved > >>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated > >>>>>> before _FIT return > >>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > >>>>> 0xFFFFFFFF for some purposes. > >>>>> So spec should be amended first or custom generated UUID should be used. > >>>> > >>>> Okay. > >>>> > >>>> I will change the changelog to reflect this fact and move the spec update > >>>> to this patch. > >>> under spec, I've meant ACPI spec where this UUID is declared > >> > >> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used > >> in the future. > >> > >> I'd prefer to custom-generated UUID, however, currently the UUID is checked > >> in OSPM, i.e, QEMU is not able to distinguish other UUIDs, > > I'd go with custom-generated UUID > > > >> so how about > >> drop the UUID check in ACPI and pass the UUID info to QEMU? > > It's a bit late to do so as it would be qemu-guest ABI change and > > one would need to maintain old and new protocol. > > > > Okay. > > How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use > it to indicate different UUIDs, i,e: > struct NvdimmDsmIn { > uint16_t handle; > uint16_t uuid_type; > uint32_t revision; > uint32_t function; > /* the remaining size in the page is used by arg3. */ > union { > uint8_t arg3[4084]; > }; > } QEMU_PACKED; > typedef struct NvdimmDsmIn NvdimmDsmIn; > > For this case, we set uuid_type = 1 to indicate the UUID is the one used > for RFIT. > > u16 for 'handle' is large enough as QEMU supports 255 memory devices. I wouldn't do above as it needlessly complicates qemu<->OSPM protocol. Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value in docs/specs/acpi_nvdimm.txt for RFIT and keep UUID checks only on ASL side. and on QEMU side add check to guaranty that auto generated handle for DIMMs would never go into reserved range.
On 10/13/2016 09:33 PM, Igor Mammedov wrote: > On Wed, 12 Oct 2016 16:20:07 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> On 10/11/2016 07:49 PM, Igor Mammedov wrote: >>> On Mon, 10 Oct 2016 21:09:30 +0800 >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: >>> >>>> On 10/10/2016 08:51 PM, Igor Mammedov wrote: >>>>> On Sat, 8 Oct 2016 15:17:14 +0800 >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: >>>>> >>>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote: >>>>>>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved >>>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated >>>>>>>> before _FIT return >>>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve >>>>>>> 0xFFFFFFFF for some purposes. >>>>>>> So spec should be amended first or custom generated UUID should be used. >>>>>> >>>>>> Okay. >>>>>> >>>>>> I will change the changelog to reflect this fact and move the spec update >>>>>> to this patch. >>>>> under spec, I've meant ACPI spec where this UUID is declared >>>> >>>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used >>>> in the future. >>>> >>>> I'd prefer to custom-generated UUID, however, currently the UUID is checked >>>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs, >>> I'd go with custom-generated UUID >>> >>>> so how about >>>> drop the UUID check in ACPI and pass the UUID info to QEMU? >>> It's a bit late to do so as it would be qemu-guest ABI change and >>> one would need to maintain old and new protocol. >>> >> >> Okay. >> >> How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use >> it to indicate different UUIDs, i,e: >> struct NvdimmDsmIn { >> uint16_t handle; >> uint16_t uuid_type; >> uint32_t revision; >> uint32_t function; >> /* the remaining size in the page is used by arg3. */ >> union { >> uint8_t arg3[4084]; >> }; >> } QEMU_PACKED; >> typedef struct NvdimmDsmIn NvdimmDsmIn; >> >> For this case, we set uuid_type = 1 to indicate the UUID is the one used >> for RFIT. >> >> u16 for 'handle' is large enough as QEMU supports 255 memory devices. > I wouldn't do above as it needlessly complicates qemu<->OSPM > protocol. > Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value > in docs/specs/acpi_nvdimm.txt for RFIT and keep UUID checks only on ASL side. > > and on QEMU side add check to guaranty that auto generated handle for DIMMs > would never go into reserved range. > Okay, I agree. As we may need to support multiple UUIDs in the future. I'd like to document the rule for handle reservation: The handle is completely QEMU internal thing, the value in range [0, 0xFFFF] indicates nvdimm device (O means nvdimm root device named NVDR), the values out of this region are reserved by other purpose. Current reserved handles: 0x10000 is reserved for RFIT.
On Fri, 14 Oct 2016 15:43:50 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > On 10/13/2016 09:33 PM, Igor Mammedov wrote: > > On Wed, 12 Oct 2016 16:20:07 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > >> On 10/11/2016 07:49 PM, Igor Mammedov wrote: > >>> On Mon, 10 Oct 2016 21:09:30 +0800 > >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >>> > >>>> On 10/10/2016 08:51 PM, Igor Mammedov wrote: > >>>>> On Sat, 8 Oct 2016 15:17:14 +0800 > >>>>> Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >>>>> > >>>>>> On 09/30/2016 09:14 PM, Igor Mammedov wrote: > >>>>>>> On Fri, 12 Aug 2016 14:54:05 +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 function index is 0xFFFFFFFF, is reserved > >>>>>>>> by QEMU to read the piece of FIT buffer. The buffer is concatenated > >>>>>>>> before _FIT return > >>>>>>> Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > >>>>>>> 0xFFFFFFFF for some purposes. > >>>>>>> So spec should be amended first or custom generated UUID should be used. > >>>>>> > >>>>>> Okay. > >>>>>> > >>>>>> I will change the changelog to reflect this fact and move the spec update > >>>>>> to this patch. > >>>>> under spec, I've meant ACPI spec where this UUID is declared > >>>> > >>>> Er. ACPI spec just said that "0xFFFF is reserved", not sure it will be used > >>>> in the future. > >>>> > >>>> I'd prefer to custom-generated UUID, however, currently the UUID is checked > >>>> in OSPM, i.e, QEMU is not able to distinguish other UUIDs, > >>> I'd go with custom-generated UUID > >>> > >>>> so how about > >>>> drop the UUID check in ACPI and pass the UUID info to QEMU? > >>> It's a bit late to do so as it would be qemu-guest ABI change and > >>> one would need to maintain old and new protocol. > >>> > >> > >> Okay. > >> > >> How about extract 16 bits from 'handle' filed in NvdimmDsmIn buffer and use > >> it to indicate different UUIDs, i,e: > >> struct NvdimmDsmIn { > >> uint16_t handle; > >> uint16_t uuid_type; > >> uint32_t revision; > >> uint32_t function; > >> /* the remaining size in the page is used by arg3. */ > >> union { > >> uint8_t arg3[4084]; > >> }; > >> } QEMU_PACKED; > >> typedef struct NvdimmDsmIn NvdimmDsmIn; > >> > >> For this case, we set uuid_type = 1 to indicate the UUID is the one used > >> for RFIT. > >> > >> u16 for 'handle' is large enough as QEMU supports 255 memory devices. > > I wouldn't do above as it needlessly complicates qemu<->OSPM > > protocol. > > Since handle is completely internal QEMU thing we can just reserve 0xFFFFFFFF value > > in docs/specs/acpi_nvdimm.txt for RFIT and keep UUID checks only on ASL side. > > > > and on QEMU side add check to guaranty that auto generated handle for DIMMs > > would never go into reserved range. > > > > Okay, I agree. As we may need to support multiple UUIDs in the future. I'd > like to document the rule for handle reservation: > The handle is completely QEMU internal thing, the value in range [0, 0xFFFF] > indicates nvdimm device (O means nvdimm root device named NVDR), the values > out of this region are reserved by other purpose. > > Current reserved handles: > 0x10000 is reserved for RFIT. sounds fine to me
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 0e2b9f0..4bbd1e7 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -886,6 +886,87 @@ 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, *fit; + + buf = aml_local(0); + buf_size = aml_local(1); + fit = aml_local(2); + + /* build helper function, RFIT. */ + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); + 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("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" + /* UUID for NVDIMM Root Device */), + aml_int(1) /* Revision 1 */, + aml_int(0xFFFFFFFF) /* Read FIT. */, + pkg, aml_int(0) /* for root device. */); + 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")); + + /* 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_NOTSERIALIZED); + 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)); + + /* 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(whilectx, ifctx); + + /* update the offset. */ + aml_append(whilectx, aml_add(offset, buf_size, offset)); + /* append the data we read out to the fit buffer. */ + aml_append(whilectx, aml_concatenate(fit, buf, fit)); + aml_append(method, whilectx); + + aml_append(dev, method); +} + static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots) { uint32_t slot; @@ -1001,6 +1082,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 function index is 0xFFFFFFFF, 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> --- hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+)