| Message ID | 1456178130-26468-2-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Toshi Kani [mailto:toshi.kani@hpe.com] > Sent: Monday, February 22, 2016 1:55 PM > To: rjw@rjwysocki.net; Williams, Dan J > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > devel@acpica.org; Toshi Kani > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to > comply ACPI 6.1 > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as follows. > - Valid Fields, Manufacturing Location, and Manufacturing Date > are added from reserved range. No change in the structure size. > - IDs defined as SPD values are arrays of bytes. The spec > clarified that they need to be represented as arrays of bytes > as well. > > This patch makes the following changes to support this update. > - Change 'struct acpi_nfit_control_region' to reflect the update. > SPD IDs are defined as arrays of bytes, so that they can be > treated in the same way regardless of CPU endianness and are > not miss-treated as little-endian numeric values. I don't think we are going to start changing the ACPI tables defined in the ACPICA headers because of this. We do in fact have macros for this purpose. > - Change the NFIT driver to show SPD ID values as array of bytes > in sysfs. > - Change sprintf format to use "0x" instead of "#" since "%#02x" > does not prepend '0' in some reason. > > link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Robert Moore <robert.moore@intel.com> > Cc: Lv Zheng <lv.zheng@intel.com> > Cc: Robert Elliott <elliott@hpe.com> > Cc: <devel@acpica.org> > --- > drivers/acpi/nfit.c | 20 +++++++++++++------- > include/acpi/actbl1.h | 24 +++++++++++++++--------- > 2 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index > ad6d8c6..4982a18 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -722,7 +722,8 @@ static ssize_t vendor_show(struct device *dev, { > struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); > > - return sprintf(buf, "%#x\n", dcr->vendor_id); > + return sprintf(buf, "0x%02x%02x\n", > + dcr->vendor_id[0], dcr->vendor_id[1]); > } > static DEVICE_ATTR_RO(vendor); > > @@ -731,7 +732,8 @@ static ssize_t rev_id_show(struct device *dev, { > struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); > > - return sprintf(buf, "%#x\n", dcr->revision_id); > + return sprintf(buf, "0x%02x%02x\n", > + dcr->revision_id[0], dcr->revision_id[1]); > } > static DEVICE_ATTR_RO(rev_id); > > @@ -740,7 +742,8 @@ static ssize_t device_show(struct device *dev, { > struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); > > - return sprintf(buf, "%#x\n", dcr->device_id); > + return sprintf(buf, "0x%02x%02x\n", > + dcr->device_id[0], dcr->device_id[1]); > } > static DEVICE_ATTR_RO(device); > > @@ -749,7 +752,7 @@ static ssize_t format_show(struct device *dev, { > struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); > > - return sprintf(buf, "%#x\n", dcr->code); > + return sprintf(buf, "0x%02x%02x\n", dcr->code[0], dcr->code[1]); > } > static DEVICE_ATTR_RO(format); > > @@ -758,7 +761,9 @@ static ssize_t serial_show(struct device *dev, { > struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); > > - return sprintf(buf, "%#x\n", dcr->serial_number); > + return sprintf(buf, "0x%02x%02x%02x%02x\n", > + dcr->serial_number[0], dcr->serial_number[1], > + dcr->serial_number[2], dcr->serial_number[3]); > } > static DEVICE_ATTR_RO(serial); > > @@ -956,7 +961,7 @@ static const struct attribute_group > *acpi_nfit_region_attribute_groups[] = { struct nfit_set_info { > struct nfit_set_info_map { > u64 region_offset; > - u32 serial_number; > + u8 serial_number[4]; > u32 pad; > } mapping[0]; > }; > @@ -1025,7 +1030,8 @@ static int acpi_nfit_init_interleave_set(struct > acpi_nfit_desc *acpi_desc, > } > > map->region_offset = memdev->region_offset; > - map->serial_number = nfit_mem->dcr->serial_number; > + memcpy(map->serial_number, nfit_mem->dcr->serial_number, > + sizeof(map->serial_number)); > } > > sort(&info->mapping[0], nr, sizeof(struct nfit_set_info_map), diff - > -git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index > 16e0136..d8df62c 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -1040,15 +1040,18 @@ struct acpi_nfit_smbios { struct > acpi_nfit_control_region { > struct acpi_nfit_header header; > u16 region_index; > - u16 vendor_id; > - u16 device_id; > - u16 revision_id; > - u16 subsystem_vendor_id; > - u16 subsystem_device_id; > - u16 subsystem_revision_id; > - u8 reserved[6]; /* Reserved, must be zero */ > - u32 serial_number; > - u16 code; > + u8 vendor_id[2]; > + u8 device_id[2]; > + u8 revision_id[2]; > + u8 subsystem_vendor_id[2]; > + u8 subsystem_device_id[2]; > + u8 subsystem_revision_id[2]; > + u8 valid_fields; > + u8 manufacturing_location; > + u8 manufacturing_date[2]; > + u8 reserved[2]; /* Reserved, must be zero */ > + u8 serial_number[4]; > + u8 code[2]; > u16 windows; > u64 window_size; > u64 command_offset; > @@ -1059,6 +1062,9 @@ struct acpi_nfit_control_region { > u8 reserved1[6]; /* Reserved, must be zero */ > }; > > +/* Valid Fields */ > +#define ACPI_NFIT_CONTROL_MFG_INFO_VALID (1) /* Manufacturing fields > are valid */ > + > /* Flags */ > > #define ACPI_NFIT_CONTROL_BUFFERED (1) /* Block Data Windows > implementation is buffered */
We have a bunch of macros in include/acmacros.h -- like this: ACPI_MOVE_16_TO_16(d, s) > -----Original Message----- > From: Toshi Kani [mailto:toshi.kani@hpe.com] > Sent: Tuesday, March 01, 2016 8:38 AM > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to > comply ACPI 6.1 > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote: > > > > > -----Original Message----- > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > Sent: Monday, February 22, 2016 1:55 PM > > > To: rjw@rjwysocki.net; Williams, Dan J > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org; > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure > > > to comply ACPI 6.1 > > > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as > > > follows. > > > - Valid Fields, Manufacturing Location, and Manufacturing Date > > > are added from reserved range. No change in the structure size. > > > - IDs defined as SPD values are arrays of bytes. The spec > > > clarified that they need to be represented as arrays of bytes > > > as well. > > > > > > This patch makes the following changes to support this update. > > > - Change 'struct acpi_nfit_control_region' to reflect the update. > > > SPD IDs are defined as arrays of bytes, so that they can be > > > treated in the same way regardless of CPU endianness and are > > > not miss-treated as little-endian numeric values. > > > > > > I don't think we are going to start changing the ACPI tables defined > > in the ACPICA headers because of this. We do in fact have macros for > > this purpose. > > Can you elaborate what macros you suggest to use for this purpose? > > Thanks, > -Toshi
On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote: > > > -----Original Message----- > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > Sent: Monday, February 22, 2016 1:55 PM > > To: rjw@rjwysocki.net; Williams, Dan J > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.or > > g; > > linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > > devel@acpica.org; Toshi Kani > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to > > comply ACPI 6.1 > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as > > follows. > > - Valid Fields, Manufacturing Location, and Manufacturing Date > > are added from reserved range. No change in the structure size. > > - IDs defined as SPD values are arrays of bytes. The spec > > clarified that they need to be represented as arrays of bytes > > as well. > > > > This patch makes the following changes to support this update. > > - Change 'struct acpi_nfit_control_region' to reflect the update. > > SPD IDs are defined as arrays of bytes, so that they can be > > treated in the same way regardless of CPU endianness and are > > not miss-treated as little-endian numeric values. > > > I don't think we are going to start changing the ACPI tables defined in > the ACPICA headers because of this. We do in fact have macros for this > purpose. Can you elaborate what macros you suggest to use for this purpose? Thanks, -Toshi
On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: > We have a bunch of macros in include/acmacros.h -- like this: > > ACPI_MOVE_16_TO_16(d, s) There is a problem in using the ACPICA byte-swap macros. ACPI is little- endian arch, so the macros are set to perform byte-swappings when the CPU arch is big-endian. This case, however, is the other way around. The fields in question are defined & stored as arrays of bytes. If you treat them as multi-bytes numeric values, then you need to byte-swap them when the CPU arch is little-endian because arrays of bytes have the same addressing as big-endian. Another issue is that it is not clear who needs to perform the byte- swapping among ACPICA and drivers. If ACPICA, drivers must agree that these fields are always treated as multi-bytes numeric values despite of the spec. If drivers, we need to make sure that only a single driver performs this byte-swapping one time as ACPI tables are global structures. I think it is much clearer to define the structure according to the ACPI spec. Thanks, -Toshi > > -----Original Message----- > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > Sent: Tuesday, March 01, 2016 8:38 AM > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure > > to > > comply ACPI 6.1 > > > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote: > > > > > > > -----Original Message----- > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > > Sent: Monday, February 22, 2016 1:55 PM > > > > To: rjw@rjwysocki.net; Williams, Dan J > > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; > > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org; > > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani > > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure > > > > to comply ACPI 6.1 > > > > > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as > > > > follows. > > > > - Valid Fields, Manufacturing Location, and Manufacturing Date > > > > are added from reserved range. No change in the structure size. > > > > - IDs defined as SPD values are arrays of bytes. The spec > > > > clarified that they need to be represented as arrays of bytes > > > > as well. > > > > > > > > This patch makes the following changes to support this update. > > > > - Change 'struct acpi_nfit_control_region' to reflect the update. > > > > SPD IDs are defined as arrays of bytes, so that they can be > > > > treated in the same way regardless of CPU endianness and are > > > > not miss-treated as little-endian numeric values. > > > > > > > > > I don't think we are going to start changing the ACPI tables defined > > > in the ACPICA headers because of this. We do in fact have macros for > > > this purpose. > > > > Can you elaborate what macros you suggest to use for this purpose? > > > > Thanks, > > -Toshi > N?????r??y???b?X???v?^?)?{.n?+????{?i?b?{ay? ???,j??f???h???z? ?w??? > ???j:+v???w?j?m????????zZ+??????j"??!?i
> -----Original Message----- > From: Toshi Kani [mailto:toshi.kani@hpe.com] > Sent: Tuesday, March 01, 2016 9:37 AM > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure to > comply ACPI 6.1 > > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: > > We have a bunch of macros in include/acmacros.h -- like this: > > > > ACPI_MOVE_16_TO_16(d, s) > > There is a problem in using the ACPICA byte-swap macros. ACPI is little- > endian arch, so the macros are set to perform byte-swappings when the CPU > arch is big-endian. This case, however, is the other way around. The > fields in question are defined & stored as arrays of bytes. That's not what I see in the ACPI spec. The fields are defined like any other ACPI table. Vendor ID 2 6 Identifier indicating the vendor of the NVDIMM. This field shall be set to the value of the NVDIMM SPD Module Manufacturer ID Code field a with byte 0 set to DDR4 SPD byte 320 and byte 1 set to DDR4 SPD byte 321. Device ID 2 8 Identifier for the NVDIMM, assigned by the module vendor. This field shall be set to the value of the NVDIMM SPD Module Product Identifier field b with byte 0 set to SPD byte 192 and byte 1 set to SPD byte 193. Revision ID 2 10 Revision of the NVDIMM, assigned by the module vendor. Byte 1 of this field is reserved. Byte 0 of this field shall be set to the value of the NVDIMM SPD Module Revision Code field a (i.e., SPD byte 349). Etc. If you treat > them as multi-bytes numeric values, then you need to byte-swap them when > the CPU arch is little-endian because arrays of bytes have the same > addressing as big-endian. > > Another issue is that it is not clear who needs to perform the byte- > swapping among ACPICA and drivers. If ACPICA, drivers must agree that > these fields are always treated as multi-bytes numeric values despite of > the spec. If drivers, we need to make sure that only a single driver > performs this byte-swapping one time as ACPI tables are global structures. > > I think it is much clearer to define the structure according to the ACPI > spec. > > Thanks, > -Toshi > > > > -----Original Message----- > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > Sent: Tuesday, March 01, 2016 8:38 AM > > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region > > > Structure to comply ACPI 6.1 > > > > > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote: > > > > > > > > > -----Original Message----- > > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > > > Sent: Monday, February 22, 2016 1:55 PM > > > > > To: rjw@rjwysocki.net; Williams, Dan J > > > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; > > > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org; > > > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani > > > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region > > > > > Structure to comply ACPI 6.1 > > > > > > > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure > > > > > as follows. > > > > > - Valid Fields, Manufacturing Location, and Manufacturing Date > > > > > are added from reserved range. No change in the structure > size. > > > > > - IDs defined as SPD values are arrays of bytes. The spec > > > > > clarified that they need to be represented as arrays of bytes > > > > > as well. > > > > > > > > > > This patch makes the following changes to support this update. > > > > > - Change 'struct acpi_nfit_control_region' to reflect the update. > > > > > SPD IDs are defined as arrays of bytes, so that they can be > > > > > treated in the same way regardless of CPU endianness and are > > > > > not miss-treated as little-endian numeric values. > > > > > > > > > > > > I don't think we are going to start changing the ACPI tables > > > > defined in the ACPICA headers because of this. We do in fact have > > > > macros for this purpose. > > > > > > Can you elaborate what macros you suggest to use for this purpose? > > > > > > Thanks, > > > -Toshi > > N r y b X ?v ^ )?{.n + { i b {ay ?? ,j f h z w > > > > j:+v w j m zZ+ ?j" ! i
> > Another issue is that it is not clear who needs to perform the byte- > > swapping among ACPICA and drivers. If ACPICA, drivers must agree that ACPICA does not ever do anything with the "data tables" like NFIT, other than handing off the table when requested by a driver. > -----Original Message----- > From: Devel [mailto:devel-bounces@acpica.org] On Behalf Of Moore, Robert > Sent: Tuesday, March 01, 2016 9:37 AM > To: Toshi Kani; rjw@rjwysocki.net; Williams, Dan J > Cc: linux-nvdimm@lists.01.org; linux-kernel@vger.kernel.org; linux- > acpi@vger.kernel.org; elliott@hpe.com; devel@acpica.org > Subject: Re: [Devel] [PATCH v2 1/3] ACPI/NFIT: Update Control Region > Structure to comply ACPI 6.1 > > > > > -----Original Message----- > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > Sent: Tuesday, March 01, 2016 9:37 AM > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure > > to comply ACPI 6.1 > > > > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: > > > We have a bunch of macros in include/acmacros.h -- like this: > > > > > > ACPI_MOVE_16_TO_16(d, s) > > > > There is a problem in using the ACPICA byte-swap macros. ACPI is > > little- endian arch, so the macros are set to perform byte-swappings > > when the CPU arch is big-endian. This case, however, is the other way > > around. The fields in question are defined & stored as arrays of bytes. > > That's not what I see in the ACPI spec. The fields are defined like any > other ACPI table. > > Vendor ID 2 6 > Identifier indicating the vendor of the NVDIMM. > This field shall be set to the value of the NVDIMM SPD Module Manufacturer > ID Code field a with byte 0 set to DDR4 SPD byte > 320 and byte 1 set to DDR4 SPD byte 321. > > Device ID 2 8 > Identifier for the NVDIMM, assigned by the module vendor. > This field shall be set to the value of the NVDIMM SPD Module Product > Identifier field b with byte 0 set to SPD byte 192 and byte 1 set to SPD > byte 193. > > Revision ID 2 10 > Revision of the NVDIMM, assigned by the module vendor. > Byte 1 of this field is reserved. > Byte 0 of this field shall be set to the value of the NVDIMM SPD Module > Revision Code field a (i.e., SPD byte 349). > > > Etc. > > > If you treat > > them as multi-bytes numeric values, then you need to byte-swap them when > > the CPU arch is little-endian because arrays of bytes have the same > > addressing as big-endian. > > > > Another issue is that it is not clear who needs to perform the byte- > > swapping among ACPICA and drivers. If ACPICA, drivers must agree that > > these fields are always treated as multi-bytes numeric values despite of > > the spec. If drivers, we need to make sure that only a single driver > > performs this byte-swapping one time as ACPI tables are global > structures. > > > > I think it is much clearer to define the structure according to the ACPI > > spec. > > > > Thanks, > > -Toshi > > > > > > -----Original Message----- > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > > Sent: Tuesday, March 01, 2016 8:38 AM > > > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > > > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > > > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > > > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region > > > > Structure to comply ACPI 6.1 > > > > > > > > On Tue, 2016-03-01 at 15:13 +0000, Moore, Robert wrote: > > > > > > > > > > > -----Original Message----- > > > > > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > > > > > Sent: Monday, February 22, 2016 1:55 PM > > > > > > To: rjw@rjwysocki.net; Williams, Dan J > > > > > > Cc: Moore, Robert; Zheng, Lv; elliott@hpe.com; > > > > > > linux-nvdimm@lists.01.or g; linux-acpi@vger.kernel.org; > > > > > > linux-kernel@vger.kernel.org; devel@acpica.org; Toshi Kani > > > > > > Subject: [PATCH v2 1/3] ACPI/NFIT: Update Control Region > > > > > > Structure to comply ACPI 6.1 > > > > > > > > > > > > ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure > > > > > > as follows. > > > > > > - Valid Fields, Manufacturing Location, and Manufacturing Date > > > > > > are added from reserved range. No change in the structure > > size. > > > > > > - IDs defined as SPD values are arrays of bytes. The spec > > > > > > clarified that they need to be represented as arrays of bytes > > > > > > as well. > > > > > > > > > > > > This patch makes the following changes to support this update. > > > > > > - Change 'struct acpi_nfit_control_region' to reflect the > update. > > > > > > SPD IDs are defined as arrays of bytes, so that they can be > > > > > > treated in the same way regardless of CPU endianness and are > > > > > > not miss-treated as little-endian numeric values. > > > > > > > > > > > > > > > I don't think we are going to start changing the ACPI tables > > > > > defined in the ACPICA headers because of this. We do in fact have > > > > > macros for this purpose. > > > > > > > > Can you elaborate what macros you suggest to use for this purpose? > > > > > > > > Thanks, > > > > -Toshi > > > N r y b X ?v ^ )?{.n + { i b {ay ?? ,j f h z w > > > > > > > j:+v w j m zZ+ ?j" ! i > _______________________________________________ > Devel mailing list > Devel@acpica.org > https://lists.acpica.org/mailman/listinfo/devel
On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote: > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: >> We have a bunch of macros in include/acmacros.h -- like this: >> >> ACPI_MOVE_16_TO_16(d, s) > > There is a problem in using the ACPICA byte-swap macros. ACPI is little- > endian arch, so the macros are set to perform byte-swappings when the CPU > arch is big-endian. This case, however, is the other way around. The > fields in question are defined & stored as arrays of bytes. If you treat > them as multi-bytes numeric values, then you need to byte-swap them when > the CPU arch is little-endian because arrays of bytes have the same > addressing as big-endian. > > Another issue is that it is not clear who needs to perform the byte- > swapping among ACPICA and drivers. If ACPICA, drivers must agree that > these fields are always treated as multi-bytes numeric values despite of > the spec. If drivers, we need to make sure that only a single driver > performs this byte-swapping one time as ACPI tables are global structures. > > I think it is much clearer to define the structure according to the ACPI > spec. I think the "ACPI tables are little-endian" assumption is pervasive throughout the implementation. Toshi, it seems all we need is conversions like: - sprintf(buf, "%#x\n", dcr->vendor_id); + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id)); ...for the values exported to userspace through sysfs, but otherwise leave the base table definitions as is. Will this suffice?
On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote: > > > -----Original Message----- > > From: Toshi Kani [mailto:toshi.kani@hpe.com] > > Sent: Tuesday, March 01, 2016 9:37 AM > > To: Moore, Robert; rjw@rjwysocki.net; Williams, Dan J > > Cc: Zheng, Lv; elliott@hpe.com; linux-nvdimm@lists.01.org; linux- > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; devel@acpica.org > > Subject: Re: [PATCH v2 1/3] ACPI/NFIT: Update Control Region Structure > > to > > comply ACPI 6.1 > > > > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: > > > We have a bunch of macros in include/acmacros.h -- like this: > > > > > > ACPI_MOVE_16_TO_16(d, s) > > > > There is a problem in using the ACPICA byte-swap macros. ACPI is > > little-endian arch, so the macros are set to perform byte-swappings > > when the CPU arch is big-endian. This case, however, is the other way > > around. The fields in question are defined & stored as arrays of > > bytes. > > That's not what I see in the ACPI spec. The fields are defined like any > other ACPI table. > > Vendor ID 2 6 > Identifier indicating the vendor of the NVDIMM. > This field shall be set to the value of the NVDIMM SPD Module > Manufacturer ID Code field a with byte 0 set to DDR4 SPD byte > 320 and byte 1 set to DDR4 SPD byte 321. They are different from other fields because the spec defines "byte locations" of the fields. The above case, Vendor ID, is defined that: - byte 0 set to DDR4 SPD byte 320 - byte 1 set to DDR4 SPD byte 321 For instance, if SPD byte 320 is 0x12 and 321 is 0x34, then this field is stored as (0x12, 0x34). If you read this field as a 16-bit int value, you will get 0x3412 on little-endian CPUs, such as x86. Section 5.2.25.9 of ACPI 6.1 further clarifies that Vendor ID needs to be represented as ("%02x%02x", byte0, byte1), which is "1234" in this case. So, we will need to byte-swap when it is handled as a 16-bit int value on little-endian CPUs. This is different from other fields with multi-byte numeric values, which are stored in little-endian format in ACPI. Hence, my patch avoids this byte-swapping as I described in the change log below. | - Change 'struct acpi_nfit_control_region' to reflect the update. | SPD IDs are defined as arrays of bytes, so that they can be | treated in the same way regardless of CPU endianness and are | not miss-treated as little-endian numeric values. I hope this makes it clear now. > > Another issue is that it is not clear who needs to perform the byte- > > swapping among ACPICA and drivers. If ACPICA, drivers must agree that > > ACPICA does not ever do anything with the "data tables" like NFIT, other > than handing off the table when requested by a driver. So, this means that the alternative is to change the NFIT driver to do the byte-swapping for the fields when the CPU arch is little-endian. If we cannot change the structure, we will have to take this course... Thanks, -Toshi
On Tue, 2016-03-01 at 10:14 -0800, Dan Williams wrote: > On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote: > > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: > > > We have a bunch of macros in include/acmacros.h -- like this: > > > > > > ACPI_MOVE_16_TO_16(d, s) > > > > There is a problem in using the ACPICA byte-swap macros. ACPI is > > little- > > endian arch, so the macros are set to perform byte-swappings when the > > CPU > > arch is big-endian. This case, however, is the other way around. The > > fields in question are defined & stored as arrays of bytes. If you > > treat > > them as multi-bytes numeric values, then you need to byte-swap them > > when > > the CPU arch is little-endian because arrays of bytes have the same > > addressing as big-endian. > > > > Another issue is that it is not clear who needs to perform the byte- > > swapping among ACPICA and drivers. If ACPICA, drivers must agree that > > these fields are always treated as multi-bytes numeric values despite > > of > > the spec. If drivers, we need to make sure that only a single driver > > performs this byte-swapping one time as ACPI tables are global > > structures. > > > > I think it is much clearer to define the structure according to the > > ACPI > > spec. > > I think the "ACPI tables are little-endian" assumption is pervasive > throughout the implementation. > > Toshi, it seems all we need is conversions like: > > - sprintf(buf, "%#x\n", dcr->vendor_id); > + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id)); > > ...for the values exported to userspace through sysfs, but otherwise > leave the base table definitions as is. Will this suffice? Yes, I am OK to go with this option to get it done. We still need to add new fields into the structure. Is it OK to make this change? If not, we will need to have a local structure to define the new fields... Thanks, -Toshi
On Tue, 2016-03-01 at 10:14 -0800, Dan Williams wrote: > On Tue, Mar 1, 2016 at 9:36 AM, Toshi Kani <toshi.kani@hpe.com> wrote: > > On Tue, 2016-03-01 at 16:03 +0000, Moore, Robert wrote: : > I think the "ACPI tables are little-endian" assumption is pervasive > throughout the implementation. > > Toshi, it seems all we need is conversions like: > > - sprintf(buf, "%#x\n", dcr->vendor_id); > + sprintf(buf, "%#x\n", le16_to_cpu(dcr->vendor_id)); nit - I think it needs to be be16_to_cpu() if I understand this macro correctly. -Toshi > > ...for the values exported to userspace through sysfs, but otherwise > leave the base table definitions as is. Will this suffice?
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index ad6d8c6..4982a18 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -722,7 +722,8 @@ static ssize_t vendor_show(struct device *dev, { struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); - return sprintf(buf, "%#x\n", dcr->vendor_id); + return sprintf(buf, "0x%02x%02x\n", + dcr->vendor_id[0], dcr->vendor_id[1]); } static DEVICE_ATTR_RO(vendor); @@ -731,7 +732,8 @@ static ssize_t rev_id_show(struct device *dev, { struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); - return sprintf(buf, "%#x\n", dcr->revision_id); + return sprintf(buf, "0x%02x%02x\n", + dcr->revision_id[0], dcr->revision_id[1]); } static DEVICE_ATTR_RO(rev_id); @@ -740,7 +742,8 @@ static ssize_t device_show(struct device *dev, { struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); - return sprintf(buf, "%#x\n", dcr->device_id); + return sprintf(buf, "0x%02x%02x\n", + dcr->device_id[0], dcr->device_id[1]); } static DEVICE_ATTR_RO(device); @@ -749,7 +752,7 @@ static ssize_t format_show(struct device *dev, { struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); - return sprintf(buf, "%#x\n", dcr->code); + return sprintf(buf, "0x%02x%02x\n", dcr->code[0], dcr->code[1]); } static DEVICE_ATTR_RO(format); @@ -758,7 +761,9 @@ static ssize_t serial_show(struct device *dev, { struct acpi_nfit_control_region *dcr = to_nfit_dcr(dev); - return sprintf(buf, "%#x\n", dcr->serial_number); + return sprintf(buf, "0x%02x%02x%02x%02x\n", + dcr->serial_number[0], dcr->serial_number[1], + dcr->serial_number[2], dcr->serial_number[3]); } static DEVICE_ATTR_RO(serial); @@ -956,7 +961,7 @@ static const struct attribute_group *acpi_nfit_region_attribute_groups[] = { struct nfit_set_info { struct nfit_set_info_map { u64 region_offset; - u32 serial_number; + u8 serial_number[4]; u32 pad; } mapping[0]; }; @@ -1025,7 +1030,8 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, } map->region_offset = memdev->region_offset; - map->serial_number = nfit_mem->dcr->serial_number; + memcpy(map->serial_number, nfit_mem->dcr->serial_number, + sizeof(map->serial_number)); } sort(&info->mapping[0], nr, sizeof(struct nfit_set_info_map), diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index 16e0136..d8df62c 100644 --- a/include/acpi/actbl1.h +++ b/include/acpi/actbl1.h @@ -1040,15 +1040,18 @@ struct acpi_nfit_smbios { struct acpi_nfit_control_region { struct acpi_nfit_header header; u16 region_index; - u16 vendor_id; - u16 device_id; - u16 revision_id; - u16 subsystem_vendor_id; - u16 subsystem_device_id; - u16 subsystem_revision_id; - u8 reserved[6]; /* Reserved, must be zero */ - u32 serial_number; - u16 code; + u8 vendor_id[2]; + u8 device_id[2]; + u8 revision_id[2]; + u8 subsystem_vendor_id[2]; + u8 subsystem_device_id[2]; + u8 subsystem_revision_id[2]; + u8 valid_fields; + u8 manufacturing_location; + u8 manufacturing_date[2]; + u8 reserved[2]; /* Reserved, must be zero */ + u8 serial_number[4]; + u8 code[2]; u16 windows; u64 window_size; u64 command_offset; @@ -1059,6 +1062,9 @@ struct acpi_nfit_control_region { u8 reserved1[6]; /* Reserved, must be zero */ }; +/* Valid Fields */ +#define ACPI_NFIT_CONTROL_MFG_INFO_VALID (1) /* Manufacturing fields are valid */ + /* Flags */ #define ACPI_NFIT_CONTROL_BUFFERED (1) /* Block Data Windows implementation is buffered */
ACPI 6.1, Table 5-133, updates NVDIMM Control Region Structure as follows. - Valid Fields, Manufacturing Location, and Manufacturing Date are added from reserved range. No change in the structure size. - IDs defined as SPD values are arrays of bytes. The spec clarified that they need to be represented as arrays of bytes as well. This patch makes the following changes to support this update. - Change 'struct acpi_nfit_control_region' to reflect the update. SPD IDs are defined as arrays of bytes, so that they can be treated in the same way regardless of CPU endianness and are not miss-treated as little-endian numeric values. - Change the NFIT driver to show SPD ID values as array of bytes in sysfs. - Change sprintf format to use "0x" instead of "#" since "%#02x" does not prepend '0' in some reason. link: http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Robert Moore <robert.moore@intel.com> Cc: Lv Zheng <lv.zheng@intel.com> Cc: Robert Elliott <elliott@hpe.com> Cc: <devel@acpica.org> --- drivers/acpi/nfit.c | 20 +++++++++++++------- include/acpi/actbl1.h | 24 +++++++++++++++--------- 2 files changed, 28 insertions(+), 16 deletions(-)