| Message ID | 1456178130-26468-2-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Not Applicable, 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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
V2UgaGF2ZSBhIGJ1bmNoIG9mIG1hY3JvcyBpbiBpbmNsdWRlL2FjbWFjcm9zLmggLS0gbGlrZSB0 aGlzOg0KDQpBQ1BJX01PVkVfMTZfVE9fMTYoZCwgcykNCg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVz c2FnZS0tLS0tDQo+IEZyb206IFRvc2hpIEthbmkgW21haWx0bzp0b3NoaS5rYW5pQGhwZS5jb21d DQo+IFNlbnQ6IFR1ZXNkYXksIE1hcmNoIDAxLCAyMDE2IDg6MzggQU0NCj4gVG86IE1vb3JlLCBS b2JlcnQ7IHJqd0Byand5c29ja2kubmV0OyBXaWxsaWFtcywgRGFuIEoNCj4gQ2M6IFpoZW5nLCBM djsgZWxsaW90dEBocGUuY29tOyBsaW51eC1udmRpbW1AbGlzdHMuMDEub3JnOyBsaW51eC0NCj4g YWNwaUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5vcmc7IGRldmVs QGFjcGljYS5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2MiAxLzNdIEFDUEkvTkZJVDogVXBk YXRlIENvbnRyb2wgUmVnaW9uIFN0cnVjdHVyZSB0bw0KPiBjb21wbHkgQUNQSSA2LjENCj4gDQo+ IE9uIFR1ZSwgMjAxNi0wMy0wMSBhdCAxNToxMyArMDAwMCwgTW9vcmUsIFJvYmVydCB3cm90ZToN Cj4gPg0KPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IFRvc2hp IEthbmkgW21haWx0bzp0b3NoaS5rYW5pQGhwZS5jb21dDQo+ID4gPiBTZW50OiBNb25kYXksIEZl YnJ1YXJ5IDIyLCAyMDE2IDE6NTUgUE0NCj4gPiA+IFRvOiByandAcmp3eXNvY2tpLm5ldDsgV2ls bGlhbXMsIERhbiBKDQo+ID4gPiBDYzogTW9vcmUsIFJvYmVydDsgWmhlbmcsIEx2OyBlbGxpb3R0 QGhwZS5jb207DQo+ID4gPiBsaW51eC1udmRpbW1AbGlzdHMuMDEub3IgZzsgbGludXgtYWNwaUB2 Z2VyLmtlcm5lbC5vcmc7DQo+ID4gPiBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBkZXZl bEBhY3BpY2Eub3JnOyBUb3NoaSBLYW5pDQo+ID4gPiBTdWJqZWN0OiBbUEFUQ0ggdjIgMS8zXSBB Q1BJL05GSVQ6IFVwZGF0ZSBDb250cm9sIFJlZ2lvbiBTdHJ1Y3R1cmUNCj4gPiA+IHRvIGNvbXBs eSBBQ1BJIDYuMQ0KPiA+ID4NCj4gPiA+IEFDUEkgNi4xLCBUYWJsZSA1LTEzMywgdXBkYXRlcyBO VkRJTU0gQ29udHJvbCBSZWdpb24gU3RydWN0dXJlIGFzDQo+ID4gPiBmb2xsb3dzLg0KPiA+ID4g wqAtIFZhbGlkIEZpZWxkcywgTWFudWZhY3R1cmluZyBMb2NhdGlvbiwgYW5kIE1hbnVmYWN0dXJp bmcgRGF0ZQ0KPiA+ID4gwqDCoMKgYXJlIGFkZGVkIGZyb20gcmVzZXJ2ZWQgcmFuZ2UuwqDCoE5v IGNoYW5nZSBpbiB0aGUgc3RydWN0dXJlIHNpemUuDQo+ID4gPiDCoC0gSURzIGRlZmluZWQgYXMg U1BEIHZhbHVlcyBhcmUgYXJyYXlzIG9mIGJ5dGVzLsKgwqBUaGUgc3BlYw0KPiA+ID4gwqDCoMKg Y2xhcmlmaWVkIHRoYXQgdGhleSBuZWVkIHRvIGJlIHJlcHJlc2VudGVkIGFzIGFycmF5cyBvZiBi eXRlcw0KPiA+ID4gwqDCoMKgYXMgd2VsbC4NCj4gPiA+DQo+ID4gPiBUaGlzIHBhdGNoIG1ha2Vz IHRoZSBmb2xsb3dpbmcgY2hhbmdlcyB0byBzdXBwb3J0IHRoaXMgdXBkYXRlLg0KPiA+ID4gwqAt IENoYW5nZSAnc3RydWN0IGFjcGlfbmZpdF9jb250cm9sX3JlZ2lvbicgdG8gcmVmbGVjdCB0aGUg dXBkYXRlLg0KPiA+ID4gwqDCoMKgU1BEIElEcyBhcmUgZGVmaW5lZCBhcyBhcnJheXMgb2YgYnl0 ZXMsIHNvIHRoYXQgdGhleSBjYW4gYmUNCj4gPiA+IMKgwqDCoHRyZWF0ZWQgaW4gdGhlIHNhbWUg d2F5IHJlZ2FyZGxlc3Mgb2YgQ1BVIGVuZGlhbm5lc3MgYW5kIGFyZQ0KPiA+ID4gwqDCoMKgbm90 IG1pc3MtdHJlYXRlZCBhcyBsaXR0bGUtZW5kaWFuIG51bWVyaWMgdmFsdWVzLg0KPiA+DQo+ID4N Cj4gPiBJIGRvbid0IHRoaW5rIHdlIGFyZSBnb2luZyB0byBzdGFydCBjaGFuZ2luZyB0aGUgQUNQ SSB0YWJsZXMgZGVmaW5lZA0KPiA+IGluIHRoZSBBQ1BJQ0EgaGVhZGVycyBiZWNhdXNlIG9mIHRo aXMuIFdlIGRvIGluIGZhY3QgaGF2ZSBtYWNyb3MgZm9yDQo+ID4gdGhpcyBwdXJwb3NlLg0KPiAN Cj4gQ2FuIHlvdSBlbGFib3JhdGUgd2hhdCBtYWNyb3MgeW91IHN1Z2dlc3QgdG8gdXNlIGZvciB0 aGlzIHB1cnBvc2U/DQo+IA0KPiBUaGFua3MsDQo+IC1Ub3NoaQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html