On Tue, 2016-03-01 at 17:37 +0000, Moore, Robert wrote: > > > -----Original Message----- > > From: Toshi Kani [mailto:toshi.kani@xxxxxxx] > > Sent: Tuesday, March 01, 2016 9:37 AM > > To: Moore, Robert; rjw@xxxxxxxxxxxxx; Williams, Dan J > > Cc: Zheng, Lv; elliott@xxxxxxx; linux-nvdimm@xxxxxxxxxxxx; linux- > > acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
